[REFACTOR] 할 일 관련 기능 1차 리팩토링 #93
Conversation
할 일 변경(오늘, 즐겨찾기, 경로별): isTomorrowScheduleCreate 필드가 응답에 포함되지 않게 한다.
WalkthroughThis pull request introduces several changes across the backend. A new error code ( Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant TC as TodoController
participant MS as ManagementService
participant MR as MemberRepository
participant TR as TodoRepository
participant EH as ExceptionHandler
C->>TC: Send update/delete request
TC->>MS: Invoke operation (e.g., patchTodo/deleteTodo)
MS->>MR: Retrieve member details
alt Member status invalid
MS->>EH: Throw InvalidMemberStatusException
EH-->>TC: Return error response (e.g., 400)
else
MS->>TR: Validate todo details
alt Todo type mismatch
MS->>EH: Throw TodoTypeMismatchException
EH-->>TC: Return error response (e.g., 400)
else
TR-->>MS: Successful operation result
MS->>TC: Return result with HTTP status based on condition
end
end
TC-->>C: HTTP response
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
backend/src/main/java/com/dubu/backend/todo/service/impl/PathTodoManagementService.java (1)
52-56: 💡 Verification agent🧩 Analysis chain
Consider adding path validation.
The path validation could be enhanced to check if the path is active or valid for the member.
🏁 Script executed:
#!/bin/bash # Check if Path entity has status or validation fields ast-grep --pattern 'class Path { $$$ private $_ status; $$$ }'Length of output: 70
I'll re-run the search for the definition of the
Pathclass to inspect its members and look for any field or method that might indicate status or activation validation.
🏁 Script executed:
#!/bin/bash # Search for the definition of the Path class to inspect its members. rg "class Path\\s*{" -A 20 -n .Length of output: 124
I'll re-run the search with a simplified regular expression to safely locate the "Path" class definition.
🏁 Script executed:
#!/bin/bash # Search for the definition of the Path class with context lines rg "class Path" -C 10 -n .Length of output: 11094
Enhance path validation in PathTodoManagementService.
I verified that the
Pathentity (located inbackend/src/main/java/com/dubu/backend/plan/domain/Path.java) does not include any fields or methods indicating its active status or validity. Furthermore, the current implementation inPathTodoManagementServiceonly checks the number of todos by path and does not verify whether the path is active or valid for the member. It would be beneficial to introduce additional validation logic (for example, checking an "active" flag or similar indicator) before processing further operations.backend/src/main/java/com/dubu/backend/todo/service/impl/TomorrowTodoManagementService.java (1)
165-167: 🛠️ Refactor suggestionAdd proper validation for difficulty enum.
The current implementation could throw an uncaught IllegalArgumentException if the difficulty value is invalid. Consider adding explicit validation.
TodoDifficulty difficulty = null; if(todoUpdateRequest.difficulty() != null){ + try { difficulty = TodoDifficulty.valueOf(todoUpdateRequest.difficulty().toUpperCase()); + } catch (IllegalArgumentException e) { + throw new InvalidTodoDifficultyException(todoUpdateRequest.difficulty()); + } }
🧹 Nitpick comments (15)
backend/src/main/java/com/dubu/backend/todo/service/impl/SaveTodoQueryService.java (2)
45-48: Consider extracting the status validation to a reusable method.The status validation logic is duplicated across multiple methods. Consider extracting it to a private helper method for better maintainability and DRY principle.
+ private void validateMemberStatus(Member member) { + if (!member.getStatus().equals(Status.STOP)) { + throw new InvalidMemberStatusException(member.getStatus().name()); + } + } @Override public PageResponse<Long, List<TodoInfo>> findSaveTodos(TodoIdentifier identifier, Long cursor, SaveTodoQueryRequest request) { Member member = memberRepository.findById(identifier.memberId()).orElseThrow(() -> new MemberNotFoundException(identifier.memberId())); - // 회원의 상태는 정지여야 한다. - if(!member.getStatus().equals(Status.STOP)){ - throw new InvalidMemberStatusException(member.getStatus().name()); - } + validateMemberStatus(member);
97-100: Document the status requirement in the interface.This is the third occurrence of the same validation logic. Consider:
- Extracting the validation as suggested earlier
- Documenting the
STOPstatus requirement in theTodoQueryServiceinterface to make it clear that all implementing classes must enforce this requirementbackend/src/main/java/com/dubu/backend/todo/service/impl/PathTodoQueryService.java (3)
49-52: Consider extracting the member status validation into a reusable method.The member status validation is duplicated across multiple methods. Consider extracting it into a private utility method to improve maintainability and reduce code duplication.
+ private void validateMemberStatus(Member member) { + // 회원의 상태가 이동 중 이어야 한다. + if (!member.getStatus().equals(Status.MOVE)) { + throw new InvalidMemberStatusException(member.getStatus().name()); + } + } @Override public List<TodoInfo> findTargetTodos(TodoIdentifier identifier) { Member member = memberRepository.findById(identifier.memberId()) .orElseThrow(() -> new MemberNotFoundException(identifier.memberId())); - // 회원의 상태가 이동 중 이어야 한다. - if(!member.getStatus().equals(Status.MOVE)){ - throw new InvalidMemberStatusException(member.getStatus().name()); - } + validateMemberStatus(member);
49-52: Consider using English comments for better international collaboration.The Korean comments could be translated to English to improve code readability for international developers.
- // 회원의 상태가 이동 중 이어야 한다. + // Member status must be MOVEAlso applies to: 65-68, 103-106, 133-136
36-178: Consider architectural improvements for cross-cutting status validation.While the current implementation is solid, consider these architectural improvements:
- Move the status validation to an aspect or interceptor since it's a cross-cutting concern.
- Document the status requirement in the
TargetTodoQueryServiceinterface.Example aspect implementation:
@Aspect @Component public class MemberStatusValidationAspect { @Before("@annotation(RequiresMoveStatus)") public void validateMemberStatus(JoinPoint joinPoint) { // Extract member from method arguments and validate status } } @Target(ElementType.METHOD) @Retention(RetentionPolicy.RUNTIME) public @interface RequiresMoveStatus {}backend/src/main/java/com/dubu/backend/todo/service/impl/TodayTodoQueryService.java (1)
51-54: Consider refactoring the status validation.The status validation is duplicated across all methods. Consider extracting it to a private helper method for better maintainability and DRY principle.
+ private void validateMemberStatus(Member member) { + if (!member.getStatus().equals(Status.STOP)) { + throw new InvalidMemberStatusException(member.getStatus().name()); + } + } @Override @Transactional public List<TodoInfo> findTargetTodos(TodoIdentifier identifier) { Member member = memberRepository.findById(identifier.memberId()) .orElseThrow(() -> new MemberNotFoundException(identifier.memberId())); - // 회원의 상태는 정지여야 한다. - if(!member.getStatus().equals(Status.STOP)){ - throw new InvalidMemberStatusException(member.getStatus().name()); - } + validateMemberStatus(member);Also, consider translating the Korean comment to English for consistency.
backend/src/main/java/com/dubu/backend/todo/dto/response/TodoManageResult.java (1)
3-7: LGTM! Consider adding documentation.The change to
Booleanaligns well withTodoSuccessResponsechanges. Consider adding Javadoc to document the nullable nature ofisTomorrowScheduleCreatedand when it might be null.+/** + * Represents the result of a todo management operation. + * @param isTomorrowScheduleCreated Can be null when the operation doesn't affect tomorrow's schedule + * @param info The operation result data + */ public record TodoManageResult<D>(Boolean isTomorrowScheduleCreated, D info) {backend/src/main/java/com/dubu/backend/todo/exception/TodoTypeMismatchException.java (1)
8-17: LGTM! Well-structured exception handling.The exception implementation effectively supports the PR's type verification objective. The formatted message with both types provides clear error context.
Consider adding
@Overrideannotation to the constructor for better code clarity:public class TodoTypeMismatchException extends BadRequestException { + @Override public TodoTypeMismatchException(TodoType todoType, TodoType requestType) { super(TODO_TYPE_MISMATCH.getMessage().formatted(todoType.name(), requestType.name())); }backend/src/main/java/com/dubu/backend/todo/service/impl/TodayTodoManagementService.java (2)
39-42: Consider using an enum method for status validation.The status check could be made more maintainable by adding a method to the Status enum.
-if(!member.getStatus().equals(Status.STOP)){ - throw new InvalidMemberStatusException(member.getStatus().name()); -} +if(!member.getStatus().isStop()){ + throw new InvalidMemberStatusException(member.getStatus().name()); +}
93-95: Consider adding error message for type mismatch.The type mismatch validation could benefit from a more descriptive error message.
-if (!todo.getType().equals(TodoType.SCHEDULED)){ - throw new TodoTypeMismatchException(todo.getType(), TodoType.SCHEDULED); +if (!todo.getType().equals(TodoType.SCHEDULED)){ + throw new TodoTypeMismatchException(todo.getType(), TodoType.SCHEDULED, + String.format("Todo type %s is not allowed for scheduled tasks", todo.getType()));backend/src/main/java/com/dubu/backend/todo/service/impl/SaveTodoManagementService.java (1)
87-97: Consider extracting parent todo cleanup logic.The parent todo relationship cleanup logic is complex and could be extracted into a separate method for better maintainability.
+private void cleanupParentTodoRelationships(Todo todo, Member member) { + // 내일 할 일 관련 + todoRepository.findWithScheduleByParentTodoAndScheduleDate(todo, LocalDate.now().plusDays(1)) + .ifPresent(Todo::clearParentTodo); + + // 오늘 할 일 관련 + Schedule todaySchedule = scheduleRepository.findLatestSchedule(member, LocalDate.now()) + .orElseThrow(ScheduleNotFoundException::new); + todoRepository.findByParentTodoAndSchedule(todo, todaySchedule) + .ifPresent(Todo::clearParentTodo); + + // 해당 할 일 관련 + todo.clearParentTodo(); +}Then use it in the modifyTodo method:
-if(todoUpdateRequest.title() != null || todoUpdateRequest.category() != null || todoUpdateRequest.difficulty() != null){ - // 내일 할 일 관련 - todoRepository.findWithScheduleByParentTodoAndScheduleDate(todo, LocalDate.now().plusDays(1)).ifPresent(Todo::clearParentTodo); - - // 오늘 할 일 관련 - Schedule todaySchedule = scheduleRepository.findLatestSchedule(member, LocalDate.now()).orElseThrow(ScheduleNotFoundException::new); - todoRepository.findByParentTodoAndSchedule(todo, todaySchedule).ifPresent(Todo::clearParentTodo); - - // 해당 할 일 관련 - todo.clearParentTodo(); +if(todoUpdateRequest.title() != null || todoUpdateRequest.category() != null || todoUpdateRequest.difficulty() != null){ + cleanupParentTodoRelationships(todo, member); }backend/src/main/java/com/dubu/backend/todo/service/impl/PathTodoManagementService.java (1)
76-80: Consider extracting path todo limit validation.The path todo limit validation is repeated and could be extracted into a reusable method.
+private void validatePathTodoLimit(Path path) { + List<Todo> todosByPath = todoRepository.findTodosByPath(path); + if(todosByPath.size() == 10){ + throw new TodoLimitExceededException("경로별", 10); + } +}Then use it in both methods:
-List<Todo> todosByPath = todoRepository.findTodosByPath(path); -if(todosByPath.size() == 10){ - throw new TodoLimitExceededException("경로별", 10); -} +validatePathTodoLimit(path);backend/src/main/java/com/dubu/backend/todo/controller/TodoController.java (1)
73-78: Consider extracting response status logic.The response status determination logic could be extracted into a helper method for better maintainability and consistency.
+private ResponseEntity<?> buildTodoResponse(Boolean isTomorrowScheduleCreated, TodoInfo info) { + if(isTomorrowScheduleCreated == null || !isTomorrowScheduleCreated) { + return ResponseEntity.ok(new TodoSuccessResponse<>(isTomorrowScheduleCreated, info)); + } + return ResponseEntity.status(HttpStatus.CREATED) + .body(new TodoSuccessResponse<>(true, info)); +}Then use it in the controller:
-Boolean isTomorrowScheduleCreated = result.isTomorrowScheduleCreated(); -if(isTomorrowScheduleCreated == null || !isTomorrowScheduleCreated) - return ResponseEntity.status(HttpStatus.OK).body(new TodoSuccessResponse<>(isTomorrowScheduleCreated, result.info())); -return ResponseEntity.status(HttpStatus.CREATED).body(new TodoSuccessResponse<>(true, result.info())); +return buildTodoResponse(result.isTomorrowScheduleCreated(), result.info());backend/src/main/java/com/dubu/backend/todo/service/impl/TomorrowTodoQueryService.java (1)
49-52: Extract duplicated member status check to a helper method.The member status validation is duplicated across all methods. Consider extracting it to a private helper method to improve maintainability and reduce duplication.
+ private void validateMemberStatus(Member member) { + if (!member.getStatus().equals(Status.STOP)) { + throw new InvalidMemberStatusException(member.getStatus().name()); + } + } @Override public List<TodoInfo> findTargetTodos(TodoIdentifier identifier) { Member member = memberRepository.findById(identifier.memberId()) .orElseThrow(() -> new MemberNotFoundException(identifier.memberId())); - if(!member.getStatus().equals(Status.STOP)){ - throw new InvalidMemberStatusException(member.getStatus().name()); - } + validateMemberStatus(member);Also applies to: 64-67, 102-105, 132-135
backend/src/main/java/com/dubu/backend/todo/service/impl/TomorrowTodoManagementService.java (1)
220-223: Add input validation to prevent potential issues.The method should validate the input list to handle edge cases gracefully.
private List<Todo> createTomorrowTodosFromTodayTodos(Schedule tomorrowSchedule, List<Todo> todayTodos) { + if (todayTodos == null || todayTodos.isEmpty()) { + return new ArrayList<>(); + } List<Todo> tomorrowTodos = todayTodos.stream() .map(t -> Todo.of(t.getTitle(), TodoType.SCHEDULED, t.getDifficulty(), t.getMemo(), t.getMember(), t.getCategory(), t.getParentTodo(), tomorrowSchedule, t.getPath())) .toList(); return todoRepository.saveAll(tomorrowTodos); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/src/main/java/com/dubu/backend/global/exception/ErrorCode.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/controller/TodoController.java(2 hunks)backend/src/main/java/com/dubu/backend/todo/dto/response/TodoManageResult.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/dto/response/TodoSuccessResponse.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/exception/TodoTypeMismatchException.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/service/impl/PathTodoManagementService.java(6 hunks)backend/src/main/java/com/dubu/backend/todo/service/impl/PathTodoQueryService.java(4 hunks)backend/src/main/java/com/dubu/backend/todo/service/impl/SaveTodoManagementService.java(4 hunks)backend/src/main/java/com/dubu/backend/todo/service/impl/SaveTodoQueryService.java(4 hunks)backend/src/main/java/com/dubu/backend/todo/service/impl/TodayTodoManagementService.java(5 hunks)backend/src/main/java/com/dubu/backend/todo/service/impl/TodayTodoQueryService.java(5 hunks)backend/src/main/java/com/dubu/backend/todo/service/impl/TomorrowTodoManagementService.java(5 hunks)backend/src/main/java/com/dubu/backend/todo/service/impl/TomorrowTodoQueryService.java(5 hunks)
🔇 Additional comments (11)
backend/src/main/java/com/dubu/backend/todo/service/impl/SaveTodoQueryService.java (2)
5-5: LGTM! Import changes align with new member status validation.The added imports support the new member status validation requirements.
Also applies to: 9-9
70-73: Duplicate validation logic found.This is the same validation logic as in
findSaveTodos. This reinforces the need to extract the validation to a reusable method.backend/src/main/java/com/dubu/backend/todo/service/impl/PathTodoQueryService.java (1)
49-52: LGTM! Member status validation is consistently implemented.The member status validation is consistently implemented across all methods, ensuring that operations are only allowed when the member's status is 'MOVE'. This aligns well with the PR objectives of adding user status logic to the business layer.
Also applies to: 65-68, 103-106, 133-136
backend/src/main/java/com/dubu/backend/todo/service/impl/TodayTodoQueryService.java (4)
5-5: LGTM! Import statements are correctly placed.The new imports support the member status validation logic.
Also applies to: 10-10
84-87: Same validation refactoring applies here.This is the same validation logic that can be replaced with the suggested helper method.
122-125: Same validation refactoring applies here.This is the same validation logic that can be replaced with the suggested helper method.
152-155: Same validation refactoring applies here.This is the same validation logic that can be replaced with the suggested helper method.
backend/src/main/java/com/dubu/backend/todo/dto/response/TodoSuccessResponse.java (1)
3-5: LGTM! Good improvement to the API response structure.The change from
booleantoBooleanwith@JsonInclude(JsonInclude.Include.NON_NULL)effectively implements the PR objective of including the tomorrow's schedule creation status only when relevant, leading to cleaner and more focused API responses.backend/src/main/java/com/dubu/backend/global/exception/ErrorCode.java (1)
63-63: LGTM! Well-placed error code addition.The new
TODO_TYPE_MISMATCHerror code is appropriately placed in the TODO section and follows the established pattern for error messages and HTTP status codes.backend/src/main/java/com/dubu/backend/todo/service/impl/TomorrowTodoQueryService.java (1)
49-52: LGTM! Well-implemented member status check.The status validation is correctly placed before the business logic and includes the actual status in the error message for better debugging.
backend/src/main/java/com/dubu/backend/todo/service/impl/TomorrowTodoManagementService.java (1)
126-128: LGTM! Well-implemented type verification.The type check is correctly placed and includes both actual and expected types in the error message for better debugging.
| Boolean isTomorrowScheduleCreated = result.isTomorrowScheduleCreated(); | ||
|
|
||
| if(isTomorrowScheduleCreated == null){ | ||
| return ResponseEntity.status(HttpStatus.NO_CONTENT).build(); | ||
| } | ||
|
|
||
| if(isTomorrowScheduleCreated) | ||
| return ResponseEntity.status(HttpStatus.CREATED).body(new TodoSuccessResponse<>(true, result.info())); | ||
|
|
||
| return ResponseEntity.status(HttpStatus.OK).body(new TodoSuccessResponse<>(false, result.info())); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider unifying response status handling.
The response status handling in deleteTodo differs from patchTodo. Consider unifying the approach for better consistency.
-Boolean isTomorrowScheduleCreated = result.isTomorrowScheduleCreated();
-if(isTomorrowScheduleCreated == null){
- return ResponseEntity.status(HttpStatus.NO_CONTENT).build();
-}
-if(isTomorrowScheduleCreated)
- return ResponseEntity.status(HttpStatus.CREATED).body(new TodoSuccessResponse<>(true, result.info()));
-return ResponseEntity.status(HttpStatus.OK).body(new TodoSuccessResponse<>(false, result.info()));
+return buildDeleteResponse(result.isTomorrowScheduleCreated(), result.info());
+private ResponseEntity<?> buildDeleteResponse(Boolean isTomorrowScheduleCreated, TodoInfo info) {
+ if(isTomorrowScheduleCreated == null) {
+ return ResponseEntity.noContent().build();
+ }
+ return buildTodoResponse(isTomorrowScheduleCreated, info);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Boolean isTomorrowScheduleCreated = result.isTomorrowScheduleCreated(); | |
| if(isTomorrowScheduleCreated == null){ | |
| return ResponseEntity.status(HttpStatus.NO_CONTENT).build(); | |
| } | |
| if(isTomorrowScheduleCreated) | |
| return ResponseEntity.status(HttpStatus.CREATED).body(new TodoSuccessResponse<>(true, result.info())); | |
| return ResponseEntity.status(HttpStatus.OK).body(new TodoSuccessResponse<>(false, result.info())); | |
| return buildDeleteResponse(result.isTomorrowScheduleCreated(), result.info()); | |
| } | |
| private ResponseEntity<?> buildDeleteResponse(Boolean isTomorrowScheduleCreated, TodoInfo info) { | |
| if (isTomorrowScheduleCreated == null) { | |
| return ResponseEntity.noContent().build(); | |
| } | |
| return buildTodoResponse(isTomorrowScheduleCreated, info); | |
| } |
Issue Number
close #88
As-Is
할 일 관련 기능 1차 리팩토링
To-Be
api 응답 바디 값을 수정한다.
할 일 관련 비즈니스 로직에 사용자의 상태를 확인하는 로직을 추가한다.
할 일 수정, 삭제 시 해당 할 일의 타입과 요청 타입이 같은지를 확인하는 로직을 추가한다.
Check List
Test Screenshot
(Optional) Additional Description
Summary by CodeRabbit
New Features
Refactor