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. > > So we can do TimeDuration.longValue(), and get the internal duration. > > Also, shouldn't the duration store the value internally as a long? > That would solve the normalization problem, and also reduce the memory > usage. > >> Why? >> >>> It should also normalize it's values. If I set a duration of 86400 >>> seconds, it should normalize to 1 day. >> Agreed. That was something I always wanted to add, but never got around >> to it. >> >>> Also, the 7 arg constructor incorrectly sets itself negative. If I >>> set a duration of 5 days, and -30 seconds, it'll think it's negative. >> If normalization code was written, then that scenario could be >> accommodated. For now a duration is positive or negative. > > I'm willing to do all these changes; first would be to have test cases > and full coverage.
Also, it might be nice to make use of TimeUnit for conversion to other values.
