RFR: 8004518 & 8010122 : Default methods on Map

[Mike Duigou]

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 <pre>
> - indentation should be 4
> - line break before }</pre>

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

> 
> Why you use both, {@code...} and <tt>...</tt> ?

legacy. We're trying to use {@code } in new work but only convert existing <tt></tt> when it's convenient. The main reason for not doing a global replacement is that we all value concise diffs and replacing <tt></tt> 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
>>  
>