Refactor: TDA 원칙 적용 - #240
Merged
Merged
Conversation
…ve 추가 OWNER 권한 검증 로직을 Facade/Service에서 엔티티로 이동하여 동일 검증의 중복 제거 및 TDA 원칙 적용 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…m 추가 이미지 유무·변경 여부 및 카테고리 변경 여부 판단을 Service에서 엔티티로 이동하여 TDA 원칙 적용 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
프로필 이미지 변경 여부 판단을 MemberService에서 엔티티로 이동하여 TDA 원칙 적용 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
알림 기준 날짜 계산 책임을 NotificationProcessor에서 엔티티로 이동하여 TDA 원칙 적용 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 45 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthrough이 PR은 Food, Member, MemberRefrigerator, AlertSchedule 엔티티에 상태/권한/날짜 판별 메서드를 추가하고, 서비스 계층의 검증/계산 로직을 이들 메서드로 위임하는 리팩터링입니다. 엔티티가 자신의 상태를 판별하도록 책임을 이전하고, 서비스의 조건 분기를 단순화하며, 모든 새 메서드에 대한 단위 테스트를 추가했습니다. ChangesFood 엔티티 상태 판별 및 서비스 통합
Member 프로필 이미지 상태 판별 및 서비스 통합
AlertSchedule 만료 기준일 계산 추출 및 NotificationProcessor 통합
MemberRefrigerator 권한 검증 메서드 추출 및 서비스 통합
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/test/java/soon/fridgely/domain/food/entity/FoodUnitTest.java (1)
275-360: ⚡ Quick win신규 테스트가 테스트 네이밍/BDD 주석 규칙을 일부 위반하고 있습니다.
메서드명이 구현 메서드명 중심(
isCategoryDifferent등)으로 작성되어 정책/현상 중심 서술 규칙과 어긋나고,// when & then결합 주석도 BDD 구분 규칙과 다릅니다.// when,// then을 분리하고, 테스트명은 도메인 행위/결과 문장으로 바꿔주세요.As per coding guidelines
src/test/java/**/*.java:BDD 기반: // given, // when, // then 주석으로 구분한다.그리고도메인 정책·현상 중심으로 기술한다. 메서드 구현 관점 금지.🤖 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/test/java/soon/fridgely/domain/food/entity/FoodUnitTest.java` around lines 275 - 360, Rename the test methods in FoodUnitTest to describe the domain behavior/result (avoid referencing implementation method names like isCategoryDifferent, hasImage, isImageChangedFrom) — e.g. "카테고리가_다르면_카테고리_변경_판단이_true를_반환한다" — and update each test's in-body BDD comments by splitting the combined "// when & then" into two separate lines "// when" and "// then"; keep references to the existing symbols (isCategoryDifferent, hasImage, isImageChangedFrom) only inside assertions, not in the method names or comments.src/test/java/soon/fridgely/domain/notification/entity/AlertScheduleTest.java (1)
79-94: ⚡ Quick winnull 입력 케이스 테스트 추가를 고려하세요.
현재 테스트는 정상 시나리오만 검증합니다.
getExpirationTargetDate에 null 검증이 추가되면, 다른 검증 메서드 테스트(lines 48-77)와 동일한 패턴으로 예외 케이스도 테스트하는 것이 좋습니다.💚 null 케이스 테스트 추가 제안
+ `@Test` + void 기준일이_null이면_예외가_발생한다() { + // given + AlertSchedule schedule = AlertSchedule.of(LocalTime.of(9, 0), 3); + + // expected + assertThatThrownBy(() -> schedule.getExpirationTargetDate(null)) + .isInstanceOf(CoreException.class) + .hasMessageContaining("기준일은 null일 수 없습니다.") + .extracting("errorType") + .isEqualTo(ErrorType.INVALID_REQUEST); + }🤖 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/test/java/soon/fridgely/domain/notification/entity/AlertScheduleTest.java` around lines 79 - 94, Add a test covering the null input case for AlertSchedule.getExpirationTargetDate similar to the existing null-validation tests: create a new unit test (or extend the parameterized test) that calls AlertSchedule.of(LocalTime.of(9,0), daysBeforeExpiration).getExpirationTargetDate(null) and asserts the expected exception (e.g., NullPointerException or the domain-specific exception) is thrown; use the same assertion pattern as the other null-validation tests to ensure consistent behavior and messaging.
🤖 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.
Inline comments:
In `@src/main/java/soon/fridgely/domain/food/service/FoodService.java`:
- Around line 71-73: The code currently treats an empty-string newImageUrl as a
change (triggering ImageDeleteEvent) but later ignores blank URLs when calling
food.update(...), causing DB/entity to keep the old URL while the file was
deleted; fix by normalizing newImageUrl once (e.g., String normalizedNewImage =
(newImageUrl == null || newImageUrl.trim().isEmpty()) ? null : newImageUrl), use
normalizedNewImage when calling food.isImageChangedFrom(...) and when invoking
food.update(...), and only publish new ImageDeleteEvent(food.getImageURL()) if
normalizedNewImage is different from the existing URL (i.e., when
isImageChangedFrom(normalizedNewImage) returns true).
In `@src/main/java/soon/fridgely/domain/notification/entity/AlertSchedule.java`:
- Around line 45-47: In AlertSchedule.getExpirationTargetDate(LocalDate now) add
a defensive null check for the now parameter before calling now.plusDays(...);
for example use Objects.requireNonNull(now, "now must not be null") (or throw an
IllegalArgumentException) at the top of the method so a clear exception is
raised instead of an NPE when now is null while the rest of the logic (return
now.plusDays(this.daysBeforeExpiration)) remains unchanged.
In `@src/test/java/soon/fridgely/domain/member/entity/MemberUnitTest.java`:
- Around line 20-21: Split the combined "// when & then" BDD comment into
separate "// when" and "// then" comments around the action and the assertion in
the MemberUnitTest, e.g., place "// when" immediately above the call that
triggers behavior (the call to member.isProfileImageChangedTo(...)) and "//
then" immediately above the assertion that checks the result (the
assertThat(...) line); apply the same change for the other occurrences noted
(the blocks around lines with member.isProfileImageChangedTo and their
corresponding assertThat calls at the other locations).
In
`@src/test/java/soon/fridgely/domain/refrigerator/entity/MemberRefrigeratorUnitTest.java`:
- Around line 28-66: The test methods that reference implementation names
(OWNER는_validateOwnership을_호출해도_예외가_발생하지_않는다,
MEMBER가_validateOwnership을_호출하면_예외가_발생한다,
MEMBER는_validateCanLeave를_호출해도_예외가_발생하지_않는다,
OWNER가_validateCanLeave를_호출하면_예외가_발생한다) should be renamed to
domain/policy-centric names (e.g., OWNER_cannot_be_deleted_by_non_owner →
ownerCanDeleteRefrigerator / memberCannotDeleteRefrigerator style in your Korean
naming convention) and the BDD comments must follow // given, // when, // then
for normal flows and use // expected only for exception assertions; locate the
tests that call MemberRefrigerator.link(...) and invoke
MemberRefrigerator.validateOwnership or MemberRefrigerator.validateCanLeave and
update the method names and comment blocks accordingly so non-exception tests
contain explicit // when and // then sections while exception tests keep //
expected.
---
Nitpick comments:
In `@src/test/java/soon/fridgely/domain/food/entity/FoodUnitTest.java`:
- Around line 275-360: Rename the test methods in FoodUnitTest to describe the
domain behavior/result (avoid referencing implementation method names like
isCategoryDifferent, hasImage, isImageChangedFrom) — e.g.
"카테고리가_다르면_카테고리_변경_판단이_true를_반환한다" — and update each test's in-body BDD comments
by splitting the combined "// when & then" into two separate lines "// when" and
"// then"; keep references to the existing symbols (isCategoryDifferent,
hasImage, isImageChangedFrom) only inside assertions, not in the method names or
comments.
In
`@src/test/java/soon/fridgely/domain/notification/entity/AlertScheduleTest.java`:
- Around line 79-94: Add a test covering the null input case for
AlertSchedule.getExpirationTargetDate similar to the existing null-validation
tests: create a new unit test (or extend the parameterized test) that calls
AlertSchedule.of(LocalTime.of(9,0),
daysBeforeExpiration).getExpirationTargetDate(null) and asserts the expected
exception (e.g., NullPointerException or the domain-specific exception) is
thrown; use the same assertion pattern as the other null-validation tests to
ensure consistent behavior and messaging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2cfb334b-03b4-42a1-a318-1b330189ed82
📒 Files selected for processing (13)
src/main/java/soon/fridgely/domain/food/entity/Food.javasrc/main/java/soon/fridgely/domain/food/service/FoodService.javasrc/main/java/soon/fridgely/domain/member/entity/Member.javasrc/main/java/soon/fridgely/domain/member/service/MemberService.javasrc/main/java/soon/fridgely/domain/notification/entity/AlertSchedule.javasrc/main/java/soon/fridgely/domain/notification/service/NotificationProcessor.javasrc/main/java/soon/fridgely/domain/refrigerator/entity/MemberRefrigerator.javasrc/main/java/soon/fridgely/domain/refrigerator/service/RefrigeratorFacade.javasrc/main/java/soon/fridgely/domain/refrigerator/service/RefrigeratorService.javasrc/test/java/soon/fridgely/domain/food/entity/FoodUnitTest.javasrc/test/java/soon/fridgely/domain/member/entity/MemberUnitTest.javasrc/test/java/soon/fridgely/domain/notification/entity/AlertScheduleTest.javasrc/test/java/soon/fridgely/domain/refrigerator/entity/MemberRefrigeratorUnitTest.java
- FoodService: 빈 문자열 imageURL을 null로 정규화하여 파일 삭제 후 DB는 이전 URL이 남는 데이터 불일치 버그 수정 - AlertSchedule.getExpirationTargetDate: null 가드 추가 - 테스트 메서드명을 구현 메서드명이 아닌 도메인 행위 중심으로 변경 - BDD 주석 스타일 통일 (when & then 분리) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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.
Summary by CodeRabbit
릴리스 노트
Refactor
Tests