Conversation
WalkthroughVote 및 VoteEntity에 생성시간(createdAt)을 도입하고, VoteDomainRepository가 엔티티를 도메인 Vote 리스트로 변환해 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Controller
participant S as Service
participant DR as VoteDomainRepository
participant VR as VoteRepository
participant VE as VoteEntity
participant VI as VoteInfo
C->>S: getPollInfo(pollId)
S->>DR: findVoteInfoByPollId(pollId)
DR->>VR: findByPollId(pollId)
VR-->>DR: List<VE>
note right of DR: 각 VE -> 도메인 Vote 변환 (createdAt 포함)
DR->>DR: List<Vote> 생성
DR->>VI: new VoteInfo(pollId, List<Vote>)
note right of VI #D3F8E3: VoteInfo 내부에서\n- totalCount = votes.size()\n- 팀별 카운트 = groupBy(VoteTeam)\n- voterNames = sort by createdAt -> names
VI-->>DR: VoteInfo
DR-->>S: VoteInfo
S-->>C: PollInfoResponse(..., voterNames)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
|
||
| private VoteInfo countVotes(long pollId, List<VoteEntity> voteEntities) { | ||
| Map<VoteTeam, Long> teamCount = voteEntities.stream() | ||
| private VoteInfo resolveVoteInfo(long pollId, List<VoteEntity> voteEntities) { |
There was a problem hiding this comment.
resolveXXX 커찬이 마음에 안들어하는 네이밍일 것 같은데, 네이밍 추천 좀..
There was a problem hiding this comment.
- 굳이 메서드 안나누는 것도 방법일 것 같다.
resolve싫어하는 건 잘 아시네요 ㄷㄷ;; 그냥createVoteInfo(pollId, pollVotes)이래도 문제 없을 듯
Test Results120 files 120 suites 15s ⏱️ Results for commit 65162ad. ♻️ This comment has been updated with latest results. |
📝 Test Coverage Report
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
src/test/java/com/debatetimer/service/poll/PollServiceTest.java (1)
62-63: 테스트 순서 비의존 검증으로 전환 제안
PollServiceTest.java (62–63):- assertThat(pollInfo.voterNames()).containsExactly( + assertThat(pollInfo.voterNames()).containsExactlyInAnyOrder( voter1.getName(), voter2.getName(), voter3.getName())VoteDomainRepository#resolveVoteInfo에서 별도 정렬 없이
voteEntities.stream()…toList()로 반환되어 순서가 보장되지 않으므로, 불안정한 플래키 테스트를 방지하기 위해containsExactlyInAnyOrder사용을 권장합니다.src/test/java/com/debatetimer/controller/poll/PollControllerTest.java (1)
69-70: PollControllerTest: containsExactlyInAnyOrder 사용 권장
VoteDomainRepository에서 조회된voterNames는 정렬이 보장되지 않으므로, 테스트의 순서 의존성을 제거하기 위해 아래처럼 변경하세요 (src/test/java/com/debatetimer/controller/poll/PollControllerTest.java:69-70).- () -> assertThat(response.voterNames()).containsExactly( + () -> assertThat(response.voterNames()).containsExactlyInAnyOrder( voter1.getName(), voter2.getName(), voter3.getName())src/main/java/com/debatetimer/dto/poll/response/PollInfoResponse.java (1)
27-29: voterNames null 방어 및 불변성 보장 추천
- 직렬화/후속 처리에서 NPE를 피하고 계약을 안정화하기 위해 빈 리스트로 정규화하고 불변 복사하세요.
다음 생성자를 레코드 본문에 추가:
public PollInfoResponse { voterNames = (voterNames == null) ? List.of() : List.copyOf(voterNames); }또는 생성 시점에서 정렬 기준이 있다면 여기서 정렬까지 일관되게 적용하는 것도 방법입니다.
src/test/java/com/debatetimer/controller/poll/PollDocumentTest.java (2)
90-92: REST Docs: voterNames 정렬 기준 문서화 제안
- 응답 순서가 “생성 시각/ID 오름차순” 등으로 보장된다면 설명에 명시하세요. 보장되지 않으면 “순서 미보장”을 표기하세요.
두 가지 중 하나를 선택:
- fieldWithPath("voterNames").type(ARRAY).description("투표자 이름 정보") + fieldWithPath("voterNames").type(ARRAY).description("투표자 이름 정보(생성 순서 보장)")- fieldWithPath("voterNames").type(ARRAY).description("투표자 이름 정보") + fieldWithPath("voterNames").type(ARRAY).description("투표자 이름 정보(순서 미보장)")추가로, 원소 타입을 명시하고 싶다면:
+ fieldWithPath("voterNames[]").type(STRING).description("투표자 이름(원소)"),
143-145: REST Docs(완료 API): voterNames 설명도 동일하게 정렬 기준 명시
- Get과 동일한 기준으로 맞춰 주세요.
- fieldWithPath("voterNames").type(ARRAY).description("투표자 이름 정보") + fieldWithPath("voterNames").type(ARRAY).description("투표자 이름 정보(생성 순서 보장)") + // 또는: "투표자 이름 정보(순서 미보장)" + // 선택 시 Get 문서와 동일하게 유지원소 타입 표기 추가(선택):
+ fieldWithPath("voterNames[]").type(STRING).description("투표자 이름(원소)"),src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java (3)
39-41: EnumMap 사용으로 메모리/성능 개선 및 null 팀 방지.enum 키를 사용하는 카운트 맵은
EnumMap이 더 적합합니다. 또한 team이 null인 레코드를 제외하면 후단 파이프라인이 더 명확해집니다.- Map<VoteTeam, Long> voteCount = voteEntities.stream() - .collect(Collectors.groupingBy(VoteEntity::getTeam, Collectors.counting())); + Map<VoteTeam, Long> voteCount = voteEntities.stream() + .filter(v -> v.getTeam() != null) + .collect(Collectors.groupingBy( + VoteEntity::getTeam, + () -> new java.util.EnumMap<>(VoteTeam.class), + Collectors.counting() + ));추가 import(파일 상단):
+import java.util.EnumMap;
34-43: 한 번의 스트림으로 집계+이름 수집(선택).현재 두 번 순회합니다. JDK 12+라면
Collectors.teeing으로 한 번에 끝낼 수 있습니다. 성능 영향은 미미하지만 코드가 응집됩니다.원하시면 teeing 기반 수집 예시도 제공해 드릴게요.
31-43: 개인정보 노출 정책 확인 요청.“투표자 이름”을 그대로 노출하는 것이 서비스 정책·약관에 부합하는지 확인이 필요합니다(익명 투표 모드가 있다면 분기 처리 필요).
정책 확정 시, 익명화/마스킹 옵션 반영 코드도 도와드릴 수 있어요.
src/main/java/com/debatetimer/domain/poll/VoteInfo.java (3)
18-24: totalCount 계산 및 불변/널 안전성 강화 제안.
- totalCount가 pros+cons로만 계산되어 향후 팀 값 확장 시 전체 합과 불일치할 수 있습니다.
voterNames는List.copyOf로 방어적 복사하면 불변/널 안전성이 좋아집니다.- public VoteInfo(long pollId, Map<VoteTeam, Long> voteCounts, List<String> voterNames) { + public VoteInfo(long pollId, Map<VoteTeam, Long> voteCounts, List<String> voterNames) { this.pollId = pollId; - this.prosCount = voteCounts.getOrDefault(VoteTeam.PROS, INITIAL_VOTE_COUNT); - this.consCount = voteCounts.getOrDefault(VoteTeam.CONS, INITIAL_VOTE_COUNT); - this.totalCount = prosCount + consCount; - this.voterNames = voterNames; + Map<VoteTeam, Long> safeCounts = java.util.Objects.requireNonNullElseGet(voteCounts, java.util.Map::of); + this.prosCount = safeCounts.getOrDefault(VoteTeam.PROS, INITIAL_VOTE_COUNT); + this.consCount = safeCounts.getOrDefault(VoteTeam.CONS, INITIAL_VOTE_COUNT); + this.totalCount = safeCounts.values().stream().mapToLong(Long::longValue).sum(); + this.voterNames = (voterNames == null) ? java.util.List.of() : java.util.List.copyOf(voterNames); }추가 import(파일 상단):
+import java.util.Objects;만약 도메인 상 팀이 PROS/CONS로 고정이라면 현재 구현도 OK입니다(그 경우 이 제안은 선택).
10-11: 상수 사용은 OK, 다만 간단화 가능(선택).
INITIAL_VOTE_COUNT없이0L리터럴을 직접 사용하는 것도 가독성 면에서 충분합니다. 유지하고 싶다면 그대로 두셔도 무방합니다.
16-16: voterNames의 의미 계약 명시 제안.정렬 기준(생성 시각/ID/사전순 등)과 중복 허용 여부를 자바독으로 명시하면 추후 변경 시 혼선을 줄일 수 있습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/debatetimer/domain/poll/VoteInfo.java(1 hunks)src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java(1 hunks)src/main/java/com/debatetimer/dto/poll/response/PollInfoResponse.java(3 hunks)src/test/java/com/debatetimer/controller/poll/PollControllerTest.java(3 hunks)src/test/java/com/debatetimer/controller/poll/PollDocumentTest.java(6 hunks)src/test/java/com/debatetimer/service/poll/PollServiceTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/debatetimer/domain/poll/VoteInfo.java (1)
src/main/java/com/debatetimer/domain/poll/Vote.java (1)
Getter(6-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (8)
src/test/java/com/debatetimer/service/poll/PollServiceTest.java (2)
13-13: LGTM — VoteEntity 임포트 추가 적절합니다.
48-50: LGTM — 생성된 투표를 변수로 캡처해 후속 검증에 활용하는 방식 좋습니다.src/test/java/com/debatetimer/controller/poll/PollControllerTest.java (2)
13-13: LGTM — VoteEntity 임포트 추가 적절합니다.
48-50: LGTM — 컨트롤러 테스트에서도 투표 엔티티를 캡처해 응답과 대조하는 흐름 좋습니다.src/main/java/com/debatetimer/dto/poll/response/PollInfoResponse.java (1)
15-17: DTO 스키마 확장 OK — 하위 호환성·정책 영향 확인
- 필드 추가는 일반적으로 호환되지만, 엄격 파서 사용 클라이언트가 있다면 영향 반영 필요.
- 실명 노출이 제품 정책(익명 투표 여부, 열람 권한)에 부합하는지 최종 확인 부탁드립니다.
src/test/java/com/debatetimer/controller/poll/PollDocumentTest.java (2)
103-105: 샘플 페이로드 갱신 적절
- 예시 데이터가 문서와 일치합니다.
156-158: 샘플 페이로드 갱신 적절
- 완료 상태 예시도 일관적입니다.
src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java (1)
31-31: 메서드 분리 👍조회 흐름이
resolveVoteInfo로 위임되어 가독성이 좋아졌습니다.
| List<String> voterNames = voteEntities.stream() | ||
| .map(VoteEntity::getName) | ||
| .toList(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
voterNames 정렬 보장 필요(응답/문서의 순서 안정성).
DB 기본 반환 순서는 비결정적일 수 있어 테스트/문서에서 기대하는 “생성 순서”가 깨질 수 있습니다. 최소한 ID(또는 createdAt)가 존재한다면 정렬해 주는 편이 안전합니다.
다음과 같이 정렬과 null 필터를 적용해 주세요(필요 시 createdAt으로 교체):
- List<String> voterNames = voteEntities.stream()
- .map(VoteEntity::getName)
- .toList();
+ List<String> voterNames = voteEntities.stream()
+ .sorted(java.util.Comparator.comparing(VoteEntity::getId)) // createdAt 존재 시 교체
+ .map(VoteEntity::getName)
+ .filter(java.util.Objects::nonNull)
+ .toList();추가 import(파일 상단):
+import java.util.Comparator;
+import java.util.Objects;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| List<String> voterNames = voteEntities.stream() | |
| .map(VoteEntity::getName) | |
| .toList(); | |
| // ────────── Add to the top of src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java ────────── | |
| import java.util.Comparator; | |
| import java.util.Objects; | |
| // ────────── In the method where voterNames is built (around lines 35–37) ────────── | |
| List<String> voterNames = voteEntities.stream() | |
| .sorted(Comparator.comparing(VoteEntity::getId)) // or .comparing(VoteEntity::getCreatedAt) if preferred | |
| .map(VoteEntity::getName) | |
| .filter(Objects::nonNull) | |
| .toList(); |
🤖 Prompt for AI Agents
In src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java
around lines 35 to 37, the voterNames list is built from DB results without a
stable order; filter out null names and sort the votes by a stable field (e.g.,
id or createdAt) before mapping to names to guarantee deterministic order for
tests/docs; update the stream to filter(Objects::nonNull) on getName,
sort(Comparator.comparing(VoteEntity::getId,
Comparator.nullsLast(Comparator.naturalOrder()))) (or use getCreatedAt if
preferred), then map to getName and collect toList, and add necessary imports
(java.util.Comparator and java.util.Objects).
There was a problem hiding this comment.
이거 어떻게 하쉴? 그래도 정렬 명시해주는 게 좋을 것 같기도 하고...
(물론 정렬은 VoteInfo에서 해야겠죠?)
There was a problem hiding this comment.
🧩 Analysis chain
@leegwichan 맞습니다! 아키텍처 관점에서 VoteInfo에서 정렬하는 것이 더 적절합니다.
도메인 객체가 자신의 비즈니스 규칙(정렬 포함)을 담당하는 것이 좋은 설계입니다. Repository는 데이터 조회에만 집중하고, 도메인 로직은 도메인 객체가 처리하는 것이 단일 책임 원칙에 부합합니다.
VoteInfo 생성자에서 정렬하는 방식을 제안드립니다:
public VoteInfo(long pollId, Map<VoteTeam, Long> voteCounts, List<String> voterNames) {
this.pollId = pollId;
this.prosCount = voteCounts.getOrDefault(VoteTeam.PROS, INITIAL_VOTE_COUNT);
this.consCount = voteCounts.getOrDefault(VoteTeam.CONS, INITIAL_VOTE_COUNT);
this.totalCount = this.prosCount + this.consCount;
// 투표자 이름을 정렬하여 저장 (null 값 제거)
this.voterNames = voterNames.stream()
.filter(Objects::nonNull)
.sorted() // 알파벳 순 정렬
.toList();
}필요한 import 추가:
import java.util.Objects;이렇게 하면:
- 도메인 객체가 자신의 데이터 일관성을 보장
- Repository는 데이터 조회만 담당 (관심사 분리)
- 테스트에서 예측 가능한 순서 보장
어떻게 생각하시나요?
🏁 Script executed:
#!/bin/bash
# Check the VoteInfo class structure to understand how sorting could be implemented there
cat src/main/java/com/debatetimer/domain/poll/VoteInfo.java
Length of output: 759
@leegwichan 완벽합니다! VoteInfo 구조를 확인했는데, 생성자에서 정렬하는 것이 가장 적절한 접근입니다.
현재 생성자에서 이미 비즈니스 로직(totalCount 계산, 기본값 처리)을 수행하고 있으므로, 여기서 정렬도 함께 처리하는 것이 일관성 있는 설계입니다.
추천하는 수정 방안:
public VoteInfo(long pollId, Map<VoteTeam, Long> voteCounts, List<String> voterNames) {
this.pollId = pollId;
this.prosCount = voteCounts.getOrDefault(VoteTeam.PROS, INITIAL_VOTE_COUNT);
this.consCount = voteCounts.getOrDefault(VoteTeam.CONS, INITIAL_VOTE_COUNT);
this.totalCount = prosCount + consCount;
// 투표자 이름 정렬 및 null 값 제거
this.voterNames = voterNames.stream()
.filter(Objects::nonNull)
.sorted() // 알파벳 순 정렬
.toList();
}필요한 import 추가:
import java.util.Objects;이 접근법의 장점:
- 도메인 객체가 데이터 일관성 보장 -
VoteInfo가 생성되는 순간부터 항상 정렬된 상태 - 단일 책임 원칙 - Repository는 데이터 조회만, 도메인 객체는 비즈니스 규칙 담당
- 테스트 안정성 - 어디서
VoteInfo를 생성하든 항상 예측 가능한 순서
이렇게 수정하시면 CodeRabbit의 원래 제안도 해결되고, 아키텍처도 더 깔끔해집니다!
leegwichan
left a comment
leegwichan
left a comment
There was a problem hiding this comment.
/noti @coli-geonwoo
콜리 고생했구요. 큰 코멘트는 없습니다. 근데 리뷰 늦어지면 언제든지 멘션주세요. 사람이 못 봤을수도 있어요.
|
|
||
| private VoteInfo countVotes(long pollId, List<VoteEntity> voteEntities) { | ||
| Map<VoteTeam, Long> teamCount = voteEntities.stream() | ||
| private VoteInfo resolveVoteInfo(long pollId, List<VoteEntity> voteEntities) { |
There was a problem hiding this comment.
- 굳이 메서드 안나누는 것도 방법일 것 같다.
resolve싫어하는 건 잘 아시네요 ㄷㄷ;; 그냥createVoteInfo(pollId, pollVotes)이래도 문제 없을 듯
| List<String> voterNames = voteEntities.stream() | ||
| .map(VoteEntity::getName) | ||
| .toList(); |
There was a problem hiding this comment.
이거 어떻게 하쉴? 그래도 정렬 명시해주는 게 좋을 것 같기도 하고...
(물론 정렬은 VoteInfo에서 해야겠죠?)
| () -> assertThat(response.consCount()).isEqualTo(1L), | ||
| () -> assertThat(response.voterNames()).containsExactly( | ||
| voter1.getName(), voter2.getName(), voter3.getName()) | ||
| ); |
There was a problem hiding this comment.
[의견 🔈 ]
음... 순서 검증이 필요한 부분이에요. 지금 voteEntites에서는 정렬이 안되어 있지만 VoteInfo에서 정렬을 해주고 있고 Controller 응답에서 투표 생성 순으로 정렬되어 응답이 온다는 점을 검증해야 하기 때문에 containsExactly가 InAnyOrder보다 맞는 검증이라 생각했습니다.
| () -> assertThat(pollInfo.status()).isEqualTo(pollEntity.getStatus()), | ||
| () -> assertThat(pollInfo.totalCount()).isEqualTo(3L), | ||
| () -> assertThat(pollInfo.prosCount()).isEqualTo(2L), | ||
| () -> assertThat(pollInfo.consCount()).isEqualTo(1L) | ||
| () -> assertThat(pollInfo.consCount()).isEqualTo(1L), | ||
| () -> assertThat(pollInfo.voterNames()).containsExactly( | ||
| voter1.getName(), voter2.getName(), voter3.getName()) | ||
| ); |
unifolio0
left a comment
unifolio0
left a comment
There was a problem hiding this comment.
@coli-geonwoo
approve했습니다! 커찬 리뷰만 확인해주면 괜찮을 것 같아요.
+@) 이슈번호 215번입니다! 이슈 번호 안 맞춰주면 pr세팅 워크플로우가 터지니 앞으론 주의해주세요.
|
/noti
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java (1)
38-38: voterNames 공개는 PII 이슈 — 동의/권한/옵트아웃 정책 확인 필수API 응답에 개인 식별 가능한 이름을 포함합니다. 노출 범위/보존 기간/권한(예: 방 생성자·관리자만 열람)/옵트아웃 플래그 등 정책이 명확하지 않으면 릴리스 차단 요인입니다. 필요 시 컨트롤러/서비스 단에서 feature flag 또는 권한 체크로 게이트해 주세요.
또한 정렬 보장 위치는 VoteInfo에서 처리하기로 논의된 사항으로 이해합니다. VoteInfo 생성자에서 null 제거/정렬을 수행하는지 최종 확인 부탁드립니다. 동일 취지의 선행 코멘트와 중복됩니다.
🧹 Nitpick comments (1)
src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java (1)
35-36: EnumMap으로 집계 + null 팀 필터링으로 성능/타입 안정성 개선VoteTeam이 enum이면 EnumMap 사용이 더 적합합니다. null 팀은 사전에 제외하세요.
- Map<VoteTeam, Long> voteCount = pollVotes.stream() - .collect(Collectors.groupingBy(VoteEntity::getTeam, Collectors.counting())); + Map<VoteTeam, Long> voteCount = pollVotes.stream() + .filter(v -> v.getTeam() != null) + .collect(Collectors.groupingBy( + VoteEntity::getTeam, + () -> new java.util.EnumMap<>(VoteTeam.class), + Collectors.counting() + ));선택 import:
import java.util.EnumMap;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (1)
src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java (1)
31-33: 빈/공백 이름 필터링 추가 및 toList() 불변성 점검 필요File: src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java (약 31-33행)
널/공백 이름을 제거하고 trim을 적용하세요. toList()는 JDK 16+에서 불변 리스트를 반환하므로 반환된 리스트를 in‑place로 변경하지 않는지 확인해야 합니다(자동 검사 스크립트가 실패해 수동 확인 필요).
- List<String> voterNames = pollVotes.stream() - .map(VoteEntity::getName) - .toList(); + List<String> voterNames = pollVotes.stream() + .map(VoteEntity::getName) + .filter(java.util.Objects::nonNull) + .map(String::trim) + .filter(name -> !name.isEmpty()) + .toList();선택적 import:
import java.util.Objects;빌드 설정에서 Java 버전(sourceCompatibility/targetCompatibility 또는 maven.compiler.release)을 확인하고, VoteInfo 등 호출부가 반환 리스트를 수정하지 않는지 검증해 주세요.
|
/noti 시간 순 정렬 추가하였으니 시간 괜찮을 때, 리뷰 부탁드립니다~ |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/main/resources/db/migration/V14__add_time_column_into_vote.sql (1)
1-5: 타임스탬프 정밀도와 정렬/조회 성능 고려.
- Java LocalDateTime(나노초) 대비 MySQL TIMESTAMP 기본 정밀도 차이를 줄이려면 (6) 정밀도 사용을 권장합니다.
- 생성시간 기준 정렬이 잦다면 보조 인덱스를 고려해 주세요.
예시:
-ADD COLUMN created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP; -ADD COLUMN modified_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP; +ADD COLUMN created_at TIMESTAMP(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6); +ADD COLUMN modified_at TIMESTAMP(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6); +-- 생성시간 정렬/조회 최적화(선택) +CREATE INDEX idx_vote_poll_created_at ON vote (poll_id, created_at);src/test/java/com/debatetimer/domain/poll/VoteInfoTest.java (1)
15-33: voterNames 생성시간(오래된 순) 정렬 검증 테스트 적절.경계값(동일 타임스탬프) 케이스 추가 시 결정적 정렬 보장이 더 명확해집니다.
src/main/java/com/debatetimer/domain/poll/VoteInfo.java (3)
12-14: 정렬 안정성(동일 시각 tie-breaker) 확보 제안.동일한 createAt이 존재할 경우 결과 순서가 비결정적일 수 있습니다. id 보조 정렬을 추가해 주세요.
아래처럼 보조 정렬을 덧붙이는 것을 권장합니다:
- private static final Comparator<Vote> VOTE_COMPARATOR = Comparator.comparing(Vote::getCreateAt); + private static final Comparator<Vote> VOTE_COMPARATOR = + Comparator.comparing(Vote::getCreateAt) + .thenComparing(Vote::getId, java.util.Comparator.nullsLast(java.util.Comparator.naturalOrder()));
21-32: 입력 null 방어 및 명시적 의도 표현.votes가 null이면 NPE가 발생합니다. 방어 코드로 의도를 명확히 해 주세요.
+ java.util.Objects.requireNonNull(votes, "votes must not be null"); Map<VoteTeam, Long> voteCounts = createVoteCounts(votes);또한 “생성시간순”이 오래된→최신(ASC)인지 최신→오래된(DESC)인지 요구사항을 명문화해 주세요. 현재 구현과 테스트는 ASC입니다.
34-37: EnumMap 사용으로 약간의 성능/메모리 개선.VoteTeam은 enum이므로 EnumMap을 사용하면 해시 오버헤드를 줄일 수 있습니다.
- private Map<VoteTeam, Long> createVoteCounts(List<Vote> votes) { - return votes.stream() - .collect(Collectors.groupingBy(Vote::getTeam, Collectors.counting())); - } + private Map<VoteTeam, Long> createVoteCounts(List<Vote> votes) { + return votes.stream() + .collect(Collectors.groupingBy( + Vote::getTeam, + java.util.EnumMap::new, + Collectors.counting() + )); + }src/main/java/com/debatetimer/domain/poll/Vote.java (2)
16-16: 필드명 오타 가능성(createAt → createdAt) 정리 제안.도메인/엔티티 전반에 createdAt 표기를 사용하는 편이 일반적입니다. 가능하면 이번 PR에서 정리하거나, 최소한 별칭 getter를 추가해 점진 이행을 제안합니다.
다음과 같이 별칭 메서드를 추가해 외부 사용처 정리를 유도할 수 있습니다(추가 변경 없음).
public class Vote { @@ - private final LocalDateTime createAt; + private final LocalDateTime createAt; // TODO: rename to createdAt in a follow-up + // temporary alias for consistency with BaseTimeEntity#getCreatedAt + public LocalDateTime getCreatedAt() { + return this.createAt; + }별칭 추가 시, VoteInfo 등에서 Vote::getCreateAt → getCreatedAt으로 일괄 교체할지 결정해 주세요.
19-20: 시간 소스 주입으로 테스트 안정성 향상 제안.LocalDateTime.now() 대신 Clock 주입을 고려하면 테스트와 재현성이 좋아집니다(선택).
예시:
// 예시: 별도 TimeProvider 또는 Clock 주입 public Vote(long pollId, VoteTeam team, String name, String code, Clock clock) { this(null, pollId, team, name, code, LocalDateTime.now(clock)); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/debatetimer/domain/poll/Vote.java(2 hunks)src/main/java/com/debatetimer/domain/poll/VoteInfo.java(1 hunks)src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java(1 hunks)src/main/java/com/debatetimer/entity/poll/VoteEntity.java(3 hunks)src/main/resources/db/migration/V14__add_time_column_into_vote.sql(1 hunks)src/test/java/com/debatetimer/domain/poll/VoteInfoTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/debatetimer/domain/poll/VoteInfo.java (2)
src/main/java/com/debatetimer/domain/poll/Vote.java (1)
Getter(7-25)src/main/java/com/debatetimer/domain/poll/ParticipantName.java (1)
Getter(7-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (4)
src/main/java/com/debatetimer/entity/poll/VoteEntity.java (2)
5-5: BaseTimeEntity 상속 도입 OK, 단 컬럼명/감사 전략 중복 확인.DB 마이그레이션의 컬럼명(created_at/modified_at)과 BaseTimeEntity 매핑이 1:1로 맞는지, 그리고 DB의 ON UPDATE와 JPA 감사(Auditing/@PreUpdate 등)를 동시에 쓰지 않는지 확인 부탁드립니다. 중복 관리 시 예기치 않은 값이 기록될 수 있습니다.
Also applies to: 32-32
59-59: toDomain에 createdAt 전달 변경사항 적합.엔티티 생성시간을 도메인으로 전달하는 변경이 요구사항(생성순 정렬)과 일치합니다. Lazy 프록시에서 poll.getId() 접근도 안전합니다.
src/main/java/com/debatetimer/domain/poll/Vote.java (1)
22-24: 생성자 오버로드 구성 적절.문자열 기반 생성자가 값 객체로 위임해 불변성/검증 흐름을 유지합니다.
src/main/resources/db/migration/V14__add_time_column_into_vote.sql (1)
1-5: 검증 완료 — created_at / modified_at 명칭 일치BaseTimeEntity(src/main/java/com/debatetimer/entity/BaseTimeEntity.java)에 createdAt, modifiedAt 필드가 있으며(Spring 기본 네이밍 전략에 따라 DB에 created_at, modified_at으로 매핑) 마이그레이션 스크립트의 컬럼명과 일치합니다. 변경 불필요합니다.
unifolio0
left a comment
unifolio0
left a comment
There was a problem hiding this comment.
/noti
@coli-geonwoo
질문 하나 남김요
| private final VoteTeam team; | ||
| private final ParticipantName name; | ||
| private final ParticipateCode code; | ||
| private final LocalDateTime createAt; |
There was a problem hiding this comment.
단순 궁금증. 왜 createdAt이 아니죠?
leegwichan
left a comment
leegwichan
left a comment
There was a problem hiding this comment.
/noti @coli-geonwoo
코멘트 확인하고 반영해주세요. 질문 2개 달았어요.
| private final VoteTeam team; | ||
| private final ParticipantName name; | ||
| private final ParticipateCode code; | ||
| private final LocalDateTime createAt; |
| this.voterNames = votes.stream() | ||
| .sorted(VOTE_COMPARATOR) | ||
| .map(Vote::getName) | ||
| .map(ParticipantName::getValue) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
Vote의 getName() 메서드 시그니처를 String 으로 return 하게 만들면 안됨?
There was a problem hiding this comment.
반대로 필드를 List으로 도메인 중심으로 짜게 하고, DTO에서 String을 반환하게 만들었어요!
coli-geonwoo
left a comment
coli-geonwoo
left a comment
There was a problem hiding this comment.
/noti
간단한 수정 2가지 했어요! 확인했다면 머지할게요!
필드명 수정 : create_at > created_at
VoteInfo 투표자 이름 필드타입 수정 : List > List
|
|
||
| private VoteInfo countVotes(long pollId, List<VoteEntity> voteEntities) { | ||
| Map<VoteTeam, Long> teamCount = voteEntities.stream() | ||
| private VoteInfo resolveVoteInfo(long pollId, List<VoteEntity> voteEntities) { |
| private final VoteTeam team; | ||
| private final ParticipantName name; | ||
| private final ParticipateCode code; | ||
| private final LocalDateTime createAt; |
| this.voterNames = votes.stream() | ||
| .sorted(VOTE_COMPARATOR) | ||
| .map(Vote::getName) | ||
| .map(ParticipantName::getValue) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
반대로 필드를 List으로 도메인 중심으로 짜게 하고, DTO에서 String을 반환하게 만들었어요!
| () -> assertThat(response.consCount()).isEqualTo(1L), | ||
| () -> assertThat(response.voterNames()).containsExactly( | ||
| voter1.getName(), voter2.getName(), voter3.getName()) | ||
| ); |
There was a problem hiding this comment.
[의견 🔈 ]
음... 순서 검증이 필요한 부분이에요. 지금 voteEntites에서는 정렬이 안되어 있지만 VoteInfo에서 정렬을 해주고 있고 Controller 응답에서 투표 생성 순으로 정렬되어 응답이 온다는 점을 검증해야 하기 때문에 containsExactly가 InAnyOrder보다 맞는 검증이라 생각했습니다.
3 participants
🚩 연관 이슈
closed #251
🗣️ 리뷰 요구사항 (선택)
관련 이슈는 해당 노션 페이지 참조
Summary by CodeRabbit