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

Tue Apr 26 11:34:32 UTC 2016

On Tuesday, April 26, 2016, 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

Don't get me wrong - I'm sure examples of "misuse" exist, as you've clearly
demonstrated.  I'm just not convinced that deprecation at this stage of the
game is a worthwhile endeavor.  I'm not even sure I like the more explicit
name in general given it "penalizes" correct usage by being overly verbose,
as I mentioned in my previous reply.

>
>
> 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.

Right, it may cause someone to pause for a second.  I'm just saying that
the method name itself doesn't say it's an Optional.  In fact, someone may
assume it's Optional since the name is somewhat unique-ish but in fact it
could be something else entirely with different behavior.  At least get()
is so "vague" that user ought to check at least what type the method is
invoked on.  But either way, this isn't really the thrust of my argument.

>
> The Optional API was designed to be chained, so you don't want to pull it
> into a local variable.

Sure, and that's usually good.  But chaining can have readability issues
associated with it when more than one type is involved in the chain.

>
> 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));

Maybe, if the lambda capture allocation isn't an issue for this use case.

>
> ========================================
>
>
> 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.

It's a "simple" and direct API, not sure it's bad.  The higher order APIs
are nice and should be preferred, but they may come with performance
overhead.

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

-- 
Sent from my phone