search

unitsofmeasurement / indriya

JSR 385 - Reference Implementation
Other
119 stars 42 forks source link

NumberQuantity.equals() not working #325

Closed keilw closed 3 years ago

keilw commented 3 years ago

Currently assertEquals(Quantities.getQuantity(0.234, CENTI(METRE)), AbstractQuantity.parse("0.234 cm")); fails although both are of type NumberQuantity. It seems NumberQuantity is missing the correct equals() or hashCode() methods.

andi-huber commented 3 years ago

Hi @keilw - I am not sure whether this is a bug, as equals() and hasCode() are well defined in AbstractQuantity. Java-doc there is pretty specific about how this works.

What you might instead test (I think) is

assertTrue(Quantities.getQuantity(0.234, MetricPrefix.CENTI(Units.METRE))
              .compareTo(AbstractQuantity.parse("0.234 cm").asType(Length.class)) == 0);
andi-huber commented 3 years ago

And more generally: I think its a bad idea to rely on Object.equals(...) when testing quantities or amounts for (semantic) equality. This is bad practice and does not produce reliable code. I'd recommend developers and consumers of the UOM libraries not to do that at all.

Same is true for the comparison operations (< = >) we provide with the library. I would not recommend consumers of the UOM libraries to use these either, at least not blindly. We can only give reliable answers to questions of how 2 amounts or quantities relate, when working with number representations that are not rounded. Eg. for doubles and BigDecimals there are only answers that can be given within a margin of numerical tolerance.

keilw commented 3 years ago

It is a bug because NumberQuantity defines an additional numeric value that is not used. assertEquals() and equals() really have to work if the class type is identical and the exact type of Unit is also the same for "cm".

andi-huber commented 3 years ago

Our AbstractQuantity#equals(...) will only ever return true if unit, scale, and value are all equal. And strictly meaning for the value that it also has to be the same object not just an object with the same numerical value. And this conforms to the associated java-doc.

I did the exercise debugging your given test case, and the equals method ends up comparing 2 value objects Double and BigDecimal, which both have roughly the same amount, but are not the same object. Hence the result of equals() is false.

keilw commented 3 years ago

Will have to check later, what debugger did you use, Eclipse or IntelliJ?

andi-huber commented 3 years ago

Eclipse.

I'd say works as designed, and I would not recommend to change it, as equals() and hashCode() are both relevant, if you want to contain instances of Quantities within Java collection types like sets or maps etc.

keilw commented 3 years ago

I think we need some more tests for that, it only came up as a suspect from #324 (which is not even a bug at all, merely improving JavaDoc missing clarity) but there could be more tests also for the collection behavior which I believe has not JUnit tests yet.