Mixing
Why use assignment in a comparison?
Clash Royale CLAN TAG#URR8PPP
up vote
7
down vote
favorite
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
edited 20 mins ago
Boann
36.1k1286118
asked 3 hours ago
user1534084
868
 |Â
show 3 more comments
up vote
7
down vote
favorite
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
edited 20 mins ago
Boann
36.1k1286118
asked 3 hours ago
user1534084
868
3
Ifvwasn't local it could prevent race conditions wherevis changed between setting and reading. But here the only advantage seems to scare off people from touching the code :-)
– Michael Butscher
3 hours ago
1
This is fromConcurrentMap, right? You might want to link to the exact source and version to make this clear. I've seen this sort of thing in many JDK classes; it's probably idiomatic to the author but doesn't really obey strict style guides. Anyways, a relevant question.
– Mick Mnemonic
3 hours ago
5
Purely difference of opinion when it comes to style.
– Joe C
3 hours ago
2
It's just a peculiar code style (Probably Doug Lea's? It would break his own conventions: "Avoid assignments (``='') inside if and while conditions. "). Would not recommend this in your own code for exactly the reasons you outlined.
– Petr JaneÄÂek
3 hours ago
Mick Mnemonic: it's just in Map. java.util.Map#computeIfAbsent I do like more readable code, indeed, but I don't think JDK authors are out of their minds, thus I was asking, why they opted for seemingly suboptimal expression.
– user1534084
3 hours ago
 |Â
show 3 more comments
up vote
7
down vote
favorite
up vote
7
down vote
favorite
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
edited 20 mins ago
Boann
36.1k1286118
asked 3 hours ago
user1534084
868
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
java
edited 20 mins ago
Boann
36.1k1286118
asked 3 hours ago
user1534084
868
edited 20 mins ago
Boann
36.1k1286118
asked 3 hours ago
user1534084
868
edited 20 mins ago
Boann
36.1k1286118
edited 20 mins ago
Boann
36.1k1286118
edited 20 mins ago
Boann
36.1k1286118
36.1k1286118
asked 3 hours ago
user1534084
868
asked 3 hours ago
user1534084
868
asked 3 hours ago
user1534084
868
868
3
Ifvwasn't local it could prevent race conditions wherevis changed between setting and reading. But here the only advantage seems to scare off people from touching the code :-)
– Michael Butscher
3 hours ago
1
This is fromConcurrentMap, right? You might want to link to the exact source and version to make this clear. I've seen this sort of thing in many JDK classes; it's probably idiomatic to the author but doesn't really obey strict style guides. Anyways, a relevant question.
– Mick Mnemonic
3 hours ago
5
Purely difference of opinion when it comes to style.
– Joe C
3 hours ago
2
It's just a peculiar code style (Probably Doug Lea's? It would break his own conventions: "Avoid assignments (``='') inside if and while conditions. "). Would not recommend this in your own code for exactly the reasons you outlined.
– Petr JaneÄÂek
3 hours ago
Mick Mnemonic: it's just in Map. java.util.Map#computeIfAbsent I do like more readable code, indeed, but I don't think JDK authors are out of their minds, thus I was asking, why they opted for seemingly suboptimal expression.
– user1534084
3 hours ago
 |Â
show 3 more comments
3
Ifvwasn't local it could prevent race conditions wherevis changed between setting and reading. But here the only advantage seems to scare off people from touching the code :-)
– Michael Butscher
3 hours ago
1
This is fromConcurrentMap, right? You might want to link to the exact source and version to make this clear. I've seen this sort of thing in many JDK classes; it's probably idiomatic to the author but doesn't really obey strict style guides. Anyways, a relevant question.
– Mick Mnemonic
3 hours ago
5
Purely difference of opinion when it comes to style.
– Joe C
3 hours ago
2
It's just a peculiar code style (Probably Doug Lea's? It would break his own conventions: "Avoid assignments (``='') inside if and while conditions. "). Would not recommend this in your own code for exactly the reasons you outlined.
– Petr JaneÄÂek
3 hours ago
Mick Mnemonic: it's just in Map. java.util.Map#computeIfAbsent I do like more readable code, indeed, but I don't think JDK authors are out of their minds, thus I was asking, why they opted for seemingly suboptimal expression.
– user1534084
3 hours ago
3
3
If
v wasn't local it could prevent race conditions where v is changed between setting and reading. But here the only advantage seems to scare off people from touching the code :-)– Michael Butscher
3 hours ago
If
v wasn't local it could prevent race conditions where v is changed between setting and reading. But here the only advantage seems to scare off people from touching the code :-)– Michael Butscher
3 hours ago
1
1
This is from
ConcurrentMap, right? You might want to link to the exact source and version to make this clear. I've seen this sort of thing in many JDK classes; it's probably idiomatic to the author but doesn't really obey strict style guides. Anyways, a relevant question.– Mick Mnemonic
3 hours ago
This is from
ConcurrentMap, right? You might want to link to the exact source and version to make this clear. I've seen this sort of thing in many JDK classes; it's probably idiomatic to the author but doesn't really obey strict style guides. Anyways, a relevant question.– Mick Mnemonic
3 hours ago
5
5
Purely difference of opinion when it comes to style.
– Joe C
3 hours ago
Purely difference of opinion when it comes to style.
– Joe C
3 hours ago
2
2
It's just a peculiar code style (Probably Doug Lea's? It would break his own conventions: "Avoid assignments (``='') inside if and while conditions. "). Would not recommend this in your own code for exactly the reasons you outlined.
– Petr JaneÄÂek
3 hours ago
It's just a peculiar code style (Probably Doug Lea's? It would break his own conventions: "Avoid assignments (``='') inside if and while conditions. "). Would not recommend this in your own code for exactly the reasons you outlined.
– Petr JaneÄÂek
3 hours ago
Mick Mnemonic: it's just in Map. java.util.Map#computeIfAbsent I do like more readable code, indeed, but I don't think JDK authors are out of their minds, thus I was asking, why they opted for seemingly suboptimal expression.
– user1534084
3 hours ago
Mick Mnemonic: it's just in Map. java.util.Map#computeIfAbsent I do like more readable code, indeed, but I don't think JDK authors are out of their minds, thus I was asking, why they opted for seemingly suboptimal expression.
– user1534084
3 hours ago
 |Â
