-
Notifications
You must be signed in to change notification settings - Fork 41.7k
Polish #14172
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
Polish #14172
Conversation
snicoll
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @izeye. I've added a suggestion.
| @Override | ||
| public void onApplicationEvent(ApplicationEvent event) { | ||
| if (event instanceof ContextRefreshedEvent) { | ||
| if (event instanceof ContextRefreshedEvent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep the two ifs please? (and combine the two conditions for the second if). It reads better IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snicoll I thought that the two if statements doing the same thing are unusual as it looks to me as follows:
if (condition1) {
doSomething();
}
if (condition2) {
doSomething();
}
WDYT? If you still prefer as-is, please let me know. Then I'll revert it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. There are but the proposal condition is a big fat IMO. I am not really leaning one way or the other actually, let me checkout the code in an IDE.
|
Thanks again @izeye! |
This PR fixes some typos and polishes trivial stuff.