RFR(m): 8140281 deprecate Optional.get()

[Stanislav Baiduzhyi]

Stuart,

Those examples are amazing. Would it be possible to expand Optional
class javadoc with some of those examples? First, second, and the last
one are the most descriptive in my opinion. I frowned at Optional for
so long, you just showed me that I was not using it properly.

On 26 April 2016 at 08:06, Stuart Marks <stuart.marks at oracle.com> wrote:
>
>
> 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