Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[java] Use Optional.orElseThrow() instead of Optional.get() on Java 10+ #2719

Open
Thunderforge opened this issue Aug 21, 2020 · 0 comments
Open
Labels
a:new-rule Proposal to add a new built-in rule

Comments

@Thunderforge
Copy link
Contributor

Thunderforge commented Aug 21, 2020

Proposed Rule Name: UseOptionalOrElseThrowInsteadOfOptionalGet

Proposed Category: Best Practices

Description: In Java 10 and later, the Javadoc entry for Optional.get() includes this:

API Note:
The preferred alternative to this method is orElseThrow().

The Javadoc for Optional.orElseThrow(), introduced in Java 10, is identical to the former Javadoc, aside from the above API note, and the internal code implementation of the two methods are identical. The proposed PMD rule will tell the user that they should replace instances of .get() with .orElseThrow().

This rule should only apply to Java 10 and later. On Java 8 and 9, Optional.get() should be considered correct, as Optional.orElseThrow() is not available.

For what it's worth Brian Goetz was the person who originally chose the name .get() and considers it "our biggest API mistake in Java 8".

Methods like orElse or ifPresent look harmless, and are; methods like orElseThrow have potentially harmful side-effects but have names that are transparent about what harm they could do. But Optional.get() is neither safe nor transparent -- and yet awfully tempting-looking -- and this leads to frequent misuse. Naming matters, and this one was wrong, and the harm is observable. I wish I'd caught it before 8 shipped.

It seems the decision was made that the cost of deprecating .get() was too high at this time, so the "API Note" was the best they could do. Perhaps the method will be deprecated in the future.

As for rule priority, I think this rule is of comparable priority to rules like UseAssertEqualsInsteadOfAssertTrue, which are Medium (3), but could be a lower priority if desired.

Code Sample:

Incorrect code

public Foo run(Optional<Foo> fooOptional) {
    return fooOptional.get();
}

Correct code

public Foo (Optional<Foo> fooOptional) {
    return fooOptional.orElseThrow();
}

Possible Properties:

  • None.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:new-rule Proposal to add a new built-in rule
Projects
None yet
Development

No branches or pull requests

1 participant