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.

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

KaziNizamul
Copy link
Contributor

Task Description:

  • refactored a few smelly code
  • fixed a sonar issue

Sonar Issue Id:

Affected files:

  • src/main/java/io/github/jhipster/online/domain/Jdl.java
  • src/main/java/io/github/jhipster/online/domain/YoRC.java
  • src/main/java/io/github/jhipster/online/service/EntityStatsService.java
  • src/main/java/io/github/jhipster/online/service/GitService.java
  • src/main/java/io/github/jhipster/online/service/SubGenEventService.java
  • src/main/java/io/github/jhipster/online/service/util/SubGenEventStatisticsService.java
  • src/main/java/io/github/jhipster/online/web/rest/GitResource.java

@KaziNizamul
Copy link
Contributor Author

Hello @DanielFran,

I tried to resolve some sonar fixes and refactoring changes for this repo.
Kindly have a look,
and let me know if any changes are required.

Thank you so much !

@KaziNizamul
Copy link
Contributor Author

KaziNizamul commented Nov 22, 2023

Hi @DanielFran
Kindly have a look and review the changes!

Thank you so much!!

@KaziNizamul
Copy link
Contributor Author

Hi @DanielFran
Kindly have a look and review the changes...
And kindlt let me know if any change is required...

@KaziNizamul
Copy link
Contributor Author

Hi @DanielFran / @deepu105 / @jdubois
Requesting you to kindly have a look at the changes

And let me know if you want some more modification!

I'm glad and grateful to contribute in this wonderful repository 😊!

@KaziNizamul
Copy link
Contributor Author

Hi @DanielFran ,

Kindly have a look at my changes,
if needed any changes please let know !

Thank you so much ! : )

@KaziNizamul
Copy link
Contributor Author

@SudharakaP @pascalgrimaud

@@ -80,16 +81,36 @@ public void setJdlMetadata(JdlMetadata jdlMetadata) {

@Override
public boolean equals(Object o) {
if (this == o) {
if (isSameObject(o)) {
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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 {
Copy link
Member

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);
Copy link
Member

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

@SudharakaP
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants