Merge pull request #1601 from danieldietrich/linked_set_map_replace

Fixes LinkedHashMap/Set replace()
vavr-io · Oct 2, 2016 · c18ab43 · c18ab43
2 parents abcde8d + 7d7712e
commit c18ab43
Show file tree

Hide file tree

Showing 4 changed files with 132 additions and 21 deletions.
diff --git a/javaslang/src/main/java/javaslang/collection/LinkedHashMap.java b/javaslang/src/main/java/javaslang/collection/LinkedHashMap.java
@@ -483,7 +483,35 @@ public LinkedHashMap<K, V> removeValues(Predicate<? super V> predicate) {
 
     @Override
     public LinkedHashMap<K, V> replace(Tuple2<K, V> currentElement, Tuple2<K, V> newElement) {
-        return Maps.replace(this, currentElement, newElement);
+        Objects.requireNonNull(currentElement, "currentElement is null");
+        Objects.requireNonNull(newElement, "newElement is null");
+
+        // We replace the whole element, i.e. key and value have to be present.
+        if (!Objects.equals(currentElement, newElement) && contains(currentElement)) {
+
+            Queue<Tuple2<K, V>> newList = list;
+            HashMap<K, V> newMap = map;
+
+            final K currentKey = currentElement._1;
+            final K newKey = newElement._1;
+
+            // If current key and new key are equal, the element will be automatically replaced,
+            // otherwise we need to remove the pair (newKey, ?) from the list manually.
+            if (!Objects.equals(currentKey, newKey)) {
+                final Option<V> value = newMap.get(newKey);
+                if (value.isDefined()) {
+                    newList = newList.remove(Tuple.of(newKey, value.get()));
+                }
+            }
+
+            newList = newList.replace(currentElement, newElement);
+            newMap = newMap.remove(currentKey).put(newElement);
+
+            return wrap(newList, newMap);
+
+        } else {
+            return this;
+        }
     }
 
     @Override

diff --git a/javaslang/src/main/java/javaslang/collection/LinkedHashSet.java b/javaslang/src/main/java/javaslang/collection/LinkedHashSet.java
@@ -685,11 +685,13 @@ public LinkedHashSet<T> removeAll(Iterable<? extends T> elements) {
         return Collections.removeAll(this, elements);
     }
 
-    // DEV-NOTE: replace does not preserve the order, the new element may already be in the set
     @Override
     public LinkedHashSet<T> replace(T currentElement, T newElement) {
-        if (map.containsKey(currentElement)) {
-            return remove(currentElement).add(newElement);
+        if (!Objects.equals(currentElement, newElement) && contains(currentElement)) {
+            final Tuple2<T, T> currentPair = Tuple.of(currentElement, currentElement);
+            final Tuple2<T, T> newPair = Tuple.of(newElement, newElement);
+            final LinkedHashMap<T, T> newMap = map.replace(currentPair, newPair);
+            return new LinkedHashSet<>(newMap);
         } else {
             return this;
         }

diff --git a/javaslang/src/test/java/javaslang/collection/LinkedHashMapTest.java b/javaslang/src/test/java/javaslang/collection/LinkedHashMapTest.java
@@ -2,6 +2,7 @@
 
 import javaslang.Tuple;
 import javaslang.Tuple2;
+import org.assertj.core.api.Assertions;
 import org.junit.Test;
 
 import java.math.BigDecimal;
@@ -82,6 +83,66 @@ public void shouldNarrowLinkedHashMap() {
         assertThat(actual).isEqualTo(3);
     }
 
+    // -- static ofAll(Iterable)
+
+    @Test
+    public void shouldWrapMap() {
+        java.util.Map<Integer, Integer> source = new java.util.HashMap<>();
+        source.put(1, 2);
+        source.put(3, 4);
+        assertThat(LinkedHashMap.ofAll(source)).isEqualTo(emptyIntInt().put(1, 2).put(3, 4));
+    }
+
+    // -- replace
+
+    @Test
+    public void shouldReturnSameInstanceIfReplacingNonExistingPairUsingNonExistingKey() {
+        final Map<Integer, String> map = LinkedHashMap.of(1, "a", 2, "b");
+        final Map<Integer, String> actual = map.replace(Tuple.of(0, "?"), Tuple.of(0, "!"));
+        assertThat(actual).isSameAs(map);
+    }
+
+    @Test
+    public void shouldReturnSameInstanceIfReplacingNonExistingPairUsingExistingKey() {
+        final Map<Integer, String> map = LinkedHashMap.of(1, "a", 2, "b");
+        final Map<Integer, String> actual = map.replace(Tuple.of(2, "?"), Tuple.of(2, "!"));
+        assertThat(actual).isSameAs(map);
+    }
+
+    @Test
+    public void shouldPreserveOrderWhenReplacingExistingPairWithSameKeyAndDifferentValue() {
+        final Map<Integer, String> map = LinkedHashMap.of(1, "a", 2, "b", 3, "c");
+        final Map<Integer, String> actual = map.replace(Tuple.of(2, "b"), Tuple.of(2, "B"));
+        final Map<Integer, String> expected = LinkedHashMap.of(1, "a", 2, "B", 3, "c");
+        assertThat(actual).isEqualTo(expected);
+        Assertions.assertThat(List.ofAll(actual)).isEqualTo(List.ofAll(expected));
+    }
+
+    @Test
+    public void shouldPreserveOrderWhenReplacingExistingPairWithDifferentKeyValue() {
+        final Map<Integer, String> map = LinkedHashMap.of(1, "a", 2, "b", 3, "c");
+        final Map<Integer, String> actual = map.replace(Tuple.of(2, "b"), Tuple.of(4, "B"));
+        final Map<Integer, String> expected = LinkedHashMap.of(1, "a", 4, "B", 3, "c");
+        assertThat(actual).isEqualTo(expected);
+        Assertions.assertThat(List.ofAll(actual)).isEqualTo(List.ofAll(expected));
+    }
+
+    @Test
+    public void shouldPreserveOrderWhenReplacingExistingPairAndRemoveOtherIfKeyAlreadyExists() {
+        final Map<Integer, String> map = LinkedHashMap.of(1, "a", 2, "b", 3, "c", 4, "d", 5, "e");
+        final Map<Integer, String> actual = map.replace(Tuple.of(2, "b"), Tuple.of(4, "B"));
+        final Map<Integer, String> expected = LinkedHashMap.of(1, "a", 4, "B", 3, "c", 5, "e");
+        assertThat(actual).isEqualTo(expected);
+        Assertions.assertThat(List.ofAll(actual)).isEqualTo(List.ofAll(expected));
+    }
+
+    @Test
+    public void shouldReturnSameInstanceWhenReplacingExistingPairWithIdentity() {
+        final Map<Integer, String> map = LinkedHashMap.of(1, "a", 2, "b", 3, "c");
+        final Map<Integer, String> actual = map.replace(Tuple.of(2, "b"), Tuple.of(2, "b"));
+        assertThat(actual).isSameAs(map);
+    }
+
     // -- scan, scanLeft, scanRight
 
     @Test
@@ -132,14 +193,6 @@ public void shouldScanRight() {
                 Tuple.of(0, "x")));
     }
 
-    @Test
-    public void shouldWrapMap() {
-        java.util.Map<Integer, Integer> source = new java.util.HashMap<>();
-        source.put(1, 2);
-        source.put(3, 4);
-        assertThat(LinkedHashMap.ofAll(source)).isEqualTo(emptyIntInt().put(1, 2).put(3, 4));
-    }
-
     // -- map
 
     @Test
@@ -150,4 +203,5 @@ public void shouldReturnModifiedKeysMapWithNonUniqueMapperAndPredictableOrder()
         Map<Integer, String> expected = LinkedHashMap.of(1, "2");
         assertThat(actual).isEqualTo(expected);
     }
