On Mon, Mar 14, 2011 at 12:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
>> My first reaction that this change was about a net wash in
>> readability for me -- in a couple places it might save me a few
>> moments thinking about what the number was meant to represent,
>> balanced against following the ctag back to the #define to see what
>> number was used for things like DAYS_PER_YEAR or DAYS_PER_MONTH.
>
>> Comments like the one Bruce cites above seem like they tip the
>> scales in favor of the patch for me. Having a place to document
>> the choice of questionable values seems like it's better than just
>> using the questionable values "bare" all over the place. Neither
>> omission of the justification nor repeating it seems better.
>
> Another advantage of the macros is that it makes it a lot easier to grep
> to see where a questionable value is being used. Originally I'd felt
> that wrapping those bogus numbers in macros was a bad idea, but the
> documentation and searching advantages are enough to make me think it's
> all right.
Yeah, I agree. And I do think that there is also some value of having
constants for SECS_PER_MINUTE and MINUTES_PER_HOUR, because otherwise
it can be unclear what 60 means in a particular context. We're at
the high end of what I consider reasonable in terms of defining
constants to represent values that aren't likely to change, but there
is tangible value in being able to grep for those constants when
you're trying to figure out what things might need changing, or just
to understand the code better.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company