mjhjmtu

When reading the source code, I stumbled upon this method in the JDK sources. Please note the declaration and initialization of v and newValue. We have here 'nice' undefined values, assignment in comparisons, which is 'great', and extra brackets for worse readability. And other code smells.

default V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) 
 Objects.requireNonNull(mappingFunction);
 V v;
 if ((v = get(key)) == null) 
 V newValue;
 if ((newValue = mappingFunction.apply(key)) != null) 
 put(key, newValue);
 return newValue;
 
 

 return v;

But why? Is there any actual benefit to writing code the above way instead of the simple (ideally with negated v comparison):

default V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) 
 Objects.requireNonNull(mappingFunction);
 V v = get(key);
 if (v == null) 
 V newValue = mappingFunction.apply(key);
 if (newValue != null) 
 put(key, newValue);
 return newValue;
 
 

 return v;

Is there any actual benefit I'm not aware of (besides showing off Java constructs), rather than going with the 'easy' way?

#microoptimization (but in case of a standard library it could matter), and:

#inertia: this pattern was common among C programmers back in the 90-ies, so the titans of computer science may still use this style.

There's no point to write such code for new business logic, unless performance is really critical.

The (micro)optimization:

The bytecode produced by javac (JDK 11) for the original ("bad") version is one JVM-operation less than the (nicer) code. Why? The JDK's version "uses" the return value of the assignment operator (rather than loading the value from a variable) for the if condition evaluation.

However, this is more a limitation of javac's optimization possibilities than a reason to write the less-readable code.

Here's the bytecode for the JDK version, cited in the question:

 0: aload_2
 1: invokestatic #2 // Method java/util/Objects.requireNonNull:(Ljava/lang/Object;)Ljava/lang/Object;
 4: pop
 5: aload_0
 6: aload_1
 7: invokevirtual #3 // Method get:(Ljava/lang/Object;)Ljava/lang/Object;
 10: dup
 11: astore_3
 12: ifnonnull 39
 15: aload_2
 16: aload_1
 17: invokeinterface #4, 2 // InterfaceMethod java/util/function/Function.apply:(Ljava/lang/Object;)Ljava/lang/Object;
 22: dup
 23: astore 4
 25: ifnull 39
 28: aload_0
 29: aload_1
 30: aload 4
 32: invokevirtual #5 // Method put:(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
 35: pop
 36: aload 4
 38: areturn
 39: aload_3
 40: areturn

Below is the bytecode of a more readable version:

public V computeIfAbsent(K key,
 Function<? super K, ? extends V> mappingFunction) 
 Objects.requireNonNull(mappingFunction);
 final V v = get(key);
 if (v == null) 
 final V newValue = mappingFunction.apply(key);
 if (newValue != null) 
 put(key, newValue);
 return newValue;
 
 

 return v;

.. and the bytecode is:

 0: aload_2
 1: invokestatic #2 // Method java/util/Objects.requireNonNull:(Ljava/lang/Object;)Ljava/lang/Object;
 4: pop
 5: aload_0
 6: aload_1
 7: invokevirtual #3 // Method get:(Ljava/lang/Object;)Ljava/lang/Object;
 10: astore_3
 11: aload_3
 12: ifnonnull 40
 15: aload_2
 16: aload_1
 17: invokeinterface #4, 2 // InterfaceMethod java/util/function/Function.apply:(Ljava/lang/Object;)Ljava/lang/Object;
 22: astore 4
 24: aload 4
 26: ifnull 40
 29: aload_0
 30: aload_1
 31: aload 4
 33: invokevirtual #5 // Method put:(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
 36: pop
 37: aload 4
 39: areturn
 40: aload_3
 41: areturn

I think it's mostly a matter of preference; I'm under the impression Doug Lea prefers this terse style. But if you look at the original version of the method, it makes a little more sense, since it's paired with newValue which does need inline assignment:

default V computeIfAbsent(K key,
 Function<? super K, ? extends V> mappingFunction) 
 V v, newValue;
 return ((v = get(key)) == null &&
 (newValue = mappingFunction.apply(key)) != null &&
 (v = putIfAbsent(key, newValue)) == null) ? newValue : v;

I agree: it's a code smell and I definitely prefer the second version.

The arcane use of assignment and comparison in a single expression can result in somewhat shorter code in case you have a loop, e.g.:

V v;
while ((v = getNext()) != null) 
 // ... do something with v ...

To remove the code smell, you need more code and you need to assign the variable in two places:

V v = getNext();
while (v != null) 
 // ...
 v = getNext();

Or you need to move the loop exit condition after the assignment:

while (true) 
 V v = getNext();
 if (v == null)
 break;
 // ...

For an if statement, it certainly makes no sense. And even for a loop, I would avoid it.