+
 }
diff --git a/javaslang/src/test/java/javaslang/collection/LinkedHashSetTest.java b/javaslang/src/test/java/javaslang/collection/LinkedHashSetTest.java
@@ -1,13 +1,14 @@
 package javaslang.collection;
 
 import javaslang.Value;
+import org.assertj.core.api.Assertions;
 import org.junit.Test;
 
 import java.math.BigDecimal;
 import java.util.ArrayList;
 import java.util.function.Function;
 import java.util.function.Supplier;
-import java.util.stream.*;
+import java.util.stream.Collector;
 import java.util.stream.Stream;
 
 public class LinkedHashSetTest extends AbstractSetTest {
@@ -190,20 +191,46 @@ public void shouldNarrowLinkedHashSet() {
         assertThat(actual).isEqualTo(3);
     }
 
-    // -- transform()
+    // -- replace
 
     @Test
-    public void shouldTransform() {
-        String transformed = of(42).transform(v -> String.valueOf(v.get()));
-        assertThat(transformed).isEqualTo("42");
+    public void shouldReturnSameInstanceIfReplacingNonExistingElement() {
+        final Set<Integer> set = LinkedHashSet.of(1, 2, 3);
+        final Set<Integer> actual = set.replace(4, 0);
+        assertThat(actual).isSameAs(set);
     }
 
-    // -- replace(old, new)
+    @Test
+    public void shouldPreserveOrderWhenReplacingExistingElement() {
+        final Set<Integer> set = LinkedHashSet.of(1, 2, 3);
+        final Set<Integer> actual = set.replace(2, 0);
+        final Set<Integer> expected = LinkedHashSet.of(1, 0, 3);
+        assertThat(actual).isEqualTo(expected);
+        Assertions.assertThat(List.ofAll(actual)).isEqualTo(List.ofAll(expected));
+    }
+
+    @Test
+    public void shouldPreserveOrderWhenReplacingExistingElementAndRemoveOtherIfElementAlreadyExists() {
+        final Set<Integer> set = LinkedHashSet.of(1, 2, 3, 4, 5);
+        final Set<Integer> actual = set.replace(2, 4);
+        final Set<Integer> expected = LinkedHashSet.of(1, 4, 3, 5);
+        assertThat(actual).isEqualTo(expected);
+        Assertions.assertThat(List.ofAll(actual)).isEqualTo(List.ofAll(expected));
+    }
+
+    @Test
+    public void shouldReturnSameInstanceWhenReplacingExistingElementWithIdentity() {
+        final Set<Integer> set = LinkedHashSet.of(1, 2, 3);
+        final Set<Integer> actual = set.replace(2, 2);
+        assertThat(actual).isSameAs(set);
+    }
+
+    // -- transform
 
-    @Override
     @Test
-    public void shouldReplaceElementOfNonNilUsingCurrNewWhenOneOccurrenceExists() {
-        assertThat(of(0, 1, 2).replace(1, 3)).isEqualTo(of(0, 2, 3));
+    public void shouldTransform() {
+        String transformed = of(42).transform(v -> String.valueOf(v.get()));
+        assertThat(transformed).isEqualTo("42");
     }
 
     // -- toLinkedSet