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

Rewrite Optional.ofNullable(A).orElse(B) to requireNonNullElse(A, B) #364

Closed
1 of 3 tasks
EnricSala opened this issue Nov 21, 2022 · 3 comments
Closed
1 of 3 tasks
Labels
Milestone

Comments

@EnricSala
Copy link

EnricSala commented Nov 21, 2022

Problem

Sometimes a nullable variable can be wrapped in an Optional simply to perform a null-or-else check.

In such cases it's simpler to use requireNonNullElse or requireNonNullElseGet from java.util.Objects.

Description of the proposed new feature

  • Support a stylistic preference.
  • Avoid a common gotcha, or potential problem.

I would like to rewrite the following code:

Optional.ofNullable(A).orElse(B);
Optional.ofNullable(A).orElseGet(() -> getB());

to:

requireNonNullElse(A, B);
requireNonNullElseGet(A, () -> getB());

Considerations

Could also consider the case Optional.of(A).orElse(B) which can be rewritten as just A, and similar for orElseGet.
EDIT: actually, Optional.of would already throw an exception if A was null, while replacing with just A could propagate the null further, so it should be replaced with requireNonNull(A).

Participation

  • I am willing to submit a pull request to implement this improvement.
@Stephan202
Copy link
Member

Nice one. The first Optional.ofNullable case can be added to this existing rule, provided that the Javadoc is tweaked a bit. Note that the same caveat around B possibly being null applies. (We might be able to partially mitigate this by applying a custom @Matches annotation 🤔.)

@Stephan202
Copy link
Member

NB: I see I had this same rule in my private TODO list, followed by:

  • requireNonNullElse(map.get(k), defaultValue) -> map.getOrDefault(k, defaultValue)

@Stephan202
Copy link
Member

Stephan202 commented Nov 22, 2022

Another observation: the Optional.of rule has a reactive analog; from my TODO list:

  • Mono.just(elem).defaultIfEmpty(elem2) -> Mono.just(elem)

Edit: though we may want to use a BugChecker to generalize this to any reactive expression that is known to be non-empty, such as flux.collect(..).

Edit 2: ah, but we already have NonEmptyMono for that. My local TODO list is outdated ;)

Edit 3: ah, but we documented that this specific case isn't covered yet 🙃,

@rickie rickie added the good first issue Good for newcomers label Dec 6, 2022
benhalasi added a commit to benhalasi/error-prone-support that referenced this issue Dec 27, 2022
…ireNonNullElseGet(A, () -> B)` over Optional alternative.

Added Rules: (NullRules)
 - prefer `requireNonNullElse(A, B)` over `Optional.ofNullable(A).orElse(B)`
 - prefer `requireNonNullElseGet(A, () -> B)` over `Optional.ofNullable(A).orElseGet(() -> B)`

Related issue: PicnicSupermarket#364
Stephan202 pushed a commit to benhalasi/error-prone-support that referenced this issue Dec 28, 2022
…ireNonNullElseGet(A, () -> B)` over Optional alternative.

Added Rules: (NullRules)
 - prefer `requireNonNullElse(A, B)` over `Optional.ofNullable(A).orElse(B)`
 - prefer `requireNonNullElseGet(A, () -> B)` over `Optional.ofNullable(A).orElseGet(() -> B)`

Related issue: PicnicSupermarket#364
rickie pushed a commit to benhalasi/error-prone-support that referenced this issue Dec 29, 2022
…ireNonNullElseGet(A, () -> B)` over Optional alternative.

Added Rules: (NullRules)
 - prefer `requireNonNullElse(A, B)` over `Optional.ofNullable(A).orElse(B)`
 - prefer `requireNonNullElseGet(A, () -> B)` over `Optional.ofNullable(A).orElseGet(() -> B)`

Related issue: PicnicSupermarket#364
rickie pushed a commit to benhalasi/error-prone-support that referenced this issue Dec 30, 2022
…ireNonNullElseGet(A, () -> B)` over Optional alternative.

Added Rules: (NullRules)
 - prefer `requireNonNullElse(A, B)` over `Optional.ofNullable(A).orElse(B)`
 - prefer `requireNonNullElseGet(A, () -> B)` over `Optional.ofNullable(A).orElseGet(() -> B)`

Related issue: PicnicSupermarket#364
rickie pushed a commit to benhalasi/error-prone-support that referenced this issue Jan 2, 2023
…ireNonNullElseGet(A, () -> B)` over Optional alternative.

Added Rules: (NullRules)
 - prefer `requireNonNullElse(A, B)` over `Optional.ofNullable(A).orElse(B)`
 - prefer `requireNonNullElseGet(A, () -> B)` over `Optional.ofNullable(A).orElseGet(() -> B)`

Related issue: PicnicSupermarket#364
@rickie rickie closed this as completed in d456821 Jan 2, 2023
@Stephan202 Stephan202 added this to the 0.7.0 milestone Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants