Skip to content

Leverage Optional.orElseThrow to increase code readability#13941

Merged
findepi merged 2 commits intotrinodb:masterfrom
findepi:findepi/leverage-optional-orelsethrow-to-increase-code-readability-37e6d9
Sep 5, 2022
Merged

Leverage Optional.orElseThrow to increase code readability#13941
findepi merged 2 commits intotrinodb:masterfrom
findepi:findepi/leverage-optional-orelsethrow-to-increase-code-readability-37e6d9

Conversation

@findepi
Copy link
Copy Markdown
Member

@findepi findepi commented Aug 31, 2022

Use of Optional.orElseThrow allows to verify the optional is not empty
before assigning to a variable. This subsequently removes need for
Optional.get() calls.


please ignore this section

  • a checkbox
  • another checkbox

@findepi findepi added the no-release-notes This pull request does not require release notes entry label Aug 31, 2022
@cla-bot cla-bot bot added the cla-signed label Aug 31, 2022
@findepi findepi force-pushed the findepi/leverage-optional-orelsethrow-to-increase-code-readability-37e6d9 branch from 23f9b77 to e181918 Compare August 31, 2022 13:35
Copy link
Copy Markdown
Member

@ebyhr ebyhr Aug 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this change unrelates to Optional.orElseThrow leverage. Is separated a commit overkill?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's related.

The pattern

Optional<...> x = expr;
if (x.isEmpty()) {
  throw;
}

should generally be avoided and you should use orElseThrow.
You should not, however, use orElseThrow where the variable would become unused. I.e. you shouldn't use it, if your goal is to check existence of some optional value. In such cases, i inlined the expression

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Sep 1, 2022

How much of this is manual changes vs how much is refactorings? Separate commits would be easier to review.

Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me however.

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Sep 2, 2022

How much of this is manual changes vs how much is refactorings? Separate commits would be easier to review.

Manual refactorings.

@findepi findepi force-pushed the findepi/leverage-optional-orelsethrow-to-increase-code-readability-37e6d9 branch from 42258bd to 5da2233 Compare September 2, 2022 15:33
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Sep 2, 2022

(just rebased)

Use of `Optional.orElseThrow` allows to verify the optional is not empty
before assigning to a variable. This subsequently removes need for
`Optional.get()` calls.
@findepi findepi force-pushed the findepi/leverage-optional-orelsethrow-to-increase-code-readability-37e6d9 branch from 5da2233 to 3e54f59 Compare September 5, 2022 10:15
@findepi findepi force-pushed the findepi/leverage-optional-orelsethrow-to-increase-code-readability-37e6d9 branch from 3e54f59 to 604c4e3 Compare September 5, 2022 10:16
@findepi findepi merged commit d21cb3b into trinodb:master Sep 5, 2022
@findepi findepi deleted the findepi/leverage-optional-orelsethrow-to-increase-code-readability-37e6d9 branch September 5, 2022 14:20
@github-actions github-actions bot added this to the 395 milestone Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

3 participants