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.
