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

Registration on challenge #1696

Draft
wants to merge 7 commits into
base: rest
Choose a base branch
from
Draft

Conversation

AVykhovanok
Copy link
Collaborator

@AVykhovanok AVykhovanok commented Feb 9, 2023

Pull Request template

Please, go through these steps before you submit a PR.

  1. Make sure that your PR fulfills these requirements:

    a. You have done your changes in a separate branch. Branches MUST have descriptive names.

    b. You have a descriptive commit message with a short title (first line).

    c. You have only one commit (if not, squash them into one commit).

    d. mvn verify doesn't throw any error. If it does, fix them first and amend your commit (git commit --amend).

  2. After these steps, you're ready to open a pull request.

    a. Your pull request MUST NOT target the main branch on this repository. You probably want to target rest instead.

    b. Give a descriptive title to your PR.

    c. Provide a description of your changes.

    d. Put closes #XXXX in your comment to auto-close the issue that your PR fixes (if such).

PLEASE REMOVE THIS TEMPLATE BEFORE SUBMITTING

Checklist:

  • I have added tests with code coverage >=75%
  • My code pass Checkstyle rules
  • PR is reviewed manually again (to make sure you have 100% ready code)
  • All reviewers agreed to merge the PR

@AVykhovanok AVykhovanok changed the title Registration on challenge needs refactoring Registration on challenge Feb 9, 2023
@AVykhovanok AVykhovanok closed this Feb 9, 2023
@AVykhovanok AVykhovanok reopened this Feb 9, 2023
@dvavoly
Copy link
Collaborator

dvavoly commented Feb 9, 2023

Improve your code:
Firstly, remove Pull Request template and add description about what exactly your code do.
Secondly, add JavaDoc to all controller and service methods.
Also, you should write tests to cover code at least 75%, mostly service layer.

@PostMapping("/admin/user-challenge/challenge/duration/registered-users")
public List<UserChallengeForAdminRegisteredUser> getListRegisteredUsersByChallengeIdChallengeDurationId(
@RequestBody UserChallengeForAdminGetByChallengeIdDurationId userChallenge) {
return userChallengeService.getListRegisteredUsersByChallengeIdChallengeDurationId(userChallenge);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add JavaDoc to explain what this method do and try to avoid naming like this getListRegisteredUsersByChallengeIdChallengeDurationId

false,
challenge,
durationEntity)));
return "Успішно";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add more information at the response. Better create response message in corresponding controller.

@Transactional
public class DurationEntityServiceImpl implements DurationEntityService {
private final DurationEntityRepository durationEntityRepository;
private final DtoConverter dtoConverter;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this unused "dtoConverter" private field

return durationEntityRepository.findAll();
}

public Set<DurationEntity> mapDurationResponseListToDurationEntity(
Copy link
Collaborator

Choose a reason for hiding this comment

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

From my point of view, the method should be private because it used only in this class.

.orElseThrow(() -> new NotExistException(
String.format(USER_CHALLENGE_STATUS_NOT_FOUND_BY_STATUS_NAME, statusName))),
UserChallengeStatusGet.class);
log.debug("**/getting UserChallengeStatusGet by statusName = " + statusName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of + for concatenation string in log messages, use {}

}
UserChallengeStatus userChallengeStatus = userChallengeStatusRepository.save(
dtoConverter.convertToEntity(userChallengeStatusAdd, new UserChallengeStatus()));
log.debug("**/adding new userChallengeStatus = " + userChallengeStatusAdd.getStatusName());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of + for concatenation string in log messages, use {}

public UserChallengeStatusUpdate updateUserChallengeStatus(UserChallengeStatusUpdate userChallengeStatusUpdate) {
UserChallengeStatus newUserChallengeStatus = getUserChallengeStatusById(userChallengeStatusUpdate.getId());
newUserChallengeStatus.setStatusName(userChallengeStatusUpdate.getStatusName());
log.debug("**/updating UserChallengeStatus by userChallengeStatusUpdate = " + newUserChallengeStatus);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of + for concatenation string in log messages, use {}

UserChallengeStatus userChallengeStatus = userChallengeStatusRepository
.getUserChallengeStatusById(id)
.orElseThrow(() -> new NotExistException(String.format(USER_CHALLENGE_STATUS_NOT_FOUND_BY_ID, id)));
log.debug("**/getting UserChallengeStatus by id = " + id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of + for concatenation string in log messages, use {}

UserChallengeStatus userChallengeStatus = getUserChallengeStatusById(id);
try {
userChallengeStatusRepository.deleteById(id);
userChallengeStatusRepository.flush();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess, userChallengeStatusRepository.flush(); is redundant here.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@dvavoly dvavoly marked this pull request as draft February 21, 2023 19:43
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.

2 participants