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

jumining · 2024-07-19T08:45:50Z

Related issue 🚀

closed modify - 챌린지 성공 여부 처리 요청 전송 시 response 변경 #183

Work Description 💚

기존에 data에 아무것도 반환해주지 않는 방식에서 챌린지의 status들 목록을 반환하도록 수정하였습니다.
~/success, ~/finish api 2개에 적용하였습니다.

PR 참고 사항

테스트는 iOS 계정으로 진행해서 iOS용 api 메소드에만 테스트 해보았습니다.

포스트맨 테스트

kseysh · 2024-07-19T09:53:24Z

src/main/java/sopt/org/hmh/domain/dailychallenge/controller/DailyChallengeController.java

            @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))));


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

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

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

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

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

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

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

kseysh · 2024-07-19T09:55:28Z

src/main/java/sopt/org/hmh/domain/dailychallenge/service/DailyChallengeFacade.java

+        return challengeService.findByIdOrElseThrow(currentChallengeId)
+                .getHistoryDailyChallenges()
+                .stream()
+                .map(DailyChallenge::getStatus)
+                .toList();


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

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

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

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

… 가능하도록 코드 수정

kseysh · 2024-07-21T16:23:44Z

src/main/java/sopt/org/hmh/domain/dailychallenge/service/DailyChallengeFacade.java

-        Long currentChallengeId = userService.getCurrentChallengeIdByUserId(userId);
-        List<ChallengeApp> currentChallengeApps =
-                challengeService.getCurrentChallengeAppByChallengeId(currentChallengeId);
+    public List<Status> addFinishedDailyChallengeHistory(Long userId, FinishedDailyChallengeListRequest requests, String os) {


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

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

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

kseysh · 2024-07-21T16:32:08Z

src/main/java/sopt/org/hmh/domain/dailychallenge/service/DailyChallengeFacade.java

            DailyChallenge dailyChallenge =
                    dailyChallengeService.findDailyChallengeByChallengeIdAndChallengePeriodIndex(
-                            currentChallengeId, request.challengePeriodIndex());
+                            challenge.getId(), request.challengePeriodIndex());


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

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

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

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

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

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

kseysh · 2024-07-21T16:33:29Z

src/main/java/sopt/org/hmh/domain/dailychallenge/service/DailyChallengeFacade.java

+        return challenge.getHistoryDailyChallenges()
+                .stream()
+                .map(DailyChallenge::getStatus)
+                .toList();


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

kseysh · 2024-07-21T16:34:19Z

src/main/java/sopt/org/hmh/domain/dailychallenge/service/DailyChallengeFacade.java

    }

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


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

kseysh · 2024-07-21T16:35:17Z

src/main/java/sopt/org/hmh/domain/dailychallenge/service/DailyChallengeFacade.java

-        Long currentChallengeId = userService.getCurrentChallengeIdByUserId(userId);
-        List<ChallengeApp> currentChallengeApps =
-                challengeService.getCurrentChallengeAppByChallengeId(currentChallengeId);
+    public List<Status> addFinishedDailyChallengeHistory(Long userId, FinishedDailyChallengeListRequest requests, String os) {


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

…록 설정

…로 분리

… modify/#183-change-response-post-challenge

kseysh · 2024-07-22T10:11:21Z

src/main/java/sopt/org/hmh/domain/dailychallenge/service/DailyChallengeFacade.java

            DailyChallenge dailyChallenge =
-                    dailyChallengeService.findDailyChallengeByChallengeIdAndChallengePeriodIndex(
-                            challenge.getId(), request.challengePeriodIndex());
+                    challenge.getHistoryDailyChallenges().get(challengeRequest.challengePeriodIndex());


요거 dailyChallenge에서 index를 가져오는 로직이 dailyChallenge가 정렬되어 있다는 보장을 할 수 없고,
.get으로 index 만큼의 index에 있는 dailyChallenge를 가져온다는 것이 나중에 다시 봤을 때 조금은 이해가 어려운 코드일 수도 있을 것 같아요

따라서 언급했던 문제를 해결하고, index의 dailyChallenge를 get할 때의 로직을 이름을 붙여서 (함수로 만들어서) 작성하는 것은 어떨지 궁금합니다!

JPA에서는 컬렉션의 순서가 보장되지 않는 문제가 있는 것을 확인하고 이 문제 언급이 많은 것 같아 여러 메소드에서 정렬하는 코드가 많은 상황인데

이 컬렉션에 대해서는 특정 상황에서만 정렬되는 것 보다는 모든 상황에서 date기준으로 정렬되는게 맞는 것 같아서
Challenge 도메인에 List historyDailyChalllenges에 @orderby 애노테이션 적용하는 것은 어떨까요?
코드가 간결해질 것 같아요 (다른 메소드에서도 정렬 조건 명시하지 않아도 되구요!)

-> 코드 중복도 줄이고 정렬 조건을 매번 명시하지 않아도 되고 여러 메소드에서 놓치지 않고 일관된 순서 보장한다는 장점

@OrderBy("challengeDate ASC")
private List<DailyChallenge> historyDailyChallenges;

이렇게 문제 해결한 뒤에, index의 dailyChallenge를 get할 때의 로직을 이름을 붙여서 (함수로 만들어서) 작성에 대해서는 함수이름만 변경하게 되는 것 같아 가독성 문제가 아니라면 불필요한 작업이 될 것 같은데 가독성이 많이 안좋을까요?

challengeRequest.challengePeriodIndex() 자체가 index를 가져오는거라서 .get()에 인자로 넣어주면 요청에 있는 인덱스로historyDaillyChallenges의 몇번째에 해당하는 날짜내용을 가져온다는 의미가 이미 충분히 보일 거라고 생각이 들어서요!

다시 한번 의견 부탁드립니둉

오호 OrderBy 어노테이션은 모르던 사실이네요 좋은 개선안인 것 같습니다! 새롭게 배웠네요

두 번째 글 같은 경우는 일부러 approve를 드리고 댓글을 남겼던게 주민님이 불필요하다 생각하면 머지해도 될 것 같다는 의미였어요! 의견까지 내주셔서 감사합니다 :)
요즘 클린코드를 읽고 있는데... 보통 기능을 파악할 때 큰 메서드를 먼저 확인하잖아요? 큰 메서드를 확인했을 때 아 이런 메서드구나라는 생각이 드는 코드가 좋은 코드라고 느꼈어요 그런 점에서 함수를 더 쪼개서 그 행동에 이름을 붙여준다면 다른 개발자가 보더라도 아 이런 기능을 하는 부분이구나를 알 수 있을 것 같아 제안해본 내용입니다! 하지만 그 행동이 불필요하고 오히려 함수가 많아지게 하는 원인이 될 것 같다면 주민님이 말씀하신대로 해도 괜찮을 것 같아요!

