Step1 #844
Step1 #844
Conversation
- 답변목록을 AnswerList 일급컬렉션으로 구현 - AnswerList에서 모든 답변을 삭제하는 기능 구현
| final Question question = deleteQuestionEvent.getQuestion(); | ||
|
|
||
| List<DeleteHistory> deleteHistories = new ArrayList<>(); | ||
| deleteHistories.add(new DeleteHistory(ContentType.QUESTION, question.getId(), question.getWriter(), LocalDateTime.now())); |
There was a problem hiding this comment.
이벤트로 처리하려다 보니 게터를 쓰게되었습니다.
그래도 Question에서 히스토리를 남기는 로직을 처리하는 것보다 이벤트가 낫다고 생각하여 이런 구조로 만들게 되었는데, 이런 겟터도 줄일 수 있을까요??
There was a problem hiding this comment.
Question 클래스에서 DeleteHistory를 생성하도록 메시지를 보내서 처리해보시면 좋을 것 같아요.
| final Question question = deleteQuestionEvent.getQuestion(); | ||
|
|
||
| List<DeleteHistory> deleteHistories = new ArrayList<>(); | ||
| deleteHistories.add(new DeleteHistory(ContentType.QUESTION, question.getId(), question.getWriter(), LocalDateTime.now())); |
There was a problem hiding this comment.
Question 클래스에서 DeleteHistory를 생성하도록 메시지를 보내서 처리해보시면 좋을 것 같아요.
| @@ -17,6 +17,7 @@ dependencies { | |||
| runtimeOnly 'com.h2database:h2' | |||
| testImplementation 'org.assertj:assertj-core:3.22.0' | |||
| testImplementation 'org.springframework.boot:spring-boot-starter-test' | |||
| implementation 'org.springframework.boot:spring-boot-starter-web' | |||
There was a problem hiding this comment.
본 과정은 스프링을 학습하는 과정이 아니기 때문에 현재로썬 불필요한 의존성 추가로 보여요.
스프링 웹 의존성 없이도 충분히 가능한 미션이기 때문에, 우선은 순수 자바로 구현해보시면 좋을 것 같습니다.
| this.deleted = deleted; | ||
| return this; | ||
| public void delete(final User loginUser) { | ||
| if (!this.writer.equals(loginUser)) { |
There was a problem hiding this comment.
기존에 존재하는 isOwner() 메서드를 활용해보면 어떨까요?
| @@ -80,11 +82,26 @@ public Question setDeleted(boolean deleted) { | |||
| return this; | |||
| } | |||
|
|
|||
| public void delete(final User loginUser) throws CannotDeleteException { | |||
| if (!this.writer.equals(loginUser)) { | |||
There was a problem hiding this comment.
이 부분도 기존의 isOwner() 메서드를 활용해보는 건 어떨까요?
| @DisplayName("작성자가 본인이면 답변을 지울 수 있다.") | ||
| @Test | ||
| public void delete_success_owner() { | ||
| assertThat(answer.isDeleted()).isFalse(); | ||
|
|
||
| answer.delete(UserTest.JAVAJIGI); | ||
|
|
||
| assertThat(answer.isDeleted()).isTrue(); | ||
| } | ||
|
|
||
| @DisplayName("작성자가 아니면 답변을 지울 수 없다.") | ||
| @Test | ||
| public void delete_fail_no_owner() { | ||
| assertThat(answer.isDeleted()).isFalse(); | ||
|
|
||
| assertThatThrownBy(() -> answer.delete(UserTest.SANJIGI)) | ||
| .isInstanceOf(CannotDeleteException.class) | ||
| .hasMessage("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다."); | ||
|
|
||
| assertThat(answer.isDeleted()).isFalse(); | ||
| } |
| @DisplayName("질문자 본인이고, 답변이 없으면 삭제가 가능하다.") | ||
| @Test | ||
| public void delete_success_owner_and_no_answer() { | ||
| assertThat(question.isDeleted()).isFalse(); | ||
|
|
||
| question.delete(UserTest.JAVAJIGI); | ||
|
|
||
| assertThat(question.isDeleted()).isTrue(); | ||
| } |
There was a problem hiding this comment.
@BeforeEach를 통해 테스트 시작 전에 question.addAnswer(answer)가 호출되기 때문에,
해당 테스트 케이스는 질문자는 본인이되, 답변이 없는 경우는 아니지 않을까요?
| import org.hibernate.annotations.Where; | ||
|
|
||
| @Embeddable | ||
| public class AnswerList { |
There was a problem hiding this comment.
- 해당 일급 콜렉션에 대한 테스트 코드도 추가해보시면 좋을 것 같아요.
- 클래스 또는 변수 이름에
List,Set과 같은 자료구조명을 포함시키는 건 지양해야 한다고 생각하는데요,
대신에 이름을 복수형으로 지어보는 건 어떻게 생각하시나요?
https://tecoble.techcourse.co.kr/post/2020-04-24-variable_naming/
| import javax.persistence.*; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import qna.CannotDeleteException; | ||
|
|
||
| @Entity | ||
| public class Question extends AbstractEntity { |
There was a problem hiding this comment.
아직 규칙 7: 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다. 요구사항을 만족하지 못하였는데 어떻게 진행하면 좋을지 조언주실 수 있나요?
서로 관련이 있는 필드들을 묶어서 새로운 객체를 정의하는 것도 하나의 방법이라고 생각합니다.
| @@ -0,0 +1,13 @@ | |||
| package qna.domain; | |||
|
|
|||
| public class DeleteQuestionEvent { | |||
There was a problem hiding this comment.
삭제 히스토리 로직을 이벤트 발행 형태로 만들고 싶어서 구조를 잡아보았는데 구독하는 쪽의 테스트는 어떻게 짜야할지 모르겠어서 작성하지 못하였습니다. 이런 구조에 대한 테스트는 어떻게 할 수 있을지 코멘트주신다면 감사하겠습니다.
1단계 미션의 주요 핵심은, 비즈니스 로직에서 테스트하기 어려운 코드와 가능한 코드를 분리하고
그 중 테스트하기 쉬운 부분에 대한 단위 테스트를 작성하며 리팩토링을 진행하는 것입니다.
힌트를 보시면 아시겠지만, 이를 위해선 핵심 비즈니스 로직을 도메인 객체 내부에 구현하는 게 좋습니다.
다만 현재 질문 및 답변의 삭제 이력 처리를 도메인 객체가 아닌 다른 객체에서 담당하고 있고,
선영님께서 구현하려고 하신 방식은 @EventListener 등의 스프링 기능에 의존적이기 때문에
테스트를 위해 추가 학습이 더 필요한, 어떻게 보면 테스트하기 어려운 구조라고 볼 수 있습니다.
물론 스프링 컨테이너를 띄우고 필요한 스프링 빈들을 주입 받아서 테스트를 수행할 수는 있지만,
어떤 구조가 테스트를 반복적으로 더 빠르게 실행할 수 있는지를 고민해보시면 좋을 것 같다는 의견을 드려봅니다.
There was a problem hiding this comment.
리뷰 감사합니다. :)
궁금한 점이 있어서 질문드립니다.
저는 "삭제 이력 기록"이 Question과 Answer의 핵심 비즈니스 로직이 아니라고 생각을 했습니다.
Question과 Answer에서 delete()는 "삭제"라는 한가지 일만 해야하지 않을까 생각을 했고, 따라서 "삭제 이력 남기기"를 delete()에서 분리하고자 이벤트를 발행하는 구조를 고려하게 되었습니다.
이벤트 발행/구독 구조가 학습 요구사항을 벗어나는 것 같지만 이런 구조가 OOP를 벗어나는 것은 아니라고 생각이 드는데 이 부분에 대한 리뷰어님의 의견은 어떠실지 궁금합니다.
There was a problem hiding this comment.
추가로 현재 코드처럼 이벤트 발행을 서비스에서 한다는 것은 잘못됐네요. 이벤트를 발행하더라고 그 주체는 서비스가 아니라 도메인이 되어야할 것 같습니다. ㅠ
계속 고민하다보니 "삭제 이력 기록"을 "delete()"에서 호출하는 것이 "삭제"라는 한가지 일만하는 것이 아니라고 생각했는데, "게시물을 삭제했을 때 처리과정"이라는 하나의 흐름에 벗어나는게 아니기 때문에 이 생각은 틀린 것 같네요 ㅎㅎ..
There was a problem hiding this comment.
이벤트 발행/구독 구조가 학습 요구사항을 벗어나는 것 같지만 이런 구조가 OOP를 벗어나는 것은 아니라고 생각이 드는데 이 부분에 대한 리뷰어님의 의견은 어떠실지 궁금합니다.
넵, OOP의 특성을 잘 살려서 구현하는 거라면 저도 말씀하신 의견에 동의합니다.
안녕하세요 리뷰어님
1단계 구현하여 리뷰 요청드립니다. :)
아직
규칙 7: 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.요구사항을 만족하지 못하였는데 어떻게 진행하면 좋을지 조언주실 수 있나요?삭제 히스토리 로직을 이벤트 발행 형태로 만들고 싶어서 구조를 잡아보았는데 구독하는 쪽의 테스트는 어떻게 짜야할지 모르겠어서 작성하지 못하였습니다. 이런 구조에 대한 테스트는 어떻게 할 수 있을지 코멘트주신다면 감사하겠습니다. 🙇♀️