Skip to content

Commit

Permalink
jqno#540 BigDecimal equality using compareTo
Browse files Browse the repository at this point in the history
  • Loading branch information
Aaron Clark committed Dec 1, 2021
1 parent c58c9e7 commit 9ebeacd
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 37 deletions.
99 changes: 99 additions & 0 deletions docs/_errormessages/bigdecimal-equality.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
---
title: "BigDecimal equality"
---
Be aware that `BigDecimal` fields with values such as `1`, `1.0`, `1.00`, ... are not considered equal.

The `Comparable` interface strongly recommends but does not require that implementations consider two objects equal using
`compareTo` whenever they are equal using `equals` and vice versa. `BigDecimal` is a class where this is not applied.

{% highlight java %}
BigDecimal one = new BigDecimal("1");
BigDecimal alsoOne = new BigDecimal("1.0");

// prints true - 1 is the same as 1.0
System.out.println(one.compareTo(alsoOne) == 0);
// prints false - 1 is not the same as 1.0
System.out.println(one.equals(alsoOne));
{% endhighlight %}

Ways to resolve this error
---
If values like `1` and `1.0` need not be considered equal then this check can be disabled by suppressing `Warning.BIGDECIMAL_EQUALITY`.

{% highlight java %}
EqualsVerifier.forClass(Foo.class)
.suppress(Warning.BIGDECIMAL_EQUALITY)
.verify();
{% endhighlight %}

If values like `1` and `1.0` *should* be considered equal then some options are:
1. **Do not use `BigDecimal` as fields.** But unfortunately I cannot recommend well-known generally accepted drop-in alternative.

