From 9a6665af3657bf548b8bbd62b82fa12cd39e5af0 Mon Sep 17 00:00:00 2001
From: Alex Herbert <aherbert@apache.org>
Date: Sat, 5 Nov 2022 08:43:58 +0000
Subject: [PATCH] Remove Comparable from the Shape class

The Shape could be used as a key so implements equals and hashCode. The
ordering of a Shape is arbitrary.
---
 .../collections4/bloomfilter/Shape.java       | 23 ++++++-----
 .../collections4/bloomfilter/ShapeTest.java   | 38 +++++++++++++------
 2 files changed, 38 insertions(+), 23 deletions(-)

diff --git a/src/main/java/org/apache/commons/collections4/bloomfilter/Shape.java b/src/main/java/org/apache/commons/collections4/bloomfilter/Shape.java
index d39df255e..2632fedac 100644
--- a/src/main/java/org/apache/commons/collections4/bloomfilter/Shape.java
+++ b/src/main/java/org/apache/commons/collections4/bloomfilter/Shape.java
@@ -16,8 +16,6 @@
  */
 package org.apache.commons.collections4.bloomfilter;
 
-import java.util.Objects;
-
 /**
  * The definition of a Bloom filter shape.
  *
@@ -43,7 +41,7 @@ import java.util.Objects;
  * [Wikipedia]</a>
  * @since 4.5
  */
-public final class Shape implements Comparable<Shape> {
+public final class Shape {
 
     /**
      * The natural logarithm of 2. Used in several calculations. Approximately 0.693147180559945.
@@ -81,19 +79,20 @@ public final class Shape implements Comparable<Shape> {
     }
 
     @Override
-    public int compareTo(Shape other) {
-        int i = Integer.compare(numberOfBits, other.numberOfBits);
-        return i == 0 ? Integer.compare(numberOfHashFunctions, other.numberOfHashFunctions) : i;
-    }
-
-    @Override
-    public boolean equals(final Object o) {
-        return (o instanceof Shape) ? compareTo((Shape) o) == 0 : false;
+    public boolean equals(Object obj) {
+        // Shape is final so no check for the same class as inheritance is not possible
+        if (obj instanceof Shape) {
+            Shape other = (Shape) obj;
+            return numberOfBits == other.numberOfBits &&
+                   numberOfHashFunctions == other.numberOfHashFunctions;
+        }
+        return false;
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(numberOfBits, numberOfHashFunctions);
+        // Match Arrays.hashCode(new int[] {numberOfBits, numberOfHashFunctions})
+        return (31 + numberOfBits) * 31 + numberOfHashFunctions;
     }
 
     /**
diff --git a/src/test/java/org/apache/commons/collections4/bloomfilter/ShapeTest.java b/src/test/java/org/apache/commons/collections4/bloomfilter/ShapeTest.java
index ecafa2ef4..e87535c9c 100644
--- a/src/test/java/org/apache/commons/collections4/bloomfilter/ShapeTest.java
+++ b/src/test/java/org/apache/commons/collections4/bloomfilter/ShapeTest.java
@@ -17,11 +17,16 @@
 package org.apache.commons.collections4.bloomfilter;
 
 import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.util.Arrays;
+
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
 
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 
 /**
  * Tests the {@link Shape} class.
@@ -45,18 +50,29 @@ public class ShapeTest {
     /**
      * Test equality of shape.
      */
-    @Test
-    public void testEquals() {
+    @ParameterizedTest
+    @CsvSource({
+        "3, 24",
+        "1, 24",
+        "1, 1",
+        "13, 124",
+        "13, 224",
+    })
+    public void testEqualsAndHashCode(int k, int m) {
+        Shape shape1 = Shape.fromKM(k, m);
+        assertEquals(shape1, shape1);
+        assertEquals(Arrays.hashCode(new int[] {m, k}), shape1.hashCode(),
+            "Doesn't match Arrays.hashCode(new int[] {m, k})");
+        assertNotEquals(shape1, null);
+        assertNotEquals(shape1, "text");
+        assertNotEquals(shape1, Integer.valueOf(3));
+        assertNotEquals(shape1, Shape.fromKM(k, m + 1));
+        assertNotEquals(shape1, Shape.fromKM(k + 1, m));
 
-        assertEquals(shape, shape);
-        assertEquals(3, shape.getNumberOfHashFunctions());
-        assertEquals(24, shape.getNumberOfBits());
-        assertEquals(shape.hashCode(), Shape.fromKM(3, 24).hashCode());
-        assertNotEquals(shape, null);
-        assertNotEquals(shape, Shape.fromKM(3, 25));
-        assertNotEquals(shape, Shape.fromKM(4, 24));
-        assertNotEquals(shape, "text");
-        assertNotEquals(shape, Integer.valueOf(3));
+        // Test this is reproducible
+        Shape shape2 = Shape.fromKM(k, m);
+        assertEquals(shape1, shape2);
+        assertEquals(shape1.hashCode(), shape2.hashCode());
     }
 
     @Test