RFR: 8004518 & 8010122 : Default methods on Map (original) (raw)

Mike Duigou mike.duigou at oracle.com
Thu Apr 11 03:20:14 UTC 2013

On Apr 8 2013, at 17:08 , Ulf Zibis wrote:

Hi Mike,

Comments for j.u.Map: To my savour the variant belongs to the left hand side of a comparison, e.g.: if (v = get(key) != null)

Yeah, you caught me. Almost everyone hates these "Yoda conditions" (http://wiert.me/2010/05/25/yoda-conditions-from-stackoverflow-new-programming-jargon-you-coined/). I am unwilling and probably unable to change my personal use of them but will use the "normal" form whenever anyone objects. Incidentally the last time I introduced an "unintentional assignment" bug was fixing one of these....

Instead 501 return (null != (v = get(key))) 502 ? v 503 : containsKey(key) ? null : defaultValue; I would code 501 return ((v = get(key) != null) || containsKey(key)) ? v : defaultValue;

done.

Use consistent formatted code examples in javadoc. I like the style from lines 558 to 561, but e.g. from line 601 to 605: - 2 spaces before 


- indentation should be 4
- line break before }






Things got a little sloppy while they were changing frequently. I will cleanup.






Why you use both, {@code...} and ... ?






legacy. We're trying to use {@code } in new work but only convert existing  when it's convenient. The main reason for not doing a global replacement is that we all value concise diffs and replacing  everywhere would generate significant noise.




For performance reasons, I think you should reverse the order of the if expressions here:
673         if (!Objects.equals(get(key), value) || !containsKey(key))
... because otherwise map lookup too often becomes executed twice, via contains() + get().




Yes, this is clearly better.




Not negate the comparison term, e.g.:
1053                 if (value == null)
1054                     return null;
1055                 if ((oldValue = putIfAbsent(key, value)) == null)
1056                     return value;




Done.








-Ulf






Am 08.04.2013 20:07, schrieb Mike Duigou:
Hello all;




This is a combined review for the new default methods on the java.util.Map interface being added for the JSR-335 lambda libraries. The reviews are being combined because they share a common unit test.
http://cr.openjdk.java.net/~mduigou/JDK-8010122/0/webrev/
8004518: Add in-place operations to Map
forEach()
replaceAll()
8010122: Add atomic operations to Map
getOrDefault()
putIfAbsent()          *
remove(K, V)
replace(K, V)
replace(K, V, V)
compute()              *
merge()                *
computeIfAbsent()      *
computeIfPresent()     *
The * operations treat null values as being absent. (ie. the same as there being no mapping for the specified key).
The default implementations provided in Map are overridden in HashMap for performance purposes, in Hashtable for atomicity and performance purposes and in Collections for atomicity.
Mike















More information about the core-libs-dev mailing list