On 4/25/16 5:46 PM, Vitaly Davidovich wrote:
We've introduced Optional to avoid this. The problem is that the obvious
thing to do is to get() the value out of the Optional, but this buys us
right back into the what-if-the-value-is-missing case that we had with
nulls.
Why is this the "obvious" thing? Because a few stackoverflow threads went like
that?
Well, the Stack Overflow exchange was a caricature. But without looking too
hard, I found this code: [2]
return stringList.stream().filter(...).findFirst().get() != null;
[2] http://stackoverflow.com/q/36346409/1441122
The person, presumably new to Java 8, was able to determine that findFirst()
returned an Optional instead of the desired value, and was able to figure out
enough to call get(), and even thought enough about the possibly-absent case to
check it against null. But the person had to ask on Stack Overflow about how to
solve the problem.
Now I obviously don't know the exact thought process that went on, but get() has
among the shortest of name of the Optional methods, and has no arguments, so it
seems like it ought to be called more more frequently compared to more
specialized methods like ifPresentOrElse() or orElseThrow().
There are other items on Stack Overflow such as these: [3] [4]. But these
probably aren't what you're looking for. :-)
[3] http://stackoverflow.com/a/26328555/1441122
[4] http://stackoverflow.com/a/23464794/1441122
It's not clear it returns an Optional - it could be returning some other type
that happens to have a getWhenPresent. What would be clear it's an Optional is
to have a local of that type and assign find(pk) to it.
Of course there's always a possibility that there's some other API that has
getWhenPresent() on it. There isn't one in the JDK, and I've been unable to find
one elsewhere. So "getWhenPresent" should be different enough to trigger a
different thought pattern, as opposed to a plain "get" which fades into the
background.
The Optional API was designed to be chained, so you don't want to pull it into a
local variable.
Descriptive names are good, but like Jon, I'm not buying this change. Is it
really wrong to tell people to read the javadoc? And is that worth the
deprecation churn? It's going to be needlessly verbose (esp in existing code
that already checked isPresent) for no strong benefit. If someone is mindlessly
having their IDE write code for them, this isn't going to prevent poor code.
For code review purposes, if it's important to call out that something is
Optional then use the type somewhere explicitly.
Personally I'm generally skeptical of simple name changes, but after having gone
through a bunch of examples of the use of get(), my skepticism melted away. It
really does seem to be misused a significant fraction of the time, possibly even
a majority of the time.
Beginners and Java 8 learners will probably fall into the trap of forgetting to
check. But JDK programmers, who are decidedly not beginners, are writing code
that uses get() unnecessarily:
========================================
http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.langtools/src/jdk.jdeps/share/classes/com/sun/tools/jdeps/DependencyFinder.java.sdiff.html
206 if (source.isPresent()) {
207 executor.runTask(source.get(), deque);
208 }
could be rewritten as
source.ifPresent(archive -> executor.runTask(archive, deque));
========================================
http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.langtools/src/jdk.jdeps/share/classes/com/sun/tools/jdeps/JdepsTask.java.sdiff.html
476 Optional<String> req = options.requires.stream()
477 .filter(mn -> !modules.containsKey(mn))
478 .findFirst();
479 if (req.isPresent()) {
480 throw new BadArgs("err.module.not.found", req.get());
481 }
could be rewritten as
options.requires.stream()
.filter(mn -> !modules.containsKey(mn))
.findFirst()
.ifPresent(s -> throw new BadArgs("err.module.not.found", s));
========================================
http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/JShellTool.java.sdiff.html
1203 String hist = replayableHistory
1204 .subList(first + 1, replayableHistory.size())
1205 .stream()
1206 .reduce( (a, b) -> a + RECORD_SEPARATOR + b)
1207 .get();
could be rewritten as
String hist = String.join(RECORD_SEPARATOR,
replayableHistory.subList(first + 1, replayableHistory.size()));
(It's also not clear to me that the sublist is guaranteed to be nonempty, so the
first snippet might fail in get().)
========================================
http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.jdk/src/java.base/share/classes/java/lang/module/Resolver.java.sdiff.html
100 if (mref.location().isPresent())
101 trace(" (%s)", mref.location().get());
could be rewritten as
mref.location().ifPresent(loc -> trace(" (%s)", loc);
========================================
http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.jdk/src/java.base/share/classes/java/lang/reflect/Layer.java.sdiff.html
364 Optional<Configuration> oparent = cf.parent();
365 if (!oparent.isPresent() || oparent.get() != this.configuration())
{
366 throw new IllegalArgumentException(
367 "Parent of configuration != configuration of this
Layer");
could be rewritten as
cf.parent()
.filter(cfg -> cfg == this.configuration())
.orElseThrow(() -> throw new IllegalArgumentException(
"Parent of configuration != configuration of this Layer"));
========================================
If you go to grepcode.com, find the source for Optional.get() -- I used 8u40-b25
-- and then click the little arrow next to get(), you can find a bunch of uses
of Optional.get(). A startlingly large fraction of them can be simplified in a
similar manner to the JDK examples I showed above, bypassing the need to call
get(). (I did a quick check of the first ten examples, and I think about seven
could be improved, depending on how you count.)
It really does look to me like get() is a bad API. It's not people "mindlessly"
having their IDE write code. The problem is that get() is deceptively
attractive. Even people remember to check it, they put the checking around the
get(), instead of looking at Optional's other methods. This is what results in
the needlessly verbose code.
A minority of the cases are justified in using get(). If this is renamed to
getWhenPresent(), it becomes slightly more verbose, but it also specifically
states that the programmer is making an assertion about the presence of a value
-- a worthwhile improvement.
Is it worth the churn? Well, the problem is only going to get worse. I believe
that if we don't deprecate get() now, a few years down the road, the problem
will be so bad, we'll say "I wish we had deprecated it back in JDK 9 when we
were talking about it then." Just like right now we're saying, "I wish we had
fixed it before JDK 8 shipped."
s'marks
