Merge pull request #290 from jbgi/deprecate-unsafe-ord

jbgi · web-flow · commit 03309fec2763 · 2016-07-20T17:22:19.000+02:00
Deprecate unsafe Ord instances and List#groupBy that use them.
diff --git a/core/src/main/java/fj/Ord.java b/core/src/main/java/fj/Ord.java
@@ -478,12 +478,14 @@ public static <A extends Comparable<A>> Ord<A> comparableOrd() {
 
   /**
    * An order instance that uses {@link Object#hashCode()} for computing the order and equality,
-   * thus objects returning the same hashCode are considered to be equals (check {@link #hashEqualsOrd()}
-   * for an additional check on {@link Object#equals(Object)}).
+   * thus objects returning the same hashCode are considered to be equals.
+   * This is not safe and therefore this method is deprecated.
    *
    * @return An order instance that is based on {@link Object#hashCode()}.
-   * @see #hashEqualsOrd()
+   *
+   * @deprecated As of release 4.7.
    */
+  @Deprecated
   public static <A> Ord<A> hashOrd() {
     return ord(a -> {
       int aHash = a.hashCode();
@@ -495,9 +497,13 @@ public static <A> Ord<A> hashOrd() {
    * An order instance that uses {@link Object#hashCode()} and {@link Object#equals} for computing
    * the order and equality. First the hashCode is compared, if this is equal, objects are compared
    * using {@link Object#equals}.
+   * WARNING: This ordering violate antisymmetry on hash collisions.
    *
    * @return An order instance that is based on {@link Object#hashCode()} and {@link Object#equals}.
+   *
+   * @deprecated As of release 4.7.
    */
+  @Deprecated
   public static <A> Ord<A> hashEqualsOrd() {
     return ord(a -> {
       int aHash = a.hashCode();
diff --git a/core/src/main/java/fj/data/List.java b/core/src/main/java/fj/data/List.java
@@ -1299,11 +1299,15 @@ public final A mode(final Ord<A> o) {
 
   /**
    * Groups the elements of this list by a given keyFunction into a {@link TreeMap}.
-   * The ordering of the keys is determined by {@link fj.Ord#hashOrd()}.
+   * The ordering of the keys is determined by {@link fj.Ord#hashOrd()} (ie. Object#hasCode).
+   * This is not safe and therefore this method is deprecated.
    *
    * @param keyFunction The function to select the keys for the map.
    * @return A TreeMap containing the keys with the accumulated list of matched elements.
+   *
+   * @deprecated As of release 4.7, use {@link #groupBy(F, Ord)}
    */
+  @Deprecated
   public final <B> TreeMap<B, List<A>> groupBy(final F<A, B> keyFunction) {
     return groupBy(keyFunction, Ord.hashOrd());
   }
@@ -1322,12 +1326,16 @@ public final <B> TreeMap<B, List<A>> groupBy(final F<A, B> keyFunction, final Or
   /**
    * Groups the elements of this list by a given keyFunction into a {@link TreeMap} and transforms
    * the matching elements with the given valueFunction. The ordering of the keys is determined by
-   * {@link fj.Ord#hashOrd()}.
+   * {@link fj.Ord#hashOrd()} (ie. Object#hasCode).
+   * This is not safe and therefore this method is deprecated.
    *
    * @param keyFunction The function to select the keys for the map.
    * @param valueFunction The function to apply on each matching value.
    * @return A TreeMap containing the keys with the accumulated list of matched and mapped elements.
+   *
+   * @deprecated As of release 4.7, use {@link #groupBy(F, F, Ord)}
    */
+  @Deprecated
   public final <B, C> TreeMap<B, List<C>> groupBy(
       final F<A, B> keyFunction,
       final F<A, C> valueFunction) {
diff --git a/demo/src/main/java/fj/demo/List_groupBy.java b/demo/src/main/java/fj/demo/List_groupBy.java
@@ -18,15 +18,15 @@ public static void main(final String... args) {
   private static void keyDemo() {
     System.out.println("KeyDemo");
     final List<String> words = list("Hello", "World", "how", "are", "your", "doing");
-    final TreeMap<Integer, List<String>> lengthMap = words.groupBy(String::length);
+    final TreeMap<Integer, List<String>> lengthMap = words.groupBy(String::length, Ord.intOrd);
 
     lengthMap.forEach(entry -> System.out.println(String.format("Words with %d chars: %s", entry._1(), entry._2())));
   }
 
   private static void keyValueDemo() {
     System.out.println("KeyValueDemo");
     final List<Integer> xs = list(1, 2, 3, 4, 5, 6, 7, 8, 9);
-    final TreeMap<Integer, List<String>> result = xs.groupBy(x -> x % 3, Integer::toBinaryString);
+    final TreeMap<Integer, List<String>> result = xs.groupBy(x -> x % 3, Integer::toBinaryString, Ord.intOrd);
 
     result.forEach(entry -> System.out.println(String.format("Numbers with reminder %d are %s", entry._1(), entry._2())));
   }
@@ -35,7 +35,7 @@ private static void keyValueAccDemo() {
     System.out.println("KeyValueAccDemo");
     final List<String> words = list("Hello", "World", "how", "are", "your", "doing");
     final TreeMap<Integer, Integer> lengthCounts =
-        words.groupBy(String::length, Function.identity(), 0, (word, sum) -> sum + 1, Ord.hashOrd());
+        words.groupBy(String::length, Function.identity(), 0, (word, sum) -> sum + 1, Ord.intOrd);
 
     lengthCounts.forEach(entry -> System.out.println(String.format("Words with %d chars: %s", entry._1(), entry._2())));
   }
diff --git a/props-core-scalacheck/src/test/scala/fj/data/CheckList.scala b/props-core-scalacheck/src/test/scala/fj/data/CheckList.scala
@@ -187,7 +187,7 @@ object CheckList extends Properties("List") {
       join(a)))
 
   property("groupBy") = forAll((a: List[Int]) => {
-    val result = a.groupBy((x: Int) => (x % 2 == 0): lang.Boolean)
+    val result = a.groupBy((x: Int) => (x % 2 == 0): lang.Boolean, Ord.booleanOrd)
     result.get(true).forall((xs: List[Int]) => xs.forall((x: Int) => (x % 2 == 0): lang.Boolean): lang.Boolean) &&
       result.get(false).forall((xs: List[Int]) => xs.forall((x: Int) => (x % 2 != 0): lang.Boolean): lang.Boolean) &&
       a.map((x: Int) => (x % 2) == 0: lang.Boolean).nub().length() == result.size()
diff --git a/props-core/src/test/java/fj/data/properties/ListProperties.java b/props-core/src/test/java/fj/data/properties/ListProperties.java
@@ -255,7 +255,7 @@ public Property nub() {
 
   public Property groupBy() {
     return property(arbList(arbInteger), list -> {
-      final TreeMap<Boolean, List<Integer>> map = list.groupBy(i -> i % 2 == 0);
+      final TreeMap<Boolean, List<Integer>> map = list.groupBy(i -> i % 2 == 0, Ord.booleanOrd);
       final List<Integer> list1 = map.get(true).orSome(nil());
       final List<Integer> list2 = map.get(false).orSome(nil());
       return prop(list.length() == list1.length() + list2.length())
