step1 #685
step1 #685
Conversation
|
#tag @neojjc2 |
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class Answers { |
There was a problem hiding this comment.
일급 컬렉션 사용 좋습니다 👍
다만 Answers가 하는일이 가지고 있는 Answer들을 일괄적으로 삭제 처리 하고
Answer들의 조회 권한만 가지게 하면 될 것 같습니다 🙇
| for (Answer answer : answers) { | ||
| answer.setDeleted(true); | ||
| } | ||
| } |
There was a problem hiding this comment.
Answer에게 delete상태로 만들어 달라 라고 하시는건 어떨까요?? 😄
answer에 set을 해서 값을 변경하는 경우라면 나중에 answer의 delete로직이 변경되거나 할 경우
Answers의 delete로직이 계속 수정되어야 합니다 🤔
그리고 delete로직 변경에 대해서 Answers, Answer 모두 영향을 받게 되니 한쪽만 영향을 받을 수 있도록
관심사를 좀 더 분리하면 좋을 것 같습니다.
저는 Question에서 해주신 것 처럼,
Answers는answer들에 대한 일괄 조작이나, 조회 역할, 작성자가 작성하지 않은 답변 있는지 검증 정도Answer에게는User를 줄테니 삭제를 하는 역할
이렇게 좀 나눠보면 될 것 같은데 어떻게 보시나요 😄
| } | ||
| } | ||
|
|
||
| public List<DeleteHistory> collectDeleteHistory() { |
There was a problem hiding this comment.
delete(), collect()를 나눠놓으신 이유가 있으실까요??
이렇게 되면 delete()를 호출하고 collect()를 호출하도록 항상 가이드 문서가 나가거나,
주의를 기울여야 하는 구조가 됩니다.
delete()를 호출하고 난 뒤 collect를 호출하는 타이밍이 느려지거나 혹은 빨라지거나 했을 경우엔
타이밍 이슈가 발생할 수 있고, 구조적으로도 delete를 한 반환 값이 history가 되는게
좀 더 자연스러울 것 같습니다 🙇
| public List<DeleteHistory> collectDeleteHistory() { | ||
| List<DeleteHistory> deleteHistories = new ArrayList<>(); | ||
| for (Answer answer : answers) { | ||
| deleteHistories.add(new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now())); |
There was a problem hiding this comment.
DeleteHistory의 경우 변경되는 값이 id, user를 제외하고는 고정 값이 될 것 같은데
정적 팩토리 method 같은 게 제공된다면 사용하는 도메인객체나, 서비스에서는 한결 사용성이 개선 될 것 같습니다 😄
// Question일 경우
deleteHistories.add(DeleteHistory.ofQuestion(id, answer.getWriter());
// Answer일 경우
deleteHistories.add(DeleteHistory.ofAnswer(answer.getId(), answer.getWriter();| if (!isOwner(loginUser)) { | ||
| throw new CannotDeleteException("질문 작성자 아닌 사람은 삭제 권한 없음"); | ||
| } | ||
| if (answers.hasAnswerFromOtherUser(writer)) { |
There was a problem hiding this comment.
answers에게 다른 답변이 있는지 확인 정도는 위임해도 괜찮을 것 같습니다만,
삭제하는 역할과 책임의 경우 answer가 할 수 있도록 해야 검증이 누락되지 않을 것 같습니다 😄
서비스에서 아래와 같이 작성할 경우 검증 없이
바로 answer는 삭제가 될 것 같습니다 🙏
List<Answer> answersOfQuestion = answerRepository.findByQuestion(questionId);
answersOfQuestion.forEach(answer -> answer.setDeleted(true));
사실 적절하게 도메인 객체들에게 역할과 책임을 위임함으로써 이런 문제들을 예방할 수 있습니다. 이런 개선이 당장은 시간이 좀 더 들더라도, 장기적으로는 유지보수성과 확장성 측면에서 큰 이득을 가져올 것입니다. 위 부분은 다소 개인적인 의견일 수 있으니 참고 정도만 해주시고 |
|
안녕하세요 @neojjc2! 마지막 미션을 함께 할 수 있게되서 너무 영광입니다 (_ _) |
| return this; | ||
| } | ||
| throw new CannotDeleteException("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다."); | ||
| } |
| deleteHistories.add(DeleteHistory.ofQuestion(id, loginUser)); | ||
| List<DeleteHistory> deletedAnswerHistory = answers.deleteAllAnswers(loginUser); | ||
| deleteHistories.addAll(deletedAnswerHistory); | ||
| return deleteHistories; |
There was a problem hiding this comment.
소소한 의견입니다만, DeleteHistory도 일급컬렉션으로 포장해주신 리뷰이 분이 계셨어서
개선까진 아니지만, 참고 해보시면 좋을 것 같아 소개 해드립니다 😄
return DeleteHistories.emptyHistories()
.record(DeleteHistory.of(ContentType.QUESTION, getId(), writer))
.recordAll(deletedAnswerHistory);
의문:
qnaService에서 get을 통해 answers를 가져와 qnaService.deleteQuestion 상에서 추가해도 좋을거같은데 question.collectDeleteHistory -> answers.collectDeleteHistory -> 반환 -> 반환 을 통하려하니 좀 복잡해지는게 아닌가 생각했습니다.