-
-
Notifications
You must be signed in to change notification settings - Fork 140
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
refactor and sonar fixes related changes #434
base: main
Are you sure you want to change the base?
Conversation
Hello @DanielFran, I tried to resolve some sonar fixes and refactoring changes for this repo. Thank you so much ! |
Hi @DanielFran Thank you so much!! |
Hi @DanielFran |
Hi @DanielFran / @deepu105 / @jdubois And let me know if you want some more modification! I'm glad and grateful to contribute in this wonderful repository 😊! |
Hi @DanielFran , Kindly have a look at my changes, Thank you so much ! : ) |
@@ -80,16 +81,36 @@ public void setJdlMetadata(JdlMetadata jdlMetadata) { | |||
|
|||
@Override | |||
public boolean equals(Object o) { | |||
if (this == o) { | |||
if (isSameObject(o)) { |
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.
These are unnecessary changes IMO, just makes code more verbose and the methods are not even reused. Same for other methods added. Please revert back to original equals
method
@@ -48,7 +48,8 @@ public class Jdl implements Serializable { | |||
@JsonIgnore | |||
private JdlMetadata jdlMetadata; | |||
|
|||
// jhipster-needle-entity-add-field - Jhipster will add fields here, do not remove | |||
// jhipster-needle-entity-add-field - Jhipster will add fields here, do not | |||
// remove |
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.
why?
EntityManager entityManager, | ||
SubGenEventMapper subGenEventMapper | ||
SubGenEventMapper subGenEventMapper, | ||
SubGenEventStatisticsService subGenEventStatisticsService |
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.
i dont see the need to create another service here
import org.springframework.stereotype.Service; | ||
|
||
@Service | ||
public class SubGenEventStatisticsService { |
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.
service should not be in utils package
) | ||
) | ||
.collect(Collectors.toList()); | ||
return subGenEventStatisticsService.getFieldCount(after, field, dbTemporalFunction); |
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.
if a seperate service is created better to call it directly from the controller instead of indirection here
@KaziNizamul : Thanks for the PR and sorry I wasn't able to look at these changes sooner. But could you please work on the changes that Deepu has suggested above and then we can work towards merging this soon. |
Task Description:
Sonar Issue Id:
Affected files: