[✨ feat] 스크랩 상태를 나타내는 ENUM 구현 #39
Conversation
BaseException과 Exception 처리만으로 충분하므로, UnsupportedOperationException, HttpMediaTypeNotSupportedException, SocketTimeoutException에 대한 별도 핸들러를 제거함.
- INVALID_USER_NAME 메시지에 공백 허용 여부를 명확히 표현
- 클래스명 PushToken → FcmToken 으로 명확히 표현 - 생성자, 팩토리 메서드 등 관련 메서드명 일괄 수정 - 예외 코드도 FCM_TOKEN_NOT_NULL 로 변경
- @Embedded 필드 타입을 PushToken → FcmToken으로 수정 - 클래스명 변경에 따른 필드 타입 일관성 유지
- FcmToken 클래스명 변경에 따라 테스트 클래스 및 내부 테스트 대상 수정 - 예외 메시지 검증 시 FCM_TOKEN_NOT_NULL 에 맞춰 assert 문 수정
- PUSH_TOKEN_NOT_NULL → FCM_TOKEN_NOT_NULL 로 수정하여 도메인 명확성 확보
- @DisplayName 어노테이션을 "푸시 토큰 테스트" → "FCM 토큰 테스트"로 변경
스크랩 상태에 대한 유효성 검사 및 중복 상태 예외를 처리하기 위한 ScrapErrorCode 열거형을 구현하였습니다. - scrapped/unscrapped 외 상태에 대한 예외 정의 - 이미 스크랩된 상태 또는 스크랩되지 않은 상태에 대한 예외 정의 - [SCRAP ERROR] prefix 포함한 에러 메시지 포맷 구성
ScrapErrorCode를 인자로 받아 스크랩 관련 예외를 처리할 수 있도록 ScrapException을 정의하였습니다. - 공통 예외 상위 클래스(BaseException) 상속 - 스크랩 도메인 전용 예외 처리 지원
- 사용자(User)와 연관된 스크랩 정보를 저장하는 Scrap 엔티티 정의 - 스크랩 상태 관리(SCRAPPED/UNSCRAPPED)를 위한 ScrapStatus Enum과 연동 - 스크랩 생성 정적 팩토리 메서드 `of(User user)` 제공 - 상태 전이 메서드(scrap, unscrap) 및 상태 확인 메서드(isScrapped, isUnscrapped) 추가 - 불필요한 Scraps 클래스 제거
- SCRAPPED, UNSCRAPPED 두 가지 상태를 정의하는 ScrapStatus Enum 구현 - 문자열 입력으로부터 Enum 매핑 기능(from) 및 유효성 검증 메서드(isValid) 제공 - 동일 상태로의 중복 전이를 방지하는 scrap(), unscrap() 메서드 구현 - 상태 확인 메서드(isScrapped, isUnscrapped) 및 Jackson 직렬화/역직렬화 설정 추가 - 잘못된 상태 입력 또는 중복 전이 시 ScrapException 발생 처리
- from() 메서드의 유효/무효 입력값 처리 검증 - scrap(), unscrap() 전이 메서드 정상 동작 및 예외 케이스 테스트 - isScrapped(), isValid() 등의 유틸성 메서드에 대한 테스트 포함 - 대소문자 구분 없이 상태 변환 가능함을 검증
There was a problem hiding this comment.
Pull Request Overview
This PR implements a ScrapStatus enum with comprehensive state transition logic and tests for scrapped/unscrapped states, while also refactoring the push token to FCM token for consistency.
- Introduces a new ScrapStatus enum with methods for state transitions and JSON serialization/deserialization.
- Adds a Scrap entity that leverages the ScrapStatus enum for state management.
- Renames and refactors PushToken to FcmToken across the domain and test packages.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/terning/scrap/domain/Scrap.java | New Scrap entity using the ScrapStatus for state management. |
| src/main/java/org/terning/scrap/domain/ScrapStatus.java | Implemented ScrapStatus enum with state transition and JSON annotations. |
| src/main/java/org/terning/scrap/common/failure/ScrapException.java | Introduced ScrapException for domain-specific error handling. |
| src/main/java/org/terning/scrap/common/failure/ScrapErrorCode.java | Defined error codes with messages for Scrap status validations. |
| src/test/java/org/terning/scrap/domain/ScrapStatusTest.java | Added tests covering valid and invalid state transitions as well as error messages. |
| src/test/java/org/terning/user/domain/vo/FcmTokenTest.java | Updated tests to reflect renaming from PushToken to FcmToken. |
| src/main/java/org/terning/user/domain/vo/FcmToken.java | Refactored PushToken to FcmToken with updated null validation and exception handling. |
| src/main/java/org/terning/user/domain/User.java | Updated associations and embedded token to reference the new Scrap and FcmToken classes. |
| src/main/java/org/terning/user/domain/vo/UserName.java | Updated validation method names for clarity. |
| src/main/java/org/terning/user/common/failure/UserErrorCode.java | Adjusted error messages to match the updated naming for FCM tokens. |
| src/main/java/org/terning/global/failure/GlobalExceptionHandler.java | Removed several exception handlers, likely as part of a broader error handling strategy. |
| src/main/java/org/terning/scrap/domain/Scraps.java | Removed deprecated Scrap entity in favor of the new Scrap class. |
Comments suppressed due to low confidence (1)
src/main/java/org/terning/user/domain/User.java:15
- Ensure that the renaming from Scraps to Scrap is consistently reflected in all dependent parts of the codebase.
import org.terning.scrap.domain.Scrap;
| } | ||
|
|
||
| @JsonCreator | ||
| public static ScrapStatus from(String input) { |
There was a problem hiding this comment.
Consider adding a null check for the input parameter in from() to avoid a potential NullPointerException when input is null.
| public static ScrapStatus from(String input) { | |
| public static ScrapStatus from(String input) { | |
| if (input == null) { | |
| throw new ScrapException(ScrapErrorCode.INVALID_SCRAP_STATUS); | |
| } |
There was a problem hiding this comment.
피드백 감사합니다! 🙌
from() 메서드에서 input 값에 대해 null 체크가 없을 경우 NPE가 발생할 수 있는 점을 반영하여,
명시적으로 null 검증을 수행하도록 수정했습니다.
if (input == null) {
throw new ScrapException(ScrapErrorCode.SCRAP_STATUS_CANNOT_BE_NULL);
}이로써 Jackson 역직렬화나 외부 입력에서 null이 들어오는 경우에도 명확한 예외 처리가 가능해졌습니다.
추가로 관련 테스트 케이스도 함께 반영하여 안정성을 확보했습니다! 👍
- ScrapStatus 입력값이 null인 경우를 명확히 처리하기 위해 SCRAP_STATUS_CANNOT_BE_NULL 코드 추가 - from() 메서드 내 null 체크와 연계하여 예외 메시지 명확화
- 입력값이 null일 경우 ScrapStatus.from()에서 NPE 방지를 위해 명시적 검증 로직 추가 - ScrapException을 통해 SCRAP_STATUS_CANNOT_BE_NULL 예외 코드 반환 - null 검증 책임을 분리한 validateNotNull() 메서드로 구현
- ScrapStatus.from(null) 호출 시 ScrapException 발생 여부 테스트 - isValid(null) 호출 시 false 반환 테스트 추가 - null 입력값 처리 로직에 대한 테스트 커버리지 보완
junggyo1020
left a comment
junggyo1020
left a comment
There was a problem hiding this comment.
네이밍이나 전체적인 구조는 깔끔하게 잘 정리된 것 같아요!!!
고생하셨습니다 :)
| public ScrapStatus scrap() { | ||
| if (this == SCRAPPED) { | ||
| throw new ScrapException(ScrapErrorCode.ALREADY_SCRAPPED); | ||
| } | ||
| return SCRAPPED; | ||
| } | ||
|
|
||
| public ScrapStatus unscrap() { | ||
| if (this == UNSCRAPPED) { | ||
| throw new ScrapException(ScrapErrorCode.ALREADY_UNSCRAPPED); | ||
| } | ||
| return UNSCRAPPED; | ||
| } |
There was a problem hiding this comment.
해당 부분의 분기를 아예 전략패턴처럼 분리하면 조금 더 가독성이 개선될 수 있을 것 같은데 어떻게 생각하시나요???
예시.
SCRAPPED {
public ScrapStatus scrap() { ... return SCRAPPED; }
public ScrapStatus unscrap() { ... return UNSCRAPPED; }
}There was a problem hiding this comment.
좋은 제안 감사합니다! 🙌
제안해주신 enum 상수별 메서드 오버라이딩 방식은 각 상태별 책임을 명확히 나누는 데에 효과적이라 확장성 면에서 강점이 있는 것 같아요.
다만 현재는 상태가 SCRAPPED, UNSCRAPPED 두 가지뿐이고, 각 메서드에서 처리하는 로직도 단순해서 기존 방식이 더 간결하고 유지보수하기 좋은 구조라고 판단했습니다.
if 조건으로도 충분히 명확하게 역할이 나뉘고, 중복 없이 로직을 표현할 수 있어서 지금 상황에선 기존 구조가 더 적합하다고 생각해요!
상태가 늘어나거나, 상태별 로직이 다양해질 경우에는 말씀해주신 구조로 전환하는 것도 좋은 방향이 될 것 같습니다 :)
좋은 인사이트 감사합니다! 🙏
| .orElseThrow(() -> new ScrapException(ScrapErrorCode.INVALID_SCRAP_STATUS)); | ||
| } |
There was a problem hiding this comment.
이부분에서 input이 어떤값이 들어왔는지 확인할 수 있으면 에러검증을 진행할 때 조금 더 개선될 수 있겠다는 생각이 드는데요! 어떻게 생각하시나요??
.orElseThrow(() ->
new ScrapException(ScrapErrorCode.INVALID_SCRAP_STATUS, "입력값: " + input));There was a problem hiding this comment.
좋은 포인트 감사합니다! 🙏
말씀해주신 것처럼 입력값을 에러 메시지에 포함시키면 디버깅에는 도움이 될 수 있다고 생각합니다.
다만, 개인적으로는 현재 메시지로도 충분히 의도를 전달할 수 있다고 판단했고,
잘못된 값을 직접 넘기기 시작하면 예외 메시지에 대한 책임이 커지고, 예외 타입별로 처리를 세분화해야 할 가능성도 생겨서 복잡도가 올라간다고 판단했습니다.
또한 "입력값: " 형식으로 값을 넘기게 되면, 결국 예외 메시지와 중복된 정보를 전달하는 셈이 되어
책임 분리 측면에서도 메시지와 예외 생성자 간 결합이 강해질 수 있어 유지보수에 부담이 될 수 있다고 생각했어요.
실제 검증을 더 세분화해야 하는 상황이 온다면,
그때는 입력값에 따라 예외 타입을 분리하거나 메시지 조합 책임을 다른 객체로 분리하는 방식으로 풀어가는 게 더 유연한 방향이라고 보고 있습니다!
좋은 질문 덕분에 예외 메시지 설계도 한 번 더 돌아보게 되었습니다. 감사합니다! 😄
📄 Work Description
💬 To Reviewers
📷 Screenshot
⚙️ ISSUE
✅ PR check list