Fix fraction hashCode
The sign must be incorporated into the hash as a fraction with the same
numerical value can have different representations:
-1/3 == 1/-3
Using the sign ensures that -1/3 and 1/3 have different hash codes. This
would not be true if the hash were constructed using only the absolute
values of numerator and denominator.
diff --git a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
index cee2a6e..3962e2e 100644
--- a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
+++ b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
@@ -1019,7 +1019,17 @@
@Override
public int hashCode() {
- return 37 * (37 * 17 + numerator.hashCode()) + denominator.hashCode();
+ // Incorporate the sign and absolute values of the numerator and denominator.
+ // Equivalent to
+ // Arrays.hashCode(new int[] {signum(), numerator.abs(), denominator.abs()})
+ // int hash = 1;
+ // hash = 31 * hash + signum();
+ // hash = 31 * hash + numerator.abs().hashCode();
+ // hash = 31 * hash + denominator.abs().hashCode();
+ // Note: BigInteger.hashCode() * BigInteger.signum() == BigInteger.abs().hashCode().
+ final int numS = numerator.signum();
+ final int denS = denominator.signum();
+ return 31 * (31 * (31 + numS * denS) + numerator.hashCode() * numS) + denominator.hashCode() * denS;
}
/**
diff --git a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
index 31e7cde..be85fb3 100644
--- a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
+++ b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
@@ -706,7 +706,17 @@
@Override
public int hashCode() {
- return 37 * (37 * 17 + numerator) + denominator;
+ // Incorporate the sign and absolute values of the numerator and denominator.
+ // Equivalent to
+ // Arrays.hashCode(new int[] {signum(), Math.abs(numerator), Math.abs(denominator)})
+ // int hash = 1;
+ // hash = 31 * hash + signum();
+ // hash = 31 * hash + Math.abs(numerator);
+ // hash = 31 * hash + Math.abs(denominator);
+ // Note: x * Integer.signum(x) is equivalent to Math.abs(x).
+ final int numS = Integer.signum(numerator);
+ final int denS = Integer.signum(denominator);
+ return 31 * (31 * (31 + numS * denS) + numerator * numS) + denominator * denS;
}
/**
diff --git a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
index e3a611b..4d3377c 100644
--- a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
+++ b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
@@ -19,13 +19,12 @@
import java.math.BigDecimal;
import java.math.BigInteger;
import java.math.RoundingMode;
+import java.util.Arrays;
import org.apache.commons.numbers.core.TestUtils;
import org.junit.jupiter.api.Assertions;
-import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
-
public class BigFractionTest {
private static void assertFraction(long expectedNumerator, long expectedDenominator, BigFraction actual) {
@@ -595,7 +594,6 @@
Assertions.assertEquals(new BigDecimal("0.333"), BigFraction.of(1, 3).bigDecimalValue(3, RoundingMode.DOWN));
}
- @Disabled
@Test
public void testEqualsAndHashCode() {
BigFraction zero = BigFraction.of(0, 1);
@@ -649,6 +647,12 @@
Assertions.assertNotSame(f1, f2, "Do not call this assertion with the same object");
Assertions.assertEquals(f1, f2);
Assertions.assertEquals(f1.hashCode(), f2.hashCode(), "Equal fractions have different hashCode");
+ // Check the hashcode computation.
+ // This is not mandated but is a recommendation.
+ final int expected = Arrays.hashCode(new Object[] {f1.signum(),
+ f1.getNumerator().abs(),
+ f1.getDenominator().abs()});
+ Assertions.assertEquals(expected, f1.hashCode(), "Hashcode not equal to using Arrays.hashCode");
}
@Test
diff --git a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
index 0888a75..ada4269 100644
--- a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
+++ b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
@@ -16,13 +16,12 @@
*/
package org.apache.commons.numbers.fraction;
+import java.util.Arrays;
import org.apache.commons.numbers.core.TestUtils;
import org.junit.jupiter.api.Assertions;
-import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
-
/**
*/
public class FractionTest {
@@ -427,7 +426,6 @@
);
}
- @Disabled
@Test
public void testEqualsAndHashCode() {
Fraction zero = Fraction.of(0, 1);
@@ -492,6 +490,12 @@
Assertions.assertNotSame(f1, f2, "Do not call this assertion with the same object");
Assertions.assertEquals(f1, f2);
Assertions.assertEquals(f1.hashCode(), f2.hashCode(), "Equal fractions have different hashCode");
+ // Check the hashcode computation.
+ // This is not mandated but is a recommendation.
+ final int expected = Arrays.hashCode(new int[] {f1.signum(),
+ Math.abs(f1.getNumerator()),
+ Math.abs(f1.getDenominator())});
+ Assertions.assertEquals(expected, f1.hashCode(), "Hashcode not equal to using Arrays.hashCode");
}
@Test
