Skip to content
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

Step1 #844

Open
wants to merge 5 commits into
base: blossun
Choose a base branch
from
Open

Step1 #844

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

본 과정은 스프링을 학습하는 과정이 아니기 때문에 현재로썬 불필요한 의존성 추가로 보여요.
스프링 웹 의존성 없이도 충분히 가능한 미션이기 때문에, 우선은 순수 자바로 구현해보시면 좋을 것 같습니다.

}

tasks.named('test') {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/qna/CannotDeleteException.java
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package qna;

public class CannotDeleteException extends Exception {
public class CannotDeleteException extends RuntimeException {
private static final long serialVersionUID = 1L;

public CannotDeleteException(String message) {
super(message);
}
}
}
9 changes: 6 additions & 3 deletions src/main/java/qna/domain/Answer.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package qna.domain;

import qna.CannotDeleteException;
import qna.NotFoundException;
import qna.UnAuthorizedException;

Expand Down Expand Up @@ -43,9 +44,11 @@ public Answer(Long id, User writer, Question question, String contents) {
this.contents = contents;
}

public Answer setDeleted(boolean deleted) {
this.deleted = deleted;
return this;
public void delete(final User loginUser) {
if (!this.writer.equals(loginUser)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존에 존재하는 isOwner() 메서드를 활용해보면 어떨까요?

throw new CannotDeleteException("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다.");
}
this.deleted = true;
}

public boolean isDeleted() {
Expand Down
35 changes: 35 additions & 0 deletions src/main/java/qna/domain/AnswerList.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package qna.domain;

import java.util.ArrayList;
import java.util.List;

import javax.persistence.CascadeType;
import javax.persistence.Embeddable;
import javax.persistence.OneToMany;
import javax.persistence.OrderBy;

import org.hibernate.annotations.Where;

@Embeddable
public class AnswerList {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 해당 일급 콜렉션에 대한 테스트 코드도 추가해보시면 좋을 것 같아요.
  • 클래스 또는 변수 이름에 List, Set과 같은 자료구조명을 포함시키는 건 지양해야 한다고 생각하는데요,
    대신에 이름을 복수형으로 지어보는 건 어떻게 생각하시나요?
    https://tecoble.techcourse.co.kr/post/2020-04-24-variable_naming/

@OneToMany(mappedBy = "question", cascade = CascadeType.ALL)
@Where(clause = "deleted = false")
@OrderBy("id ASC")
private final List<Answer> answers = new ArrayList<>();

public boolean isEmpty() {
return answers.isEmpty();
}

public void add(final Answer answer) {
answers.add(answer);
}

public void deleteAll(final User loginUser) {
answers.forEach(answer -> answer.delete(loginUser));
}

public List<Answer> getAnswers() {
return answers;
}
}
33 changes: 33 additions & 0 deletions src/main/java/qna/domain/DeleteEventHandler.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package qna.domain;

import java.time.LocalDateTime;
import java.util.ArrayList;
import java.util.List;

import javax.annotation.Resource;

import org.springframework.context.event.EventListener;
import org.springframework.stereotype.Component;

import qna.service.DeleteHistoryService;

@Component
public class DeleteEventHandler {

@Resource(name = "deleteHistoryService")
private DeleteHistoryService deleteHistoryService;

@EventListener
public void saveHistory(DeleteQuestionEvent deleteQuestionEvent) {
System.out.println("Subscribe deleteQuestion event!!!!!");
final Question question = deleteQuestionEvent.getQuestion();

List<DeleteHistory> deleteHistories = new ArrayList<>();
deleteHistories.add(new DeleteHistory(ContentType.QUESTION, question.getId(), question.getWriter(), LocalDateTime.now()));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이벤트로 처리하려다 보니 게터를 쓰게되었습니다.
그래도 Question에서 히스토리를 남기는 로직을 처리하는 것보다 이벤트가 낫다고 생각하여 이런 구조로 만들게 되었는데, 이런 겟터도 줄일 수 있을까요??

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question 클래스에서 DeleteHistory를 생성하도록 메시지를 보내서 처리해보시면 좋을 것 같아요.


for (Answer answer : question.getAnswers().getAnswers()) {
deleteHistories.add(new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now()));
}
deleteHistoryService.saveAll(deleteHistories);
}
}
13 changes: 13 additions & 0 deletions src/main/java/qna/domain/DeleteQuestionEvent.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package qna.domain;

public class DeleteQuestionEvent {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

삭제 히스토리 로직을 이벤트 발행 형태로 만들고 싶어서 구조를 잡아보았는데 구독하는 쪽의 테스트는 어떻게 짜야할지 모르겠어서 작성하지 못하였습니다. 이런 구조에 대한 테스트는 어떻게 할 수 있을지 코멘트주신다면 감사하겠습니다.

1단계 미션의 주요 핵심은, 비즈니스 로직에서 테스트하기 어려운 코드와 가능한 코드를 분리하고
그 중 테스트하기 쉬운 부분에 대한 단위 테스트를 작성하며 리팩토링을 진행하는 것입니다.
힌트를 보시면 아시겠지만, 이를 위해선 핵심 비즈니스 로직을 도메인 객체 내부에 구현하는 게 좋습니다.
다만 현재 질문 및 답변의 삭제 이력 처리를 도메인 객체가 아닌 다른 객체에서 담당하고 있고,
선영님께서 구현하려고 하신 방식은 @EventListener 등의 스프링 기능에 의존적이기 때문에
테스트를 위해 추가 학습이 더 필요한, 어떻게 보면 테스트하기 어려운 구조라고 볼 수 있습니다.

물론 스프링 컨테이너를 띄우고 필요한 스프링 빈들을 주입 받아서 테스트를 수행할 수는 있지만,
어떤 구조가 테스트를 반복적으로 더 빠르게 실행할 수 있는지를 고민해보시면 좋을 것 같다는 의견을 드려봅니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 감사합니다. :)
궁금한 점이 있어서 질문드립니다.

저는 "삭제 이력 기록"이 Question과 Answer의 핵심 비즈니스 로직이 아니라고 생각을 했습니다.
Question과 Answer에서 delete()는 "삭제"라는 한가지 일만 해야하지 않을까 생각을 했고, 따라서 "삭제 이력 남기기"를 delete()에서 분리하고자 이벤트를 발행하는 구조를 고려하게 되었습니다.

이벤트 발행/구독 구조가 학습 요구사항을 벗어나는 것 같지만 이런 구조가 OOP를 벗어나는 것은 아니라고 생각이 드는데 이 부분에 대한 리뷰어님의 의견은 어떠실지 궁금합니다.

Copy link
Author

@blossun blossun May 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로 현재 코드처럼 이벤트 발행을 서비스에서 한다는 것은 잘못됐네요. 이벤트를 발행하더라고 그 주체는 서비스가 아니라 도메인이 되어야할 것 같습니다. ㅠ
계속 고민하다보니 "삭제 이력 기록"을 "delete()"에서 호출하는 것이 "삭제"라는 한가지 일만하는 것이 아니라고 생각했는데, "게시물을 삭제했을 때 처리과정"이라는 하나의 흐름에 벗어나는게 아니기 때문에 이 생각은 틀린 것 같네요 ㅎㅎ..

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이벤트 발행/구독 구조가 학습 요구사항을 벗어나는 것 같지만 이런 구조가 OOP를 벗어나는 것은 아니라고 생각이 드는데 이 부분에 대한 리뷰어님의 의견은 어떠실지 궁금합니다.

넵, OOP의 특성을 잘 살려서 구현하는 거라면 저도 말씀하신 의견에 동의합니다.

private final Question question;

public DeleteQuestionEvent(final Question question) {
this.question = question;
}

public Question getQuestion() {
return question;
}
}
35 changes: 26 additions & 9 deletions src/main/java/qna/domain/Question.java
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
package qna.domain;

import org.hibernate.annotations.Where;
import javax.persistence.Column;
import javax.persistence.Embedded;
import javax.persistence.Entity;
import javax.persistence.ForeignKey;
import javax.persistence.JoinColumn;
import javax.persistence.Lob;
import javax.persistence.ManyToOne;

import javax.persistence.*;
import java.util.ArrayList;
import java.util.List;
import qna.CannotDeleteException;

@Entity
public class Question extends AbstractEntity {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아직 규칙 7: 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다. 요구사항을 만족하지 못하였는데 어떻게 진행하면 좋을지 조언주실 수 있나요?

서로 관련이 있는 필드들을 묶어서 새로운 객체를 정의하는 것도 하나의 방법이라고 생각합니다.

Expand All @@ -18,10 +22,8 @@ public class Question extends AbstractEntity {
@JoinColumn(foreignKey = @ForeignKey(name = "fk_question_writer"))
private User writer;

@OneToMany(mappedBy = "question", cascade = CascadeType.ALL)
@Where(clause = "deleted = false")
@OrderBy("id ASC")
private List<Answer> answers = new ArrayList<>();
@Embedded
private final AnswerList answers = new AnswerList();

private boolean deleted = false;

Expand Down Expand Up @@ -80,11 +82,26 @@ public Question setDeleted(boolean deleted) {
return this;
}

public void delete(final User loginUser) throws CannotDeleteException {
if (!this.writer.equals(loginUser)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분도 기존의 isOwner() 메서드를 활용해보는 건 어떨까요?

throw new CannotDeleteException("질문자 본인만 삭제할 수 있습니다.");
}

if (answers.isEmpty()) {
this.deleted = true;
return;
}

answers.deleteAll(loginUser);

this.deleted = true;
}

public boolean isDeleted() {
return deleted;
}

public List<Answer> getAnswers() {
public AnswerList getAnswers() {
return answers;
}

Expand Down
33 changes: 6 additions & 27 deletions src/main/java/qna/service/QnAService.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,14 @@

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.context.ApplicationEventPublisher;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
import qna.CannotDeleteException;
import qna.NotFoundException;
import qna.domain.*;

import javax.annotation.Resource;
import java.time.LocalDateTime;
import java.util.ArrayList;
import java.util.List;

@Service("qnaService")
public class QnAService {
Expand All @@ -20,11 +18,8 @@ public class QnAService {
@Resource(name = "questionRepository")
private QuestionRepository questionRepository;

@Resource(name = "answerRepository")
private AnswerRepository answerRepository;

@Resource(name = "deleteHistoryService")
private DeleteHistoryService deleteHistoryService;
@Resource(name = "applicationEventPublisher")
private ApplicationEventPublisher applicationEventPublisher;

@Transactional(readOnly = true)
public Question findQuestionById(Long id) {
Expand All @@ -35,24 +30,8 @@ public Question findQuestionById(Long id) {
@Transactional
public void deleteQuestion(User loginUser, long questionId) throws CannotDeleteException {
Question question = findQuestionById(questionId);
if (!question.isOwner(loginUser)) {
throw new CannotDeleteException("질문을 삭제할 권한이 없습니다.");
}

List<Answer> answers = question.getAnswers();
for (Answer answer : answers) {
if (!answer.isOwner(loginUser)) {
throw new CannotDeleteException("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다.");
}
}

List<DeleteHistory> deleteHistories = new ArrayList<>();
question.setDeleted(true);
deleteHistories.add(new DeleteHistory(ContentType.QUESTION, questionId, question.getWriter(), LocalDateTime.now()));
for (Answer answer : answers) {
answer.setDeleted(true);
deleteHistories.add(new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now()));
}
deleteHistoryService.saveAll(deleteHistories);
question.delete(loginUser);

applicationEventPublisher.publishEvent(new DeleteQuestionEvent(question));
}
}
39 changes: 39 additions & 0 deletions src/test/java/qna/domain/AnswerTest.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,45 @@
package qna.domain;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

import qna.CannotDeleteException;

public class AnswerTest {
public static final Answer A1 = new Answer(UserTest.JAVAJIGI, QuestionTest.Q1, "Answers Contents1");
public static final Answer A2 = new Answer(UserTest.SANJIGI, QuestionTest.Q1, "Answers Contents2");

private Answer answer;

@BeforeEach
void setUp() {
answer = new Answer(11L, UserTest.JAVAJIGI, QuestionTest.Q1, "Answers Contents1");
}

@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();
}
Comment on lines +23 to +43

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트 케이스를 잘 정리해서 테스트해주셨네요 👍


}
82 changes: 82 additions & 0 deletions src/test/java/qna/domain/QuestionTest.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,88 @@
package qna.domain;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assertions.assertAll;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

import qna.CannotDeleteException;

public class QuestionTest {
public static final Question Q1 = new Question("title1", "contents1").writeBy(UserTest.JAVAJIGI);
public static final Question Q2 = new Question("title2", "contents2").writeBy(UserTest.SANJIGI);

private Question question;
private Answer answer;
private Answer answer2;

@BeforeEach
public void setUp() {
question = new Question(1L, "title1", "contents1").writeBy(UserTest.JAVAJIGI);
answer = new Answer(11L, UserTest.JAVAJIGI, QuestionTest.Q1, "Answers Contents1");
answer2 = new Answer(UserTest.JAVAJIGI, question, "answer contents2");
question.addAnswer(answer);
}


@DisplayName("질문자가 본인이 아니면 질문을 삭제할 수 없다.")
@Test
public void delete_fail_not_owner() {
assertThatThrownBy(() -> question.delete(UserTest.SANJIGI))
.isInstanceOf(CannotDeleteException.class);
}

@DisplayName("질문자 본인이고, 답변이 없으면 삭제가 가능하다.")
@Test
public void delete_success_owner_and_no_answer() {
assertThat(question.isDeleted()).isFalse();

question.delete(UserTest.JAVAJIGI);

assertThat(question.isDeleted()).isTrue();
}
Comment on lines +37 to +45

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BeforeEach를 통해 테스트 시작 전에 question.addAnswer(answer)가 호출되기 때문에,
해당 테스트 케이스는 질문자는 본인이되, 답변이 없는 경우는 아니지 않을까요?


@DisplayName("질문자가 본인이고, 답변이 있는 경우, 모든 답변자가 본인이면 삭제가 가능하다.")
@Test
public void delete_success_question_and_answer_owner() {
question.addAnswer(answer);
assertThat(question.isDeleted()).isFalse();

question.delete(UserTest.JAVAJIGI);

assertThat(question.isDeleted()).isTrue();
}

@DisplayName("질문자가 본인이고, 답변이 있는 경우, 모든 답변자가 본인이 아니면 삭제가 불가능하다.")
@Test
public void delete_fail_answer_other() {
question.addAnswer(AnswerTest.A1);
question.addAnswer(AnswerTest.A2);

assertThatThrownBy(() -> question.delete(UserTest.JAVAJIGI))
.isInstanceOf(CannotDeleteException.class)
.hasMessage("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다.");
}

@DisplayName("질문이 삭제되면 답변도 모두 삭제된다.")
@Test
public void delete_success_all_answer_deleted() {
question.addAnswer(answer2);
assertAll(
() -> assertThat(question.isDeleted()).isFalse(),
() -> assertThat(answer.isDeleted()).isFalse(),
() -> assertThat(answer2.isDeleted()).isFalse()
);

question.delete(UserTest.JAVAJIGI);

assertAll(
() -> assertThat(question.isDeleted()).isTrue(),
() -> assertThat(answer.isDeleted()).isTrue(),
() -> assertThat(answer2.isDeleted()).isTrue()
);
}

}
Loading