On Tue, 28 Jan 2003, Tim Bunce wrote:
> I've a bunch of relatively minor efficiency concerns but I'll address
> those by sending a patch once more dust has settled :)
There's still plenty of stuff to fix and write before worrying about
speed. But once that's done optimization patches will be welcome.
> The 'floating' param needs a better name. Perhaps 'no_timezone', or
> at least someing that better indicates it's related to timezones.
> Maybe just use a specific value for the 'time_zone' parameter:
> time_zone => undef.
>
> The "elsif ( exists( $args{offset} ) ) " branch at the end of new()
> contains "if ($args{floating} " which will never be true there.
That code needs rewriting to work with the new DateTime::TimeZone stuff.
Basically, all offset _handling_ code in DateTime.pm will disappear, and
instead it'll use DateTime::TimeZone for everything.
As to renaming floating, we could do "time_zone => 'floating'" maybe.
> All internal functions (not methods) should probably begin with an
> underscore for the same reasons as internal methods do.
The functions will be turned into methods, actually, since subclasses may
want to inherit them or override them.
> Class data is bad (just doesn't scale to big applications, consider
> mod_perl for example). Use of class data should be limited to
> providing default values for undefined object attributes.
A lot of the class data is for error handling. I was trying to emulate
DBI's flexibility there, but I've realized that won't work, because
there's many different classes that should share the DateTime error
handling, but since most of them won't inherit from DateTime.pm, the
current implementation is wrong. I'll probably have to go with something
simpler, and just throw exceptions or return undef.
That would just leave the language parameter, which is indeed used for
undefined object attributes.
> The docs for new() are confusing about timezone handling.
> First it says "It defaults times to UTC (Greenwich Mean Time, also
> called Zulu)." Then "The new() method tries to be intelligent about
> figuring out your local time zone. If you enter a time that's not
> *explicitly* in UTC, it looks at the environment variable C<TZ>".
See above.
> The ymd, mdy, and dmy methods all use two digit years. That's not
> ideal but I can understand people might like it. I'd strongly suggest
> that they be renamed yymmdd, mmddyy, ddmmyy, and the corresponding
> four digit year methods added as: yyyymmdd, mmddyyyy, ddmmyyyy.
> (Similary hms could be renamed hhmmss for consistency.)
Um, no they don't.
> The parsetime method should be renamed hour_min_sec and return
> values in that order (removing the need for a reverse in hms)
> and documented as a public method. It's very common for all to be
> needed together.
Seems reasonable.
> A corresponding public year_month_day method should be provided.
> So your example:
Also not a bad idea.
> I think hour_min_sec (aka parsetime) either shouldn't int() the seconds
> or a separate hour_min_secf method should be provided.
This will get resolved once the module handles sub-second resolutions, I
think.
> Don't memoize until the API is stable and only after you've given myself
> and others a chance to optimize the internals. Most of the core
> DateTime.pm code can probably be made fast enough that the overhead
> of memoizing may be too great to make an improvement.
Absolutely.
> Again, don't rush. The most important thing to get right is the API.
> All else is changable in time.
The good thing about the time zone code is it's all generated, which gives
us a chance to do a lot of pre-calculation when generating the code.
For example, I'm going to make it so that we pre-generate time zone
changes for at least 10 years into the future, so that those don't need to
be calculated on the fly.
> Overall it looks good.
Thanks.
-dave
/*=======================
House Absolute Consulting
www.houseabsolute.com
=======================*/
