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 >> >
