-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat#21 게시물 생성, 삭제, 수정 API 구현 #26
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다👍
리뷰 확인 부탁드립니다!!
Member author=memberService.findMemberByTag(authorTag); | ||
Post post=postService.createPost(postDTO,author); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 비즈니스 로직은 Service layer로 분리하는게 좋아보입니다👍
public ResponseEntity<ApiData<PostDTO>> updatePost(@PathVariable Long postId,@RequestBody @Validated PostDTO postDTO){ | ||
Post updatedPost=postService.updatePost(postId,postDTO); | ||
return ApiData.ok(new PostDTO( | ||
updatedPost.getId(), | ||
updatedPost.getTitle(), | ||
updatedPost.getContent() | ||
)); | ||
} | ||
|
||
@DeleteMapping("/{postId}/delete") | ||
public ResponseEntity<ApiData<Void>> deletePost(@PathVariable Long postId){ | ||
postService.deletePost(postId); | ||
return ApiData.ok(null); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DTO를 사용하기로 한 이상, 각각의 Request와 Response에 대해서 각각 다른 DTO를 사용하는게 좋습니다.
만약, 여러 컨트롤러가 같은 DTO를 사용하고있는 상황에서 하나의 컨트롤러의 변경으로 인해 DTO가 변경된다면 다른 컨트롤러까지 영향을 받으니까요🤔 같은 필드를 가진 DTO라고 하더라도 분리해서 사용하는건 어떠신가요?
(저는 개인적으로 DTO는 무한으로 복사되어도 상관 없다는 생각이긴 합니다...ㅎㅎ😅)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정하였습니다. 확인 한번 부탁드립니다.
|
||
@DeleteMapping("/{postId}/delete") | ||
public ResponseEntity<ApiData<Void>> deletePost(@PathVariable Long postId){ | ||
postService.deletePost(postId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서도 본인이 쓴 게시물인지 확인하는 로직도 필요해보여요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
구현했습니다. 확인한번 부탁드립니다.
|
||
@PatchMapping("/{postId}/edit") | ||
public ResponseEntity<ApiData<PostDTO>> updatePost(@PathVariable Long postId,@RequestBody @Validated PostDTO postDTO){ | ||
Post updatedPost=postService.updatePost(postId,postDTO); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
본인이 쓴 게시물인지 확인하는 로직이 필요해 보여요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
구현했습니다. 확인 한번 부탁드립니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!!
return ApiData.created(new CommentResponseDTO(comment.getId(),comment.getContent())); | ||
} | ||
|
||
@DeleteMapping("/{commentId}/delete") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/delete를 꼭 추가하지 않고 /{commentId}로만 해도 괜찮아보여요
.build(); | ||
return commentRepository.save(comment); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
트랜잭션을 추가해서 데이터의 무결성을 유지하는게 좋아보여요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그리고 댓글 수정 기능도 필요해 보입니다
@NotBlank | ||
private String content;//내용 | ||
|
||
private String imageUrl;//이미지 url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이미지를 여러개 올리기 위해 이미지는 따로 entity로 분리하는게 좋아보여요
return ApiData.created(postService.toPostResponseDTO(post)); | ||
} | ||
|
||
@PatchMapping("/{postId}/edit") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTTP 메서드 자체가 어떤 동작을 수행하는지 보이기때문에 edit 등을 경로에 동작을 명시 안하는게 좋아보입니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
삭제했습니다!!
)); | ||
} | ||
|
||
@DeleteMapping("/{postId}/delete") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서도 delete를 경로에 동작을 명시 안하는게 좋아보입니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 그렇네요!! 어차피 DeleteMapping해서 없어도 될 것 같아요!! 삭제했습니다.
return toPostResponseDTO(post); | ||
} | ||
|
||
public PostResponseDTO toPostResponseDTO(Post post){//자식 게시물 리스트를 DTO로 변환 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PostResponseDTO에 구현하는게 좋아보입니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 이 부분은 가장 중요한 게시물 서비스 내용이라고 해서 일부로 PostService안에 넣어놨습니다!!
1. 무슨 이유로 코드를 변경했나요?
게시물 생성, 삭제, 수정을 구현하였습니다. 아직 공유 관련 기능은 아직 구현되지 않았습니다. 먼저 이 부분에 대한 피드백을 받고 진행하겠습니다.
2. 어떤 위험이나 장애를 발견했나요?
3. 관련 스크린샷을 첨부해주세요.
4. 완료 사항
5. 추가 사항