Robert Haas wrote:
> 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.
Yes, I did have to study the code to figure out which to use:
if (type == TZ || type == DTZ) { tz = -(val * MINS_PER_HOUR); result = dt2local(timestamp, tz); }
We measure timezone differences in minutes.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ It's impossible for everything to be true. +
