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

Show file tree
Hide file tree
Changes from 3 commits
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
5 changes: 3 additions & 2 deletions src/main/java/sopt/org/hmh/domain/dailychallenge/controller/DailyChallengeApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.springframework.web.bind.annotation.RequestHeader;
import sopt.org.hmh.domain.dailychallenge.dto.request.FinishedDailyChallengeListRequest;
import sopt.org.hmh.domain.dailychallenge.dto.request.FinishedDailyChallengeStatusListRequest;
import sopt.org.hmh.domain.dailychallenge.dto.response.ChallengeStatusesResponse;
import sopt.org.hmh.global.auth.jwt.JwtConstants;
import sopt.org.hmh.global.common.constant.CustomHeaderType;
import sopt.org.hmh.global.common.response.BaseResponse;
Expand All @@ -34,7 +35,7 @@ public interface DailyChallengeApi {
responseCode = "500",
description = "서버 내부 오류입니다.",
content = @Content)})
ResponseEntity<BaseResponse<EmptyJsonResponse>> orderAddHistoryDailyChallenge(
ResponseEntity<BaseResponse<ChallengeStatusesResponse>> orderAddHistoryDailyChallenge(
@Parameter(hidden = true) final Long userId,
@RequestHeader(CustomHeaderType.TIME_ZONE) final String os,
@RequestHeader(CustomHeaderType.TIME_ZONE) final String timeZone,
Expand All @@ -55,7 +56,7 @@ ResponseEntity<BaseResponse<EmptyJsonResponse>> orderAddHistoryDailyChallenge(
responseCode = "500",
description = "서버 내부 오류입니다.",
content = @Content)})
ResponseEntity<BaseResponse<EmptyJsonResponse>> orderChangeStatusDailyChallenge(
ResponseEntity<BaseResponse<ChallengeStatusesResponse>> orderChangeStatusDailyChallenge(
@Parameter(hidden = true) final Long userId,
@RequestHeader(CustomHeaderType.TIME_ZONE) final String timeZone,
@RequestBody final FinishedDailyChallengeStatusListRequest request
Expand Down
13 changes: 9 additions & 4 deletions src/main/java/sopt/org/hmh/domain/dailychallenge/controller/DailyChallengeController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@
import lombok.RequiredArgsConstructor;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.*;
import sopt.org.hmh.domain.challenge.dto.response.ChallengeResponse;
import sopt.org.hmh.domain.challenge.service.ChallengeFacade;
import sopt.org.hmh.domain.dailychallenge.domain.exception.DailyChallengeSuccess;
import sopt.org.hmh.domain.dailychallenge.dto.request.FinishedDailyChallengeListRequest;
import sopt.org.hmh.domain.dailychallenge.dto.request.FinishedDailyChallengeStatusListRequest;
import sopt.org.hmh.domain.dailychallenge.dto.response.ChallengeStatusesResponse;
import sopt.org.hmh.domain.dailychallenge.service.DailyChallengeFacade;
import sopt.org.hmh.global.auth.UserId;
import sopt.org.hmh.global.common.constant.CustomHeaderType;
Expand All @@ -22,7 +25,7 @@ public class DailyChallengeController implements DailyChallengeApi {

@Override
@PostMapping("/finish")
public ResponseEntity<BaseResponse<EmptyJsonResponse>> orderAddHistoryDailyChallenge(
public ResponseEntity<BaseResponse<ChallengeStatusesResponse>> orderAddHistoryDailyChallenge(
@UserId final Long userId,
@RequestHeader(CustomHeaderType.OS) final String os,
@RequestHeader(CustomHeaderType.TIME_ZONE) final String timeZone,
Expand All @@ -31,19 +34,21 @@ public ResponseEntity<BaseResponse<EmptyJsonResponse>> orderAddHistoryDailyChall
dailyChallengeFacade.addFinishedDailyChallengeHistory(userId, request, os);
return ResponseEntity
.status(DailyChallengeSuccess.SEND_FINISHED_DAILY_CHALLENGE_SUCCESS.getHttpStatus())
.body(BaseResponse.success(DailyChallengeSuccess.SEND_FINISHED_DAILY_CHALLENGE_SUCCESS, new EmptyJsonResponse()));
.body(BaseResponse.success(DailyChallengeSuccess.SEND_FINISHED_DAILY_CHALLENGE_SUCCESS,
new ChallengeStatusesResponse(dailyChallengeFacade.getChangedChallengeStatuses(userId))));
}

@Override
@PostMapping("/success")
public ResponseEntity<BaseResponse<EmptyJsonResponse>> orderChangeStatusDailyChallenge(
public ResponseEntity<BaseResponse<ChallengeStatusesResponse>> orderChangeStatusDailyChallenge(
@UserId final Long userId,
@RequestHeader(CustomHeaderType.TIME_ZONE) final String timeZone,
@RequestBody final FinishedDailyChallengeStatusListRequest request
) {
dailyChallengeFacade.changeDailyChallengeStatusByIsSuccess(userId, request);
return ResponseEntity
.status(DailyChallengeSuccess.SEND_FINISHED_DAILY_CHALLENGE_SUCCESS.getHttpStatus())
.body(BaseResponse.success(DailyChallengeSuccess.SEND_FINISHED_DAILY_CHALLENGE_SUCCESS, new EmptyJsonResponse()));
.body(BaseResponse.success(DailyChallengeSuccess.SEND_FINISHED_DAILY_CHALLENGE_SUCCESS,
new ChallengeStatusesResponse(dailyChallengeFacade.getChangedChallengeStatuses(userId))));
Copy link
Member

Choose a reason for hiding this comment

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

P2: 기존에 addFinishedDailyChallengeHistory 및 changeDailyChallengeStatusByIsSuccess에서 currentChallengeId도 찾아두었고, 그 currentChallenge를 통해 dailyChallenge에서 dailyChallenge를 찾는 로직도 있어서 이를 메서드를 분리해서 또 이를 찾는 것은 쿼리 낭비라는 생각이 들어요.

Response를 주기 위해서 기존의 void였던 addFinishedDailyChallengeHistory 및 changeDailyChallengeStatusByIsSuccess의 메서드를 수정해서 response를 반환하는 것이 쿼리 성능 측면에서 나을 것으로 생각됩니다

Copy link
Member

Choose a reason for hiding this comment

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

또한, controller에 최대한 비즈니스 로직을 집어넣지 않기 위해서 facade 메서드를 controller에 최대한 줄이고자 하는데 이 의견에 대한 생각도 궁금합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

제가 트랜잭션에 대해서 잘못 이해하고 있었네요!
수정 방향은 기존에 이미 불러온 챌린지 id를 활용할 수 있도록 챌린지를 구하고 다른 코드에서도 불러온 챌린지를 재활용하도록 코드를 수정하였고 return형태를 변경했습니다.
+) challengeService의 getCurrentChallengeAppByChallengeId()호출을 삭제해서 이건 deprecated된 메소드에서만 호출되게 되었네요

Copy link
Collaborator Author

@jumining jumining Jul 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

또한, controller에 최대한 비즈니스 로직을 집어넣지 않기 위해서 facade 메서드를 controller에 최대한 줄이고자 하는데 이 의견에 대한 생각도 궁금합니다!
-> 컨트롤러에서는 facade나 service의 메서드 최대한 적게(이 로직을 수행하는 메서드 한 개정도로 연결하는 정도로) 호출하는 방향 말씀하시는거죵?

Copy link
Member

Choose a reason for hiding this comment

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

또한, controller에 최대한 비즈니스 로직을 집어넣지 않기 위해서 facade 메서드를 controller에 최대한 줄이고자 하는데 이 의견에 대한 생각도 궁금합니다! -> 컨트롤러에서는 facade나 service의 메서드 최대한 적게(이 로직을 수행하는 메서드 한 개정도로 연결하는 정도로) 호출하는 방향 말씀하시는거죵?

네 맞습니다! controller layer에서는 최대한 비즈니스 로직을 제거하여 controller layer의 책임만 수행할 수 있도록 레이어간 책임 분리를 하는 것이 좋을 것 같아서요!

}
}
10 changes: 10 additions & 0 deletions src/main/java/sopt/org/hmh/domain/dailychallenge/dto/response/ChallengeStatusesResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package sopt.org.hmh.domain.dailychallenge.dto.response;

import sopt.org.hmh.domain.dailychallenge.domain.Status;

import java.util.List;

public record ChallengeStatusesResponse(
List<Status> statuses
) {
}
9 changes: 9 additions & 0 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 @@ -54,4 +54,13 @@ public void changeDailyChallengeStatusByIsSuccess(Long userId, FinishedDailyChal
}
});
}

public List<Status> getChangedChallengeStatuses(Long userId) {
Long currentChallengeId = userService.getCurrentChallengeIdByUserId(userId);
return challengeService.findByIdOrElseThrow(currentChallengeId)
.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.

이렇게 찾는 로직을 사용하더라도 challengeDate로 정렬하는 로직이 추가되었으면 좋았을 것 같아요.

Copy link
Collaborator Author

@jumining jumining Jul 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

그러면 기존 달성 현황 조회할 때도 상태값들을 리스트로 반환하는데 .getHistoryDailyChallenges()가 사용되어서 그 부분도 정렬을 추가하면 좋을까요?
정확히는 ChallengeResponse레코드의 of static 메소드가 되겠네요!

Copy link
Member

Choose a reason for hiding this comment

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

그러면 기존 달성 현황 조회할 때도 상태값들을 리스트로 반환하는데 .getHistoryDailyChallenges()가 사용되어서 그 부분도 정렬을 추가하면 좋을까요? 정확히는 ChallengeResponse레코드의 of static 메소드가 되겠네요!

오호 그러면 너무 좋을 것 같아요!

}
}
Loading