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

Discuss how onComplete or onSuccess/onFailure can be used #5431

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

julianladisch
Copy link
Contributor

@julianladisch julianladisch commented Jan 3, 2025

Motivation:

Simplify the code by replacing onComplete + if-ar.succeeded-else with
either onComplete(res, e) or with other processing.

Avoiding onComplete + if-ar.succeeded-else results in more concise and understandable
code because it avoids the additional nesting of the if-else clause.

Extend the exampleFuture* code to discuss all possibilities how onComplete and
onSuccess/onFailure might be used.

Conformance:

@julianladisch julianladisch force-pushed the CoreExamples-onSuccess-onFailure branch from 0fb18e0 to 3aaa5d6 Compare January 3, 2025 22:40
Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

Thanks @julianladisch for bringing this up.

I have reservations about this one. Invoking onSuccess and onFailure at the same place implies creating two future listeners where a single one would do the trick.

If the goal is to avoid writing the if block, then there is an onComplete overload that takes two arguments, a success and a failure handler.

We should promote the usage of this method, imo.

That doesn't mean onSucess and onFailure don't have any use case (sometimes they're convenient, like when a method returning a future needs to perform a side action when the future is completed).

@julianladisch julianladisch force-pushed the CoreExamples-onSuccess-onFailure branch from 3aaa5d6 to 09fde45 Compare January 8, 2025 23:08
@julianladisch julianladisch changed the title Replace onComplete with onSuccess + onFailure Discuss how onComplete or onSuccess/onFailure can be used Jan 8, 2025
@julianladisch julianladisch force-pushed the CoreExamples-onSuccess-onFailure branch from 09fde45 to d5a5ca5 Compare January 8, 2025 23:20
Simplify the code by replacing onComplete + if-ar.succeeded-else with
either onComplete(res, e) or with other processing.

Avoiding onComplete + if-ar.succeeded-else results in more concise and understandable
code because it avoids the additional nesting of the if-else clause.

Extend the `exampleFuture*` code to discuss all possibilities how onComplete and
onSuccess/onFailure might be used.

Remove unused exampleFuture2 from lines 173-186. A new exampleFuture2 is created in line 145.
@julianladisch julianladisch force-pushed the CoreExamples-onSuccess-onFailure branch from d5a5ca5 to e9fe56f Compare January 8, 2025 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants