-
Notifications
You must be signed in to change notification settings - Fork 4
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(Comment) : 댓글 기능 추가 #102
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.
댓글 기능 PR 주신 것 확인하고 몇 가지의 사항을 리뷰 해보았습니다.
수고하셨습니다 👍
public record CommentDeleteResponseDTO( | ||
boolean deleted | ||
) { | ||
public static CommentDeleteResponseDTO from(Comment comment) { | ||
return new CommentDeleteResponseDTO( | ||
comment.isDeleted() | ||
); | ||
} | ||
} |
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.
코드 리뷰 중 deleteResponseDTO
클래스에 대해 ... ! 의견 남겨봅니다.
현재 구현된 내용을 보면서, 이 클래스가 반드시 필요한지에 대해 고민해 보았습니다.
해당 클래스에서 isDeleted
만 들어가는 거라면 deleteResponseDTO
클래스 보다는 CommentResponseDTO
를 사용하는 것이 코드의 단순화와 유지 보수 측면에서 조금 더 효율적일 수 있다는 생각이 듭니다.
프론트에서 응답을 받았을 때 isDeleted
만 받았을 때는 어떤 댓글에 대한 삭제를 했는지를 나타낼 수 없지만, CommentResponseDTO
를 사용하면 기존 코드를 재사용 하면서도 프론트에서 좀 더 자세하게 알아볼 수 있을 것 같아요.
어떻게 생각하시는지 궁금합니다!
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.
불필요한 데이터 전송을 최소화하고자 따로 만들었는데, 유지 보수 측면을 생각해보면 CommentResponseDTO로 통일 시키는 것도 괜찮아 보이네요.
프론트에서 댓글을 삭제했을 때 CommentResponseDTO로 응답을 받아도 null값과 isDeleted의 true값만을 받아서 어떤 댓글에 대한 삭제를 했는지 알기는 어려울 것 같습니다. 그렇다면 CommentDeletedDTO를 활용하여 삭제된 댓글의 id값과 댓글 내용을 같이 보내주어 프론트측에서 어떤 댓글이 삭제되었는지 명확히 알 수 있도록 하는 방법도 괜찮아 보이는데 어떻게 생각하시는지 궁금합니다!
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.
추가적으로, 예슬님 피드백을 보다보니 CommentResponseDTO에 CommentID값을 같이 보내주도록 추가하는 것이 필수적인 것 같습니다. 감사합니다!
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.
CommentId가 없었군요? 추가 하는 것 좋습니다 👍👍
public record CommentUpdateRequestDTO( | ||
String content | ||
) { | ||
} |
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.
해당 클래스도 deleteResponseDTO
에 대한 동일한 의견 드립니다.
} | ||
|
||
@Transactional | ||
public CommentResponseDTO update(Long CommentId, CommentUpdateRequestDTO requestDTO, Long logInMemberId) { |
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.
update
와 delete
메서드에서 첫 번째 매개변수 CommentId
에서 대문자로 시작하고 있어요! 다른 변수들과 동일하도록 commentId
로 수정하면 좋을 것 같습니다.
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.
헉 못보고 지나친 부분인데 짚어주셔서 감사합니다!
@Transactional(readOnly = true) | ||
public List<CommentResponseDTO> findAll(Pageable pageable, Long diaryId) { | ||
Diary diary = getDiaryById(diaryId); | ||
List<Comment> comments = commentDAO.findAllByDiaryId(diaryId, pageable); | ||
|
||
List<Comment> hierarchicalComments = convertHierarchy(comments); | ||
|
||
return hierarchicalComments.stream() | ||
.map(this::mapToDTO) | ||
.collect(Collectors.toList()); | ||
} |
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.
이 부분에서 mapToDTO
를 CommentResponseDTO::from
으로 코드를 재사용 가능할 것 같아 보여요!
public record CommentResponseDTO( | ||
// 멤버 정보(사진, 아이디), 댓글내용, 작성 시간 | ||
// todo: 사진정보 추가 | ||
Long memberId, | ||
String content, | ||
boolean deleted, | ||
String createdDate, | ||
String createdAt, | ||
List<CommentResponseDTO> childComments | ||
) { | ||
public static CommentResponseDTO from(Comment comment) { | ||
return new CommentResponseDTO( | ||
comment.getMember().getId(), | ||
comment.getContent(), | ||
comment.isDeleted(), | ||
comment.getCreatedAt().format(DateTimeFormatter.ofPattern("yy년 MM월 dd일")), | ||
comment.getCreatedAt().format(DateTimeFormatter.ofPattern("HH:mm")), | ||
comment.getChildComments().stream().map(CommentResponseDTO::from).collect(Collectors.toList()) | ||
); | ||
} | ||
|
||
public static CommentResponseDTO fromDeleted(Comment comment) { | ||
return new CommentResponseDTO( | ||
null, | ||
null, | ||
true, | ||
null, | ||
null, | ||
comment.getChildComments().stream().map(CommentResponseDTO::from).collect(Collectors.toList()) | ||
); | ||
} |
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.
from
메서드와 fromDeleted
메서드를 합칠 수도 있을 것 같아요. isDeleted
값으로 반환 값이 달라지는 경우이니 if문을 사용해서 return 을 해주면 어떨까요? 그리고 모든 값을 null로 보내기 보다는 content 부분에 "삭제 된 댓글입니다."와 같은 멘트가 들어있으면 프론트에서 처리하기 더 좋을 것 같아요.
아래는 예시 로직입니다!
public static CommentResponseDTO from(Comment comment) {
if (comment.isDeleted()) {
return new CommentResponseDTO(
null,
"삭제된 댓글입니다.",
true,
null,
null,
comment.getChildComments().stream().map(CommentResponseDTO::from).collect(Collectors.toList())
);
}
return new CommentResponseDTO(동일 코드);
}
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가 데이터를 전송하기 위해 사용되는 객체인 만큼, 지워진 댓글인지를 판별하는 것은 서비스 영역에서 해야된다고 생각이 들어서 두 부분을 분리하기로 결정했던 기억이 납니다!
삭제된 댓글 여부는 말씀해주신대로 content에 적어서 보내주면 확실히 좀 더 명확할 것 같습니다! 감사합니다!
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이 들어간 Comment와 값이 들어간 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.
대략 이런 느낌일 것 같아요
CommentServiceClass () {
...
is_deleted_판단하는_메서드(Comment comment) {
if (comment.isDeleted()) {
return CommentResponseDTO.fromDeleted();
}
return CommentResponseDTO.from();
}
}
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 CommentResponseDTO mapToDTO(Comment comment) {
if (comment.isDeleted()) {
return CommentResponseDTO.fromDeleted(comment);
}
return CommentResponseDTO.from(comment);
}
@Transactional(readOnly = true) | ||
public List<CommentResponseDTO> findAll(Pageable pageable, Long diaryId) { | ||
Diary diary = getDiaryById(diaryId); | ||
List<Comment> comments = commentDAO.findAllByDiaryId(diaryId, pageable); |
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.
pageable
을 사용하시는데, findAllByDiaryId
보다는 findAllByDiaryIdWithPagination
은 어떨까요?
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.
댓글의 pageable과 같은 경우는 그냥 모두 불러오도록 List로 받도록 했는데, 파라미터를 미처 삭제하지 못했습니다 ㅋㅋㅋ ㅠㅠ
pageable을 사용해서 일정 개수만 불러오도록 하는게 좋을지 고민이 됩니다!
@GetMapping("/{diaryId}") | ||
public List<CommentResponseDTO> findAll( | ||
Pageable pageable, | ||
@PathVariable(name = "diaryId") Long diaryId | ||
) { | ||
return commentService.findAll(pageable, diaryId); | ||
} |
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.
Pageable을 사용할 때 default 개수를 설정 해 주는 것은 어떨까요? 몇 개 정도가 적당하다고 생각히시는지 궁금해요. 5개 정도면 괜찮으려나요?
@OneToMany(mappedBy = "diary") | ||
private List<Comment> comments = new ArrayList<>(); | ||
|
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.
다이어리 DB에 댓글을 넣는 이유가 있을까요?
해당 DB에 넣지 않아도 댓글 DB에 다이어리 id가 들어있으니 식별이 가능할 것 같아요.
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.
음 ... 주석 처리된 이 클래스는 삭제해주시면 어떨까 합니다!
PR 내용은 댓글 기능 추가인데 해당 클래스가 들어있는 것도 PR 목적과 다른 클래스가 들어있는 것 같습니다.
주요 기능
계층형 댓글 구조란?
댓글 시스템에서 댓글과 대댓글이 계층적으로 연결되어 있는 형태를 의미
어떻게 구현했나?
Adjacency List(인접 리스트)를 사용해서 구현.
댓글 저장
로그인 된 사용자의 정보를 바탕으로 댓글을 생성하고, 댓글이 달린 일기와 부모 댓글 정보를 포함하여 최종적으로 데이터베이스에 저장된 댓글 정보를 클라이언트에게 반환
댓글 수정
로그인된 사용자의 정보를 바탕으로 수정하려는 댓글을 조회하고, 댓글 작성자인지 확인한 후 댓글 내용을 업데이트하여 최종적으로 클라이언트에게 반환
댓글 삭제
로그인된 사용자의 정보를 바탕으로 삭제하려는 댓글을 조회하고, 댓글 작성자인지 확인한 후 댓글을 삭제 상태로 변경하여 최종적으로 클라이언트에게 반환
댓글이 삭제 되어도 해당 댓글에 대한 대댓글을 유지하기 위해, 실질적으로 삭제하지 않고 deleted라는 삭제 여부를 저장하는 컬럼을 둠.
CommentResponseDTO에 fromDeleted 메서드를 추가하여 삭제된 상태일 때는 댓글에 대한 정보를 null값을 받아 반환하도록 함.
댓글 불러오기