feat - 달성현황 정보 불러오기 #50
Conversation
… feat/#20-get-challenge-api
| public static DailyChallenge getTodayDailyChallengeByUserId(ChallengeRepository challengeRepository, | ||
| DailyChallengeRepository dailyChallengeRepository, | ||
| final Long userId) { | ||
| val challenge = challengeRepository.findFirstByUserIdOrderByCreatedAtDesc(userId); | ||
| val startDateOfChallenge = challenge.getDailyChallenges().get(0).getCreatedAt().toLocalDate(); | ||
|
|
||
| return dailyChallengeRepository.findFirstByChallengeIdOrderByCreatedAtDesc(challenge.getId()); | ||
| val todayDailyChallengeIndex = (int) ChronoUnit.DAYS.between(LocalDateTime.now().toLocalDate(), startDateOfChallenge); | ||
| return challenge.getDailyChallenges().get(todayDailyChallengeIndex); | ||
| } | ||
| } |
There was a problem hiding this comment.
P3: repository를 매개변수로 넣어준다는 로직이 조금 어색한 것 같아요. 제가 봤었던 코드들에서는 보통 repository는 생성자 주입을 통해 관리했었기 때문입니다...!
getUserId도 global에 있는 것보다 auth 폴더에서 접근하는 것이 더 옳다고 생각하여 변경할 예정인데, getTodayDailyChallengeByUserId도 dailyChallenge의 폴더에서 관리하는 것이 어떨지 여쭤보고 싶습니다!
| val todayDailyChallengeIndex = (int) ChronoUnit.DAYS.between(LocalDateTime.now().toLocalDate(), startDateOfChallenge); | ||
| return challenge.getDailyChallenges().get(todayDailyChallengeIndex); |
There was a problem hiding this comment.
P3: 이 부분에 사용되는 sql query가 너무 많다고 생각됩니다. dailyChllenge에 오늘이 챌린지 기간 내 며칠째인지의 필드를 추가하는 것을 고려해보는 것은 어떨까요?
There was a problem hiding this comment.
sql query는 가장 최신의 챌린지를 구하는 val challenge = challengeRepository.findFirstByUserIdOrderByCreatedAtDesc(userId);의 findFirst~~에서만 사용되고 나머지는 리스트에서 인덱스를 통해 요소에 접근하는 방식입니다! :) dailyChllenge에 오늘이 챌린지 기간 내 며칠째인지의 필드를 추가하는 경우에도 DailyChallengeIndex를 구하는 코드 한 줄만 삭제될 것 같은데 저 코드가 단순 일수 차이를 반환하는 로직이기 때문에 리소스가 크게 든다고 생각되지 않습니다. erd, db설계 고려 회의시 date필드(createdAt)를 넣고 며칠째인지 나타내는 필드는 db에 저장하지 않도록 논의했던 것 같습니다! 읽고 다시 편하게 의견 주십쇼!!ㅎㅎ😀
There was a problem hiding this comment.
아하 제가 sql 쿼리라고 잘못 작성했네요...
이렇게 보니 큰 차이는 없긴한 것 같아요!
제가 이 말씀을 드린 이유는!
// 일단 이 부분은 challenge의 .getCreatedAt을 변경되는 것이 맞을 것 같아요!
val startDateOfChallenge = challenge.getDailyChallenges().get(0).getCreatedAt().toLocalDate();
val todayDailyChallengeIndex = (int) ChronoUnit.DAYS.between(LocalDateTime.now().toLocalDate(), startDateOfChallenge);
return challenge.getDailyChallenges().get(todayDailyChallengeIndex);
물론 저도 아직 성능 테스트를 해보지는 않았지만... 로직의 복잡성을 증가시키는 것 보다 DB에 필드를 하나 더 추가하는 것이 성능에 더 유리할 것 같다 생각한 것 같아요. 또 인덱스를 구하는 것은 자주 사용되는 로직일 것 같아서요! 지금 보니 하나의 메서드를 추가로 사용하는 것이면 굳이 DB에 추가하지 않아도 될 것 같습니다! :)
다만, challenge.getDailyChallenges().get(0).getCreatedAt()은 challenge.getCreatedAt()과 같다고 생각되어 그렇게 수정해주시면 좋을 것 같습니다!
| val dailyChallenges = challenge.getDailyChallenges(); | ||
|
|
||
| val statuses = new ArrayList<Status>(); | ||
| for (val dailyChallenge : dailyChallenges) { | ||
| statuses.add(dailyChallenge.getStatus()); | ||
| } |
There was a problem hiding this comment.
P3: statuses도 DailyChallengeResponse의 apps 반환한 방식처럼 response내에서 변환하는 것이 더 가독성 및 service 클래스의 책임 측면에서 더 좋을 것 같습니다!
| return ChallengeResponse.of(challenge, dailyChallenges.get(todayIndex), statuses, todayIndex); | ||
| } |
There was a problem hiding this comment.
P3: response를 위한 response 값 세팅은 response dto내에서 한다면 전달하는 매개변수가 적어질 수 있을 것 같습니다!
서비스 클래스 내에서는 비즈니스 로직을 처리하는 것에 중점을 두는 것으로 하는 것이 좋을 것 같습니다!
Related issue 🚀
Work Description 💚
PR 참고 사항
통신 성공