From 7d7712e78079595765a2265a843f1a95884d6f14 Mon Sep 17 00:00:00 2001 From: Daniel Dietrich Date: Sun, 2 Oct 2016 22:36:32 +0200 Subject: [PATCH] Fixes LinkedHashMap/Set replace() --- .../javaslang/collection/LinkedHashMap.java | 30 +++++++- .../javaslang/collection/LinkedHashSet.java | 8 ++- .../collection/LinkedHashMapTest.java | 70 ++++++++++++++++--- .../collection/LinkedHashSetTest.java | 45 +++++++++--- 4 files changed, 132 insertions(+), 21 deletions(-) diff --git a/javaslang/src/main/java/javaslang/collection/LinkedHashMap.java b/javaslang/src/main/java/javaslang/collection/LinkedHashMap.java index 854b550ac0..0f3e458327 100644 --- a/javaslang/src/main/java/javaslang/collection/LinkedHashMap.java +++ b/javaslang/src/main/java/javaslang/collection/LinkedHashMap.java @@ -483,7 +483,35 @@ public LinkedHashMap removeValues(Predicate predicate) { @Override public LinkedHashMap replace(Tuple2 currentElement, Tuple2 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> newList = list; + HashMap 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 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 index 1b7bb78140..bba5714fa5 100644 --- a/javaslang/src/main/java/javaslang/collection/LinkedHashSet.java +++ b/javaslang/src/main/java/javaslang/collection/LinkedHashSet.java @@ -685,11 +685,13 @@ public LinkedHashSet removeAll(Iterable 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 replace(T currentElement, T newElement) { - if (map.containsKey(currentElement)) { - return remove(currentElement).add(newElement); + if (!Objects.equals(currentElement, newElement) && contains(currentElement)) { + final Tuple2 currentPair = Tuple.of(currentElement, currentElement); + final Tuple2 newPair = Tuple.of(newElement, newElement); + final LinkedHashMap 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 index 983af5fd3e..1c486fbc6d 100644 --- 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 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 map = LinkedHashMap.of(1, "a", 2, "b"); + final Map actual = map.replace(Tuple.of(0, "?"), Tuple.of(0, "!")); + assertThat(actual).isSameAs(map); + } + + @Test + public void shouldReturnSameInstanceIfReplacingNonExistingPairUsingExistingKey() { + final Map map = LinkedHashMap.of(1, "a", 2, "b"); + final Map actual = map.replace(Tuple.of(2, "?"), Tuple.of(2, "!")); + assertThat(actual).isSameAs(map); + } + + @Test + public void shouldPreserveOrderWhenReplacingExistingPairWithSameKeyAndDifferentValue() { + final Map map = LinkedHashMap.of(1, "a", 2, "b", 3, "c"); + final Map actual = map.replace(Tuple.of(2, "b"), Tuple.of(2, "B")); + final Map 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 map = LinkedHashMap.of(1, "a", 2, "b", 3, "c"); + final Map actual = map.replace(Tuple.of(2, "b"), Tuple.of(4, "B")); + final Map 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 map = LinkedHashMap.of(1, "a", 2, "b", 3, "c", 4, "d", 5, "e"); + final Map actual = map.replace(Tuple.of(2, "b"), Tuple.of(4, "B")); + final Map 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 map = LinkedHashMap.of(1, "a", 2, "b", 3, "c"); + final Map 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 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 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 index 862fe1782c..412444b050 100644 --- 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 set = LinkedHashSet.of(1, 2, 3); + final Set actual = set.replace(4, 0); + assertThat(actual).isSameAs(set); } - // -- replace(old, new) + @Test + public void shouldPreserveOrderWhenReplacingExistingElement() { + final Set set = LinkedHashSet.of(1, 2, 3); + final Set actual = set.replace(2, 0); + final Set 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 set = LinkedHashSet.of(1, 2, 3, 4, 5); + final Set actual = set.replace(2, 4); + final Set 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 set = LinkedHashSet.of(1, 2, 3); + final Set 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