LinkedHashMap.put(existing key) should keep order #1838 #1983

ruslansennov · 2017-05-08T20:38:10Z

Also equality of LinkedHashMap with LinkedHashMap fixed (see #1555)

ruslansennov · 2017-05-08T20:42:10Z

vavr/src/main/java/io/vavr/collection/LinkedHashMap.java

-            newMap = newMap.remove(key);
+        final Queue<Tuple2<K, V>> newList;
+        final Option<V> currentEntry = get(key);
+        if (currentEntry.isDefined()) {


maybe we should check that oldValue.equals(newValue) is true and in this case do nothing (?)

It is not necessary not to add the entry if the old value/new value are equal, instead we will act like java.util.HashMap. It will be important when implementing the MapView later.

public class Test { public static void main(String[] args) { A x = new A(1, 1); A y = new A(1, 2); // true System.out.println(x.equals(y)); java.util.Map<Integer, A> map = new java.util.HashMap<>(); map.put(1, x); map.put(1, y); // true System.out.println(map.get(1) == y); } static class A { final int a; final int b; A(int a, int b) { this.a = a; this.b = b; } @Override public boolean equals(Object o) { if (o == this) { return true; } else if (o instanceof A) { final A that = (A) o; return this.a == that.a; } else { return false; } } @Override public int hashCode() { return a; } @Override public String toString() { return "A(a:" + a + ", b:" + b + ")"; } } }

codecov-io · 2017-05-08T20:47:22Z

Codecov Report

Merging #1983 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master   #1983      +/-   ##
===========================================
- Coverage     98.01%     98%   -0.01%     
+ Complexity     5175    5173       -2     
===========================================
  Files            91      91              
  Lines         11788   11788              
  Branches       1530    1530              
===========================================
- Hits          11554   11553       -1     
  Misses          116     116              
- Partials        118     119       +1

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/io/vavr/collection/LinkedHashMap.java	`98.52% <100%> (ø)`	`128 <4> (-1)`	⬇️
...r/src/main/java/io/vavr/concurrent/FutureImpl.java	`82.5% <0%> (-1.25%)`	`21% <0%> (-1%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3a746d...b8475ee. Read the comment docs.

danieldietrich

Hi Ruslan, thank you!
While doing the review I realized that this change is not correct. We should close the PR.
What do you think?
Greets,
Daniel

danieldietrich · 2017-05-09T11:26:00Z

vavr/src/test/java/io/vavr/collection/LinkedHashMapTest.java

@@ -181,18 +181,23 @@ public void shouldReturnModifiedKeysMapWithNonUniqueMapperAndPredictableOrder()
    // -- put

    @Test
-    public void shouldAppendToTheEndWhenPuttingAnExistingKeyAndNonExistingValue() {
+    public void shouldIgnoreOrderOfEntriesWhenComparingForEquality() {


please add an @Override here

danieldietrich · 2017-05-09T11:29:55Z

vavr/src/test/java/io/vavr/collection/AbstractMapTest.java

-                assertThat(actualMap.size()).isEqualTo(expectedMap.size());
-                actualMap.forEach((k, v) -> assertThat(v).isEqualTo(expectedMap.get(k)));
-                return this;
+                if (actual instanceof LinkedHashMap && obj instanceof LinkedHashMap) {


Please override the assertThat(Iterable<T> actual) method in LinkedHashMapTest test instead. A super call does inherently rely on the type hierarchy, which is not necessary and error prone.

Instead LinkedHashMapTest should defined what equality means (even if it looks like duplicate code at a first glance).

@Override protected <T> IterableAssert<T> assertThat(Iterable<T> actual) { ... }

danieldietrich · 2017-05-09T11:40:40Z

vavr/src/main/java/io/vavr/collection/LinkedHashMap.java

-            newMap = newMap.remove(key);
+        final Queue<Tuple2<K, V>> newList;
+        final Option<V> currentEntry = get(key);
+        if (currentEntry.isDefined()) {


It is not necessary not to add the entry if the old value/new value are equal, instead we will act like java.util.HashMap. It will be important when implementing the MapView later.

public class Test { public static void main(String[] args) { A x = new A(1, 1); A y = new A(1, 2); // true System.out.println(x.equals(y)); java.util.Map<Integer, A> map = new java.util.HashMap<>(); map.put(1, x); map.put(1, y); // true System.out.println(map.get(1) == y); } static class A { final int a; final int b; A(int a, int b) { this.a = a; this.b = b; } @Override public boolean equals(Object o) { if (o == this) { return true; } else if (o instanceof A) { final A that = (A) o; return this.a == that.a; } else { return false; } } @Override public int hashCode() { return a; } @Override public String toString() { return "A(a:" + a + ", b:" + b + ")"; } } }

danieldietrich · 2017-05-09T11:47:46Z

vavr/src/main/java/io/vavr/collection/LinkedHashMap.java

+            return Collections.equals(this.list, ((LinkedHashMap<?, ?>) o).list);
+        } else {
+            return Collections.equals(this, o);
+        }


We changed the equals behavior.

This is not correct according to the Traversable.equals() javadoc. We act like Scala, which is:

scala> import scala.collection.immutable._ import scala.collection.immutable._ scala> val map1 = ListMap((1, 'a'), (2, 'b')) map1: scala.collection.immutable.ListMap[Int,Char] = ListMap(1 -> a, 2 -> b) scala> val map2 = ListMap((2, 'b'), (1, 'a')) map2: scala.collection.immutable.ListMap[Int,Char] = ListMap(2 -> b, 1 -> a) scala> map1 == map2 res0: Boolean = true

Note: The immutable LinkedHashMap is called ListMap in Scala

danieldietrich · 2017-05-09T11:55:43Z

Note: We still are able to use eq() for structural equality (which takes the order into account)

ruslansennov · 2017-05-09T11:57:04Z

This PR (and issue) is very problemastic. It's OK for me to keep all things as is. I'm offline now, please close both.

danieldietrich · 2017-05-09T12:24:45Z

I will keep it open for a while. Maybe there is some discussion...

This PR (and issue) is very problemastic.

Does it break s.th. for you?

danieldietrich · 2017-05-09T20:52:37Z

@ruslansennov Just one more word on this: this PR would break the Liskov Substitution Principle.

The following should be true for Maps that contain (1, "a"), (2, "b") - regardless of the order. Especially for LinkedHashMap(1, "a", 2, "b") and LinkedHashMap(2, "b", 1, "a").

final Map<Integer, String> map1 = ...
final Map<Integer, String> map2 = ...
assert map1.equals(map2);

danieldietrich · 2017-05-09T23:36:58Z

@ruslansennov oh noes, my review was 💩
Of course the LinkedHashMap.put(k,v) should preserve existing order (on existing key). I focused on the equals stuff in my review.

Could you please make a PR for the put() fix, only? We will take it into the 0.9.0 release. Nearly all relevant 0.9.0 issues are finished now!

LinkedHashMap.put(existing key) should keep order vavr-io#1838

b8475ee

ruslansennov requested review from danieldietrich and v1ctor May 8, 2017 20:38

ruslansennov commented May 8, 2017

View reviewed changes

danieldietrich suggested changes May 9, 2017

View reviewed changes

danieldietrich closed this May 9, 2017

ruslansennov deleted the lhm-order branch May 10, 2017 04:12

danieldietrich added !BACK-COMPAT design/refactoring/improvement «vavr-collection» labels Sep 16, 2017

danieldietrich added this to the vavr-0.9.0 milestone Sep 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LinkedHashMap.put(existing key) should keep order #1838 #1983

LinkedHashMap.put(existing key) should keep order #1838 #1983

ruslansennov commented May 8, 2017

ruslansennov May 8, 2017

danieldietrich May 9, 2017 •

edited

Loading

codecov-io commented May 8, 2017

danieldietrich left a comment

danieldietrich May 9, 2017

danieldietrich May 9, 2017

danieldietrich May 9, 2017 •

edited

Loading

danieldietrich May 9, 2017

danieldietrich commented May 9, 2017

ruslansennov commented May 9, 2017

danieldietrich commented May 9, 2017

danieldietrich commented May 9, 2017 •

edited

Loading

danieldietrich commented May 9, 2017 •

edited

Loading

LinkedHashMap.put(existing key) should keep order #1838 #1983

LinkedHashMap.put(existing key) should keep order #1838 #1983

Conversation

ruslansennov commented May 8, 2017

ruslansennov May 8, 2017

Choose a reason for hiding this comment

danieldietrich May 9, 2017 • edited Loading

Choose a reason for hiding this comment

codecov-io commented May 8, 2017

Codecov Report

danieldietrich left a comment

Choose a reason for hiding this comment

danieldietrich May 9, 2017

Choose a reason for hiding this comment

danieldietrich May 9, 2017

Choose a reason for hiding this comment

danieldietrich May 9, 2017 • edited Loading

Choose a reason for hiding this comment

danieldietrich May 9, 2017

Choose a reason for hiding this comment

danieldietrich commented May 9, 2017

ruslansennov commented May 9, 2017

danieldietrich commented May 9, 2017

danieldietrich commented May 9, 2017 • edited Loading

danieldietrich commented May 9, 2017 • edited Loading

danieldietrich May 9, 2017 •

edited

Loading

danieldietrich May 9, 2017 •

edited

Loading

danieldietrich commented May 9, 2017 •

edited

Loading

danieldietrich commented May 9, 2017 •

edited

Loading