-
Notifications
You must be signed in to change notification settings - Fork 27.4k
Refactor: Fix code smells detected by PMD analysis #2135
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
base: main
Are you sure you want to change the base?
Conversation
81b2a8e to
d655a31
Compare
|
I think another PR was merged that conflicts with this one. You probably need to rebase and force push. |
Thanks for the heads up! Conflicts are resolved and the branch is up to date. |
|
Github still says "This branch cannot be rebased due to conflicts". Can you rebase, squash and force push please? There's also a build failure that you should be able to see locally (just try to |
3ec82de to
e4756cc
Compare
Done! I ran the Maven formatter to fix the build errors and squashed/rebased the commits as requested. |
src/test/java/org/springframework/samples/petclinic/PostgresIntegrationTests.java
Show resolved
Hide resolved
Fix ControlStatementBraces, GuardLogStatement, DoubleBraceInitialization and UseUtilityClass patterns. Signed-off-by: AleGTorres <[email protected]>
09a13a7 to
caf93b6
Compare
| static MySQLContainer container() { | ||
| return new MySQLContainer(DockerImageName.parse("mysql:9.5")); | ||
| static MySQLContainer<?> container() { | ||
| return new MySQLContainer<>(DockerImageName.parse("mysql:9.5")); |
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.
Error: reason: cannot use '<>' with non-generic class org.testcontainers.mysql.MySQLContainer
Oops
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.
Some of the changes aren't useful IMO. The addition of missing braces is the best part of it.
| @ImportRuntimeHints(PetClinicRuntimeHints.class) | ||
| public class PetClinicApplication { | ||
|
|
||
| private PetClinicApplication() { |
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 is a false positive and need to be reverted. The application is instantiated, it's a configuration class.
| @Configuration | ||
| public class MysqlTestApplication { | ||
|
|
||
| private MysqlTestApplication() { |
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.
Same this should be reverted.
| public void printProperties() { | ||
| for (EnumerablePropertySource<?> source : findPropertiesPropertySources()) { | ||
| log.info("PropertySource: " + source.getName()); | ||
| if (log.isInfoEnabled()) { |
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.
There's no need for such optimization for an info log
| else { | ||
| log.info(name + "=" + value + " OVERRIDDEN to " + resolved); | ||
|
|
||
| if (log.isInfoEnabled()) { |
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.
Same, please revert.
Hello! I ran a PMD analysis on the project and spotted a few code smells that could be cleaned up.
I've fixed 4 specific patterns to improve code consistency and performance:
if/elseblocks (e.g., inPetController.java) to prevent scope errors.isInfoEnabled()checks to avoid unnecessary string processing.I hope these contributions are helpful to the project. Thank you!