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.

Sign up for GitHub

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

Jump to bottom

Sonar code smell fixes in web package #4512

Merged
merged 13 commits into from
Feb 26, 2024
Merged

Sonar code smell fixes in web package #4512

merged 13 commits into from
Feb 26, 2024

Conversation

ginoaugustine
Copy link
Member

  1. Created a new Builder for search helper class and used that for created search helper instance( This was done to reduce number of parameters passed to constructor)
  2. Reduced complexity of methods in search helper and utl classes

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Dec 10, 2023
@ginoaugustine
Changes is is null check
8c7171c 
Signed-off-by: Gino Augustine <[email protected]>
@ginoaugustine ginoaugustine marked this pull request as ready for review December 13, 2023 14:55
@ginoaugustine ginoaugustine requested a review from vladak December 13, 2023 14:55
vladak
vladak requested changes Dec 14, 2023
View reviewed changes
Copy link
Member

@vladak vladak left a comment

Choose a reason for hiding this comment

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

found a bunch of nits. Will do more in depth review for the second pass.

@ginoaugustine
Java doc changes
02282a7 
Signed-off-by: Gino Augustine <[email protected]>
@ginoaugustine ginoaugustine requested a review from vladak December 16, 2023 15:54
vladak
vladak requested changes Jan 18, 2024
View reviewed changes
Copy link
Member

@vladak vladak left a comment

Choose a reason for hiding this comment

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

the ErrorMessageCollector needs some clarification w.r.t. the double prefix. Also, a test case for this class would be welcome - to see in examples how it works in practice.

@ginoaugustine ginoaugustine added the Sonar problems found by Sonar static analyzer label Feb 16, 2024
@ginoaugustine ginoaugustine requested a review from vladak February 19, 2024 05:29
@ginoaugustine
Copy link
Member Author

Removed double prefix and added a UT class to test these changes

@ginoaugustine
Bumped Year in Copyright
8f5bf9d 
Updated test case assert based on review comments
@ginoaugustine ginoaugustine requested a review from vladak February 22, 2024 03:33
vladak
vladak requested changes Feb 23, 2024
View reviewed changes
Copy link
Member

@vladak vladak left a comment

Choose a reason for hiding this comment

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

fix the nit and this is good to go

@vladak
Copy link
Member

vladak commented Feb 23, 2024

Also, some level of UI testing would be useful for this change, I believe. In particular of the slider for the search results.

@vladak vladak merged commit 1f50206 into oracle:master Feb 26, 2024
9 checks passed
@ginoaugustine ginoaugustine deleted the sonarwebpackage branch February 27, 2024 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladak vladak vladak requested changes

Assignees
No one assigned
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement. Sonar problems found by Sonar static analyzer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ginoaugustine @vladak