[FEAT] 연주 엔티티 구현 - #15
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPlaying과 MidiEvent JPA 엔티티를 추가하고, 연주 모드·상태와 MIDI 타입을 정의했습니다. 정적 생성 팩토리와 필수값·범위 검증을 제공하며, 관련 오류 코드를 Changes연주 도메인 모델
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (2)
src/main/java/com/mr/domain/playing/entity/Playing.java (2)
89-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBPM 기본값 로직을 생성자 쪽으로 모아주세요.
createFreePlay와createBackingTrack이 각각bpm != null ? bpm : 120을 중복 계산하고 있고,validateBpm은bpm != null && ...로 null을 그대로 통과시킵니다.bpm컬럼은nullable = false(Line 43-44)인데, 만약 향후 새 팩토리가 기본값 적용 없이builder()를 호출하면this.bpm에 null이 대입되어 영속화 시점에야 제약 위반이 드러나게 됩니다. MidiEvent 쪽 validate 메서드들은 모두 null까지 검증하는 것과 비교하면 일관성도 떨어집니다.기본값 해석과 null 방어를
validateBpm(또는 생성자) 내부로 옮기면 중복도 없어지고 계약이 더 명확해집니다.♻️ 제안하는 리팩터
- private static void validateBpm(Integer bpm) { - if (bpm != null && (bpm < 50 || bpm > 200)) { - throw new GeneralException(PlayingErrorStatus.INVALID_BPM_RANGE); - } - } + private static final int DEFAULT_BPM = 120; + + private static int resolveBpm(Integer bpm) { + int resolved = bpm != null ? bpm : DEFAULT_BPM; + if (resolved < 50 || resolved > 200) { + throw new GeneralException(PlayingErrorStatus.INVALID_BPM_RANGE); + } + return resolved; + }그리고 팩토리에서는
.bpm(bpm != null ? bpm : 120)대신.bpm(resolveBpm(bpm))처럼 단순화할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/mr/domain/playing/entity/Playing.java` around lines 89 - 134, BPM 기본값 처리와 null 방어를 Playing의 생성 경로에 일관되게 중앙화하세요. validateBpm 또는 생성자에 resolveBpm 로직을 추가해 null은 120으로 변환하고, 변환된 값이 50~200 범위인지 검증하도록 한 뒤 createFreePlay와 createBackingTrack의 중복 삼항식을 해당 로직 사용으로 교체하세요.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win경계값 검증 로직에 대한 단위 테스트를 추가해주세요.
Playing과MidiEvent모두 이번 PR에서 새로 도입된 핵심 검증 로직(BPM 50200, 필수값, pitch/velocity 0127, timestamp≥0)을 가지고 있지만 두 파일 모두 테스트 코드가 없습니다. 도메인 검증은 회귀에 민감한 영역이라 경계값(예: bpm=49/50/200/201) 위주의 단위 테스트를 지금 추가해두면 이후 리팩터링 안전망이 됩니다.
src/main/java/com/mr/domain/playing/entity/Playing.java#L66-135:createFreePlay/createBackingTrack의 BPM 경계값(49,50,200,201),userIdnull,backingTrackIdnull 케이스에 대한 단위 테스트를 추가하세요.src/main/java/com/mr/domain/playing/entity/MidiEvent.java#L46-104:create의 pitch/velocity 경계값(-1,0,127,128), timestamp 음수,playing/typenull 케이스에 대한 단위 테스트를 추가하세요.원하시면 두 파일에 대한 JUnit 테스트 스켈레톤을 바로 작성해 드릴 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/mr/domain/playing/entity/Playing.java` at line 1, Playing 및 MidiEvent의 핵심 검증 로직에 경계값 단위 테스트를 추가하세요. Playing 테스트에서는 createFreePlay/createBackingTrack의 BPM 49·50·200·201, userId null, backingTrackId null을 검증하고, MidiEvent 테스트에서는 create의 pitch·velocity -1·0·127·128, 음수 timestamp, playing/type null을 검증하세요. 각 입력이 기대된 성공 또는 예외 결과를 내는지 JUnit 테스트로 명확히 확인하세요.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/java/com/mr/domain/playing/entity/Playing.java`:
- Around line 89-134: BPM 기본값 처리와 null 방어를 Playing의 생성 경로에 일관되게 중앙화하세요.
validateBpm 또는 생성자에 resolveBpm 로직을 추가해 null은 120으로 변환하고, 변환된 값이 50~200 범위인지
검증하도록 한 뒤 createFreePlay와 createBackingTrack의 중복 삼항식을 해당 로직 사용으로 교체하세요.
- Line 1: Playing 및 MidiEvent의 핵심 검증 로직에 경계값 단위 테스트를 추가하세요. Playing 테스트에서는
createFreePlay/createBackingTrack의 BPM 49·50·200·201, userId null,
backingTrackId null을 검증하고, MidiEvent 테스트에서는 create의 pitch·velocity -1·0·127·128,
음수 timestamp, playing/type null을 검증하세요. 각 입력이 기대된 성공 또는 예외 결과를 내는지 JUnit 테스트로
명확히 확인하세요.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ec01cd73-9178-4207-8079-71444bbbf225
📒 Files selected for processing (6)
src/main/java/com/mr/domain/playing/entity/MidiEvent.javasrc/main/java/com/mr/domain/playing/entity/Playing.javasrc/main/java/com/mr/domain/playing/entity/enums/MidiType.javasrc/main/java/com/mr/domain/playing/entity/enums/PlayingMode.javasrc/main/java/com/mr/domain/playing/entity/enums/PlayingStatus.javasrc/main/java/com/mr/domain/playing/exception/PlayingErrorStatus.java
| @Getter | ||
| @Table( | ||
| name = "midi_event", | ||
| indexes = { |
There was a problem hiding this comment.
수고하셨습니다.
수정이 필요한 건 아니지만
이런 거 줄바꿈 하는 것도 다같이 맞춰서 하면 좋을 것 같습니다!
한 사람이 짠 코드 처럼요!
There was a problem hiding this comment.
네 확인했습니다!
논의 후 수정할게요~
ownue
left a comment
ownue
left a comment
There was a problem hiding this comment.
private builder와 정적 팩토리 메서드로 생성자를 통일한 게 좋은 것 같아요 👍👍
수고하셨습니다~!
3 participants
📍 개요
⛓️💥 관련 이슈
🛠️ 작업 내용
🔥 리뷰 요청 사항
playing_id,timestamp_ms복합 인덱스 설정✅ 체크리스트
📎 참고 사항
Summary by CodeRabbit