Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exact Quantity.equals(…) comparison is inconsistent with Quantity.toString() #284

Closed
martinmo opened this issue Nov 26, 2019 · 2 comments
Assignees
Milestone

Comments

@martinmo
Copy link
Contributor

Let's say I have two Quantity objects q1 and q2, both with metric == Metric.UNIT and

  • q1.amount equal to new BigDecimal("1.0"), and
  • q2.amount equal to new BigDecimal("1").

(Note that the two BigDecimal objects are not equal as per BigDecimal.equals().)

Now I compare q1 and q2 using assertThat(q1).isEqualTo(q2). This fails with the following error message:

org.opentest4j.AssertionFailedError: 
Expecting:
 <"1 (Quantity@5acf93bb)">
to be equal to:
 <"1 (Quantity@7e7be63f)">
but was not.

The misleading identical output of actual vs. expected is because the current toString() always strips trailing zeroes. (By the way, the objects are considered unequal because they use an exact comparison of amount, instead of BigDecimal.compareTo().)

I suggest to:

  • include the canonical representation of amount (as returned by BigDecimal.toString()), making it easier to spot the difference, and
  • complete the compareTo()-like feature set of Quantity by adding isEqualTo() to the already existing isGreaterThan(), isLessThan(), etc.

Using BigDecimal.compareTo() instead of BigDecimal.equals() in Quantity.equals() is probably not an option because of inconsistencies with hashCode().

@martinmo martinmo self-assigned this Nov 26, 2019
@martinmo martinmo added this to the 7.3 milestone Nov 26, 2019
@odrotbohm
Copy link
Member

Are you sure you're testing a current version of Salespoint? Asking because AssertJ outputs Quantity@5acf93bb while we actually have a proper implementation of toString() formatting the value and the metric.

@odrotbohm
Copy link
Member

Looks like AssertJ is printing the object hash in case the toString() representations are equal. I have a fix here that forwards the precision of the underlying BigDecimal to the format to be used for rendering. I'll add isEqualTo() as well.

@odrotbohm odrotbohm assigned odrotbohm and unassigned martinmo Nov 26, 2019
odrotbohm added a commit that referenced this issue Nov 26, 2019
Quantity.isEqualTo(…) allows to compare quantities without looking at the precision of the underlying amount, i.e. 1 is treated equally to 1.0 (in contrast to the ….equals(…) method).
odrotbohm added a commit that referenced this issue Nov 26, 2019
We now forward the precision of the underlying amount to the formatter used in Quantity.toString(). This allows a quantity of 1 to be differentiated from 1.0 for plain ….toString() renderings.
odrotbohm added a commit that referenced this issue Nov 26, 2019
We now use var for local variables. Whitespace.
odrotbohm added a commit that referenced this issue Nov 26, 2019
Quantity.isEqualTo(…) allows to compare quantities without looking at the precision of the underlying amount, i.e. 1 is treated equally to 1.0 (in contrast to the ….equals(…) method).
odrotbohm added a commit that referenced this issue Nov 26, 2019
We now forward the precision of the underlying amount to the formatter used in Quantity.toString(). This allows a quantity of 1 to be differentiated from 1.0 for plain ….toString() renderings.
odrotbohm added a commit that referenced this issue Nov 26, 2019
We now use var for local variables. Whitespace.
@odrotbohm odrotbohm modified the milestones: 7.3, 7.2.2 Nov 26, 2019
@odrotbohm odrotbohm changed the title Exact Quantity.equals() comparison is inconsistent with Quantity.toString() Exact Quantity.equals(…) comparison is inconsistent with Quantity.toString() Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants