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?
