-
Notifications
You must be signed in to change notification settings - Fork 300
1단계 레거시 코드 리펙터링 #791
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
1단계 레거시 코드 리펙터링 #791
Changes from 6 commits
28f2534
dd20c19
358644a
83711f3
c939d7b
96a98e5
f128cc2
5713dc8
62487bb
33aa64c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| # 문자열 사칙 연산 계산기 구현 | ||
| ## 기능 요구사항 | ||
|
|
||
| ## 삭제 방식 변경 | ||
| - [x] 질문 데이터를 완전히 삭제하지 않는다(soft delete) | ||
| - [x] deleted 상태(boolean type)를 true로 변경한다 | ||
|
|
||
| ## 삭제 권한 검증 | ||
| - [x] 로그인 사용자와 질문 작성자가 같은 경우에만 삭제 가능하다 | ||
| - [x] 답변이 없는 경우 삭제가 가능하다 | ||
| - [x] 질문자와 모든 답변 작성자가 같은 경우 삭제가 가능하다 | ||
| - [x] 질문자와 답변자가 다른 경우 질문을 삭제할 수 없다 | ||
|
|
||
| ## 연관 데이터 처리 | ||
| - [x] 질문을 삭제할 때 관련된 모든 답변도 함께 삭제한다 | ||
| - [x] 답변의 삭제도 soft deled로 deleted 상태를 true로 변경한다 | ||
|
|
||
| ## 삭제 이력 관리 | ||
| - [x] 질문 삭제 시 DeleteHistory를 생성하여 이력을 남긴다 | ||
| - [x] 답변 삭제 시에도 DeleteHistory를 생성하여 이력을 남긴다 | ||
|
|
||
|
|
||
| ## 리팩터링 요구사항 | ||
| - [ ] deleteQuestion() 메서드의 비즈니스 로직을 도메인으로 이동한다. | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,6 +72,11 @@ public void toQuestion(Question question) { | |
| this.question = question; | ||
| } | ||
|
|
||
| public DeleteHistory delete(){ | ||
| this.deleted = true; | ||
| return new DeleteHistory(ContentType.ANSWER, id, writer, LocalDateTime.now()); | ||
| } | ||
|
Comment on lines
+66
to
+69
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 객체의 삭제 상태를 변경하는 책임과 DeleteHistory 생성하고 반환하는 책임 |
||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "Answer [id=" + getId() + ", writer=" + writer + ", contents=" + contents + "]"; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| package nextstep.qna.domain; | ||
|
|
||
| import nextstep.qna.CannotDeleteException; | ||
| import nextstep.users.domain.NsUser; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class Answers { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 일급 콜렉션 추가 👍 |
||
| private final List<Answer> answers; | ||
|
|
||
| public Answers() { | ||
| this(new ArrayList<>()); | ||
| } | ||
|
|
||
| public Answers(List<Answer> answers) { | ||
| this.answers = new ArrayList<>(answers); | ||
| } | ||
|
|
||
| public void add(Answer answer) { | ||
| answers.add(answer); | ||
| } | ||
|
|
||
| public void checkDeletable(NsUser loginUser) throws CannotDeleteException { | ||
| if (answers.stream().anyMatch(answer -> !answer.isOwner(loginUser))) { | ||
| throw new CannotDeleteException("다른 사람의 답변이 존재하여 삭제할 수 없습니다."); | ||
| } | ||
| } | ||
|
|
||
| public List<DeleteHistory> delete() { | ||
|
||
| List<DeleteHistory> histories = new ArrayList<>(); | ||
| for (Answer answer : answers) { | ||
| histories.add(answer.delete()); | ||
| } | ||
| return histories; | ||
| } | ||
|
|
||
| public List<Answer> getAnswers() { | ||
| return new ArrayList<>(answers); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,14 +2,15 @@ | |||||||
|
|
||||||||
| import nextstep.qna.CannotDeleteException; | ||||||||
| import nextstep.qna.NotFoundException; | ||||||||
| import nextstep.qna.domain.*; | ||||||||
| import nextstep.qna.domain.AnswerRepository; | ||||||||
| import nextstep.qna.domain.DeleteHistory; | ||||||||
| import nextstep.qna.domain.Question; | ||||||||
| import nextstep.qna.domain.QuestionRepository; | ||||||||
| import nextstep.users.domain.NsUser; | ||||||||
| import org.springframework.stereotype.Service; | ||||||||
| import org.springframework.transaction.annotation.Transactional; | ||||||||
|
|
||||||||
| import javax.annotation.Resource; | ||||||||
| import java.time.LocalDateTime; | ||||||||
| import java.util.ArrayList; | ||||||||
| import java.util.List; | ||||||||
|
|
||||||||
| @Service("qnaService") | ||||||||
|
|
@@ -26,24 +27,7 @@ public class QnAService { | |||||||
| @Transactional | ||||||||
| public void deleteQuestion(NsUser loginUser, long questionId) throws CannotDeleteException { | ||||||||
| Question question = questionRepository.findById(questionId).orElseThrow(NotFoundException::new); | ||||||||
| 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())); | ||||||||
| } | ||||||||
| List<DeleteHistory> deleteHistories = question.delete(loginUser); | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
이와 같이 삭제 메시지와 deleteHistories 생성을 분리하는 것과 현재와 같이 구현하는 것 중 어느 접근 방식이 좋을까?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 한번의 메서드 호출로 끝나서 간결해지지만 삭제로 상태도 변경하면서 히스토리도 반환하는 점이 좀 걸리네요 |
||||||||
| deleteHistoryService.saveAll(deleteHistories); | ||||||||
| } | ||||||||
| } | ||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| package nextstep.qna.domain; | ||
|
|
||
| import nextstep.qna.CannotDeleteException; | ||
| import nextstep.users.domain.NsUserTest; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| public class AnswersTest { | ||
|
|
||
| @Test | ||
| public void 모든_답변이_같은작성자면_삭제_가능() throws CannotDeleteException { | ||
| Question question = new Question(1L, NsUserTest.JAVAJIGI, "title", "contents"); | ||
| Answer answer1 = new Answer(11L, NsUserTest.JAVAJIGI, question, "answer1"); | ||
| Answer answer2 = new Answer(12L, NsUserTest.JAVAJIGI, question, "answer2"); | ||
| Answers answers = new Answers(Arrays.asList(answer1, answer2)); | ||
|
|
||
| answers.checkDeletable(NsUserTest.JAVAJIGI); | ||
| } | ||
|
|
||
| @Test | ||
| public void 다른_작성자의_답변이_있으면_예외() { | ||
| Question question = new Question(1L, NsUserTest.JAVAJIGI, "title", "contents"); | ||
| Answer answer1 = new Answer(11L, NsUserTest.JAVAJIGI, question, "answer1"); | ||
| Answer answer2 = new Answer(12L, NsUserTest.SANJIGI, question, "answer2"); | ||
| Answers answers = new Answers(Arrays.asList(answer1, answer2)); | ||
|
|
||
| assertThatThrownBy(() -> { | ||
| answers.checkDeletable(NsUserTest.JAVAJIGI); | ||
| }).isInstanceOf(CannotDeleteException.class) | ||
| .hasMessageContaining("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다."); | ||
| } | ||
|
|
||
| @Test | ||
| public void 모든_답변_삭제_및_DeleteHistory_반환() { | ||
| Question question = new Question(1L, NsUserTest.JAVAJIGI, "title", "contents"); | ||
| Answer answer1 = new Answer(11L, NsUserTest.JAVAJIGI, question, "answer1"); | ||
| Answer answer2 = new Answer(12L, NsUserTest.JAVAJIGI, question, "answer2"); | ||
| Answers answers = new Answers(Arrays.asList(answer1, answer2)); | ||
|
|
||
| List<DeleteHistory> deleteHistories = answers.delete(); | ||
|
|
||
| assertThat(answer1.isDeleted()).isTrue(); | ||
| assertThat(answer2.isDeleted()).isTrue(); | ||
| assertThat(deleteHistories).hasSize(2); | ||
| assertThat(deleteHistories.get(0).getContentType()).isEqualTo(ContentType.ANSWER); | ||
| assertThat(deleteHistories.get(1).getContentType()).isEqualTo(ContentType.ANSWER); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,59 @@ | ||
| package nextstep.qna.domain; | ||
|
|
||
| import nextstep.qna.CannotDeleteException; | ||
| import nextstep.users.domain.NsUserTest; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| 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 | ||
| public void 답변없는질문_삭제() throws CannotDeleteException { | ||
| List<DeleteHistory> deleteHistories = Q1.delete(NsUserTest.JAVAJIGI); | ||
|
|
||
| assertThat(Q1.isDeleted()).isTrue(); | ||
| assertThat(deleteHistories).hasSize(1); | ||
| assertThat(deleteHistories.get(0).getContentType()).isEqualTo(ContentType.QUESTION); | ||
| } | ||
|
|
||
| @Test | ||
| public void 질문자와_답변자가_같은경우_삭제() throws CannotDeleteException { | ||
| Answer answer1 = new Answer(11L, NsUserTest.JAVAJIGI, Q1, "answer1"); | ||
| Answer answer2 = new Answer(12L, NsUserTest.JAVAJIGI, Q1, "answer2"); | ||
| Q1.addAnswer(answer1); | ||
| Q1.addAnswer(answer2); | ||
|
|
||
| List<DeleteHistory> deleteHistories = Q1.delete(NsUserTest.JAVAJIGI); | ||
|
|
||
| assertThat(Q1.isDeleted()).isTrue(); | ||
| assertThat(answer1.isDeleted()).isTrue(); | ||
| assertThat(answer2.isDeleted()).isTrue(); | ||
| assertThat(deleteHistories).hasSize(3); | ||
| } | ||
|
|
||
| @Test | ||
| public void 다른사람의_답변이_있는경우_삭제() { | ||
| Question question = new Question(1L, NsUserTest.JAVAJIGI, "title", "contents"); | ||
| Answer answer = new Answer(11L, NsUserTest.SANJIGI, question, "answer"); | ||
| question.addAnswer(answer); | ||
|
|
||
| assertThatThrownBy(() -> { | ||
| question.delete(NsUserTest.JAVAJIGI); | ||
| }).isInstanceOf(CannotDeleteException.class) | ||
| .hasMessageContaining("다른 사람의 답변이 존재하여 삭제할 수 없습니다."); | ||
| } | ||
|
|
||
| @Test | ||
| public void 질문자가_아닌경우_삭제불가() { | ||
| assertThatThrownBy(() -> { | ||
| Q1.delete(NsUserTest.SANJIGI); | ||
| }).isInstanceOf(CannotDeleteException.class) | ||
| .hasMessageContaining("질문을 삭제할 권한이 없습니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 구현한 방식과 아래와 같이 글쓴이 인지 여부를 이 메서드가 판단하도록 구현하는 것 중 어느 방식으로 구현하는 것이 좋을지 고민해 본다.
위 두 가지 방식 중 선택한 방법으로 구현하는 것이 좋은 이유에 대해 피드백으로 남겨본다.