[REFACTOR] 타임박스 도메인 - 엔티티 분리 / 도메인 구현#198
Conversation
|
""" Walkthrough커스터마이즈 가능한 타임박스 도메인 계층이 새롭게 도입되어, 추상 클래스와 이를 상속하는 구체 클래스(Normal, TimeBased)가 추가되었습니다. 각 클래스는 입력값 검증 및 도메인 로직을 구현하며, 이에 대한 단위 테스트도 각각 작성되었습니다. 기존 Stance enum에는 중립 스탠스 확인 메서드가 추가되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DomainFactory
participant CustomizeTimeBoxDomain
participant NormalTimeBoxDomain
participant TimeBasedTimeBoxDomain
User->>DomainFactory: 타임박스 생성 요청(stance, speechType, speaker, time/팀별 시간 등)
DomainFactory->>+CustomizeTimeBoxDomain: (추상) 생성자 호출 및 입력값 검증
alt Normal 타입
DomainFactory->>+NormalTimeBoxDomain: 생성자 호출 및 time 검증
NormalTimeBoxDomain-->>-DomainFactory: 인스턴스 반환
else TimeBased 타입
DomainFactory->>+TimeBasedTimeBoxDomain: 생성자 호출, stance/시간 검증
TimeBasedTimeBoxDomain-->>-DomainFactory: 인스턴스 반환
end
DomainFactory-->>User: 타임박스 도메인 객체 반환
sequenceDiagram
participant Test
participant DomainClass
participant Exception
Test->>DomainClass: 생성자 호출(유효/무효 입력)
alt 입력값 무효
DomainClass-->>Exception: DTClientErrorException 발생
Exception-->>Test: 예외 처리 및 메시지 확인
else 입력값 유효
DomainClass-->>Test: 정상 인스턴스 생성
end
""" 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (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
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Test Results 82 files 82 suites 12s ⏱️ Results for commit 062e8ed. ♻️ This comment has been updated with latest results. |
📝 Test Coverage Report
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/main/java/com/debatetimer/domain/customize/TimeBasedTimeBoxDomain.java (2)
26-36: 검증 로직 개선을 제안합니다.현재 검증 순서에서 개선할 점이 있습니다.
timePerSpeaking이 null인 경우 조기 반환하지만, 더 명확한 구조로 개선할 수 있습니다.private void validateTimes(Integer timePerTeam, Integer timePerSpeaking) { validateTime(timePerTeam); - if (timePerSpeaking == null) { - return; - } - - validateTime(timePerSpeaking); - if (timePerTeam < timePerSpeaking) { - throw new DTClientErrorException(ClientErrorCode.INVALID_TIME_BASED_TIME); - } + if (timePerSpeaking != null) { + validateTime(timePerSpeaking); + if (timePerTeam < timePerSpeaking) { + throw new DTClientErrorException(ClientErrorCode.INVALID_TIME_BASED_TIME); + } + } }
54-58: getTime() 메서드 반환값을 명확히 문서화하는 것을 제안합니다.시간 기반 타임박스에서 단일 시간 값이 없음을 의미하는 null 반환이지만, 이에 대한 JavaDoc 주석이 있으면 더 명확할 것입니다.
@Override @Nullable +/** + * 시간 기반 타임박스는 단일 시간 값을 갖지 않으므로 null을 반환합니다. + * 대신 {@link #getTimePerTeam()}과 {@link #getTimePerSpeaking()}을 사용하세요. + */ public Integer getTime() { return null; }src/test/java/com/debatetimer/domain/customize/TimeBasedTimeBoxDomainTest.java (1)
4-4: import 문 일관성 개선을 제안합니다.
assertThatThrownBy를AssertionsForClassTypes에서 가져오는 것보다 일반적인Assertions에서 가져오는 것이 더 일관성 있습니다.-import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy; +import static org.assertj.core.api.Assertions.assertThatThrownBy;src/test/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomainTest.java (1)
65-66: 불필요한 빈 줄을 제거하세요.연속된 빈 줄이 있습니다. 하나로 줄여주세요.
} - static class InheritedCustomizeTimeBoxDomain extends CustomizeTimeBoxDomain {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomain.java(1 hunks)src/main/java/com/debatetimer/domain/customize/NormalTimeBoxDomain.java(1 hunks)src/main/java/com/debatetimer/domain/customize/Stance.java(1 hunks)src/main/java/com/debatetimer/domain/customize/TimeBasedTimeBoxDomain.java(1 hunks)src/test/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomainTest.java(1 hunks)src/test/java/com/debatetimer/domain/customize/NormalTimeBoxDomainTest.java(1 hunks)src/test/java/com/debatetimer/domain/customize/TimeBasedTimeBoxDomainTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/main/java/com/debatetimer/domain/customize/TimeBasedTimeBoxDomain.java (1)
src/main/java/com/debatetimer/exception/custom/DTClientErrorException.java (1)
DTClientErrorException(5-10)
src/main/java/com/debatetimer/domain/customize/NormalTimeBoxDomain.java (1)
src/main/java/com/debatetimer/exception/custom/DTClientErrorException.java (1)
DTClientErrorException(5-10)
src/main/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomain.java (1)
src/main/java/com/debatetimer/exception/custom/DTClientErrorException.java (1)
DTClientErrorException(5-10)
⏰ 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 (20)
src/main/java/com/debatetimer/domain/customize/Stance.java (1)
10-12: 유틸리티 메서드 구현이 깔끔합니다.
isNeutralStance()메서드는 명확하고 효율적으로 구현되었습니다. 메서드명이 직관적이며==연산자를 사용한 enum 비교도 적절합니다.src/test/java/com/debatetimer/domain/customize/NormalTimeBoxDomainTest.java (3)
18-24: 매개변수화된 테스트 활용이 효과적입니다.
@ValueSource를 사용한 매개변수화된 테스트로 여러 경계값을 효율적으로 검증하고 있습니다. 0, 음수, 최솟값을 포함한 테스트 케이스가 적절합니다.
26-33: null 검증 테스트가 별도로 구성되어 있어 좋습니다.매개변수화된 테스트와 별도로 null 검증을 분리한 것이 테스트 의도를 명확하게 보여줍니다.
35-42: 성공 케이스 테스트가 포함되어 완전한 테스트 커버리지를 제공합니다.양수 값에 대한 정상 동작 확인으로 테스트가 균형 잡혀 있습니다.
src/main/java/com/debatetimer/domain/customize/TimeBasedTimeBoxDomain.java (4)
7-7: 클래스 선언이 적절합니다.
CustomizeTimeBoxDomain을 확장하여 시간 기반 타임박스 도메인을 구현한 설계가 좋습니다.
9-12: 필드 선언이 명확합니다.
timePerTeam은 필수,timePerSpeaking은 선택적으로 설계된 것이 도메인 로직을 잘 반영합니다.@Nullable어노테이션 사용도 적절합니다.
38-42: 시간 검증 메서드가 재사용 가능하게 잘 설계되었습니다.
validateTime메서드가 null 체크와 양수 검증을 모두 처리하여 코드 중복을 방지했습니다.
44-47: 스탠스 검증 로직이 명확합니다.
stance.isNeutralStance()메서드를 활용하여 중립 스탠스만 허용하는 비즈니스 로직이 잘 구현되었습니다.src/main/java/com/debatetimer/domain/customize/NormalTimeBoxDomain.java (5)
7-7: final 클래스 선언이 적절합니다.
NormalTimeBoxDomain을 final로 선언하여 더 이상 상속되지 않도록 한 설계가 좋습니다.
11-16: 생성자 구현이 깔끔합니다.부모 클래스 생성자 호출 후 검증 로직을 수행하는 순서가 적절하며, 필드 초기화도 명확합니다.
18-22: 검증 로직이 재사용 가능하게 구현되었습니다.
TimeBasedTimeBoxDomain의 검증 로직과 동일한 패턴을 사용하여 일관성을 유지했습니다.
24-27: 스탠스 검증 로직이 명확합니다.일반 타임박스에서 모든 스탠스를 허용하는 비즈니스 로직이
return true로 간단명료하게 구현되었습니다.
39-49: null 반환 메서드들의 일관성이 좋습니다.
getTimePerTeam()과getTimePerSpeaking()메서드에서 null을 반환하는 것이 일반 타임박스의 특성을 잘 반영합니다.@Nullable어노테이션 사용도 적절합니다.src/test/java/com/debatetimer/domain/customize/TimeBasedTimeBoxDomainTest.java (4)
15-36: 스탠스 검증 테스트가 포괄적입니다.중립 스탠스만 허용하는 비즈니스 로직을 성공/실패 케이스 모두로 검증하고 있어 완전한 테스트 커버리지를 제공합니다.
41-61: 팀 당 발언 시간 검증 테스트가 철저합니다.매개변수화된 테스트로 여러 경계값을 검증하고, null 케이스를 별도로 테스트하여 완전한 검증을 수행하고 있습니다.
63-82: 개인 당 발언 시간 검증 테스트가 적절합니다.개인 발언 시간의 선택적 특성(null 허용)과 양수 검증을 모두 테스트하고 있어 도메인 로직을 잘 반영합니다.
84-94: 시간 관계 검증 테스트가 비즈니스 로직을 잘 반영합니다.팀 당 발언 시간이 개인 발언 시간보다 크거나 같아야 한다는 비즈니스 규칙을 매개변수화된 테스트로 효과적으로 검증하고 있습니다.
src/main/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomain.java (3)
7-27: 도메인 클래스 설계가 잘 되어 있습니다.추상 클래스를 통한 공통 검증 로직 구현과 하위 클래스에서 구체적인 로직을 구현하도록 하는 설계가 좋습니다. 불변 객체로 설계되어 있고, 검증 로직이 생성자에서 수행되는 것도 적절합니다.
37-41: speechType 검증 로직이 적절합니다.null, blank, 길이 검사를 모두 수행하여 견고한 검증을 구현했습니다.
isBlank()메서드를 사용하여 공백 문자열도 적절히 처리하고 있습니다.
49-61: getter 메서드들이 final로 선언되어 캡슐화가 잘 되어 있습니다.하위 클래스에서 getter 메서드를 오버라이드하지 못하도록 final로 선언한 것이 좋습니다. 이는 도메인 객체의 일관성을 보장하는 좋은 설계입니다.
src/test/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomainTest.java
Show resolved
Hide resolved
src/test/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomainTest.java
Show resolved
Hide resolved
src/main/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomain.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/test/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomainTest.java (1)
88-91: 테스트용 구현체의 stance 검증 로직을 개선하세요.
isValidStance메서드가 항상true를 반환하여 실제 stance 검증 로직을 테스트할 수 없습니다. 이로 인해 19-24번 줄의 null stance 테스트가 의미있는 검증을 수행하지 못합니다.다음과 같이 수정하여 실제 검증 로직을 테스트할 수 있도록 개선하세요:
@Override protected boolean isValidStance(Stance stance) { - return true; + return stance != null; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomain.java(1 hunks)src/test/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomainTest.java(1 hunks)src/test/java/com/debatetimer/domain/customize/TimeBasedTimeBoxDomainTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/com/debatetimer/domain/customize/TimeBasedTimeBoxDomainTest.java
- src/main/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomain.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
🔇 Additional comments (3)
src/test/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomainTest.java (3)
1-13: LGTM! 필요한 의존성들이 적절히 임포트되었습니다.테스트에 필요한 AssertJ, JUnit 5, 그리고 도메인 예외 클래스들이 모두 올바르게 임포트되었습니다.
33-61: 발언 종류 검증 테스트가 잘 구현되었습니다.길이 초과, 빈 값, 유효한 값에 대한 테스트 케이스들이 포괄적으로 구현되어 있고, 파라미터화된 테스트를 통해 다양한 빈 값 케이스를 효과적으로 테스트하고 있습니다.
63-80: 발언자 검증 테스트가 적절히 구현되었습니다.길이 제한과 null 허용에 대한 테스트가 명확하게 구현되어 있어 발언자 필드의 검증 로직을 잘 검증합니다.
src/test/java/com/debatetimer/domain/customize/CustomizeTimeBoxDomainTest.java
Show resolved
Hide resolved
| } | ||
|
|
||
| private void validateStance(Stance stance) { | ||
| if (stance == null || !isValidStance(stance)) { |
There was a problem hiding this comment.
isValidStance가 지금 not조건으로만 쓰이는 것 같은데 차라리 isInvalidStance로 사용하는 건 어떤가요?
There was a problem hiding this comment.
단순하게 말 만 적어서 잘 모르겠는데, 아래와 같은 로직을 말하는 건가요?
private void validateStance(Stance stance) {
if (stance != null && isValidStance(stance)) {
return;
}
throw new DTClientErrorException(ClientErrorCode.INVALID_TIME_BOX_STANCE);
}There was a problem hiding this comment.
private void validateStance(Stance stance) {
if (stance == null && isInvalidStance(stance)) {
throw new DTClientErrorException(ClientErrorCode.INVALID_TIME_BOX_STANCE);
}
return;
}
이걸 말한 거긴 합니다.
There was a problem hiding this comment.
구현하는 추상 메서드 입장에서는 isInvalidStance() 라고 하면 충분히 헷갈릴 수 있다고 생각해서 isValidStance()를 사용했습니다.
| @ParameterizedTest | ||
| @ValueSource(ints = {0, -1, Integer.MIN_VALUE}) | ||
| void 시간은_0보다_커야_한다(int time) { | ||
| assertThatThrownBy(() -> new NormalTimeBoxDomain(Stance.PROS, "비토", null, time)) | ||
| .isInstanceOf(DTClientErrorException.class) | ||
| .hasMessage(ClientErrorCode.INVALID_TIME_BOX_TIME.getMessage()); | ||
| } |
There was a problem hiding this comment.
단순 질문입니다. 경계값만 테스트하는게 아닌 이렇게 테스트하는 이유가 있나요?
There was a problem hiding this comment.
깃허브 코파일럿이 작성해주길래 문제 없어서 위와 같이 테스트 진행했습니다.
저도 경계값만 테스트해도 무방하다고 생각하는데, 경계값으로 바꿀까요?
leegwichan
left a comment
There was a problem hiding this comment.
/noti @unifolio0
여유있을 때 코멘트에 대한 답변 달았어요~ 의견 추가 해주시면 감사드립니다~
| @ParameterizedTest | ||
| @ValueSource(ints = {0, -1, Integer.MIN_VALUE}) | ||
| void 시간은_0보다_커야_한다(int time) { | ||
| assertThatThrownBy(() -> new NormalTimeBoxDomain(Stance.PROS, "비토", null, time)) | ||
| .isInstanceOf(DTClientErrorException.class) | ||
| .hasMessage(ClientErrorCode.INVALID_TIME_BOX_TIME.getMessage()); | ||
| } |
There was a problem hiding this comment.
깃허브 코파일럿이 작성해주길래 문제 없어서 위와 같이 테스트 진행했습니다.
저도 경계값만 테스트해도 무방하다고 생각하는데, 경계값으로 바꿀까요?
| } | ||
|
|
||
| private void validateStance(Stance stance) { | ||
| if (stance == null || !isValidStance(stance)) { |
There was a problem hiding this comment.
단순하게 말 만 적어서 잘 모르겠는데, 아래와 같은 로직을 말하는 건가요?
private void validateStance(Stance stance) {
if (stance != null && isValidStance(stance)) {
return;
}
throw new DTClientErrorException(ClientErrorCode.INVALID_TIME_BOX_STANCE);
}|
/noti |
| public static final int SPEECH_TYPE_MAX_LENGTH = 10; | ||
| public static final int SPEAKER_MAX_LENGTH = 5; | ||
|
|
||
| private final Stance stance; |
There was a problem hiding this comment.
[제안]
Nullable은 명세차원이라 적어준 것이죠? Stance와 speechType은 NotNull이면 안적어준 이유가 있는지 궁금하네요! 밑에 검증해서??!
There was a problem hiding this comment.
Nullable은 명세차원이라 적어준 것이죠?
이거 어떤 것은 Nullable 하고 어떤 것은 not null 이어서 코드 작업할 때 쉽게 파악하라고 적어두었습니다.
NotNull이면 안적어준 이유가 있는지 궁금하네요!
기본적으로 Not null 하게 코드를 짠다는 것이 서로 코드 스타일로 자리잡혀있다고 생각해서 따로 명시해주지는 않았습니다.
|
|
||
| protected abstract boolean isValidStance(Stance stance); | ||
|
|
||
| private void validateSpeechType(String speechType) { |
There was a problem hiding this comment.
[받아들여지지 않아도 꿋꿋히 해보는 제안]
Table에서도 마찬가지였는데 값 객체로 감싸는 건 싫은가요? SpeechType, Speaker 등등이요
오히려 도메인 분리에 있어서 각 도메인을 식별하기 쉽고 검증로직도 응집성있게 모일 것 같아서요. 지금은 도메인 -엔티티의 클래스를 분리했다 이상으로 깔끔함이 느껴지진 않는 것 같다는 개인적인 생각입니다.
There was a problem hiding this comment.
지금은 도메인 -엔티티의 클래스를 분리했다 이상으로 깔끔함이 느껴지진 않는 것 같다는 개인적인 생각입니다.
딱 그만큼 깔끔해지기를 원했습니다.
[받아들여지지 않아도 꿋꿋히 해보는 제안] Table에서도 마찬가지였는데 값 객체로 감싸는 건 싫은가요? SpeechType, Speaker 등등이요
네
그것보다는 "엔티티/도메인 분리" 작업 자체가 분량이 꽤 되기 때문에 지금 고려하지는 않았습니다. 일단 시급한 불부터 끄고, 값 객체는 그 다음에 보시죠
| } | ||
|
|
||
| @Nullable | ||
| public final String getSpeaker() { |
| return; | ||
| } | ||
|
|
||
| validateTime(timePerSpeaking); |
There was a problem hiding this comment.
[필수 검토]
timePerSpeaking Nullable 하지 않나요? 그런데 validateTime의 경우 null이면 오류를 반환하고 있어요. 로직 재활용하려고 앞에서 Null 검증한 것 같은데 validateTime의 경우 NormalTimeBox에서도 쓰이니 validateTimeRange 라고 명명하여 상위 추상 클래스로 올리고, timePerSpeaking은 아예 validateTimePerSpeaking으로 따로 검증로직 만드는 건 어떤가요?
혹은 값 객체로 DebateTimerPerTeam 에 int timePerTeam과 timePerSpeaking을 필드로 지니게 해도 좋다고 생각합니다.
There was a problem hiding this comment.
이거 복사해서 붙여온건데, 4/11에 내가 쓴거네;;
일단 위에서 언급한대로 값 객체는 조금 나중에 생각하는 걸로 하고, 그냥 메서드를 따로 만들께요.
|
|
||
| @Test | ||
| void 발언_입장은_비어있을_수_없다() { | ||
| assertThatThrownBy(() -> new InheritedCustomizeTimeBoxDomain(null, "비토", "발언자")) |
There was a problem hiding this comment.
이전 논의사항을 잘 반영해주신 테스트네요 굳 👍
| } | ||
|
|
||
| @Test | ||
| void 개인_당_시간이_양수이어야_한다() { |
There was a problem hiding this comment.
사소한 부분인데 도메인 오해가 있을 수 있을 것 같아서요
개인당 시간 > 회당 발언시간으로 정정하면 좋을 것 같습니다.
- 특정 로직을 무리하게 재사용하지 않고, 두 검증 로직을 분리함
- 개인 발언 시간 -> 회 당 발언 시간
leegwichan
left a comment
There was a problem hiding this comment.
/noti @coli-geonwoo
달아준 코멘트 일부 반영했어요~ 일단 머지하고, 다른 내용은 다음 PR에서 이야기하죠!
| public static final int SPEECH_TYPE_MAX_LENGTH = 10; | ||
| public static final int SPEAKER_MAX_LENGTH = 5; | ||
|
|
||
| private final Stance stance; |
There was a problem hiding this comment.
Nullable은 명세차원이라 적어준 것이죠?
이거 어떤 것은 Nullable 하고 어떤 것은 not null 이어서 코드 작업할 때 쉽게 파악하라고 적어두었습니다.
NotNull이면 안적어준 이유가 있는지 궁금하네요!
기본적으로 Not null 하게 코드를 짠다는 것이 서로 코드 스타일로 자리잡혀있다고 생각해서 따로 명시해주지는 않았습니다.
|
|
||
| protected abstract boolean isValidStance(Stance stance); | ||
|
|
||
| private void validateSpeechType(String speechType) { |
There was a problem hiding this comment.
지금은 도메인 -엔티티의 클래스를 분리했다 이상으로 깔끔함이 느껴지진 않는 것 같다는 개인적인 생각입니다.
딱 그만큼 깔끔해지기를 원했습니다.
[받아들여지지 않아도 꿋꿋히 해보는 제안] Table에서도 마찬가지였는데 값 객체로 감싸는 건 싫은가요? SpeechType, Speaker 등등이요
네
그것보다는 "엔티티/도메인 분리" 작업 자체가 분량이 꽤 되기 때문에 지금 고려하지는 않았습니다. 일단 시급한 불부터 끄고, 값 객체는 그 다음에 보시죠
| return; | ||
| } | ||
|
|
||
| validateTime(timePerSpeaking); |
There was a problem hiding this comment.
이거 복사해서 붙여온건데, 4/11에 내가 쓴거네;;
일단 위에서 언급한대로 값 객체는 조금 나중에 생각하는 걸로 하고, 그냥 메서드를 따로 만들께요.

🚩 연관 이슈
#191
🗣️ 리뷰 요구사항 (선택)
XXXDomain->XXX,XXX->XXXEntity)Summary by CodeRabbit
신규 기능
버그 수정
테스트