show 3 more comments
3 Answers
3
active
oldest
votes
up vote
5
down vote
accepted
#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
answered 3 hours ago
Alex Shesterov
14.2k83965
I believe this is the correct answer. I presume, that JDK devels cannot drive, well if anythihg at least shouldn't drive their work based on own preferences, but based on measured performance. And core libraries like collections has to be fast. Thus microoptimization makes sense here. My eyes still hurt a little from this code, but that's just my habbit what I'm used to look at. So again, I believe this is the correct answer. Thanks for helping me understand it!
– user1534084
2 hours ago
While this might be an optimization, I think shmosel's answer makes more sense. Only the author knows the real reason.
– Mick Mnemonic
2 hours ago
"this is more a limitation of javac's optimization possibilities than a reason to write the less-readable code" - isn't optimiser limitations a possible reason to write less readable code?
– Dukeling
1 hour ago
add a comment |Â
up vote
5
down vote
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;
answered 3 hours ago
shmosel
34.8k43689
add a comment |Â
up vote
4
down vote
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.
answered 3 hours ago
Codo
49.1k11109146
2
I wouldn't call it arcane by any means. Ubiquitous but not recommended perhaps?
– Mad Physicist
3 hours ago
It very much reminds me of old C code likeif (t = p->next)...which additionally depends on the implicit conversion of a non-null pointer to true / null pointer to false. But you're right: arcane is not the right word.
– Codo
3 hours ago
add a comment |Â
3 Answers
3
active
oldest
votes
3 Answers
3
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
5
down vote
accepted
#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
answered 3 hours ago
Alex Shesterov
14.2k83965
I believe this is the correct answer. I presume, that JDK devels cannot drive, well if anythihg at least shouldn't drive their work based on own preferences, but based on measured performance. And core libraries like collections has to be fast. Thus microoptimization makes sense here. My eyes still hurt a little from this code, but that's just my habbit what I'm used to look at. So again, I believe this is the correct answer. Thanks for helping me understand it!
– user1534084
2 hours ago
While this might be an optimization, I think shmosel's answer makes more sense. Only the author knows the real reason.
– Mick Mnemonic
2 hours ago
"this is more a limitation of javac's optimization possibilities than a reason to write the less-readable code" - isn't optimiser limitations a possible reason to write less readable code?
– Dukeling
1 hour ago
add a comment |Â
up vote
5
down vote
accepted
#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
answered 3 hours ago
Alex Shesterov
14.2k83965
I believe this is the correct answer. I presume, that JDK devels cannot drive, well if anythihg at least shouldn't drive their work based on own preferences, but based on measured performance. And core libraries like collections has to be fast. Thus microoptimization makes sense here. My eyes still hurt a little from this code, but that's just my habbit what I'm used to look at. So again, I believe this is the correct answer. Thanks for helping me understand it!
– user1534084
2 hours ago
While this might be an optimization, I think shmosel's answer makes more sense. Only the author knows the real reason.
– Mick Mnemonic
2 hours ago
"this is more a limitation of javac's optimization possibilities than a reason to write the less-readable code" - isn't optimiser limitations a possible reason to write less readable code?
– Dukeling
1 hour ago
add a comment |Â
up vote
5
down vote
accepted
up vote
5
down vote
accepted
#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
answered 3 hours ago
Alex Shesterov
14.2k83965
#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
answered 3 hours ago
Alex Shesterov
14.2k83965
edited 2 hours ago
answered 3 hours ago
Alex Shesterov
14.2k83965
answered 3 hours ago
Alex Shesterov
14.2k83965
answered 3 hours ago
Alex Shesterov
14.2k83965
14.2k83965
I believe this is the correct answer. I presume, that JDK devels cannot drive, well if anythihg at least shouldn't drive their work based on own preferences, but based on measured performance. And core libraries like collections has to be fast. Thus microoptimization makes sense here. My eyes still hurt a little from this code, but that's just my habbit what I'm used to look at. So again, I believe this is the correct answer. Thanks for helping me understand it!
– user1534084
2 hours ago
While this might be an optimization, I think shmosel's answer makes more sense. Only the author knows the real reason.
– Mick Mnemonic
2 hours ago
"this is more a limitation of javac's optimization possibilities than a reason to write the less-readable code" - isn't optimiser limitations a possible reason to write less readable code?
– Dukeling
1 hour ago
add a comment |Â
I believe this is the correct answer. I presume, that JDK devels cannot drive, well if anythihg at least shouldn't drive their work based on own preferences, but based on measured performance. And core libraries like collections has to be fast. Thus microoptimization makes sense here. My eyes still hurt a little from this code, but that's just my habbit what I'm used to look at. So again, I believe this is the correct answer. Thanks for helping me understand it!
– user1534084
2 hours ago
While this might be an optimization, I think shmosel's answer makes more sense. Only the author knows the real reason.
– Mick Mnemonic
2 hours ago
"this is more a limitation of javac's optimization possibilities than a reason to write the less-readable code" - isn't optimiser limitations a possible reason to write less readable code?
– Dukeling
1 hour ago
I believe this is the correct answer. I presume, that JDK devels cannot drive, well if anythihg at least shouldn't drive their work based on own preferences, but based on measured performance. And core libraries like collections has to be fast. Thus microoptimization makes sense here. My eyes still hurt a little from this code, but that's just my habbit what I'm used to look at. So again, I believe this is the correct answer. Thanks for helping me understand it!
– user1534084
2 hours ago
I believe this is the correct answer. I presume, that JDK devels cannot drive, well if anythihg at least shouldn't drive their work based on own preferences, but based on measured performance. And core libraries like collections has to be fast. Thus microoptimization makes sense here. My eyes still hurt a little from this code, but that's just my habbit what I'm used to look at. So again, I believe this is the correct answer. Thanks for helping me understand it!
– user1534084
2 hours ago
While this might be an optimization, I think shmosel's answer makes more sense. Only the author knows the real reason.
– Mick Mnemonic
2 hours ago
While this might be an optimization, I think shmosel's answer makes more sense. Only the author knows the real reason.
– Mick Mnemonic
2 hours ago
"this is more a limitation of javac's optimization possibilities than a reason to write the less-readable code" - isn't optimiser limitations a possible reason to write less readable code?
– Dukeling
1 hour ago
"this is more a limitation of javac's optimization possibilities than a reason to write the less-readable code" - isn't optimiser limitations a possible reason to write less readable code?
– Dukeling
1 hour ago
add a comment |Â
up vote
5
down vote
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;
answered 3 hours ago
shmosel
34.8k43689
add a comment |Â
up vote
5
down vote
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;
answered 3 hours ago
shmosel
34.8k43689
add a comment |Â
up vote
5
down vote
up vote
5
down vote
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;
answered 3 hours ago
shmosel
34.8k43689
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;
answered 3 hours ago
shmosel
34.8k43689
answered 3 hours ago
shmosel
34.8k43689
answered 3 hours ago
shmosel
34.8k43689
answered 3 hours ago
shmosel
34.8k43689
34.8k43689
add a comment |Â
add a comment |Â
up vote
4
down vote
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.
answered 3 hours ago
Codo
49.1k11109146
2
I wouldn't call it arcane by any means. Ubiquitous but not recommended perhaps?
– Mad Physicist
3 hours ago
It very much reminds me of old C code likeif (t = p->next)...which additionally depends on the implicit conversion of a non-null pointer to true / null pointer to false. But you're right: arcane is not the right word.
– Codo
3 hours ago
add a comment |Â
up vote
4
down vote
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.
answered 3 hours ago
Codo
49.1k11109146
2
I wouldn't call it arcane by any means. Ubiquitous but not recommended perhaps?
– Mad Physicist
3 hours ago
It very much reminds me of old C code likeif (t = p->next)...which additionally depends on the implicit conversion of a non-null pointer to true / null pointer to false. But you're right: arcane is not the right word.
– Codo
3 hours ago
add a comment |Â
up vote
4
down vote
up vote
4
down vote
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.
answered 3 hours ago
Codo
49.1k11109146
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.
answered 3 hours ago
Codo
49.1k11109146
answered 3 hours ago
Codo
49.1k11109146
answered 3 hours ago
Codo
49.1k11109146
answered 3 hours ago
Codo
49.1k11109146
49.1k11109146
2
I wouldn't call it arcane by any means. Ubiquitous but not recommended perhaps?
– Mad Physicist
3 hours ago
It very much reminds me of old C code likeif (t = p->next)...which additionally depends on the implicit conversion of a non-null pointer to true / null pointer to false. But you're right: arcane is not the right word.
– Codo
3 hours ago
add a comment |Â
2
I wouldn't call it arcane by any means. Ubiquitous but not recommended perhaps?
– Mad Physicist
3 hours ago
It very much reminds me of old C code likeif (t = p->next)...which additionally depends on the implicit conversion of a non-null pointer to true / null pointer to false. But you're right: arcane is not the right word.
– Codo
3 hours ago
2
2
I wouldn't call it arcane by any means. Ubiquitous but not recommended perhaps?
– Mad Physicist
3 hours ago
I wouldn't call it arcane by any means. Ubiquitous but not recommended perhaps?
– Mad Physicist
3 hours ago
It very much reminds me of old C code like
if (t = p->next)... which additionally depends on the implicit conversion of a non-null pointer to true / null pointer to false. But you're right: arcane is not the right word.– Codo
3 hours ago
It very much reminds me of old C code like
if (t = p->next)... which additionally depends on the implicit conversion of a non-null pointer to true / null pointer to false. But you're right: arcane is not the right word.– Codo
3 hours ago
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f53016055%2fwhy-use-assignment-in-a-comparison%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
3
If
vwasn't local it could prevent race conditions wherevis changed between setting and reading. But here the only advantage seems to scare off people from touching the code :-)– Michael Butscher
3 hours ago
1
This is from
ConcurrentMap, right? You might want to link to the exact source and version to make this clear. I've seen this sort of thing in many JDK classes; it's probably idiomatic to the author but doesn't really obey strict style guides. Anyways, a relevant question.– Mick Mnemonic
3 hours ago
5
Purely difference of opinion when it comes to style.
– Joe C
3 hours ago
2
It's just a peculiar code style (Probably Doug Lea's? It would break his own conventions: "Avoid assignments (``='') inside if and while conditions. "). Would not recommend this in your own code for exactly the reasons you outlined.
– Petr JaneÄÂek
3 hours ago
Mick Mnemonic: it's just in Map. java.util.Map#computeIfAbsent I do like more readable code, indeed, but I don't think JDK authors are out of their minds, thus I was asking, why they opted for seemingly suboptimal expression.
– user1534084
3 hours ago