Why use assignment in a comparison?

user_1534084 0
Viewed 612

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?

java
3 Answers

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

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;
}
user_15538510

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.

user_4133370
Related