On Wed, 25 Aug 2010 11:34:10 -0500, Lars Tandle Kyllingstad
<[email protected]> wrote:
I've had the chance to look at your date lib now, and (without being
anywhere near an expert on the matter) I think it seems rather good.
Thanks!
1. I'm not sure I agree that DayOfWeek should be a named enum.
Personally, I'd prefer if the weekdays were enumerated like the months,
alias int DayOfWeek;
enum : DayOfWeek { Monday, Tuesday, ... }
That way you can get rid of the DOW alias. In general, I'm not fond of
aliases that are just abbreviations, so I suggest removing WeekNo as
well. ;)
TBH, I dislike anonymous enums. They tend to polute the scope where they
are imported, and sometimes the value name is too ambiguous without the
enum name prepended. But in this case, I think the week days are
self-describing enough. In fact the enum with the month names also started
as named enum, but after writing MonthName.September for the tenth time, I
got tired and changed it. Yes, this is a good idea to make this change.
2. The month enum should have the Month type, I guess?
enum : Month { January, February, ... }
Well, it kinda get complicated. Originally, the Month alias was going to
be a full fledged structure, but then the things started to get unwieldy
because as the month can also be represented with a number (or an enum), I
would had to write a lot of operator overloads (for mathematical
operations and comparison with ints). So at the end I settled with an
integer. But if there's the need to revert back to the Month struct, then
the enum wouldn't make sense (an enum of structs?). Short version: Month
can change of type so that's why the enum shouldn't be of the
aforementioned type.
3. I found it rather strange that the doc comment says that
DatePeriod.last() returns the day *after* the period, while
DatePeriod.end() is the last day of the period. I checked
boost::date_time, and indeed it seems you've switched the two -- at
least in the documentation.
Yes. Those are some silly mistakes I did and I'll correct them pronto.
4. The doc comments need a bit of cleaning up. Examples:
- There should be some ///ditto comments for the Month,
Day, and DayOfYear types.
Yes. I'll do that.
- The docs for DateRange.this() refer to WeekRange.
Yes, that's one of the perils of copy/paste :( I copied the definition of
WeekRange and refactored it to GenericRange but I forgot to update the
docs.
- The docs for Date.toIso8601() mention IsoFormat.Normal,
rather than IsoFormat.Basic.
Oops. An oversight. Originally I named the IsoFormat values as Normal and
Extended, but the ISO-8601 spec uses the terms Basic and Extended. I
forgot to update the docs.
5. Is there any reason why you define the trivial opEquals() for
DateCount? I think the compiler takes care of that for you.
Unfortunately, if I remove the trivial opEquals (the one with the
non-const parameter) the compiler complains when I compare a const
DayCount with a non-cons one.
I'm looking forward to seeing the time stuff as well!
-Lars
Thanks again for the time you dedicated to do this review. I have set up a
Bitbucket repo with this code at:
http://bitbucket.org/gomez/yao-library/src
So whenever I do a change you can go there and review the modifications.
Cheers.
--
Yao G.
_______________________________________________
phobos mailing list
[email protected]
http://lists.puremagic.com/mailman/listinfo/phobos
