[REFACTOR] CustomizeTimeBoxEntities 제거#214
Merged
leegwichan merged 5 commits intodevelopfrom Aug 10, 2025
Merged
Conversation
Walkthrough
Changes
Sequence Diagram(s)(생성할 시나리오가 단순한 내부 변환 로직 리팩토링에 불과하므로 시퀀스 다이어그램 생략) Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(요구사항 외 변경사항 없음) Possibly related PRs
Suggested reviewers
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Test Results119 files 119 suites 14s ⏱️ Results for commit 8581c4d. ♻️ This comment has been updated with latest results. |
📝 Test Coverage Report
|
coli-geonwoo
requested changes
Aug 5, 2025
Contributor
coli-geonwoo
left a comment
coli-geonwoo
left a comment
There was a problem hiding this comment.
/noti
커찬, 빠른 작업 감사합니다 🙇 간단한 몇가지 생각남겼슴다. 바쁜 거 아니까 천천히 리뷰해주세요. 의견 듣고 싶어 일단 RC로 둘게요!
| .toList(); | ||
| } | ||
|
|
||
| @Transactional |
Contributor
There was a problem hiding this comment.
[제안]
isContained로 O(N)으로 계속 탐색하는게 마음에 들지 않아서 + 해석해야 할 로직이 많은 것 같아 스트림 최대한 활용해본 건데 커찬 기호에 맞게 도입해도 좋을 것 같아요. 지금 커찬 로직도 좋은 것 같아요.
private List<CustomizeTimeBox> toCustomizeTimeBoxes2(
List<CustomizeTimeBoxEntity> timeBoxEntities,
List<BellEntity> bellEntities
) {
Map<Long, List<BellEntity>> groupingBellEntities = bellEntities.stream()
.collect(Collectors.groupingBy(bellEntity -> bellEntity.getCustomizeTimeBox().getId()));
return timeBoxEntities.stream()
.map(timeBox -> toTimeBox(timeBox, groupingBellEntities.get(timeBox.getId())))
.toList();
}
private CustomizeTimeBox toTimeBox(CustomizeTimeBoxEntity timeBoxEntity, List<BellEntity> bellEntities) {
return bellEntities.stream()
.map(BellEntity::toDomain)
.collect(Collectors.collectingAndThen(Collectors.toList(), timeBoxEntity::toDomain));
}| .toList(); | ||
| } | ||
|
|
||
| private List<Bell> getBells(CustomizeTimeBoxEntity timeBox, List<BellEntity> bells) { |
Contributor
There was a problem hiding this comment.
[사소한 제안]
엔티티랑 도메인이랑 분리되면서 필드명에서도 timeBoxEntity, bellEntities 처럼 엔티티는 엔티티 suffix를 붙여도 좋을 것 같은데 어떤가요?
| private List<CustomizeTimeBox> toCustomizeTimeBoxes(List<CustomizeTimeBoxEntity> timeBoxEntities, | ||
| List<BellEntity> bellEntities) { | ||
| return timeBoxEntities.stream() | ||
| .map(timebox -> timebox.toDomain(getBells(timebox, bellEntities))) |
Contributor
There was a problem hiding this comment.
[typo]
Suggested change
| .map(timebox -> timebox.toDomain(getBells(timebox, bellEntities))) | |
| .map(timeBox -> timeBox.toDomain(getBells(timebox, bellEntities))) |
| return toCustomizeTimeBoxes(timeBoxEntityList, bellEntityList); | ||
| } | ||
|
|
||
| private List<CustomizeTimeBox> toCustomizeTimeBoxes(List<CustomizeTimeBoxEntity> timeBoxEntities, |
Contributor
There was a problem hiding this comment.
[개인적인 느낌]
지금은 충분히 용납가능한 수준이지만 Domain Repository에 List로 매핑하는 메서드가 3개 이상되다 보니까 타고타고 들어가서 로직을 보게 되는 경향이 있는 것 같아요.
내가 코드를 읽었던 흐름 : getCustomizeTimeBoxes > toCustomizetimeBoxes > getBells > 다시 toCustomizeTimeBoxes로 돌아와 로직 이해
이건 어떤가요? CustomizeTimeBoxes를 만들고
CustomizeTimeBoxes(List customizeTimeBoxes, List bells) 로 생성자를 열되 지금 있는 매핑 로직을 도메인 일급 컬렉션에 넣는 거에요.
도메인 단의 일급 컬렉션은 타임박스들의 연속된 배열을 하나의 객체로 바라보고 역할을 분담해 줄 수 있다는 점에서 매력적인 것 같아서요. 아마 커찬은 꺼려할 것 같긴 한데 한번 고려해주세유 👍
Member
Author
There was a problem hiding this comment.
이거는 대형 공사가 되서... (메서드 시그니처 바꾸려면, 서비스부터 도메인 레포지토리 테스트까지 전부 바꿔야 함)
PR 나눠서 진행해야 될 것 같아요. 다음 스프린트 TODO List에 넣어놓을께요~!
| class CustomizeTimeBoxEntitiesTest { | ||
|
|
||
| @Nested | ||
| class SortedBySequence { |
Contributor
There was a problem hiding this comment.
[질문]
혹시 이 테스트를 옮기지 않고 그냥 삭제만 한 이유 물어봐도 되나요?
Member
Author
There was a problem hiding this comment.
CustomizeTableDomainRepositoryTest > GetCustomizeTimeBoxes > 테이블의_시간박스는_순서대로_가져온다()
해당 테스트에 반영했습니다~
unifolio0
approved these changes
Aug 7, 2025
Contributor
unifolio0
left a comment
unifolio0
left a comment
There was a problem hiding this comment.
/noti
@leegwichan
콜리 의견만 확인해주면 제가 더 이상 언급할 건 없는 것 같아요. approe하겠습니다.
leegwichan
commented
Aug 10, 2025
Member
Author
leegwichan
left a comment
leegwichan
left a comment
There was a problem hiding this comment.
/noti @coli-geonwoo
일부 반영했습니다~ 리뷰 부탁드려요
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/java/com/debatetimer/fixture/entity/CustomizeTimeBoxEntityGenerator.java (1)
32-46: 오버로드 추가는 적절합니다. 기본 메서드로 위임해 중복을 제거하세요.현재 두 generate 메서드가 거의 동일한 객체 생성 로직을 중복합니다. DRY 관점에서 기본값(180초)을 사용하는 기존 메서드가 새 오버로드로 위임하도록 정리하면 유지보수가 쉬워집니다. 또한 180, "입론", "콜리" 등 매직 리터럴은 상수로 추출해 의미를 드러내는 것도 권장합니다.
기존 메서드(변경 범위 밖) 예시:
public CustomizeTimeBoxEntity generate(CustomizeTableEntity testTable, CustomizeBoxType boxType, int sequence) { return generate(testTable, boxType, sequence, 180); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/debatetimer/domainrepository/customize/CustomizeTableDomainRepository.java(2 hunks)src/test/java/com/debatetimer/domainrepository/customize/CustomizeTableDomainRepositoryTest.java(1 hunks)src/test/java/com/debatetimer/fixture/entity/CustomizeTimeBoxEntityGenerator.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/debatetimer/domainrepository/customize/CustomizeTableDomainRepository.java
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: leegwichan
PR: debate-timer/debate-timer-be#198
File: src/test/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomainTest.java:82-112
Timestamp: 2025-07-15T01:28:24.899Z
Learning: CustomizeTimeBoxDomain 클래스에서는 생성자에서 stance null 검증을 먼저 수행한 후 isValidStance() 추상 메서드를 호출하는 구조로 되어 있어, 테스트용 구현체에서 isValidStance()는 단순히 true를 반환해도 된다.
Learnt from: leegwichan
PR: debate-timer/debate-timer-be#198
File: src/test/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomainTest.java:82-112
Timestamp: 2025-07-15T01:28:24.899Z
Learning: CustomizeTimeBoxDomain에서 validateStance 메서드는 null 체크를 먼저 수행한 후에 isValidStance() 추상 메서드를 호출하는 구조로, 테스트용 구현체에서 isValidStance()가 단순히 true를 반환하는 것이 적절하다.
Learnt from: leegwichan
PR: debate-timer/debate-timer-be#201
File: src/main/java/com/debatetimer/entity/customize/BellEntity.java:50-54
Timestamp: 2025-07-18T01:04:20.985Z
Learning: BellEntity에서는 도메인 레벨에서 이미 유효성 검사가 수행되므로 엔티티 레벨의 유효성 검사는 중복이며, 향후 엔티티의 유효성 검사를 제거할 예정이다.
📚 Learning: 2025-07-15T01:28:24.899Z
Learnt from: leegwichan
PR: debate-timer/debate-timer-be#198
File: src/test/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomainTest.java:82-112
Timestamp: 2025-07-15T01:28:24.899Z
Learning: CustomizeTimeBoxDomain 클래스에서는 생성자에서 stance null 검증을 먼저 수행한 후 isValidStance() 추상 메서드를 호출하는 구조로 되어 있어, 테스트용 구현체에서 isValidStance()는 단순히 true를 반환해도 된다.
Applied to files:
src/test/java/com/debatetimer/fixture/entity/CustomizeTimeBoxEntityGenerator.javasrc/test/java/com/debatetimer/domainrepository/customize/CustomizeTableDomainRepositoryTest.java
📚 Learning: 2025-07-15T01:28:24.899Z
Learnt from: leegwichan
PR: debate-timer/debate-timer-be#198
File: src/test/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomainTest.java:82-112
Timestamp: 2025-07-15T01:28:24.899Z
Learning: CustomizeTimeBoxDomain에서 validateStance 메서드는 null 체크를 먼저 수행한 후에 isValidStance() 추상 메서드를 호출하는 구조로, 테스트용 구현체에서 isValidStance()가 단순히 true를 반환하는 것이 적절하다.
Applied to files:
src/test/java/com/debatetimer/fixture/entity/CustomizeTimeBoxEntityGenerator.javasrc/test/java/com/debatetimer/domainrepository/customize/CustomizeTableDomainRepositoryTest.java
⏰ 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
...test/java/com/debatetimer/domainrepository/customize/CustomizeTableDomainRepositoryTest.java
Show resolved
Hide resolved
...test/java/com/debatetimer/domainrepository/customize/CustomizeTableDomainRepositoryTest.java
Show resolved
Hide resolved
coli-geonwoo
approved these changes
Aug 10, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚩 연관 이슈
closed #213
🗣️ 리뷰 요구사항 (선택)
Summary by CodeRabbit
Refactor
Tests