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

Improve readability of method "isRetryException" #1057

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

Conversation

stefanbirkner
Copy link

The "isRetryExhausted" check only depends on the throwable itself. The
new code makes this more clear because the check is no longer part of
anyMatch's lambda expression. I also extracted the method
"isInstanceOfRetryType" because it makes it obvious that we do the same
check on the throwable itself and its cause.

The "isRetryExhausted" check only depends on the throwable itself. The
new code makes this more clear because the check is no longer part of
anyMatch's lambda expression. I also extracted the method
"isInstanceOfRetryType" because it makes it obvious that we do the same
check on the throwable itself and its cause.
}

private boolean isInstanceOfRetryType(Throwable throwable) {
return typesThatTriggerRetry.stream().anyMatch(type -> type.isInstance(throwable));
Copy link
Author

Choose a reason for hiding this comment

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

I would like to replace it with

Suggested change
return typesThatTriggerRetry.stream().anyMatch(type -> type.isInstance(throwable));
return throwable instanceof IOException
|| throwable instanceof TimeoutException
|| throwable instanceof RetryableStatusCodeException;

because it is simpler and it moves the information about the types close to where they are used. For me that makes the code easier to read. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not stream twice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants