feat: 명예의 전당 api 구현#849
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
~02.15 : 리뷰 |
| JOIN teams t ON t.team_id = m.team_id | ||
| WHERE r.rnk <= :limit | ||
| AND m.deleted_at IS NULL | ||
| AND m.team_id IS NOT NULL |
There was a problem hiding this comment.
R: 현재 members 테이블의 필터링 조건(deleted_at, team_id)이 최외곽 SELECT 절에 위치해 있어요!
이 경우, 만약 체크인 횟수 상위 10명을 뽑았는데 그중 탈퇴한 회원이 포함되어 있다면, 최종 결과물은 10개 미만(예: 9개)으로 노출되는 문제가 발생할 수 있을 것 같아요!
의도하신 limit만큼의 데이터를 정확히 보장하려면, member_counts CTE 내부나 조인 시점에서 멤버 필터링을 미리 처리하는 것이 로직의 일관성 측면에서 더 안전해 보입니다.
그래야, 사전에 미리 걸르고 걸러서 limit만큼 안전하게 뽑을 수 있지 않을까요?
There was a problem hiding this comment.
피드백 감사합니다! 그런 문제가 있을 수 있겠군요
말씀주신대로 members 필터를 집계 단계(member_counts)로 이동했습니다
| @Override | ||
| public ResponseEntity<LeaderboardsResponse> findAllTop(final int limit) { | ||
| var responses = leaderboardService.findAllTop(limit); | ||
| return ResponseEntity.ok(LeaderboardsResponse.of(responses)); |
| private final List<LeaderboardQuery> queries; | ||
|
|
||
| @PostConstruct | ||
| void init() { |
There was a problem hiding this comment.
C: 리더보드 목록들을 관리하는 역할의 객체를 하나 부여하는 것도 좋아보여요! 리더보드서비스가 그 역할까지 하고 있는 것 처럼보입니다.
서비스에서는 비즈니스 로직만 수행하고, Factory같은 것을 도입해서 객체 생성 책임을 부여하면 어떨까요?
There was a problem hiding this comment.
LeaderboardQuery 목록 관리, 조회를 LeaderboardQueryRegistry로 분리했고, 서비스에서는 비즈니스 로직만 수행하도록 수정했습니다!
| .containsExactly(fora.getId(), mint.getId()); | ||
| } | ||
| // | ||
| // @DisplayName("승리요정 명예의 전당에서 동점이면 공동 1등이 rank=1로 모두 반환된다 (limit=1이어도).") |
There was a problem hiding this comment.
A: 이 주석은 검증 안하는 테스트인가요? 그렇다면 지워도 괜찮을 것 같아ㅏ요!
📌 관련 이슈
✨ 변경 내용
API 설명
집계 기준
🎸 기타 참고 사항