-
Notifications
You must be signed in to change notification settings - Fork 135
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
Code clean up in Job #4462
Code clean up in Job #4462
Conversation
* Make logger final * Reduce visibility of public constructor * Use built in logger formatting * Remove unnecessary override of run() * Remove unnecessary Boolean literal * Combine identical catch clauses * Rearrange logic in build methods for readability A separate change will replace the synchronized HashTable instances
Kudos, SonarCloud Quality Gate passed! |
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.
LGTM
@@ -110,6 +109,7 @@ public boolean isEnabled() { | |||
try { | |||
enabled = mConfig.isEnabled(); | |||
} catch (EBaseException e) { | |||
// Swallow exceptions? |
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.
Ideally we should not do this, but this is happening all over the place. Feel free to fix it whenever you see it, but just beware that it might change the behavior of the code.
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.
Yeah I try not to alter the control/exception flow when I am wearing my janitor uniform, but I often document that I have not touched things as I go.
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.
LGTM
A separate change will replace the synchronized HashTable instances