Re-worked TreeMap comparators, considering null-keys #1695

danieldietrich · 2016-11-20T16:30:12Z

Update: This behavioral change was reverted in #1696

Map<K, V> map = TreeMap.empty();

// ordinary Maps are expected to accept null values:
map.put(null, "ok");

danieldietrich

Some comments

danieldietrich · 2016-11-20T16:34:47Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

@@ -448,7 +448,7 @@ private TreeMap(RedBlackTree<Tuple2<K, V>> entries) {
     * @return A new Map containing the given entries
     */
    public static <K, V> TreeMap<K, V> of(Comparator<? super K> keyComparator, K k1, V v1, K k2, V v2, K k3, V v3, K k4, V v4) {
-        return of(keyComparator, k1, v1, k2, v2, k3, v3).put(k4, v4);


The previous version called TreeMap.put, which was bad for these reasons:

TreeMap.put was called, which creates RedBlackTree-wrappers

of(...) recursively called O(n) times of() before starting to add values

The current version

calls put on RedBlackTree, which does not create intermediate TreeMap objects

turns the recursive call into an iteration, which is more efficient in this case

danieldietrich · 2016-11-20T16:36:10Z

javaslang/src/test/java/javaslang/collection/TreeMapTest.java

@@ -37,7 +37,7 @@ protected String className() {

    @Override
    protected <T1 extends Comparable<? super T1>, T2> TreeMap<T1, T2> emptyMap() {
-        return TreeMap.empty(nullsFirst());


Using nullsFirst() was terribly wrong. It made unit tests artificially work which were not intended to work.

danieldietrich · 2016-11-20T16:37:24Z

javaslang/src/test/java/javaslang/collection/AbstractMapTest.java

+                1, null, 2, null, 3, null);
+        treeMap2.get(2);
+
+    }


will remove this in a second...

danieldietrich · 2016-11-20T16:38:19Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

-        for (Tuple2<? extends K, ? extends V> entry : entries) {
-            tree = tree.insert((Tuple2<K, V>) entry);
-        }
-        return tree.isEmpty() ? (TreeMap<K, V>) empty() : new TreeMap<>(tree);


this was wrong, because the comparator was not used in the empty case

danieldietrich · 2016-11-20T16:40:30Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

-        Objects.requireNonNull(keyComparator, "keyComparator is null");
-        Objects.requireNonNull(entry, "entry is null");
-        return TreeMap.<K, V> empty(keyComparator).put(entry);
+    public static <K, V> TreeMap<K, V> ofAll(Comparator<? super K> keyComparator, java.util.Map<? extends K, ? extends V> map) {


this method was missing

codecov-io · 2016-11-20T16:49:35Z

Current coverage is 97.04% (diff: 89.07%)

Merging #1695 into master will decrease coverage by 0.01%

@@             master      #1695   diff @@
==========================================
  Files            89         89          
  Lines         11247      11277    +30   
  Methods           0          0          
  Messages          0          0          
  Branches       1888       1888          
==========================================
+ Hits          10917      10944    +27   
- Misses          187        193     +6   
+ Partials        143        140     -3

Powered by Codecov. Last update ee2f094...2767cf7

danieldietrich · 2016-11-20T16:50:27Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+    }
+
+    @Override
+    public <K2, V2> TreeMap<K2, V2> bimap(Comparator<? super K2> keyComparator,


no, it must not

danieldietrich · 2016-11-20T16:50:31Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+    }
+
+    @Override
+    public <K2, V2> TreeMap<K2, V2> bimap(Comparator<? super K2> keyComparator,


no, it must not

danieldietrich · 2016-11-20T16:50:35Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

@@ -1225,34 +1183,201 @@ public String toString() {
        return mkString(stringPrefix() + "(", ", ", ")");
    }

+    // -- private helpers
+
+    private static <K, K2, V, V2> TreeMap<K2, V2> bimap(TreeMap<K, V> map, EntryComparator<K2, V2> entryComparator,


no, it must not

danieldietrich · 2016-11-20T16:50:39Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

@@ -1225,34 +1183,201 @@ public String toString() {
        return mkString(stringPrefix() + "(", ", ", ")");
    }

+    // -- private helpers
+
+    private static <K, K2, V, V2> TreeMap<K2, V2> bimap(TreeMap<K, V> map, EntryComparator<K2, V2> entryComparator,


no, it must not

danieldietrich · 2016-11-20T16:50:42Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+        return createTreeMap(entryComparator, map.entries, entry -> entry.map(keyMapper, valueMapper));
+    }
+
+    private static <K, V, K2, V2> TreeMap<K2, V2> flatMap(TreeMap<K, V> map, EntryComparator<K2, V2> entryComparator,


no, it must not

danieldietrich · 2016-11-20T16:50:46Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+        return createTreeMap(entryComparator, map.entries, entry -> entry.map(keyMapper, valueMapper));
+    }
+
+    private static <K, V, K2, V2> TreeMap<K2, V2> flatMap(TreeMap<K, V> map, EntryComparator<K2, V2> entryComparator,


no, it must not

danieldietrich · 2016-11-20T16:50:49Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+        return createTreeMap(entryComparator, map.entries.iterator().flatMap(entry -> mapper.apply(entry._1, entry._2)));
+    }
+
+    private static <K, K2, V, V2> TreeMap<K2, V2> map(TreeMap<K, V> map, EntryComparator<K2, V2> entryComparator,


no, it must not

danieldietrich · 2016-11-20T16:50:53Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+        return createTreeMap(entryComparator, map.entries.iterator().flatMap(entry -> mapper.apply(entry._1, entry._2)));
+    }
+
+    private static <K, K2, V, V2> TreeMap<K2, V2> map(TreeMap<K, V> map, EntryComparator<K2, V2> entryComparator,


no, it must not

danieldietrich · 2016-11-20T16:50:56Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+        return new TreeMap<>(tree);
+    }
+
+    private static <K, K2, V, V2> TreeMap<K2, V2> createTreeMap(EntryComparator<K2, V2> entryComparator,


no, it must not

danieldietrich · 2016-11-20T16:50:59Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+        return new TreeMap<>(tree);
+    }
+
+    private static <K, K2, V, V2> TreeMap<K2, V2> createTreeMap(EntryComparator<K2, V2> entryComparator,


no, it must not

danieldietrich · 2016-11-20T16:52:15Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+
+            private static final long serialVersionUID = 1L;
+
+            final Comparator<K> keyComparator;


private: ok
accessor methods: oh my god - no! (no getters and setters please. we have keyComparator())

sputnikci · 2016-11-20T17:12:31Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+    }
+
+    @Override
+    public <K2, V2> TreeMap<K2, V2> bimap(Comparator<? super K2> keyComparator,


[Checkstyle] ERROR: Name 'K2' must match pattern '^[A-Z]$'.

sputnikci · 2016-11-20T17:12:32Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+    }
+
+    @Override
+    public <K2, V2> TreeMap<K2, V2> bimap(Comparator<? super K2> keyComparator,


[Checkstyle] ERROR: Name 'V2' must match pattern '^[A-Z]$'.

sputnikci · 2016-11-20T17:12:32Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

@@ -1225,34 +1183,201 @@ public String toString() {
        return mkString(stringPrefix() + "(", ", ", ")");
    }

+    // -- private helpers
+
+    private static <K, K2, V, V2> TreeMap<K2, V2> bimap(TreeMap<K, V> map, EntryComparator<K2, V2> entryComparator,


[Checkstyle] ERROR: Name 'K2' must match pattern '^[A-Z]$'.

sputnikci · 2016-11-20T17:12:32Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

@@ -1225,34 +1183,201 @@ public String toString() {
        return mkString(stringPrefix() + "(", ", ", ")");
    }

+    // -- private helpers
+
+    private static <K, K2, V, V2> TreeMap<K2, V2> bimap(TreeMap<K, V> map, EntryComparator<K2, V2> entryComparator,


[Checkstyle] ERROR: Name 'V2' must match pattern '^[A-Z]$'.

sputnikci · 2016-11-20T17:12:32Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+        return createTreeMap(entryComparator, map.entries, entry -> entry.map(keyMapper, valueMapper));
+    }
+
+    private static <K, V, K2, V2> TreeMap<K2, V2> flatMap(TreeMap<K, V> map, EntryComparator<K2, V2> entryComparator,


[Checkstyle] ERROR: Name 'K2' must match pattern '^[A-Z]$'.

sputnikci · 2016-11-20T17:12:32Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+        return createTreeMap(entryComparator, map.entries, entry -> entry.map(keyMapper, valueMapper));
+    }
+
+    private static <K, V, K2, V2> TreeMap<K2, V2> flatMap(TreeMap<K, V> map, EntryComparator<K2, V2> entryComparator,


[Checkstyle] ERROR: Name 'V2' must match pattern '^[A-Z]$'.

sputnikci · 2016-11-20T17:12:33Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+        return createTreeMap(entryComparator, map.entries.iterator().flatMap(entry -> mapper.apply(entry._1, entry._2)));
+    }
+
+    private static <K, K2, V, V2> TreeMap<K2, V2> map(TreeMap<K, V> map, EntryComparator<K2, V2> entryComparator,


[Checkstyle] ERROR: Name 'K2' must match pattern '^[A-Z]$'.

sputnikci · 2016-11-20T17:12:33Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+        return createTreeMap(entryComparator, map.entries.iterator().flatMap(entry -> mapper.apply(entry._1, entry._2)));
+    }
+
+    private static <K, K2, V, V2> TreeMap<K2, V2> map(TreeMap<K, V> map, EntryComparator<K2, V2> entryComparator,


[Checkstyle] ERROR: Name 'V2' must match pattern '^[A-Z]$'.

sputnikci · 2016-11-20T17:12:33Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+        return new TreeMap<>(tree);
+    }
+
+    private static <K, K2, V, V2> TreeMap<K2, V2> createTreeMap(EntryComparator<K2, V2> entryComparator,


[Checkstyle] ERROR: Name 'K2' must match pattern '^[A-Z]$'.

sputnikci · 2016-11-20T17:12:33Z

javaslang/src/main/java/javaslang/collection/TreeMap.java

+        return new TreeMap<>(tree);
+    }
+
+    private static <K, K2, V, V2> TreeMap<K2, V2> createTreeMap(EntryComparator<K2, V2> entryComparator,


[Checkstyle] ERROR: Name 'V2' must match pattern '^[A-Z]$'.

danieldietrich · 2016-11-20T17:54:03Z

This was the last fix for 2.0.5 - now starting to cherry-pick.

danieldietrich added bug «vavr-collection» labels Nov 20, 2016

danieldietrich added this to the 2.0.5 milestone Nov 20, 2016

danieldietrich commented Nov 20, 2016

View reviewed changes

Re-worked TreeMap comparators, considering null-keys

2767cf7

sputnikci reviewed Nov 20, 2016

View reviewed changes

danieldietrich merged commit e4459b1 into vavr-io:master Nov 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-worked TreeMap comparators, considering null-keys #1695

Re-worked TreeMap comparators, considering null-keys #1695

danieldietrich commented Nov 20, 2016 •

edited

Loading

danieldietrich left a comment

danieldietrich Nov 20, 2016

danieldietrich Nov 20, 2016

danieldietrich Nov 20, 2016

danieldietrich Nov 20, 2016

danieldietrich Nov 20, 2016

codecov-io commented Nov 20, 2016 •

edited

Loading

danieldietrich Nov 20, 2016

danieldietrich Nov 20, 2016

danieldietrich Nov 20, 2016

danieldietrich Nov 20, 2016

danieldietrich Nov 20, 2016

danieldietrich Nov 20, 2016

danieldietrich Nov 20, 2016

danieldietrich Nov 20, 2016

danieldietrich Nov 20, 2016

danieldietrich Nov 20, 2016

danieldietrich Nov 20, 2016 •

edited

Loading

sputnikci Nov 20, 2016

sputnikci Nov 20, 2016

sputnikci Nov 20, 2016

sputnikci Nov 20, 2016

sputnikci Nov 20, 2016

sputnikci Nov 20, 2016

sputnikci Nov 20, 2016

sputnikci Nov 20, 2016

sputnikci Nov 20, 2016

sputnikci Nov 20, 2016

danieldietrich commented Nov 20, 2016


		private static final long serialVersionUID = 1L;

		final Comparator<K> keyComparator;

Re-worked TreeMap comparators, considering null-keys #1695

Re-worked TreeMap comparators, considering null-keys #1695

Conversation

danieldietrich commented Nov 20, 2016 • edited Loading

danieldietrich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 20, 2016 • edited Loading

Current coverage is 97.04% (diff: 89.07%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldietrich Nov 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldietrich commented Nov 20, 2016

danieldietrich commented Nov 20, 2016 •

edited

Loading

codecov-io commented Nov 20, 2016 •

edited

Loading

danieldietrich Nov 20, 2016 •

edited

Loading