Conversation
| if(!post.getSpace().getSpaceId().equals(spaceId)) { | ||
| throw new CustomException(POST_IS_NOT_IN_SPACE); | ||
| } | ||
|
|
There was a problem hiding this comment.
게시글이 해당 스페이스에 속하는지를 검증하는 코드를 Post 객체 내부로 옮겨보는건 어떤가요??
디미터의 법칙에 위배되지 않는 깔끔한 코드로 수정할 수 있을것 같습니다!
| }).collect(Collectors.toList()); | ||
|
|
||
| post.updatePost(updatePostReqeust.getTitle(), updatePostReqeust.getContent(), updateImages); | ||
|
|
There was a problem hiding this comment.
LGTM
Post의 update 처리를 Post 객체에게 맡기는 코드가 좋네여
| // TODO 2: 유저가 스페이스에 속하는 지 검증 | ||
| Optional<UserSpace> userInSpace = userSpaceUtils.isUserInSpace(userId, spaceId); | ||
| log.info("UserName = {}, UserSpaceAuth = {}", userInSpace.get().getUserName(), userInSpace.get().getUserSpaceAuth()); | ||
|
|
There was a problem hiding this comment.
여기도 또 제가 싸질러놓은 utils이 등장하네요 하하,,
컨트롤러단에서는 비즈니스 로직이 없는것이 훨씬 깔끔하다고 생각합니다
서비스단 안으로 해당 로직을 수행하는 코드를 옮겨보는건 어떤가요??
| // TODO 1: 유저가 스페이스에 속하는지 검증 | ||
| Optional<UserSpace> userInSpace = userSpaceUtils.isUserInSpace(userId, spaceId); | ||
| log.info("UserName = {}, UserSpaceAuth = {}", userInSpace.get().getUserName(), userInSpace.get().getUserSpaceAuth()); | ||
|
|
There was a problem hiding this comment.
이 친구도 마찬가지로 비즈니스로직이므로 서비스단으로 코드가 옮겨지면 좋을거같습니다!
| if(!post.getSpace().getSpaceId().equals(spaceId)) { | ||
| throw new CustomException(POST_IS_NOT_IN_SPACE); | ||
| } | ||
|
|
There was a problem hiding this comment.
이것도 마찬가지로 게시글이 특정 스페이스에 속하는지에 대한 검증로직을 Post 객체에게 책임을 전가하는것이 좋을것 같습니다
| // TODO 3: 게시글 수정 작업 수행 | ||
| postService.updatePost(userId, spaceId, postId, updatePostRequest); | ||
|
|
||
| return new BaseResponse<>("게시글이 수정되었습니다."); |
There was a problem hiding this comment.
P2: 응답 반환할 때 string을 바로 반환하기보다는, DTO를 생성해서 성공 여부를 담아주는 방식은 어떨까요
여기 뿐 아니라 다른 컨트롤러들에서도 string을 바로 반환하는 곳들이 있던데, 요청이 실패했을 경우의 응답까지 고려하는 게 좋을 것 같네요 다같이 한 번 의논해봅시다!
There was a problem hiding this comment.
엇 저는 단순히 success string을 반환했던 이유가, 요청에 실패할 경우 어차피 exception을 터뜨리기 때문에 문제가 없을 것 같다는 생각이었습니다
혹시 response dto를 생성해서 성공여부를 담아주는 방식이라면, 서비스단의 비즈니스 로직 실행중 exception이 터졌을때, 이를 catch 해서 response 내의 isSuccess 라는 flag를 false 로 바꾸는 느낌인건가요??
There was a problem hiding this comment.
네네 맞아요
다른 필드를 추가하게 될 경우에 대한 확장성 면에서도 그렇고,
프론트에서 데이터를 다룰 때에도 좀 더 통일된 방식으로 다루는 게 효율적일 것 같아서 제안해봤습니다
There was a problem hiding this comment.
오오 좋은것같습니다! 저도 리펙토링할때 response dto 형식을 이렇게 수정해보겠습니다!
📝 요약
테스트코드 브랜치를 따로 파서 작업하겠습니다..
그래서 일단 api 개발한 코드부터 올렸습니다!
이슈 번호 : #132
🔖 변경 사항
✅ 리뷰 요구사항
📸 확인 방법 (선택)
📌 PR 진행 시 이러한 점들을 참고해 주세요