I’ve reviewed some code with a colleague today: fairly complex boolean conditions regarding time intervals. The dates were compared by checking the sign of AbsoluteDate.compareTo(). While in the middle of complex code, this verbose method of comparison was pretty difficult to read and tripped us several times.
What would the community think of adding extra methods so we could use snippets of code that would look like:
(and potential variants such as isStrictlyBefore…)
While our struggle today involved AbsoluteDate instances, these methods would probably be right at home in the TimeStamped interface. Maybe in the form of default methods right in the interface ?
Such methods would be clearly redundant with compareTo(), so it could be argued that they would bloat the API. What is the policy of Orekit on such redundant methods ?
+1 for adding these methods to AbsoluteDate. It would improve readability.
I’m hesitant about adding the methods to TimeStamped since in many implementations the method names may have different semantic meaning. E.g. with the Range class it would be unexpected that range1.isBetween(range2, range3) compares the date. Having the getDate() method makes it clearer what is being compared at the expense of some verbosity. Another option would be only implementing the methods in AbsoluteDate but accepting a TimeStamped as the method parameters.
Yes, I can definitely see how the isBetween method could be misinterpreted when working with measurements.
Having the method in AbsoluteDate but accepting any TimeStamped as an argument is a great idea ! It should allow us to write measurement1.getDate().is between(measurement2, measurement3) which I find pretty concise yet unambiguous.
Thanks ! I personally prefer the isAfter form, because I think the verb helps to convey the fact that the return type is Boolean. But I will of course settle for what most people here agree with.
You’re right that we should match the standard API where possible. So if isAfter is strict, then should we introduce isAfterOrSimultaneous ? isAfterOrEqual ? I cannot think of anything less verbose at the moment…
We could also leave them out, since the standard API does not provide them, and using the equals method is always possible.
+1 for your suggestion @yannick.
It would be a very good addition to have these methods.
I tend to prefer the “is” too.
I’m ok with the “isAfterOrEqual”; I don’t find it too verbose.
I agree with your suggestion @yannick , it will increase the readability, and the suggestion of @evan.ward for TimeStamped as parameters makes it great.
I prefer the simple strict forms isBefore, isEqual, isAfter and the very useful isBetween (not strict). The isEqual would be nice to avoid the confusing behavior of equals(Object o) with a TimeStamped parameter.
Could you please elaborate on this ? What do you expect the isEqual(TimeStamped t) to do differently ?
I must admit that I am not familiar with the details of the behavior of AbsoluteDate.equals(Object o). I tend to check that dates are “close enough” using compareTo() and a tolerance, because my dates might come out of different computations with approximations. I was considering introducing a isEqual(TimeStamped t, float tolerance) to make this approach easier.
This will strictly compare the dates as expected, while equals(ts) always return false if ts is not an AbsoluteDate (it is the way equals is written). So adding isAfter and isBefore should imply a isEqual with the same semantic to avoid confusion.
I agree also on introducing isEqual(TimeStamped ts, double tolerance), that is a very common need.
I have opened issue #609 in gitlab, in which I have described the methods that I plan to introduce. I’ve attempted to take into account all advice provided in this discussion, while also trying to have a consistent naming convention that matches some popular Java libraries.
If anyone disagrees with the choices that I’ve made, please tell me.
