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] 레거시 코드 리팩터링 #468

Open
wants to merge 1 commit into
base: giraffeb
Choose a base branch
from
Open
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
40 changes: 40 additions & 0 deletions src/main/java/nextstep/qna/domain/Answers.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package nextstep.qna.domain;

Choose a reason for hiding this comment

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

질문 도메인에 삭제하는 조건이 들어가는게 맞을까요?
질문을 삭제하려면 답변의 작성자를 확인해야합니다. 데이터베이스에서 모두 답변을 조회 후 확인하는 형태가 맞을까요?

현재 Question이 Answers를 가지고 있는 형태로 구성되어 있습니다. 일반적으로 JPA를 사용할 것이라 Lazy 로딩으로 가져온 Answers가 다 조회된 것이라 가정하고 작성하셔도 무방하고, 그게 아니라면 직접 AnswerRepository를 통해 조회한 후 Question의 Answers에 값이 할당된 상태로 인스턴스를 만들어주시면 될 것 같습니다.


import nextstep.users.domain.NsUser;

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

public class Answers {
private List<Answer> answerList = new ArrayList<Answer>();

public void add(Answer answer){
this.answerList.add(answer);
}

public boolean isAllOwner(NsUser loginUser){
return this.answerList
.stream()
.allMatch( answer -> answer.isOwner(loginUser));
}

private boolean isEmpty(){
return this.answerList.isEmpty();
}

public boolean isDeletable(NsUser loginUser){
boolean isEmpty = this.isEmpty();
boolean isAllOwner = this.isAllOwner(loginUser);

return isEmpty || isAllOwner;
}

public void setAllDeleted(boolean deleted){

Choose a reason for hiding this comment

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

set이 아닌 deleteAll 등의 동사 형태로 표현해도 무방할 것 같아요.

this.answerList
.forEach( answer -> answer.setDeleted(deleted));
}

public List<Answer> getAll(){
return this.answerList;
Comment on lines +37 to +38

Choose a reason for hiding this comment

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

외부에 참조가 그대로 노출되고 있네요. 외부에서 추가 및 삭제를 할 수 있지 않을까요? Collections.unmodifiableList를 사용하시거나 Stream의 toList 등을 활용해보시면 어떨까요?

}
}
41 changes: 41 additions & 0 deletions src/main/java/nextstep/qna/domain/DeleteHistories.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package nextstep.qna.domain;

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

public class DeleteHistories {
private List<DeleteHistory> deleteHistories;

public DeleteHistories(){
this.deleteHistories = new ArrayList<>();
}

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

private void addDeleteHistory(DeleteHistory deleteHistory){
this.deleteHistories.add(deleteHistory);
}

public void addQuestion(Question question){
DeleteHistory deleteHistory = new DeleteHistory(ContentType.QUESTION, question.getId(), question.getWriter(), LocalDateTime.now());
this.addDeleteHistory(deleteHistory);
}

public void addAnswer(Answer answer){
DeleteHistory deleteHistory = new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now());
this.addDeleteHistory(deleteHistory);
}

public void addAnswers(Answers answers){
for(Answer answer : answers.getAll()){
this.addAnswer(answer);
}
}
Comment on lines +18 to +36

Choose a reason for hiding this comment

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

가변 List에 question, answers에 따라 삭제 이력을 더해주는 형태로 구성되어 있네요. 코드 안에서 DeleteHistories 인스턴스를 사용하다 보면 Question, Answers가 어디까지 저장된 상태인지 알기 어려워 보여요. 정적 팩터리 등을 통해 명확하게 Answers, Question 삭제 이력에 대한 DeleteHistories를 분리해서 생성하는 방법도 고려해볼 수 있을까요?


