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: 리팩터링 #969

Open
wants to merge 10 commits into
base: hungryjayy
Choose a base branch
from
25 changes: 24 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,27 @@
* 모든 피드백을 완료하면 다음 단계를 도전하고 앞의 과정을 반복한다.

## 온라인 코드 리뷰 과정
* [텍스트와 이미지로 살펴보는 온라인 코드 리뷰 과정](https://github.com/next-step/nextstep-docs/tree/master/codereview)
* [텍스트와 이미지로 살펴보는 온라인 코드 리뷰 과정](https://github.com/next-step/nextstep-docs/tree/master/codereview)

## 질문 삭제하기 요구사항
* [x] 질문 데이터를 완전히 삭제하는 것이 아니라 데이터의 상태를 삭제 상태(deleted - boolean type)로 변경한다.
* [x] 로그인 사용자와 질문한 사람이 같은 경우 삭제 가능하다.
* [x] 답변이 없는 경우 삭제가 가능하다.
* [x] 질문자와 답변 글의 모든 답변자 같은 경우 삭제가 가능하다.
* [x] 질문을 삭제할 때 답변 또한 삭제해야 하며, 답변의 삭제 또한 삭제 상태(deleted)를 변경한다.
* [x] 질문자와 답변자가 다른 경우 답변을 삭제할 수 없다.
* [x] 질문과 답변 삭제 이력에 대한 정보를 DeleteHistory를 활용해 남긴다.

## 프로그래밍 요구사항
* [x] qna.service.QnaService의 deleteQuestion()는 앞의 질문 삭제 기능을 구현한 코드이다. 이 메소드는 단위 테스트하기 어려운 코드와 단위 테스트 가능한 코드가 섞여 있다.
* [x] 단위 테스트하기 어려운 코드와 단위 테스트 가능한 코드를 분리해 단위 테스트 가능한 코드 에 대해 단위 테스트를 구현한다.

### 힌트1
* 객체의 상태 데이터를 꺼내지(get)말고 메시지를 보낸다.
* 규칙 8: 일급 콜렉션을 쓴다.
* Question의 List를 일급 콜렉션으로 구현해 본다.
* 규칙 7: 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.
인스턴스 변수의 수를 줄이기 위해 도전한다.

### 힌트2
* 테스트하기 쉬운 부분과 테스트하기 어려운 부분을 분리해 테스트 가능한 부분만 단위테스트한다.
9 changes: 8 additions & 1 deletion 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 @@ -52,7 +53,7 @@ public boolean isDeleted() {
return deleted;
}

public boolean isOwner(User writer) {
private boolean isOwner(User writer) {
return this.writer.equals(writer);
}

Expand All @@ -72,4 +73,10 @@ public void toQuestion(Question question) {
public String toString() {
return "Answer [id=" + getId() + ", writer=" + writer + ", contents=" + contents + "]";
}

public void validateAnswerExists(User loginUser) throws CannotDeleteException {
if (!isOwner(loginUser)) {
throw new CannotDeleteException("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다.");
}
}
}
31 changes: 31 additions & 0 deletions src/main/java/qna/domain/DeleteHistories.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package qna.domain;

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

public class DeleteHistories {
private List<DeleteHistory> deleteHistories = new ArrayList<>();

public DeleteHistories() {
}

public DeleteHistories(List<DeleteHistory> deleteHistories) {
this.deleteHistories = deleteHistories;
}

public void add(long questionId, Question question) {

Choose a reason for hiding this comment

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

DeleteHistory 내부에서 question 과 answer 을 다루기보다
anwser 과 question 이 DeleteHistory 를 리턴하는 구조가 더 자연스럽지 않을까요?

question.setDeleted(true);
LocalDateTime now = LocalDateTime.now();
deleteHistories.add(new DeleteHistory(ContentType.QUESTION, questionId, question.getWriter(), now));

for (Answer answer : question.getAnswers()) {
answer.setDeleted(true);
deleteHistories.add(new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), now));
}
}

public List<DeleteHistory> getHistories() {
return deleteHistories;
}
}
40 changes: 23 additions & 17 deletions src/main/java/qna/domain/Question.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package qna.domain;

import org.hibernate.annotations.Where;
import qna.CannotDeleteException;

import javax.persistence.*;
import java.util.ArrayList;
Expand Down Expand Up @@ -33,28 +34,16 @@ public Question(String title, String contents) {
this.contents = contents;
}

public Question(long id, String title, String contents) {
super(id);
public Question(String title, String contents, List<Answer> answers) {
this.title = title;
this.contents = contents;
this.answers = answers;
}

public String getTitle() {
return title;
}

public Question setTitle(String title) {
public Question(long id, String title, String contents) {
super(id);
this.title = title;
return this;
}

public String getContents() {
return contents;
}

public Question setContents(String contents) {
this.contents = contents;
return this;
}

public User getWriter() {
Expand All @@ -71,7 +60,7 @@ public void addAnswer(Answer answer) {
answers.add(answer);
}

public boolean isOwner(User loginUser) {
private boolean isOwner(User loginUser) {
return writer.equals(loginUser);
}

Expand All @@ -92,4 +81,21 @@ public List<Answer> getAnswers() {
public String toString() {
return "Question [id=" + getId() + ", title=" + title + ", contents=" + contents + ", writer=" + writer + "]";
}

public void validate(User loginUser) throws CannotDeleteException {
validateQuestionAuthority(loginUser);
validateAnswerExists(loginUser);
}

private void validateQuestionAuthority(User loginUser) throws CannotDeleteException {
if (!isOwner(loginUser)) {
throw new CannotDeleteException("질문을 삭제할 권한이 없습니다.");
}
}

private void validateAnswerExists(User loginUser) throws CannotDeleteException {
for (Answer answer : answers) {
answer.validateAnswerExists(loginUser);
}
}
}
24 changes: 5 additions & 19 deletions src/main/java/qna/service/QnAService.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,24 +35,10 @@ 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.validate(loginUser);

DeleteHistories deleteHistories = new DeleteHistories();
deleteHistories.add(questionId, question);
deleteHistoryService.saveAll(deleteHistories.getHistories());
}
}
15 changes: 15 additions & 0 deletions src/test/java/qna/domain/AnswerTest.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,21 @@
package qna.domain;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import qna.CannotDeleteException;

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

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");

@Test
@DisplayName("다른 사람이 쓴 답변이 있는 경우 CannotDeleteException을 throw한다.")
public void validate_다른_사람의_답변_있는_글() {
assertThatThrownBy(() -> {
A1.validateAnswerExists(UserTest.SANJIGI);
}).isInstanceOf(CannotDeleteException.class)
.hasMessageContaining("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다.");
}
}
29 changes: 29 additions & 0 deletions src/test/java/qna/domain/DeleteHistoriesTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package qna.domain;

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

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

import static org.assertj.core.api.Assertions.assertThat;
import static qna.domain.QuestionTest.Q1;

class DeleteHistoriesTest {
public static final DeleteHistory DELETE_HISTORY1 = new DeleteHistory(ContentType.QUESTION, 1L, Q1.getWriter(),
LocalDateTime.now());

public static final DeleteHistories DELETE_HISTORIES1 = new DeleteHistories(List.of(DELETE_HISTORY1));

public static final DeleteHistories DELETE_HISTORIES2 = new DeleteHistories();

@Test
@DisplayName("Delete 히스토리를 추가한다.")
public void add_히스토리를_추가한다() {
int questionId = 1;

DELETE_HISTORIES2.add(questionId, Q1);

assertThat(DELETE_HISTORIES2.getHistories()).isEqualTo(DELETE_HISTORIES1.getHistories());
}
}
28 changes: 28 additions & 0 deletions src/test/java/qna/domain/QuestionTest.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,34 @@
package qna.domain;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import qna.CannotDeleteException;

import java.util.List;

import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static qna.domain.AnswerTest.A2;

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);
public static final Question Q3 = new Question("title1", "contents1", List.of(A2)).writeBy(UserTest.JAVAJIGI);

@Test
@DisplayName("다른 사람이 쓴 글의 경우 CannotDeleteException을 throw한다.")
public void validate_다른_사람이_쓴_글() {
assertThatThrownBy(() -> {
Q1.validate(UserTest.SANJIGI);
}).isInstanceOf(CannotDeleteException.class)
.hasMessageContaining("질문을 삭제할 권한이 없습니다.");
}

@Test
@DisplayName("다른 사람이 쓴 답변이 있는 경우 CannotDeleteException을 throw한다.")
public void validate_다른_사람의_답변_있는_글() {
assertThatThrownBy(() -> {
Q3.validate(UserTest.JAVAJIGI);
}).isInstanceOf(CannotDeleteException.class)
.hasMessageContaining("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다.");
}
}