Step1 : 레거시 코드 리팩터링 (#795)

* refactor : List<Answer> 부분 일급 컬렉션으로 Answers 전환 및 비즈니스 로직 이관

* doc : 리팩터링 절차 및 계획안 마련

* refactor
1. DeleteHistories 로 일급컬렉션 뽑아내고, 관련 로직 도메인으로 이관
2. answer 제거 및 history 저장관련 내용 메서드 분리

* refactor
1. 메서드명 수정하여 그 의미 명확화
2. DeleteHistories 생성자 추가하여 생성 역할 다양화
3. deleteQuestion 메서드 리팩터링
4. Question의 일부 비즈니스 로직 도메인 내로 이관

* refactor
1. 불필요한 setter 제거

* refactor : 기존적인 생성, 수정일시 관련 공통된 인스턴스 필드 분리

* docs : 리팩토링 상황 메모

* refactor : 규칙 7 인스턴스 변수 줄이기

* refactor : 컨벤션 및 import 정리

* refactor : 테스트 픽스쳐 청리

* refactor : 깨진 코드 재 커밋

* refactor : 깨진 코드 재 커밋2

* refactor : 깨진 코드 해결용 커밋

* refactor
1. 도메인 규칙은 해당하는 도메인 내부에 캡슐화 시켜서 도메인 불변식을 유지한다. 그 결과 해당 도메인을 모르는 제3의 서비스에서 삭제를 할 때 삭제 규칙을 누락하는 경우를 방지한다.
2. `규칙7 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않기` 를 무리하게 이용된 부분 제거

* doc : 리팩터링 진행 상황 메모

* refactor : deleteAll, delete 메서드의 빈 리스트, 클래스 반환하여 하는 방법이 아닌, 예외 전파방식으로 무의미한 DeleteHistories 생성 방지

* refactor : 무의미한 방어코드를 제거(도메인 내부는 무조건 true / 실익은 영속성 레이어, 트랜잭셔널 범위에서 검증에서)

* refactor :
1. Answer 을 수동적으로 바라보아서 Answer에 대한 검증을 Answers 에서 하게한 것을 Answer로 이관
2. 불필요한 메서드 및 테스트 코드 제거

* doc : 이유 작성

---------

Co-authored-by: 모재영 <mo@utransfer.com>

Author: Mo-bile

README.md

@@ -6,4 +6,65 @@
 * 모든 피드백을 완료하면 다음 단계를 도전하고 앞의 과정을 반복한다.
 
 ## 온라인 코드 리뷰 과정
-* [텍스트와 이미지로 살펴보는 온라인 코드 리뷰 과정](https://github.com/next-step/nextstep-docs/tree/master/codereview)
+* [텍스트와 이미지로 살펴보는 온라인 코드 리뷰 과정](https://github.com/next-step/nextstep-docs/tree/master/codereview)
+
+
+# 1단계 : 레거시 코드 리팩터링
+## 리팩터링 요구사항
+- nextstep.qna.service.QnaService의 `deleteQuestion()`는 앞의 질문 삭제 기능을 구현한 코드이다. 
+  - 이 메소드는 단위 테스트하기 어려운 코드와 단위 테스트 가능한 코드가 섞여 있다.
+- QnaService의 `deleteQuestion()` 메서드에 단위 테스트 가능한 코드(핵심 비지니스 로직)를 도메인 모델 객체에 구현한다.
+- QnaService의 비지니스 로직을 도메인 모델로 이동하는 리팩터링을 진행할 때 TDD로 구현한다.
+- QnaService의 `deleteQuestion()` 메서드에 대한 단위 테스트는 src/test/java 폴더 `nextstep.qna.service.QnaServiceTest`이다. 
+  - 도메인 모델로 로직을 이동한 후에도 QnaServiceTest의 모든 테스트는 통과해야 한다.
+
+## 리팩터링 절차
+1. 테스트하기 쉬운 부분(비즈니스 로직)과 어려운 부분(날짜, 셔플, 랜덤, DB, API call)을 구분해보기
+2. 분리 시 먼저 단위테스트 추가
+3. 관련 비즈니스 로직을 도메인 객체로 이동
+
+
+## 리팩터링 계획
+1. `List<Answer>` 의 일급컬렉션으로 래핑 및 주요 로직 이동
+   - [x] : 이동 및 테스트 완료
+2. `List<DeleteHistory>` 의 일급컬렉션으로 래핑 및 주요 로직 이동
+    - [x] : 이동 및 테스트 완료
+3. `Question` 의 주요 로직 이동
+    - [x] : 이동 및 테스트 완료
+4. 불필요한 setter 제거
+5. 인스턴스 변수 수 줄이기
+
+## 1차 피드백
+
+- [x] : 분리된 글쓴이 판단 -> 삭제 상태로 변경 한번에
+    - [x] : 이 방식이 좋은 이유?
+        - 반대의견 : `deleteQuestion()` 은 삭제가 행동이지만, 삭제의 큰 흐름은 알아야한다
+            - 권한이 있는지, 연관 `Answers` 까지 아는지? 그리고 너무 과하게 코드를 줄인다.
+        - 찬성의견 : 해당 도메인 내부에 있는 값을 굳이 노출시키거나 꺼내서 해결할 필요는 없다
+            - `deleteQuestion()`의 관심사는 말 그대로 질문 삭제일 뿐이다
+                - 얘는 삭제 권한이있는지, 답변이 모드 같은 사람이 쓴건지 이런것은 관심이 없다.
+                - 그냥 삭제만 하고 싶어할 뿐이다.
+    - 정리한 찬성 의견
+        1. 도메인 불변식 유지 : 모델이 깨지면 안되는 규칙 -> 어떤 경로이든 참이어야 함
+            - 규칙검증을 서비스에 두면 제3의 다른서비스는 이 규칙 검증을 건너뛸 수있게 됨
+            - 규칙이 도메인 밖으로 새면 안됨
+        2. 서비스 레이어 메서드는 외부(DB, I/O, 셔플)와 `@Transactional` 가 관심사임
+            - 비즈니스 규칙 자체는 도메인 내부에서 해결해야함
+- [x] : 연관된 답변까지 Question 에서 한꺼번에 삭제
+- [x] : 불필요하게 분리된 객체의 필요성 고민
+    - 고민 결과 도메인만의 규칙, 행동이 없으므로 롤백
+
+## 2차 피드백
+
+- [x] : 불필요한 setter 와 getter의 무의미한 재귀 호출 제거 (재귀호출 stack over flow 위험)
+- [x] : 삭제 시 삭제됐는지 확인하는 메서드 필요한지? 필요없다면 이유 작성
+    - 이유
+        - this.isQuestionDeleteDone(); 같은 도메인 내부에서 값의 변화를 검증하는 부분인데,
+            1. 도메인 내부 값 변화는 확실한 점
+            2. 그래서 도메인 내부에서 값 변화를 체크하는 방어코드는 무의미한 점
+            3. 만약 검증하려면 db(영속성) 레이어 혹은 트랜잭셔널 범위(서비스 레이어) 등에서 검증해야하지만 금번 과제는 리팩터링 이므로 추가 할 필요가 없는점
+        - 이므로 제거
+- [x] : `Answer` 에 대한 삭제 테스트 필요하자않은가? 이유작성
+    - 이유 : 필요하다 Answer 을 수동적으로 바라보았기에 비즈니스 로직에 대한 테스트 코드가 생략되었다.
+- [ ] : `Answers` 에서 한땀한땀 삭제하는건 `Answer` 에 대한 수동적으로 바라보는 느낌아닌가? 이유 작성
+    - 이유 : 맞다 또한 검증에 대해서도 모두 수행하는것은 바람직하지 않음, 검증은 `Answer` 내부에서 해야함

src/main/java/nextstep/qna/domain/Answer.java

@@ -1,26 +1,21 @@
 package nextstep.qna.domain;
 
+import nextstep.qna.CannotDeleteException;
 import nextstep.qna.NotFoundException;
 import nextstep.qna.UnAuthorizedException;
 import nextstep.users.domain.NsUser;
 
-import java.time.LocalDateTime;
-
-public class Answer {
+public class Answer extends Base {
     private Long id;
-
+    
     private NsUser writer;
-
+    
     private Question question;
-
+    
     private String contents;
-
+    
     private boolean deleted = false;
 
-    private LocalDateTime createdDate = LocalDateTime.now();
-
-    private LocalDateTime updatedDate;
-
     public Answer() {
     }
 
@@ -37,7 +32,6 @@ public Answer(Long id, NsUser writer, Question question, String contents) {
         if(question == null) {
             throw new NotFoundException();
         }
-
         this.writer = writer;
         this.question = question;
         this.contents = contents;
@@ -46,34 +40,50 @@ public Answer(Long id, NsUser writer, Question question, String contents) {
     public Long getId() {
         return id;
     }
-
-    public Answer setDeleted(boolean deleted) {
-        this.deleted = deleted;
-        return this;
+    
+    public void delete(NsUser loginUser) throws CannotDeleteException {
+        isHaveAuthority(loginUser);
+        deleteAnswer();
+    }
+    
+    public void deleteAnswer() {
+        this.deleted = true;
     }
 
     public boolean isDeleted() {
         return deleted;
     }
-
-    public boolean isOwner(NsUser writer) {
-        return this.writer.equals(writer);
+    
+    private void isHaveAuthority(NsUser loginUser) throws CannotDeleteException {
+        if(isNotOwner(loginUser)) {
+            throw new CannotDeleteException("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다.");
+        }
+    }
+    
+    public boolean isNotOwner(NsUser writer) {
+        return !this.writer.equals(writer);
     }
 
     public NsUser getWriter() {
-        return writer;
+        return this.writer;
     }
 
     public String getContents() {
-        return contents;
+        return this.contents;
     }
 
     public void toQuestion(Question question) {
         this.question = question;
     }
-
+    
     @Override
     public String toString() {
-        return "Answer [id=" + getId() + ", writer=" + writer + ", contents=" + contents + "]";
+        return "Answer{" +
+            "id=" + id +
+            ", writer=" + writer +
+            ", question=" + question +
+            ", contents='" + contents + '\'' +
+            ", deleted=" + deleted +
+            '}';
     }
 }

src/main/java/nextstep/qna/domain/Answers.java

@@ -0,0 +1,67 @@
+package nextstep.qna.domain;
+
+import java.time.LocalDateTime;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.stream.Collectors;
+import nextstep.qna.CannotDeleteException;
+import nextstep.users.domain.NsUser;
+
+public class Answers {
+    
+    private final List<Answer> answers;
+    
+    public Answers() {
+        this(List.of());
+    }
+    
+    public Answers(Answer... answers) {
+        this(Arrays.asList(answers));
+    }
+    
+    public Answers(List<Answer> answers) {
+        this.answers = new ArrayList<>(answers);
+    }
+    
+    public void addAnswer(Answer answer) {
+        this.answers.add(answer);
+    }
+    
+    public List<DeleteHistory> deleteAll(NsUser loginUser) throws CannotDeleteException {
+        deleteAllAnswers(loginUser);
+        return addInDeleteHistory();
+    }
+    
+    private void deleteAllAnswers(NsUser loginUser) throws CannotDeleteException {
+        for(Answer answer: answers) {
+            answer.delete(loginUser);
+        }
+    }
+    
+    public List<DeleteHistory> addInDeleteHistory() {
+        return this.answers.stream()
+            .map(answer -> new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now()))
+            .collect(Collectors.toList());
+    }
+    
+    private boolean isNotAllMatch(NsUser loginUser) {
+        return this.answers.stream()
+            .anyMatch(answer -> !answer.isNotOwner(loginUser));
+    }
+    
+    @Override
+    public boolean equals(Object o) {
+        if(o == null || getClass() != o.getClass()) {
+            return false;
+        }
+        Answers answers1 = (Answers) o;
+        return Objects.equals(answers, answers1.answers);
+    }
+    
+    @Override
+    public int hashCode() {
+        return Objects.hashCode(answers);
+    }
+}

src/main/java/nextstep/qna/domain/Base.java

@@ -0,0 +1,8 @@
+package nextstep.qna.domain;
+
+import java.time.LocalDateTime;
+
+public class Base {
+    protected LocalDateTime createdDate = LocalDateTime.now();
+    protected LocalDateTime updatedDate;
+}

src/main/java/nextstep/qna/domain/DeleteHistories.java

@@ -0,0 +1,40 @@
+package nextstep.qna.domain;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+public class DeleteHistories {
+    
+    private List<DeleteHistory> deleteHistories = new ArrayList<>();
+    
+    public DeleteHistories(DeleteHistory... deleteHistories) {
+        this(Arrays.asList(deleteHistories));
+    }
+    
+    public DeleteHistories(DeleteHistory deleteHistory, List<DeleteHistory> deleteHistories) {
+        this (Stream.concat(Stream.of(deleteHistory), deleteHistories.stream()).collect(Collectors.toList()));
+    }
+    
+    public DeleteHistories(List<DeleteHistory> deleteHistories) {
+        this.deleteHistories.addAll(deleteHistories);
+    }
+    
+    public void addHistories(DeleteHistories deleteHistories) {
+        addHistories(deleteHistories.deleteHistories);
+    }
+    
+    public void addHistories(List<DeleteHistory> deleteHistories) {
+        this.deleteHistories.addAll(deleteHistories);
+    }
+    
+    public void addHistories(DeleteHistory deleteHistories) {
+        this.deleteHistories.add(deleteHistories);
+    }
+    
+    public List<DeleteHistory> toList() {
+        return this.deleteHistories;
+    }
+}

src/main/java/nextstep/qna/domain/DeleteHistory.java

@@ -1,21 +1,18 @@
 package nextstep.qna.domain;
 
-import nextstep.users.domain.NsUser;
-
 import java.time.LocalDateTime;
 import java.util.Objects;
+import nextstep.users.domain.NsUser;
 
-public class DeleteHistory {
+public class DeleteHistory extends Base{
     private Long id;
-
+    
     private ContentType contentType;
-
+    
     private Long contentId;
-
+    
     private NsUser deletedBy;
 
-    private LocalDateTime createdDate = LocalDateTime.now();
-
     public DeleteHistory() {
     }
 
@@ -25,26 +22,28 @@ public DeleteHistory(ContentType contentType, Long contentId, NsUser deletedBy,
         this.deletedBy = deletedBy;
         this.createdDate = createdDate;
     }
-
+    
     @Override
     public boolean equals(Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
+        if(o == null || getClass() != o.getClass()) {
+            return false;
+        }
         DeleteHistory that = (DeleteHistory) o;
-        return Objects.equals(id, that.id) &&
-                contentType == that.contentType &&
-                Objects.equals(contentId, that.contentId) &&
-                Objects.equals(deletedBy, that.deletedBy);
+        return Objects.equals(id, that.id) && contentType == that.contentType && Objects.equals(contentId, that.contentId) && Objects.equals(deletedBy, that.deletedBy);
     }
-
+    
     @Override
     public int hashCode() {
         return Objects.hash(id, contentType, contentId, deletedBy);
     }
-
+    
     @Override
     public String toString() {
-        return "DeleteHistory [id=" + id + ", contentType=" + contentType + ", contentId=" + contentId + ", deletedBy="
-                + deletedBy + ", createdDate=" + createdDate + "]";
+        return "DeleteHistory{" +
+            "id=" + id +
+            ", contentType=" + contentType +
+            ", contentId=" + contentId +
+            ", deletedBy=" + deletedBy +
+            '}';
     }
 }