-
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
[Feature] 게시물 아카이브 대응 및 리팩토링 #43
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.
고생하셨습니다!
전체적으로 코드 수준이 높아졌어요!
추가적인 개선 사항이 보여서 코멘트 남겼습니다. 어떻게 생각하시는지 알려주세요!
public void createUpdateMemo(Post post, String content) { | ||
post.setMemo(content); | ||
post.setMemoCreatedAt(LocalDateTime.now()); | ||
post.updatePostMemo(content, LocalDateTime.now()); | ||
postRepository.save(post); | ||
} | ||
|
||
public void deleteMemo(Post post) { | ||
post.setMemo(null); | ||
post.setMemoCreatedAt(null); | ||
post.updatePostMemo(null, null); | ||
postRepository.save(post); |
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.
👍
@@ -61,27 +71,27 @@ public PostDetailResponse getPostDetail(Long userId, | |||
PostDetailDto postDetailDto = postReader.readPostDetailWithTags(userId, | |||
postDetailServiceRequest); | |||
|
|||
return new PostDetailResponse(postDetailDto); | |||
return PostDetailResponse.from(postDetailDto); |
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.
👍
Long postId = postManager.createTempPost(user, createPostServiceRequest.getUrl(), summary); | ||
SummaryResultDto summaryResultDto = summaryAIManager.getSummary(createPostServiceRequest); | ||
User user = userReader.readUserByIdOrNull(userId); | ||
Post post = postManager.createPost(user, createPostServiceRequest.getUrl(), summaryResultDto); |
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.
👍
UpdatePostServiceRequest updatePostServiceRequest) { | ||
User user = userReader.readUserById(userId); | ||
Post post = postReader.readActivatedPostAndWriter(postId); | ||
Post updatedPost = postManager.updatePost(user, post, updatePostServiceRequest); |
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.
👍
public Post createPost(User user, String url, SummaryResultDto summaryResultDto) { | ||
Post newPost = Post.createPost(user, summaryResultDto.getTitle(), summaryResultDto.getContent(), | ||
PostStatus.DRAFT, url); | ||
|
||
return postRepository.save(newPost); |
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.
👍
import java.time.LocalDateTime; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
import lombok.*; |
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.
👍
Post post = Post.builder().title(title).content(content).type(PostType.PRIVATE).status(status) | ||
.url(url).activated(true).build(); |
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.
ide에서 자동으로 포맷팅하는 거에 걸렸나보네요 수정하겠습니다~
@@ -17,22 +17,22 @@ | |||
|
|||
@RestController | |||
@RequiredArgsConstructor | |||
@RequestMapping("/memo") | |||
@RequestMapping("/posts/{postId}/memos") |
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.
👍
public UpdatePostServiceRequest toServiceRequest() { | ||
return UpdatePostServiceRequest.builder() | ||
.title(title) | ||
.content(content) | ||
.tagList(tagList) | ||
.archiveId(archiveId) | ||
.memo(memo) | ||
.build(); |
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.
👍
@@ -34,6 +34,16 @@ public static Specification<Post> getSearch(String search) { | |||
}; |
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.
이 클래스에서 null 이면 바로 null을 반환하니 else 가 필요 없는 메서드들이 있는 거 같아요.
그리고
getSearch의 경우 이렇게 하면 더 깔끔해지지 않을까 생각해봅니다.
public static Specification getSearch(String search) {
if (search == null) {
return null;
}return (root, query, criteriaBuilder) -> { if (search.startsWith("#")) { Join<Post, PostTag> postTagJoin = root.join("postTagList"); Join<PostTag, Tag> tagJoin = postTagJoin.join("tag"); return criteriaBuilder.like(tagJoin.get("name"), "%" + search.substring(1) + "%"); } return criteriaBuilder.like(root.get("title"), "%" + search + "%"); };
}
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.
좋은 의견 감사합니다. 하지만 이 경우에 준섭님 의견대로 구현했을 때, 저는 로직이 분리되어 약간 가독성이 떨어지는 느낌이 났습니다.
일단 else문을 제거하고 if문 블럭 두 개로 수정해보았습니다.
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.
좋아요!
private Map<String, String> extractSummaryDataFromJson(JSONObject jsonObject) { | ||
try { | ||
String title = jsonObject.getString("title"); | ||
String content = jsonObject.getString("content"); | ||
|
||
Map<String, String> summaryDataMap = new HashMap<>(); | ||
|
||
summaryDataMap.put("title", title); | ||
summaryDataMap.put("content", content); | ||
|
||
return summaryDataMap; | ||
|
||
} catch (JSONException e) { | ||
throw new CustomException(ErrorCode.INVALID_SUMMARY); | ||
} | ||
} |
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.
👍
💡 연관된 이슈
#41
📝 작업 내용
💬 리뷰 요구 사항
�그동안 말씀해주신 리뷰 사항들 수정하였습니다.
놓친 부분 있다면 알려주세요.