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

[Stephen Colebourne]

In OpenGamma Strata we have 546 uses of Optional.get(). Renaming this
would be painful and add no value.

While I understand the pain from some developers not understanding the
feature, this is hardly unique in the world of Java. Developers learn
the right way of doing something soon enough.

And while

if (opt.isPresent()) {
  opt.get()
}

is sometimes not ideal, in other cases it is the only practical choice
(eg. where the method needs to have a return statement inside the if
statement).

Changing this to

if (opt.isPresent()) {
  opt.getWhenPresent()
}

is just noise - I can see the "present" part twice.

I just don't think I can support the rename (although many of the
webrev tidy-ups are probably good).

Stephen



On 26 April 2016 at 00:05, Stuart Marks <stuart.marks at oracle.com> wrote:
> Hi all,
>
> Please review these webrevs that deprecate Optional.get() and to replace it
> with Optional.getWhenPresent(). The corresponding changes are also applied
> to OptionalDouble.getAsDouble(), OptionalInt.getAsInt(), and
> OptionalLong.getAsLong().
>
> Unlike most deprecations, this isn't about the function or the utility of
> some API, it's about the name. The solution is basically to rename the API.
> The problem is that "get" shows up as the "obvious" choice in things like
> IDE code completion, leading to code that mishandles empty Optionals.
> Typical Stack Overflow discourse runs something like this:
>
>     Q: what do I do with this Optional thing
>
>     A: just call get()
>
>     Q: thanks, it works!
>
> Of course, it works until it doesn't.
>
> Examining the JDK's use of Optional.get(), I didn't see very many cases that
> called get() without first checking for the presence of a value. But I did
> see quite a number of cases like this:
>
>     if (opt.isPresent()) {
>         doSomething(opt.get());
>     } else {
>         doSomethingElse();
>     }
>
> In many of these cases, the code could be refactored to use other Optional
> methods such as filter(), map(), or ifPresent().
>
> In any case this reinforces the contention that use of get() leads to poor
> code.
>
> For this changeset, in just about all cases I've simply replaced the call to
> get() with a call to getWhenPresent(). In a couple cases I replaced the
> stream calls
>
>     .filter(Optional::isPresent).map(Optional::get)
>
> with
>
>     .flatMap(Optional::stream)
>
> which I hope will become the new idiom for unwrapping a stream of Optionals.
>
> While many cases could be cleaned up further, I didn't change them. The
> reasons are that I didn't want to spend too much time putting code cleanup
> into the critical path of this changeset (I'd be happy to help later); doing
> so would create potential conflicts with code coming in from the Jigsaw
> forest; and there are non-obvious places where converting from a conditional
> to one of the lambda-based methods could cause performance problems at
> startup.
>
> There are also a few cases where simplification is prevented because it
> would end up causing the resulting lambda expressions to throw checked
> exceptions. :-(
>
> Webrevs here:
>
>   http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.langtools/
>
>   http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.jdk/
>
> Thanks,
>
> s'marks