--- On Mon, 2/15/10, Adam Heath <[email protected]> wrote: > From: Adam Heath <[email protected]> > Subject: Re: TimeDuration is not immutable > To: [email protected] > Date: Monday, February 15, 2010, 10:55 PM > Adrian Crum wrote: > > Adam Heath wrote: > >> Adrian Crum wrote: > >>> Adam Heath wrote: > >>>> Adrian Crum wrote: > >>>>> Adam Heath wrote: > >>>>>> The javadoc for > org.ofbiz.base.util.TimeDuration says it is > >>>>>> immutable; > >>>>>> however, it's fields are not > final, and lots of methods modify it's > >>>>>> fields. > >>>>> The methods that modify fields are > directly or indirectly called by > >>>>> the > >>>>> constructor and are protected. Let me > know if any public method > >>>>> modifies > >>>>> a field. > >>>> fromLong modifies the fields directly, not > from the constructor. > >>> fromLong is a static method that returns an > immutable instance. > >> > >> Right you are, but the statement still > stands. fromLong should use > >> the constructor to set that. > >> > >>>> makeNegative modifies the fields. > >>> makeNegative is protected and used > internally. > >> > >> The the same statement for set() applies. > >> > >>>> set() is protected, TimeDuration is not > final, so sub-classes could > >>>> change the fields. > >>> If I need to extend TimeDuration, then I might > also need to change the > >>> fields. > >> > >> If fromLong just used the constructor, after doing > math on the long, > >> then it wouldn't need to call makeNegative > itself. Then, the only > >> places that would call set() or makeNegative would > be the constructor, > >> so the code could be inlined. Once it is > inlined, then the fields can > >> be made final, and the class becomes a proper > immutable. > >> > >> Additionally, becase fromLong creates an empty > TimeDuration, then sets > >> the fields one by one, the vm is free to re-order > those values, and > >> even delay then for a *long* time, well after > fromLong returns to it's > >> caller. There is no synchronization on this; > if these fields were > >> only set from the constructor, then this problem > wouldn't occur. > >> > >>>> Ideally, this class should extend Number, > or some other number class. > >> > >>> Why? > >> So we can do TimeDuration.longValue(), and get the > internal duration. > > > > What would it mean if you called > TimeDuration.intValue()? What would > > that value represent? > > > > In designing the API, I was trying to avoid equating a > duration with a > > long value. The current support for long values was > added to accommodate > > existing entity fields that store durations as > longs. The long value > > should not be confused with elapsed milliseconds - it > is a TimeDuration > > *encoded* as a long. (Another change I never got > around to was renaming > > the parameters so there is no reference to millis.) > > > > The whole idea of TimeDuration is that it is a data > type. It would be > > nice to be able to do math on TimeDurations (X > duration plus Y duration > > equals Z duration), but at the same time, you > shouldn't be able to do > > math on the internal representation of the duration > (since the > > implementation is supposed to be hidden). > > > >> Also, shouldn't the duration store the value > internally as a long? > >> That would solve the normalization problem, and > also reduce the memory > >> usage. > > > > I don't recall my reasoning for that. I guess it was > for performance > > reasons. If the internal value was a long, then each > accessor would have > > to decode the long. > > > >> I'm willing to do all these changes; first would > be to have test cases > >> and full coverage. > > > > That would be great! I created the class to make > calendaring code > > simpler. It could use some TLC. > > Ok, sorry to say this, but TimeDuration needs to be > removed. > com.ibm.icu.text.DurationFormat, which handles things like > 2 days from > now and 3 hours ago, uses a long for the duration. > > Additionally, trying to duplicate leap year calculation, > there is not > a whole number of days in a year(it's not quite > 365.24). If long is > good enough for icu, then why should we try to use > something else?
Grasshopper, TimeDuration is not concerned with such details. They are delegated to the Calendar class. Tell me, using com.ibm.icu.text.DurationFormat - how do you add two durations?
