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

[Vitaly Davidovich]

On Monday, April 25, 2016, Stuart Marks <stuart.marks at oracle.com> wrote:

>
>
> On 4/25/16 4:35 PM, Jonathan Gibbons wrote:
>
>> Since the justification for this change appears to be that the IDEs might
>> help
>> write people write bad code, why not look to the IDEs to generate a
>> warning when
>> using Optional.get outside of the context of Optional.isPresent?
>>
>> In other words, is this really such a good change?
>>
>
> Well I think it's more than just IDEs. The root cause is the API name
> "get". In Java we're getting things all the time, and we think nothing of
> it. The problem is when code tries to get something that isn't there.
> Historically, callers have had to check for null, and of course it's always
> been a problem if that check is forgotten.
>
> 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?

>
> Consider this code from LogManager.java from the jdk webrev: [1]
>
> 2106                 for (String pk : properties) {
> 2107                     ConfigProperty cp = ConfigProperty.find(pk).get();
>
> OK, so we find something and get something out of it. Big deal. But with
> the change to getWhenPresent(), this code is now:
>
> 2106                 for (String pk : properties) {
> 2107                     ConfigProperty cp =
> ConfigProperty.find(pk).getWhenPresent();
>
> From this, it's clear that find() returns an Optional, and that this code
> is asserting that a value is always present in this Optional.

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.

>
> Offhand I don't know if this assertion is in fact true for this code. If
> it is true, then the name "getWhenPresent" is telling the reader an
> important fact about the relationships among the data structures this code
> uses. If it's *not* true, when this code is being reviewed (or when it's
> debugged after a failure), it ought to raise questions like, "Under what
> circumstances is this value absent?" Those are important points that aren't
> raised by a simple get().

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.

My $.02

>
> s'marks
>
>
>
> [1]
> http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.jdk/src/java.logging/share/classes/java/util/logging/LogManager.java.sdiff.html
>


-- 
Sent from my phone