Fraction: fix compareTo for positive numbers with two negative parts
The numerator and denominator can both be negative. Thus the comparison
by magnitude (when the signum is equal) must use the absolute values.
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 a72cdb1..1e60668 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
@@ -953,9 +953,9 @@
if (lhsSigNum == 0) {
return 0;
}
- // Compare magnitude
- final BigInteger nOd = numerator.multiply(other.denominator);
- final BigInteger dOn = denominator.multiply(other.numerator);
+ // Compare absolute magnitude
+ final BigInteger nOd = numerator.abs().multiply(other.denominator.abs());
+ final BigInteger dOn = denominator.abs().multiply(other.numerator.abs());
return nOd.compareTo(dOn);
}
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 99151f5..5154eff 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
@@ -696,15 +696,27 @@
*/
@Override
public int compareTo(Fraction other) {
- final int lhsSigNum = signum();
- final int rhsSigNum = other.signum();
+ // Compute the sign of each part
+ final int lns = Integer.signum(numerator);
+ final int lds = Integer.signum(denominator);
+ final int rns = Integer.signum(other.numerator);
+ final int rds = Integer.signum(other.denominator);
+
+ final int lhsSigNum = lns * lds;
+ final int rhsSigNum = rns * rds;
if (lhsSigNum != rhsSigNum) {
return (lhsSigNum > rhsSigNum) ? 1 : -1;
}
- // Same sign: compare magnitude
- final long nOd = ((long) numerator) * other.denominator;
- final long dOn = ((long) denominator) * other.numerator;
+ // Same sign.
+ // Avoid a multiply if both fractions are zero
+ if (lhsSigNum == 0) {
+ return 0;
+ }
+ // Compare absolute magnitude.
+ // Multiplication by the signum is equal to the absolute.
+ final long nOd = ((long) numerator) * lns * other.denominator * rds;
+ final long dOn = ((long) denominator) * lds * other.numerator * rns;
return Long.compare(nOd, dOn);
}
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 777d0a9..bdc8003 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
@@ -211,9 +211,9 @@
Assertions.assertEquals(0, f.compareTo(a));
Assertions.assertEquals(1, f.compareTo(e));
Assertions.assertEquals(-1, e.compareTo(f));
-// Assertions.assertEquals(-1, g.compareTo(a));
+ Assertions.assertEquals(-1, g.compareTo(a));
Assertions.assertEquals(-1, g.compareTo(f));
-// Assertions.assertEquals(1, a.compareTo(g));
+ Assertions.assertEquals(1, a.compareTo(g));
Assertions.assertEquals(-1, d.compareTo(g));
Assertions.assertEquals(0, BigFraction.of(0, 3).compareTo(BigFraction.of(0, -2)));
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 82fe803..7205c6b 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
@@ -170,9 +170,9 @@
Assertions.assertEquals(0, f.compareTo(a));
Assertions.assertEquals(1, f.compareTo(e));
Assertions.assertEquals(-1, e.compareTo(f));
-// Assertions.assertEquals(-1, g.compareTo(a));
+ Assertions.assertEquals(-1, g.compareTo(a));
Assertions.assertEquals(-1, g.compareTo(f));
-// Assertions.assertEquals(1, a.compareTo(g));
+ Assertions.assertEquals(1, a.compareTo(g));
Assertions.assertEquals(-1, d.compareTo(g));
Assertions.assertEquals(0, Fraction.of(0, 3).compareTo(Fraction.of(0, -2)));