추가로 index로 get할 때 없는 경우 에러처리가 필요할 것 같아서,
기존에 service에 있던 에러처리 메소드를 활용하여 메소드를 따로 분리했습니다!

DailyChallenge dailyChallenge = dailyChallengeService.findDailyChallengeByChallengePeriodIndex(challenge, challengeRequest.challengePeriodIndex());

jumining added 3 commits July 19, 2024 17:37

feat - #183 챌린지 상태값을 반환하는 response record 추가

f999db4

feat - #183 controller의 BaseResponse 수정

445cb02

feat - #183 일별 챌린지 결과 전송 후 챌린지의 상태값들을 반환하는 메소드 작성

39006e5

jumining added 👩🏻‍💻 주민 주민이가 작성한 Label 🔧 Modify 코드 수정 (기능의 변화가 있을 때) labels Jul 19, 2024

jumining requested a review from kseysh July 19, 2024 08:45

jumining self-assigned this Jul 19, 2024

kseysh reviewed Jul 19, 2024

View reviewed changes

jumining changed the title ~~modify - #183 일별챌린지 결과 전송 후 챌린지의 변경된 상태값들을 반환하도록 수정~~ modify - 일별챌린지 결과 전송 후 챌린지의 변경된 상태값들을 반환하도록 수정 Jul 21, 2024

jumining added 3 commits July 21, 2024 23:42

modify - #183 서비스 내의 메소드를 하나만 호출하도록 컨트롤러 변경

7189932

modify - #183 기존 메소드의 return형태 void에서 List<Status>로 수정 및 불러온 챌린지를 재활용…

a016925

… 가능하도록 코드 수정

feat - #183 challengeDate로 정렬하는 조건 추가

d3fa095

kseysh reviewed Jul 21, 2024

View reviewed changes

jumining added 5 commits July 22, 2024 14:54

feat - #183 challengeResponse의 of 메소드에서 challengeDate를 기준으로 정렬해서 리턴하도…

1886a83

…록 설정

chore - #183 challengeDate로 정렬해서 historyDailyChallenge 가져오는 중복 코드 메소드…

b4f96b8

…로 분리

chore - #183 메소드에서 사용되는 변수명 수정

22ab912

modify - #183 불필요한 쿼리 사용이 없도록 dailyChallenge구해오는 코드 수정

2ca0d57

Merge branch 'develop' of https://github.com/Team-HMH/HMH-Server into…

948da00

… modify/#183-change-response-post-challenge

kseysh approved these changes Jul 22, 2024

View reviewed changes

jumining added 4 commits July 29, 2024 14:23

feat - #183 historyDailyChallenges가 항상 challengeDate기준으로 정렬되도록 설정

62c0b7a

feat - #183 sorted 삭제

e78100d

chore - #183 index로 dailyChallenge 구하고 에러처리하는 메소드 수정

8377bea

chore - #183 메소드명에서 필요없는 이름 수정

4fbb30f

jumining merged commit c03f2d1 into develop Jul 29, 2024
1 check passed

jumining deleted the modify/#183-change-response-post-challenge branch November 19, 2024 04:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jumining commented Jul 19, 2024

kseysh Jul 19, 2024

kseysh Jul 19, 2024

jumining Jul 21, 2024

jumining Jul 21, 2024 •

edited

Loading

kseysh Jul 21, 2024

kseysh Jul 19, 2024

jumining Jul 21, 2024 •

edited

Loading

kseysh Jul 21, 2024

kseysh Jul 21, 2024

kseysh Jul 21, 2024

kseysh Jul 21, 2024

kseysh Jul 21, 2024

jumining Jul 22, 2024 •

edited

Loading

kseysh Jul 21, 2024

kseysh Jul 21, 2024

kseysh Jul 21, 2024

kseysh Jul 22, 2024

jumining Jul 28, 2024

jumining Jul 28, 2024 •

edited

Loading

kseysh Jul 28, 2024

jumining Jul 29, 2024

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

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

Conversation

jumining commented Jul 19, 2024

Related issue 🚀

Work Description 💚

PR 참고 사항

포스트맨 테스트

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jumining Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jumining Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jumining Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jumining Jul 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jumining Jul 21, 2024 •

edited

Loading

jumining Jul 21, 2024 •

edited

Loading

jumining Jul 22, 2024 •

edited

Loading

jumining Jul 28, 2024 •

edited

Loading