public List<DeleteHistory> getAll(){
return this.deleteHistories;
}
}
5 changes: 3 additions & 2 deletions src/main/java/nextstep/qna/domain/Question.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public class Question {

private NsUser writer;

private List<Answer> answers = new ArrayList<>();
private Answers answers = new Answers();

private boolean deleted = false;

Expand Down Expand Up @@ -81,10 +81,11 @@ public boolean isDeleted() {
return deleted;
}

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


@Override
public String toString() {
return "Question [id=" + getId() + ", title=" + title + ", contents=" + contents + ", writer=" + writer + "]";
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/nextstep/qna/service/DeleteHistoryService.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package nextstep.qna.service;

import nextstep.qna.domain.DeleteHistories;
import nextstep.qna.domain.DeleteHistory;
import nextstep.qna.domain.DeleteHistoryRepository;
import org.springframework.stereotype.Service;
Expand All @@ -18,4 +19,9 @@ public class DeleteHistoryService {
public void saveAll(List<DeleteHistory> deleteHistories) {
deleteHistoryRepository.saveAll(deleteHistories);
}

@Transactional(propagation = Propagation.REQUIRES_NEW)
public void saveAll(DeleteHistories deleteHistories) {
deleteHistoryRepository.saveAll(deleteHistories.getAll());
}
}
19 changes: 8 additions & 11 deletions src/main/java/nextstep/qna/service/QnAService.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,17 @@ public void deleteQuestion(NsUser loginUser, long questionId) throws CannotDelet
throw new CannotDeleteException("질문을 삭제할 권한이 없습니다.");
}

Choose a reason for hiding this comment

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

질문 삭제 권한과 관련된 부분도 Question 도메인으로 옮겨주시면 좋겠습니다.


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

Comment on lines +34 to 37

Choose a reason for hiding this comment

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

QnAService에 삭제 예외 처리 로직이 있네요. 이 부분을 도메인(Answers)로 옮겨보면 좋겠습니다.

List<DeleteHistory> deleteHistories = new ArrayList<>();
DeleteHistories deleteHistories = new DeleteHistories();
question.setDeleted(true);

Choose a reason for hiding this comment

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

false로 설정될 일이 없을 것 같은데 delete라는 동사형 이름으로 짓고 파라미터를 받지 않는건 어떨까요?

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()));
}
deleteHistories.addQuestion(question);
answers.setAllDeleted(true);

Choose a reason for hiding this comment

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

항상 true가 넘어갈 것 같은데 외부에서 프로퍼티를 받을 필요가 있을까요?

deleteHistories.addAnswers(answers);

deleteHistoryService.saveAll(deleteHistories);

Choose a reason for hiding this comment

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

QnAService는 질문과 답변에 대한 처리를 하고 있습니다. 생성, 수정 등에는 DeleteHistoryService가 사용되지 않음에도 의존을 하고 있는 응집도가 낮은 상태라고 할 수 있겠네요.

이 부분을 이벤트 리스너 등을 통해 QnAService와 DeleteHistoryService간의 결합을 끊어볼 수 있을까요?

}
}
15 changes: 15 additions & 0 deletions src/test/java/nextstep/qna/domain/AnswerTest.java
Original file line number Diff line number Diff line change
@@ -1,8 +1,23 @@
package nextstep.qna.domain;

import nextstep.users.domain.NsUser;
import nextstep.users.domain.NsUserTest;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

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

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



@Test
@DisplayName("삭제 상태 질문 테스트")
public void testDeletedQuestionStatus(){
A1.setDeleted(true);
assertThat(A1.isDeleted()).isTrue();
}

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

import nextstep.users.domain.NsUserTest;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

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

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

@Test
@DisplayName("로그인한 사용자의 질문 테스트")
public void testIsLoginUserQuestion(){
boolean isLoginUser = Q1.isOwner(NsUserTest.JAVAJIGI);
assertThat(isLoginUser).isTrue();
}

@Test
@DisplayName("로그인한 사용자가 아닌 질문 테스트")
public void testIsNotLoginUserQuestion(){
boolean isLoginUser = Q1.isOwner(NsUserTest.SANJIGI);
assertThat(isLoginUser).isFalse();
}

@Test
@DisplayName("미삭제 상태 질문 테스트")
public void testIsNotDeleted(){
boolean isDeleted = Q1.isDeleted();
assertThat(isDeleted).isFalse();
}

@Test
@DisplayName("삭제 상태 질문 테스트")
public void testIsDeleted(){
Q1.setDeleted(true);
boolean isDeleted = Q1.isDeleted();
assertThat(isDeleted).isTrue();
}
}
7 changes: 5 additions & 2 deletions src/test/java/nextstep/qna/service/QnaServiceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.refEq;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

Expand Down Expand Up @@ -82,9 +83,11 @@ public void setUp() throws Exception {
}

private void verifyDeleteHistories() {
List<DeleteHistory> deleteHistories = Arrays.asList(
List<DeleteHistory> deleteHistorieList = Arrays.asList(
new DeleteHistory(ContentType.QUESTION, question.getId(), question.getWriter(), LocalDateTime.now()),
new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now()));
verify(deleteHistoryService).saveAll(deleteHistories);

DeleteHistories deleteHistories = new DeleteHistories(deleteHistorieList);
verify(deleteHistoryService).saveAll(refEq(deleteHistories));
}
}