The argument for this: it is not great having to complicate classes with the options below. A valid `equals`and
`hashCode` is already easy enough to get wrong (option 2) and it is easy to forget and hard to validate `BigDecimal`
fields have been normalised (option 3). More special handling in `equals` and `hashCode` moves against standardisation,
such as making things easier e.g. `Objects.equals`, `Objects.hashCode`, or eliminating the need to think about it e.g.
Lombok's `@EqualsAndHashCode` or Java 16 (preview since 14) `record` classes: it is a shame having to add boilerplate
for every [`@EqualsAndHashCode` annotated class](https://stackoverflow.com/questions/36625347/how-to-make-lomboks-equalsandhashcode-work-with-bigdecimal)
or every [`record` class](https://stackoverflow.com/questions/68690126/java-16-records-bigdecimal-equals-hashcode)
that has a `BigDecimal` field.

2. **Implement `equals` to use `compareTo` for `BigDecimal` fields** and ensure values of those fields that are equal using
`compareTo` will produce the same hashcode.

EqualsVerifier checks this by default which causes the *BigDecimal equality* error messages.

It wouldn't be unwise to use utility methods as this is not as simple as a call to Java's `Objects.equals` and `Objects.hashcode`.
The logic for correct equality is:

{% highlight java %}
// true if bdField and other.bdField are
// either both null or are equal using compareTo
boolean comparablyEqual = (bdField == null && other.bdField == null)
|| (bdField != null && other.bdField != null && bdField.compareTo(other.bdField) == 0);
{% endhighlight %}

A consistent hashcode needs a way to normalise the value that it represents. A simple normalisation is:

{% highlight java %}
// Remove trailing zeros from the unscaled value of the
// BigDecimal to yield a consistently scaled instance
int consistentHashcode = Objects.hashCode(bdField.stripTrailingZeros());
{% endhighlight %}

3. **Normalise the `BigDecimal` fields** during your class's construction (and setters if it is mutable) such that there
is only one way it represents the same value for these fields. This means standard `equals` and `hashCode` can be used.
For example, your class ensures all variants of `1` such as `1.0`, `1.00`, etc are converted to `1` in its constructor.

A simple normalisation is:

{% highlight java %}
class Foo {
...
Foo(BigDecimal bdField) {
// Now if bdField is final it will only have instances
// that are equal when compareTo is equal.
// If mutable then setters will need to do the same.
this.bdField = bdField.stripTrailingZeros();
}
}
{% endhighlight %}

Unfortunately it is difficult to confirm this has been done. This check will then want disabling by suppressing `Warning.BIGDECIMAL_EQUALITY`
and will not catch regressions.

If performance is important then you will need to consider the costs of using `BigDecimal` and of where and how normalisation
is achieved. Option 2 performs the work when objects are stored in a `HashSet` or used as keys in a `HashMap`. Option 3
performs work on creation of each object but is then cheaper to hash. There may be better normalisations than `stripTrailingZeros`.
`BigDecimal` already has a cost traded in return for its accuracy.

Why does this happen for BigDecimal?
---
`BigDecimal` can have multiple representations of the same value. It uses an *unscaled value* and a *scale* (both integers).
For example, the value of 1 can be represented as unscaled value 1 with scale of 0 (scale is the number of places after
the decimal point when 0 or greater) or as unscaled value 10 with scale of 1 resolving to 1.0. Its `equals` and `hashCode`
methods use both of these attributes in their calculation rather than the resolved value.

There is more information on `compareTo` and `equals` in the `Comparable` Javadoc and Effective Java's chapter on implementing `Comparable`.

There is more information on `BigDecimal` in its Javadoc (and its representation can be seen by printing `unscaledValue()` and `scale()`).
30 changes: 0 additions & 30 deletions docs/_manual/12-bigdecimal-equality.md

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ For these situations, there are several types of warnings that you can suppress:
* `Warning.STRICT_HASHCODE`: disables the check that all fields used in `equals` must also be used in `hashCode`.
* `Warning.STRICT_INHERITANCE`: disables the check that classes or their `equals` methods be final, or that inheritance is properly accounted for. Read more about this topic on the [page about inheritance](/equalsverifier/manual/inheritance).
* `Warning.TRANSIENT_FIELDS`: disables the check that transient fields do not participate in `equals`. This applies both to Java's `transient` keyword, which applies to serialization, and to JPA's `@Transient` annotation, which applies to, well, JPA.
* `Warning.BIGDECIMAL_EQUALITY`: disables the check that equality of `BigDecimal` fields is implemented using `compareTo` rather than `equals`. Read more about this topic on the [page about BigDecimal equality](/equalsverifier/manual/bigdecimal-equality).
* `Warning.BIGDECIMAL_EQUALITY`: disables the check that equality of `BigDecimal` fields is implemented using `compareTo` rather than `equals`. Read more about this topic on the [page about BigDecimal equality](/equalsverifier/errormessages/bigdecimal-equality).

Of course, once you have sufficient test coverage, you _will_ come back and fix these issues, right? 😉

3 changes: 2 additions & 1 deletion docs/_pages/errormessages.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,14 @@ Before you continue, please check your class's `toString` method. Since EqualsVe
This is not a complete list. I'll add to it as needed, so if you need help with an error message, please [file an issue](https://github.com/jqno/equalsverifier/issues) or let me know on the [discussion group](https://groups.google.com/forum/?fromgroups#!forum/equalsverifier), and I'll add an explanation as soon as possible.

* [Abstract delegation](/equalsverifier/errormessages/abstract-delegation)
* [BigDecimal equality](/equalsverifier/errormessages/bigdecimal-equality)
* [ClassCastException: java.lang.Object cannot be cast to …](/equalsverifier/errormessages/classcastexception)
* [Coverage is not 100%](/equalsverifier/errormessages/coverage-is-not-100-percent)
* [Double: equals doesn't use Double.compare for field foo](/equalsverifier/errormessages/double-equals-doesnt-use-doublecompare-for-field-foo)
* [Float: equals doesn't use Float.compare for field foo](/equalsverifier/errormessages/float-equals-doesnt-use-floatcompare-for-field-foo)
* [Mutability: equals depends on mutable field](/equalsverifier/errormessages/mutability-equals-depends-on-mutable-field)
* [NoClassDefFoundError](/equalsverifier/errormessages/noclassdeffounderror)
* [Non-nullity: equals/hashCode/toString throws NullPointerExcpetion](/equalsverifier/errormessages/non-nullity-equals-hashcode-tostring-throws-nullpointerexception)
* [Non-nullity: equals/hashCode/toString throws NullPointerException](/equalsverifier/errormessages/non-nullity-equals-hashcode-tostring-throws-nullpointerexception)
* [Precondition: two objects are equal to each other](/equalsverifier/errormessages/precondition-two-objects-are-equal-to-each-other)
* [Recursive datastructure](/equalsverifier/errormessages/recursive-datastructure)
* [Redefined superclass: object should not equal superclass instance](/equalsverifier/errormessages/redefined-superclass-object-should-not-equal-superclass-instance)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

public class BigDecimalFieldCheck<T> implements FieldCheck<T> {

public static final String ERROR_DOC_TITLE = "BigDecimal equality";

private final CachedHashCodeInitializer<T> cachedHashCodeInitializer;

public BigDecimalFieldCheck(CachedHashCodeInitializer<T> cachedHashCodeInitializer) {
Expand Down Expand Up @@ -50,8 +52,9 @@ private void checkEquals(
T right
) {
Formatter f = Formatter.of(
"BigDecimal equality by comparison: object does not equal a copy of itself where BigDecimal field %%" +
" has a value that is equal using compareTo: %% compared to %%" +
ERROR_DOC_TITLE +
": object does not equal a copy of itself" +
" where BigDecimal field %% has a value that is equal using compareTo: %% compared to %%" +
"\nIf these values should be considered equal then use compareTo rather than equals for this field." +
"\nIf these values should not be considered equal, suppress Warning.%% to disable this check.",
field.getName(),
Expand All @@ -70,9 +73,9 @@ private void checkHashCode(
T right
) {
Formatter f = Formatter.of(
"BigDecimal equality by comparison: hashCode of object does not equal hashCode of a copy of itself" +
" where BigDecimal field %%" +
" has a value that is equal using compareTo: %% compared to %%" +
ERROR_DOC_TITLE +
": hashCode of object does not equal hashCode of a copy of itself" +
" where BigDecimal field %% has a value that is equal using compareTo: %% compared to %%" +
"\nIf these values should be considered equal then make sure to derive the same constituent hashCode from this field." +
"\nIf these values should not be considered equal, suppress Warning.%% to disable this check.",
field.getName(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import java.util.Objects;
import nl.jqno.equalsverifier.EqualsVerifier;
import nl.jqno.equalsverifier.Warning;
import nl.jqno.equalsverifier.internal.checkers.fieldchecks.BigDecimalFieldCheck;
import nl.jqno.equalsverifier.testhelpers.ExpectedException;
import org.junit.jupiter.api.Test;

Expand All @@ -21,6 +22,7 @@ public void fail_whenBigDecimalsComparedUsingEqualsWithComparablyConsistentHashC
)
.assertFailure()
.assertMessageContains(
BigDecimalFieldCheck.ERROR_DOC_TITLE,
"BigDecimal",
"equals",
"compareTo",
Expand All @@ -35,6 +37,7 @@ public void fail_whenBigDecimalsComparedUsingCompareToWithInconsistentHashCode()
.when(() -> EqualsVerifier.forClass(BigDecimalInconsistentHashCode.class).verify())
.assertFailure()
.assertMessageContains(
BigDecimalFieldCheck.ERROR_DOC_TITLE,
"BigDecimal",
"hashCode",
"compareTo",
Expand Down

0 comments on commit 9ebeacd

Please sign in to comment.