Conversation
📝 Walkthrough📋 개요이 변경사항은 종료된 챌린지를 제외하는 리포지토리 메서드를 추가하고, 오늘의 인기 챌린지 조회에서 이를 활용하도록 업데이트합니다. 관련 단위 테스트도 함께 추가되었습니다. 🔄 변경사항
📊 코드 리뷰 예상 소요 시간🎯 2 (Simple) | ⏱️ ~10분 분석 근거:
✨ 축하 시
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/hrr/backend/domain/challenge/service/ChallengeServiceImpl.java (1)
368-395:⚠️ Potential issue | 🟠 Major
enrichChallengeInfo에 기존 NPE 위험 및 중복 계산이 존재합니다이 PR의 변경 사항은 아니지만,
getDailyTopChallenges의 핵심 경로에 있으므로 짚어드립니다:
- Line 370:
infoDto.getStartDate().toLocalDate()—startDate가null이면 NPE 발생- Line 373:
challengeStartDate == null체크가 이미 NPE가 발생한 이후에 위치함- Lines 387-388: if-else 블록 바깥에서
dDay와isUpcomingResult를 중복 계산하고 있으며, 이 값들은 사용되지 않음 (dead code)
startDate가 항상 non-null이라면 현재는 문제없지만,Projections.bean매핑에서 DB 값이 null인 경우를 대비해 방어 코드를 권장합니다.♻️ 개선 제안
return rawInfos.stream() .map(infoDto -> { - LocalDate challengeStartDate = infoDto.getStartDate().toLocalDate(); LocalDate today = LocalDate.now(); + LocalDateTime startDateTime = infoDto.getStartDate(); - if (challengeStartDate == null) { + if (startDateTime == null) { infoDto.setIsUpcoming(false); infoDto.setDDayUntilStart(0); } else { + LocalDate challengeStartDate = startDateTime.toLocalDate(); long dDay = ChronoUnit.DAYS.between(today, challengeStartDate); boolean isUpcomingResult = (dDay >= 0) && (dDay <= UPCOMING_DAYS_CRITERIA); infoDto.setIsUpcoming(isUpcomingResult); infoDto.setDDayUntilStart((int)Math.max(0, dDay)); } - long dDay = ChronoUnit.DAYS.between(today, challengeStartDate); - boolean isUpcomingResult = (dDay >= 0) && (dDay <= UPCOMING_DAYS_CRITERIA); - infoDto.setThumbnailUrl(s3UrlUtil.toFullUrl(infoDto.getThumbnailUrl())); infoDto.setDaysOfWeek(daysMap.getOrDefault(infoDto.getChallengeId(), List.of())); return infoDto; }).toList();
🧹 Nitpick comments (3)
src/main/java/com/hrr/backend/domain/challenge/repository/ChallengeRepositoryCustomImpl.java (1)
83-99: 빈ids리스트 전달 시 SQL 오류 가능성 확인 필요
ids가 빈 리스트일 경우qChallenge.id.in(ids)는 DB에 따라IN ()구문으로 변환되어 SQL 오류를 유발할 수 있습니다. 호출부(getDailyTopChallenges)에서currentListSize == 0이면 조기 반환하므로 현재는 안전하지만, 메서드가 다른 곳에서도 호출될 수 있으므로 방어 코드를 추가하는 것을 권장합니다.🛡️ 방어 코드 제안
`@Override` public List<ChallengeResponseDto.InfoDto> findNotFinishedChallengesByIds(List<Long> ids) { + if (ids == null || ids.isEmpty()) { + return List.of(); + } return jpaQueryFactory .select(Projections.bean(ChallengeResponseDto.InfoDto.class,src/test/java/com/hrr/backend/domain/challenge/service/ChallengeServiceImplTest.java (2)
39-80: 종료 챌린지 필터링에 대한 핵심 시나리오가 잘 검증되었습니다. 🎯Redis → RDB → 필터링 → 결과 매핑의 전체 흐름을 깔끔하게 테스트하고 있습니다. 한 가지 보강 포인트를 제안드립니다:
verify(challengeRepository).findNotFinishedChallengesByIds(...)호출 검증을 추가하면, 실제로 새 메서드가 호출되었는지 명시적으로 확인할 수 있습니다.- 추후 엣지 케이스(예: Redis에 있는 모든 챌린지가 종료된 경우 → 빈 리스트 반환) 테스트도 추가하면 더욱 견고해질 것입니다.
30-37:@InjectMocks사용 시 누락된 의존성 주의
ChallengeServiceImpl은 다수의 의존성을 가지고 있으나, 현재 테스트에서는 5개만@Mock으로 선언되어 있습니다. Mockito의@InjectMocks는 매칭되지 않는 필드에null을 주입합니다. 현재 테스트 메서드에서는 해당 경로를 타지 않으므로 문제없지만, 향후 테스트 메서드 추가 시 예상치 못한 NPE가 발생할 수 있습니다.필요한 모든 의존성을
@Mock으로 선언하거나, 클래스 상단에 주석으로 의도를 명시해 두면 다음 개발자에게 도움이 됩니다.
1 participant
#️⃣ 연관된 이슈
✨ 작업 내용 (Summary)
오늘의 인기 챌린지 조회 API에서 종료된 챌린지는 조회되지 않도록 수정하였습니다.
✅ 변경 사항 체크리스트
🧪 테스트 결과
📸 스크린샷
💬 리뷰 요구사항
📎 참고 자료
Summary by CodeRabbit
릴리스 노트
Bug Fixes
Tests