-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Best Practices PMD rules #14904
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
Best Practices PMD rules #14904
Conversation
608c6c1
to
6f0fdd3
Compare
if (metricClient != null) { | ||
throw new RuntimeException("You cannot initialize configuration more than once."); | ||
} | ||
|
||
if (configs.getMetricClient().equals(DATADOG_METRIC_CLIENT)) { | ||
if (DATADOG_METRIC_CLIENT.equals(configs.getMetricClient())) { |
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.
actually saw a bug in dev recently due to this, so this is good to fix.
LOGGER.warn("Failed to successfully notify auto-disable connection warning: {}", notification); | ||
} | ||
|
||
if (FAILURE_NOTIFICATION == action) { |
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.
looking at this again, thinking maybe I should use a default and log something like "unknown action"?
case STDERR -> LOGGER.error(message); | ||
// assumption that this is an empty frame that connotes the container exiting. | ||
case END -> LOGGER.error(service + " stopped!!!"); | ||
// default includes STDOUT and anything else | ||
default -> LOGGER.info(message); |
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.
opinions on setting this as the default?
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.
That seems fine. Worst case scenario is that we get a few extra cases logged to INFO.
case DEBUG -> logger.debug(logMessage.getMessage()); | ||
case TRACE -> logger.trace(logMessage.getMessage()); | ||
default -> logger.info(logMessage.getMessage()); |
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.
opinions on this default? (similar to above)
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.
This seems fine.
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.
Implement the following Best Practices PMD rules:
LiteralsFirstInComparisons
LooseCoupling
PreserveStackTrace
SwitchStmtsShouldHaveDefault
SystemPrintln
UseCollectionIsEmpty
UseVarargs