[4주차] 이설아/[feat] todo-list 상세 기능 구현#5
Hidden character warning
Conversation
rootTiket
left a comment
rootTiket
left a comment
There was a problem hiding this comment.
정말 너무 고생하셨습니다!! 제가 생각한 것 이상으로 디테일하고 많은 고민들이 있던 과제였던것 같아요
개선하면 좋을점, 그리고 인상깊었던 점을 코멘트로 남겨두었으니 읽어보시면 좋을것 같아요 😄
| import org.springframework.web.bind.annotation.RestController; | ||
|
|
||
| @RestController | ||
| @RequestMapping("/member") |
There was a problem hiding this comment.
조금 사소하지만 URI 는 복수형으로 많이 사용하는 것이 일반적이에요~!!
There was a problem hiding this comment.
추가적으로 auth 라는 URI 로 분리하기도 한답니다 !! 취향차이이긴 하지만 참고하시면 좋을것 같아요
| @PatchMapping("/{todoId}") | ||
| public ResponseEntity<TodoResponse> updateTodoStatus( | ||
| @AuthenticationPrincipal CustomUserDetails userDetails, | ||
| @PathVariable(name = "todoId") Long todoId, | ||
| @RequestBody TodoStatusUpdateRequest request | ||
| ) { | ||
| TodoResponse response = todoService.updateTodoStatus(userDetails.member(), todoId, request); | ||
| return ResponseEntity.status(HttpStatus.OK).body(response); | ||
| } |
There was a problem hiding this comment.
update에서 응답으로 성공내용을 보여주는 이유가 있을까요?? (이건 요구사항에 제가 작성하지 않아서, 어떤 생각으로 작성하셨는지 의견이 궁금해요!!)
변경에 성공했습니다 라는 응답만 보낼수도 있으니까요 🤔 뭐가 더 나을지 함께 고민해보면 좋을것 같아요
There was a problem hiding this comment.
어떤 투두를 변경했는지에 대한 내용을 넘겨주려고 했다 보니 해당 response를 같이 넘겨줬습니다!
반환 시 해당 todoId를 넘겨주게 되는데, 단순히 변경에 성공했다는 메시지만 줘도 괜찮은가요??
There was a problem hiding this comment.
정답이 있는 문제가 아니었습니다 ㅎㅎ .. 의사소통할 프론트 개발자의 요청에 따라 변경될 수 있는 부분이에요!
하지만 그냥 자연스럽게 넘겨주려고 했다 보다는 명확한 이유가 있는 편이 더 좋지 않을까 해서 드린 질문이었습니다!!
변경에 성공했다는 메시지만 주어도 충분할 경우도 있다는 걸 알아두시면 좋을것 같아용
| } | ||
|
|
||
| @DeleteMapping | ||
| @ResponseStatus(HttpStatus.NO_CONTENT) |
| import java.util.List; | ||
|
|
||
| @RestController | ||
| @RequestMapping("/todo") |
There was a problem hiding this comment.
다른 컨트롤러이지만 URI를 공유하는 이유가 있을까요?!
There was a problem hiding this comment.
/todo/detail 과 같이 REST API를 설계해서 그랬습니다! 따로 나누는 게 더 좋았을까요..? 🥺
There was a problem hiding this comment.
크게보면 todo에 포함되는 도메인이기 때문에 나눠야 할지 조금 애매한 부분이 있네요!
그러나 todo의 하위개념이기에, uri도 공유하고 있어 하나의 컨트롤러에서 구현되는 편이 조금은 더 자연스러울 수 있겠다는게 제 의견입니다 !!!
| @Id | ||
| @GeneratedValue(strategy = GenerationType.IDENTITY) | ||
| @Column(name = "member_id") | ||
| private Long id; |
There was a problem hiding this comment.
id는 null이 되면 안될것 같아요! 원시타입으로 변경을 고려해보시는것도 좋을것 같습니다 :):)
|
|
||
| @Id | ||
| @GeneratedValue(strategy = GenerationType.IDENTITY) | ||
| private Long id; |
| @Setter(PROTECTED) | ||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "todo_id") | ||
| private Todo todo; |
There was a problem hiding this comment.
현재는 크게 문제가 있지는 않겠지만, Todo에서 OneToMany, TodoDetail에서 ManyToOne을 사용하여 방향 매핑을 하게 되면 추후 순환참조 등의 문제에 빠지기 쉬워요!
이건 저의 경우이지만 ManyToOne만 사용 -> Todo 삭제시에는 TodoDetail에서 해당 TodoId를 검색 후 삭제
이런 방식으로 구현할것 같아요! 구현에 정답은 없으나 양방향 매핑의 편리함에 너무 익숙해질 수 있겠다 싶어 댓글로 남겨둡니다~!
There was a problem hiding this comment.
저도 처음엔 단방향으로만 매핑을 했다가, Todo 조회/삭제 시 세부 할 일들도 함께 조회/삭제 되어야 해서 양방향으로 할지, 단방향으로 해서 연관관계 메소드를 추가할지 고민하다가 양방향으로 매핑을 했습니다!
저도 양방향으로 매핑할 경우 예기치 못한 상황이 일어날 수 있어서 조심스러웠는데, 단방향으로만 진행해도 괜찮을까요??
There was a problem hiding this comment.
단방향으로 충분합니다!!
todo 삭제시 세부내용을 먼저 삭제하고 todo를 삭제하게 하거나, 쿼리를 통해 둘을 조인해서 한번에 삭제할 수 도 있을것 같아요
There was a problem hiding this comment.
모든 custom 예외에 대해서 핸들러에서 핸들링해주고 있네요! 하지만 이 경우에는 추가적인 예외를 작성할 경우 매번 핸들러에 추가해줘야 할것 같아요! 정말 불편할 수 있겠죠?
RuntimeException을 상속받아 전역으로 사용할 수 있는 Exception을 만들어 두고 해당 Exception을 상속받아 커스텀 예외를 만들면 조금 쉬워질 것 같아요!
예를들어
public class BusinessException extends RuntimeException {
private final ExceptionCode exceptionCode;
public BusinessException(ExceptionCode exceptionCode) {
super(exceptionCode.getMessage());
this.exceptionCode = exceptionCode;
}
public ExceptionCode getErrorCode() {
return exceptionCode;
}
}이렇게 구현해두고 핸들러에는
@ExceptionHandler(BusinessException.class)
public ResponseEntity<ExceptionResponse> handleBusinessException(BusinessException ex) {
ExceptionCode exceptionCode = ex.getErrorCode();
ExceptionResponse exceptionResponse = ExceptionResponse.of(exceptionCode);
return new ResponseEntity<>(exceptionResponse, exceptionCode.getStatus());
}이렇게 등록한다면 BusinessException에 대한 예외를 한번에 잡을 수 있겠죠?
고려해보시면 좋을것 같네요 !!
There was a problem hiding this comment.
예외코드는 저처럼 따로 구현해도 되고, 아니면 code 등을 받아서 직접 구현하셔도 됩니다 방법에 정해진건 없어요 😃
| private final TodoDetailRepository todoDetailRepository; | ||
|
|
||
| @Transactional | ||
| public TodoDetailResponse createTodoDetail(Member member, Long parentTodoId, TodoRequest request) { |
There was a problem hiding this comment.
인자가 Long이라는건 null이 들어올 수도 있지 않을까요? 원시타입으로 변경해보시는걸 추천합니당
| // 해당 사용자의 투두인지 확인하는 메소드 | ||
| private void verifyTodoOwner(Member member, Todo todo) { | ||
| if (!todo.getMember().getId().equals(member.getId())) { | ||
| throw new ForbiddenAccessException("해당 목록에 접근 권한이 없습니다: todo_id = " + todo.getId()); | ||
| } | ||
| } | ||
|
|
||
| // 완료된 투두인지 확인하는 메소드 | ||
| private void verifyTodoIsCompleted(Todo todo) { | ||
| if (todo.getStatus() != TodoStatus.COMPLETED) { | ||
| throw new InvalidDeleteException("완료된 TODO만 삭제 가능합니다: todo_id = " + todo.getId() + ", status = " + todo.getStatus()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
자세히 예외 상황에 대해 응답하려는 부분이 인상깊네요 👀
|
코멘트 확인했습니다!! 질문 사항이 있어서 각 코멘트 밑에 달아뒀으니 확인해주시면 감사하겠습니다 👀 |
2 participants
내용
ERD 설계 및 요구사항 명세서를 작성했습니다.
로그인 기능 및 TODO 등록, 조회, 변경, 삭제 기능을 구현했습니다.
테스트 결과
MemberServiceTest
TodoServiceTest
TodoDetailServiceTest
실행 결과
회원가입
로그인
TODO 등록
세부 할 일 등록
TODO 조회
TODO 변경(상태)
세부 할 일 변경(상태)
TODO 삭제
ids에 1개의 TODO 아이디를 작성하면 한 개씩 삭제 가능합니다.
ids에 여러 개의 TODO 아이디를 작성하면 한 번에 여러 개 삭제가 가능합니다.
완료되지 않은 TODO를 삭제하려고 할 경우 오류 메시지를 반환합니다.
세부 할 일 삭제
ids에 1개의 세부 할 일 아이디를 작성하면 한 개씩 삭제 가능합니다.
ids에 여러 개의 세부 할 일 아이디를 작성하면 한 번에 여러 개 삭제가 가능합니다.
추가사항
closed #3