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

modify - 일별챌린지 결과 전송 후 챌린지의 변경된 상태값들을 반환하도록 수정 #184

Merged
merged 15 commits into from
Jul 29, 2024
Merged

modify - 일별챌린지 결과 전송 후 챌린지의 변경된 상태값들을 반환하도록 수정 #184

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f999db4
feat - #183 챌린지 상태값을 반환하는 response record 추가
jumining Jul 19, 2024
445cb02
feat - #183 controller의 BaseResponse 수정
jumining Jul 19, 2024
39006e5
feat - #183 일별 챌린지 결과 전송 후 챌린지의 상태값들을 반환하는 메소드 작성
jumining Jul 19, 2024
7189932
modify - #183 서비스 내의 메소드를 하나만 호출하도록 컨트롤러 변경
jumining Jul 21, 2024
a016925
modify - #183 기존 메소드의 return형태 void에서 List<Status>로 수정 및 불러온 챌린지를 재활용…
jumining Jul 21, 2024
d3fa095
feat - #183 challengeDate로 정렬하는 조건 추가
jumining Jul 21, 2024
1886a83
feat - #183 challengeResponse의 of 메소드에서 challengeDate를 기준으로 정렬해서 리턴하도…
jumining Jul 22, 2024
b4f96b8
chore - #183 challengeDate로 정렬해서 historyDailyChallenge 가져오는 중복 코드 메소드…
jumining Jul 22, 2024
22ab912
chore - #183 메소드에서 사용되는 변수명 수정
jumining Jul 22, 2024
2ca0d57
modify - #183 불필요한 쿼리 사용이 없도록 dailyChallenge구해오는 코드 수정
jumining Jul 22, 2024
948da00
Merge branch 'develop' of https://github.com/Team-HMH/HMH-Server into…
jumining Jul 22, 2024
62c0b7a
feat - #183 historyDailyChallenges가 항상 challengeDate기준으로 정렬되도록 설정
jumining Jul 29, 2024
e78100d
feat - #183 sorted 삭제
jumining Jul 29, 2024
8377bea
chore - #183 index로 dailyChallenge 구하고 에러처리하는 메소드 수정
jumining Jul 29, 2024
4fbb30f
chore - #183 메소드명에서 필요없는 이름 수정
jumining Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 15 additions & 14 deletions src/main/java/sopt/org/hmh/domain/dailychallenge/service/DailyChallengeFacade.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import org.springframework.transaction.annotation.Transactional;
import sopt.org.hmh.domain.app.domain.ChallengeApp;
import sopt.org.hmh.domain.app.service.HistoryAppService;
import sopt.org.hmh.domain.challenge.domain.Challenge;
import sopt.org.hmh.domain.challenge.service.ChallengeService;
import sopt.org.hmh.domain.dailychallenge.domain.DailyChallenge;
import sopt.org.hmh.domain.dailychallenge.domain.Status;
Expand All @@ -23,27 +24,31 @@ public class DailyChallengeFacade {
private final UserService userService;

@Transactional
public void addFinishedDailyChallengeHistory(Long userId, FinishedDailyChallengeListRequest requests, String os) {
Long currentChallengeId = userService.getCurrentChallengeIdByUserId(userId);
List<ChallengeApp> currentChallengeApps =
challengeService.getCurrentChallengeAppByChallengeId(currentChallengeId);
public List<Status> addFinishedDailyChallengeHistory(Long userId, FinishedDailyChallengeListRequest requests, String os) {
Copy link
Member

Choose a reason for hiding this comment

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

제가 한건거 같긴한데... FinishedDailyChallengeListRequest requests 에서 변수명에 s가 들어가는 것이 찜찜해서 혹시 request로 변경 가능할까요?

그러려면 스트림에서 사용한 request 변수의 네이밍을 challengeRequest로 바꿔주면 좋을 것 같아요!

Copy link
Member

Choose a reason for hiding this comment

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

아래 메서드도 s가 들어가있던데 한 번 부탁드립니다..!

Challenge challenge = challengeService.findByIdOrElseThrow(userService.getCurrentChallengeIdByUserId(userId));

requests.finishedDailyChallenges().forEach(request -> {
DailyChallenge dailyChallenge =
dailyChallengeService.findDailyChallengeByChallengeIdAndChallengePeriodIndex(
currentChallengeId, request.challengePeriodIndex());
challenge.getId(), request.challengePeriodIndex());
dailyChallengeService.changeStatusByCurrentStatus(dailyChallenge);
historyAppService.addHistoryApp(currentChallengeApps, request.apps(), dailyChallenge, os);
historyAppService.addHistoryApp(challenge.getApps(), request.apps(), dailyChallenge, os);
});

return challenge.getHistoryDailyChallenges()
.stream()
.map(DailyChallenge::getStatus)
.toList();
Copy link
Member

Choose a reason for hiding this comment

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

이 부분은 아래 코드와도 중복된 부분이니 메서드로 중복을 제거해주는 것도 좋을 것 같아요

}

@Transactional
public void changeDailyChallengeStatusByIsSuccess(Long userId, FinishedDailyChallengeStatusListRequest requests) {
Long currentChallengeId = userService.getCurrentChallengeIdByUserId(userId);
public List<Status> changeDailyChallengeStatusByIsSuccess(Long userId, FinishedDailyChallengeStatusListRequest requests) {
Copy link
Member

Choose a reason for hiding this comment

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

여기 requests도... 부탁드립니다

Challenge challenge = challengeService.findByIdOrElseThrow(userService.getCurrentChallengeIdByUserId(userId));

requests.finishedDailyChallenges().forEach(request -> {
DailyChallenge dailyChallenge =
dailyChallengeService.findDailyChallengeByChallengeIdAndChallengePeriodIndex(
currentChallengeId, request.challengePeriodIndex());
challenge.getId(), request.challengePeriodIndex());
Copy link
Member

Choose a reason for hiding this comment

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

저는 레포지토리 사용을
userService에서 유저 찾기 -> 쿼리 한 번
찾은 유저에서 currentChallengeId를 찾아 dailyChallenge 찾기 -> 쿼리 한 번
의 총 두 번의 쿼리를 사용하였는데요!

주민님은 현재 변경한 코드를 봤을 때,
userService에서 유저 찾기 -> 쿼리 한 번
challengeService에서 챌린지 찾기 -> 쿼리 한 번
challengeId와 challengePeriodIndex로 dailyChallenge찾기 -> 쿼리 한 번
총 세번의 쿼리를 사용하시는 것 같아요!

챌린지를 이미 찾았다면, 그 Challenge 객체의 dailyChallenge에서 periodIndex를 계산하는 것이 더 쿼리를 줄일 수 있는 방법일 것 같은데 어떻게 생각하시나요?

물론 challenge를 찾고 dailyChallenge를 가져올 때 JPA에서 알아서 지연로딩으로 가져와서 성능상에는 똑같을 지도 모르긴 하는데... 혹시 고려하시고 한 것인지 궁금합니다!

Copy link
Member

Choose a reason for hiding this comment

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

가능하시다면 쿼리 어떻게 나가나 한 번 봐주세요..!

Copy link
Collaborator Author

@jumining jumining Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

마지막에 challenge의 상태리스트를 반환할 때 재사용하려고 challengeId구하기->challenge구하기로 수정했는데 그렇게 되면 말씀처럼 중간에 dailyChallenge를 구할 때 비효율적으로 쿼리를 사용하게 되네요! 중간 부분을 확인하지 못했네요 예리해유
challenge를 이미 구해왔으니 구해온 챌린지에서 인덱스를 사용한 .get()으로 가져오는 것이 쿼리 나가는 거 확인해본 결과 사용이 더 적은 것으로 확인되었습니다!

if (request.isSuccess()) {
dailyChallengeService.validateDailyChallengeStatus(dailyChallenge.getStatus(), List.of(Status.NONE));
dailyChallenge.changeStatus(Status.UNEARNED);
Expand All @@ -53,12 +58,8 @@ public void changeDailyChallengeStatusByIsSuccess(Long userId, FinishedDailyChal
dailyChallenge.changeStatus(Status.FAILURE);
}
});
}

public List<Status> getChangedChallengeStatuses(Long userId) {
Long currentChallengeId = userService.getCurrentChallengeIdByUserId(userId);
return challengeService.findByIdOrElseThrow(currentChallengeId)
.getHistoryDailyChallenges()
return challenge.getHistoryDailyChallenges()
.stream()
.map(DailyChallenge::getStatus)
.toList();
Expand Down