RE: [boost] Date/Time formal review

From: Jeff Garland (jeff_at_[hidden])
Date: 2002-04-23 15:08:14

> Code comments:
> gregorian/formatters.hpp
> to_iso_string, to_iso_extended_string, and to_simple_string, should
> probably take a const & date. to_iso_string and to_simple_string should
> take a const date_period

Agreed.

> I'm also not sure, but it is probably worth using the template
> "basic_string", instead of std::string. That way you can have w_char if
> its supported. (That is to say, write these inline functions as function
> templates.) 

Makes sense.  I'm a little scared about the portability issues, but I agree
that we should try to support w_char. 
 
> gdtl/adjust_functors.hpp
> get_offset should take a const &date_type.
> (or boost::call_traits<date_type>::const_reference d)

Agreed.

> Should the "week_functor" instead of multiplying by "7", get this value
> from the date_type?  I know it sounds weird but "7" is arbritary. And one
> could imagine a Martian calender with more or less days in a week than 7.

Yes, this would be more generic.
 
> Also as a general coding style I'd add "explicit" to all the default
> constructors and those taking only one non default argument. This seems to
> catch a few coding errors of unintentional conversion.

In general I agree. However, there are a couple places that depend on 
implicit conversion.  

> In date_clock_device.hpp
> the references to the namespace "std", need leading "::",as in
> 
> "::std::local_time(t);"
> 
> Otherwise its looking in boost::gdtl::std::local_time which shouldn't work
> on non broken compilers.
> (Same in int_adapter.hpp, time_clock.hpp)

Thanks -- will fix.
 
> date_formating.hpp
> class month_formatter
> I think the switch statement could be written as a compile time switch?
> 
> If format_type returned different types for the call ::month_format(). As
> it looks like once the class is instantiated, its always the same result.
>
> (same for ymd_to_string, in ymd_formatter)

Cool idea!
 
> Point of coding style:
>   if (cond)
>   {
>     return;
>   }
>   else
>   {
>     return;
>   }
> 
> should be written
> 
>   if (cond)
>   {
>     return;
>   }
> 
>   return;
> 
> 
> Why? Some compilers don't eliminate the extra jump instruction, also it
> removes unnecessary indentation/nesting.

I hate when that happens :-)
 
>   if (cond)
>   {
>      if (cond2)
>        return;
>      else
>        return;
>   }
>   else
>   {
>     if (cond3)
>       return
>     else
>      return
>   }
>  
> 
> 
> becomes
>    if (cond)
>    {
>       if (cond2)
>         return;
> 
>       return;
>    }
> 
>    if (cond3)
>       return;
> 
>    return;
> 
> (Example file period.hpp)

Ok, will fix.

> time_iterator.hpp
> Shouldn't time_itr be declared with iterator_traits, of forward_iterator?

Yes, I believe so.

> I guess the only thing I want is the ability to mix and match dates, as in
> given a date "now", add 240 seconds, or - 1 week, or + 1 month and ask
> "when". I missed this in the
> documentation, but is the kind of application I have right now for
> date/time.

Yes, this doesn't exactly mix and match as you say.  The
reason is that 240 seconds is a 'physical time duration' while and 
'weeks' and 'months' are concepts.  As it turns out, for your purposes 
weeks will be easy because they turn out to be a fixed length.  

So here's how I would do the first part.

using namespace boost::gregorian;
using namespace boost::posix_time;
ptime now = second_clock::local_time();
ptime plus_sec = now + seconds(240);
ptime plus_week = now + day_duration(7);

Months are nasty.  That is, what is the answer if I add 1 month to 
Feb 27th?  Mar 27 or Mar 31? So to do this, you not only need to 
decide on the handling of end of month rules, you need to have the 
context of the current date to perform the calculation. There are a 
couple options, but I recommend defining a function to calculate 
a physical duration given the current date.  The current month_functor 
provides one set of rules for doing this, so if those meet your needs 
you could do the following:

typedef boost::gdtl::month_functor<date> month_functor;
month_functor month_adder(1); //add one month at a time
ptime plus_month = now + month_adder.get_offset(d);

If the rules of the current month_functor don't meet your needs then
we would need to write one that does.  

Jeff

Thanks for the insightful comments.

Jeff