[step1] 질문 삭제하기 기능 리팩토링, 제출합니다. #446

loop-study · 2021-03-31T09:41:28Z

안녕하세요 박현철이라고 합니다.
이번에 리팩토링 과제를 하면서
JPA 기반(아직 사용한적 없습니다. 이론공부만...)에 데이터베이스까지 연계되어 있다보니
이전과는 너무 다른 환경에 접근하기 어려웠네요.
정리하면서 진행하긴 했지만 제대로 했는지 잘 모르겠습니다.
진행하면서 궁금한점 질문드리겠습니다. 의견 부탁드립니다.🧐🧐🧐

기존에 List 사용되는 경우 모두 일급 객체로 리팩토링 했습니다. 하지만 중간에 @entity 요소가 존재하던데, 이 경우에 @Embeeded 사용하는게 맞는지 궁금합니다.
레거시코드 QnAService 에서 각 객체별 isOwner 로 삭제가 가능한지 예외처리 하고있습니다. 여기서 2가지 방향으로 생각했습니다.
- 첫째, 예외처리도 Question 객체에게 넘긴다.
- 둘쨰, 삭제 관련된 서비스 로직이다보니, 그냥 서비스로직에 두자
  ~~후자가 더 괜찮다 생각되어 진행했는데, 옳게했는지 궁금합니다.~~
  삭제 가능 여부도 Question 에게 묻는게 더 나아보여 수정했습니다.
DeleteHistory 를 add 하는 방법에 대해서도 2가지로 생각해봤습니다
- 첫째, DeleteHistories 에게 Question, Answers 를 넘겨 삭제객체가 인자를 받아 내부에서 추가하게 한다.
- 둘째, 기존 DeleteHistory.add() 를 위해 객체에게 Question.questionToDeleteHistory 를 추가해서 삭제이력을 보내달라 한다.
  첫째일 경우, 질문,답변 객체는 삭제이력에 대한걸 몰라도 되니 괜찮아보이기도 합니다. 하지만 삭제객체는 둘 다 알아야한다는 단점이 있습니다.
  둘째일 경우, 각 객체는 삭제이력을 알아야하니 객체간 결합도가 올라간다 생각됩니다. 대신 삭제객체는 add 하나만으로 사용법이 끝나니 단순해질거 같습니다.
  ~~고민을 하다가 첫번째 방법을 했습니다~~ 생활체조 9 원칙을 생각하여 두번째 방법으로 수정했습니다. 생활체조 원칙이 없다는 가정하에 결합도와 응집도만 생각하면 어느 방법이 더 괜찮을지 의견이 궁금합니다.

아직 객체지향 설계예 미흡한 점이 있다보니
많은 피드백 부탁드리겠습니다!📖

2021. 03. 31. 요구사항 및 구현사항 내역 정리 Resolves : loop-study See also : wheejuni

2021. 03. 31. - 작성자 확인 테스트 코드 추가 - 작성자 null 에러 테스트 코드 추가 - 게시글 null 에러 테스트 코드 추가 Resolves : loop-study See also : wheejuni

2021. 03. 31. - 삭제여부 변경 테스트 추가 Resolves : loop-study See also : wheejuni

2021. 03. 31. - 댓글 -> 답변으로 변경 Resolves : loop-study See also : wheejuni

2021. 03. 31. - 아이디 변경 테스트 추가 - 비밀번호 변경 테스트 추가 - 이름 변경 테스트 추가 - 이메일 변경 테스트 추가 - 유저 업데이트 테스트 추가 Resolves : loop-study See also : wheejuni

2021. 03. 31. - 아이디 테스트 추가 - 패스워드 테스트 추가 Resolves : loop-study See also : wheejuni

2021. 03. 31. - 제목 변경 테스트 추가 - 삭제 여부 변경 테스트 추가 Resolves : loop-study See also : wheejuni

2021. 03. 31. AnswresTest 생성 Resolves : loop-study See also : wheejuni

2021. 03. 31. - 답변 작성자 존재 테스트 - 답변 작성자 미존재 테스트 Resolves : loop-study See also : wheejuni

2021. 03. 31. - isOwner 추가 Resolves : loop-study See also : wheejuni

2021. 03. 31. - anyMatch -> allMatch 변경 Resolves : loop-study See also : wheejuni

2021. 03. 31. - true -> false 로 변경 Resolves : loop-study See also : wheejuni

2021. 03. 31. - DeleteHistories 추가 - Question 의 List<Answer> -> Answers 로 변경 - Question delete 메소드 추가 Resolves : loop-study See also : wheejuni

2021. 03. 31.동 삭제 가능 여부 로직 QnAService -> Question 으로 이동 Resolves : loop-study See also : wheejuni

2021. 03. 31. 코드 개선 Resolves : loop-study See also : wheejuni

2021. 03. 31. 각 객체에게 삭제이력 반환하는 방식으로 변경 Resolves : loop-study See also : wheejuni

2021. 03. 31. 미사용 getAnswers() 삭제 Resolves : loop-study See also : wheejuni

wheejuni

우선 지금 과제의 요구사항과 과제 진행 환경을 기반으로 생각해봤을 때, 완전히 구동 가능한 JPA 기반 앱을 만드는 것은 무리가 있습니다.
@Embeddable, @Embedded 와 같은 엔티티 구성 애노테이션과 JPA 관계 매핑 등의 주제는 추후에 다른 기회를 통해 조금 더 심화 학습하시기를 권합니다.

지금 @Embeddable 을 사용하신 것처럼 관계 매핑을 갖고 있는 클래스에도 @Embeddable 을 사용할 수 있기는 하나 개인적으론 선호하는 방법은 아닙니다. 사실 JPA 기반으로 데이터 매핑 구조를 설계하면서 일급 컬렉션을 사용할 것을 요구받는 상황 자체를 제가 그리 흔히 접해보진 못하기는 했습니다.

따라서 지금 과제에서는 자바 클래스의 설계 및 객체지향 시각으로 객체에 접근하는 방법을 연습하는 쪽으로 집중하시면 어떨까 합니다.

지금 단순히 삭제 가능 여부 를 묻는 메소드를 Question 클래스에 둔 것은 매우 어색해보여 개선 의견 드렸습니다.
DeleteHistories 는 모든 DeleteHistory 를 생성 시점에 받도록 하고, 추후 .add() 하는 방향은 최소화하도록 개선 시도해보시면 어떨까 합니다.

피드백 확인하시고, 추가 질문 남겨주시거나 반영해 주시면 감사하겠습니다. 추가 질문은 DM보다는 이 PR에서 공개 댓글로 진행해주시면 정말 감사하겠습니다.

wheejuni · 2021-03-31T15:03:18Z

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

+    public void allDelete(boolean isDelete) {
+        answers.forEach(answer -> answer.setDeleted(isDelete));
+    }
+
+    public List<DeleteHistory> answersToDeleteHistories() {
+        return answers.stream()
+                .map(answer -> new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now()))
+                .collect(Collectors.toList());
+    }


이 두 메소드는 하나로 합병할 수 있을 것 같습니다. 또 그렇게 하는 것이 단위테스트 용이성에 영향을 주지 않습니다.
하나로 합칠 수 있는 방법을 고민해 주세요.

삭제이력을 변환할 때 같이해도 되겠네요.👀

wheejuni · 2021-03-31T15:04:33Z

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

+    public void add(DeleteHistory deleteHistory) {
+        deleteHistories.add(deleteHistory);
+    }
+
+    public void add(List<DeleteHistory> deleteHistories) {
+        this.deleteHistories.addAll(deleteHistories);
+    }


.add() 가 꼭 필요할까요? .add() 없이도 로직의 구현이 가능해야 진짜 일급 컬렉션 아닐까요?
이 두 메소드를 걷어낼 수 있는지 살펴봐 주시고, 그렇게 할 수 있다면 그렇게 진행해 주시기 바랍니다.
아울러 이 클래스에는 스태틱 팩토리 메서드가 필요합니다. 하나 구현해주세요.

일급 객체는 불변인데, 내가 왜 그랬을까... 반성합니다..🙈

wheejuni · 2021-03-31T15:06:21Z

src/main/java/qna/domain/Question.java

+    public void canDelete(User loginUser) throws CannotDeleteException {
+        if (!isOwner(loginUser)) {
+            throw new CannotDeleteException(QUESTION_NOT_OWNER);
+        }
+
+        if (!answers.isAllOwner(loginUser)) {
+            throw new CannotDeleteException(ANSWER_NOT_OWNER);
+        }
+    }


Question 이 자신에게 딸린 Answer 를 전적으로 관리하며 삭제하는 책임도 전담한다면, 이 메소드가 public 으로 오픈되어야 할 이유가 없을 것 같습니다.

Answer 를 지우는 액션을 위한 엔트리 포인트를 이 클래스로 단일화시킬 수 있는 방법에 대해 한번 구상해 주시기 바랍니다.

canDelete 가 많이 어색하네요.
삭제이력을 만들 때 확인하는게 더 맞을거 같네요.

wheejuni · 2021-03-31T15:07:04Z

src/main/java/qna/service/QnAService.java

+        DeleteHistories deleteHistories = new DeleteHistories();
+        deleteHistories.add(question.questionToDeleteHistory());
+        deleteHistories.add(question.answersToDeleteHistories());


DeleteHistories 의 명시적 생성자 호출 없이 이 로직이 모두 수행되어야 할 것 같습니다.

2021. 04. 01. - Answers isAllDelete 삭제, answersToDeleteHistories 에서 delete 상태 변경. Resolves : loop-study See also : wheejuni

2021. 04. 01. - DeleteHistories -> 정적팩토리메소드로 변경 - Question -> canDelete 삭제, 각 객체의 toDeleteHistory 에서 삭제가능 판단 - this.delete = true -> 상수로 변경 Resolves : loop-study See also : wheejuni

2021. 04. 01. 불필요한 throw CannotDeleteException 제거 Resolves : loop-study See also : wheejuni

loop-study · 2021-04-01T12:44:16Z

안녕하세요 박현철입니다.
리뷰어님의 소중한 피드백으로 실수와 잘못된 점 깨닫고 수정했습니다.😂

Question 의 canDelete 에서 Question, Answer 삭제 가능 여부를 체크하는걸 지우고 각 객체에서 deleteHistory 를 할때 확인하도록 변경했습니다.
isAllDelete 을 지우고 deleteHistory 에서 this.delete = true(상수) 하도록 변경했습니다.
DeleteHistories 의 생성자와 add() 메소드를 지우고 정적팩토리메소드 방식으로 변경했습니다.

확인 부탁드리겠습니다
감사합니다👍

wheejuni

1단계를 단순한 몸풀기 과제로 생각하지 않고, 최선을 다해주시는 모습 보기 좋았습니다. 💯
이제 1단계는 이 정도로 충분한 것 같고요, merge 하고 다음 단계로 넘어갔으면 좋겠습니다 🥇

loop-study · 2021-04-01T13:30:34Z

1단계를 단순한 몸풀기 과제로 생각하지 않고, 최선을 다해주시는 모습 보기 좋았습니다. 💯
이제 1단계는 이 정도로 충분한 것 같고요, merge 하고 다음 단계로 넘어갔으면 좋겠습니다 🥇

이번 과제가 제일 중요하다 생각되서 평소보다 깊게 생각하다보니 실수가 잦았네요.
JPA 기반에 디비까지 연결된 레거시코드를 리팩토링하는건 이 과제가 유일하다보니
접근 방식에 따라 다양한 해석이 나올거 같습니다.
이건 토비님도 꼭 해보라고 하는 과제라서 여러번 더 복습할 거 같네요.

loop-study added 19 commits March 31, 2021 16:02

docs : 1단계 정리

f2c4801

2021. 03. 31. 요구사항 및 구현사항 내역 정리 Resolves : loop-study See also : wheejuni

test : Answer 테스트 추가

e9b1a8d

2021. 03. 31. - 작성자 확인 테스트 코드 추가 - 작성자 null 에러 테스트 코드 추가 - 게시글 null 에러 테스트 코드 추가 Resolves : loop-study See also : wheejuni

test : Answer 테스트 추가

de5792e

2021. 03. 31. - 삭제여부 변경 테스트 추가 Resolves : loop-study See also : wheejuni

test : Answer @DisplayName 변경

6c18019

2021. 03. 31. - 댓글 -> 답변으로 변경 Resolves : loop-study See also : wheejuni

test : User 테스트 추가

d4c76e8

2021. 03. 31. - 아이디 변경 테스트 추가 - 비밀번호 변경 테스트 추가 - 이름 변경 테스트 추가 - 이메일 변경 테스트 추가 - 유저 업데이트 테스트 추가 Resolves : loop-study See also : wheejuni

test : User 테스트 추가

e926c94

2021. 03. 31. - 아이디 테스트 추가 - 패스워드 테스트 추가 Resolves : loop-study See also : wheejuni

test : User 테스트 추가

55a19c1

2021. 03. 31. - 아이디 테스트 추가 - 패스워드 테스트 추가 Resolves : loop-study See also : wheejuni

test : Question 테스트 추가

e807619

2021. 03. 31. - 제목 변경 테스트 추가 - 삭제 여부 변경 테스트 추가 Resolves : loop-study See also : wheejuni

test : Answers 테스트 추가

a320d07

2021. 03. 31. AnswresTest 생성 Resolves : loop-study See also : wheejuni

test : Answers 테스트 추가

21fb9ed

2021. 03. 31. AnswresTest 생성 Resolves : loop-study See also : wheejuni

test : Answers 테스트 추가

fa6fe6a

2021. 03. 31. - 답변 작성자 존재 테스트 - 답변 작성자 미존재 테스트 Resolves : loop-study See also : wheejuni

feat : Answers 기능

a672108

2021. 03. 31. - isOwner 추가 Resolves : loop-study See also : wheejuni

refector : Answers 기능 변경

3921e82

2021. 03. 31. - anyMatch -> allMatch 변경 Resolves : loop-study See also : wheejuni

test : 오타 수정

fe1e80a

2021. 03. 31. - true -> false 로 변경 Resolves : loop-study See also : wheejuni

feat : QnA 리팩토링

55d11cc

2021. 03. 31. - DeleteHistories 추가 - Question 의 List<Answer> -> Answers 로 변경 - Question delete 메소드 추가 Resolves : loop-study See also : wheejuni

refector : 삭제 가능 책임 이동

f16be55

2021. 03. 31.동 삭제 가능 여부 로직 QnAService -> Question 으로 이동 Resolves : loop-study See also : wheejuni

refector : QnAService 개선

82dff30

2021. 03. 31. 코드 개선 Resolves : loop-study See also : wheejuni

refector : 삭제이력 추가 개선

5800836

2021. 03. 31. 각 객체에게 삭제이력 반환하는 방식으로 변경 Resolves : loop-study See also : wheejuni

refector : 미사용 메소드 삭제

1f0d8a0

2021. 03. 31. 미사용 getAnswers() 삭제 Resolves : loop-study See also : wheejuni

wheejuni requested changes Mar 31, 2021

View reviewed changes

loop-study added 3 commits April 1, 2021 11:54

refector : 피드백 반영

4d06194

2021. 04. 01. - Answers isAllDelete 삭제, answersToDeleteHistories 에서 delete 상태 변경. Resolves : loop-study See also : wheejuni

refector : 피드백 반영 및 수정

9e5f3ae

2021. 04. 01. - DeleteHistories -> 정적팩토리메소드로 변경 - Question -> canDelete 삭제, 각 객체의 toDeleteHistory 에서 삭제가능 판단 - this.delete = true -> 상수로 변경 Resolves : loop-study See also : wheejuni

refector : 코드 삭제

4aaf299

2021. 04. 01. 불필요한 throw CannotDeleteException 제거 Resolves : loop-study See also : wheejuni

wheejuni approved these changes Apr 1, 2021

View reviewed changes

wheejuni merged commit 542b9c3 into next-step:loop-study Apr 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[step1] 질문 삭제하기 기능 리팩토링, 제출합니다. #446

[step1] 질문 삭제하기 기능 리팩토링, 제출합니다. #446

loop-study commented Mar 31, 2021 •

edited

wheejuni left a comment

wheejuni Mar 31, 2021

loop-study Apr 1, 2021

wheejuni Mar 31, 2021

loop-study Apr 1, 2021

wheejuni Mar 31, 2021

loop-study Apr 1, 2021

wheejuni Mar 31, 2021

loop-study commented Apr 1, 2021 •

edited

wheejuni left a comment

loop-study commented Apr 1, 2021

[step1] 질문 삭제하기 기능 리팩토링, 제출합니다. #446

[step1] 질문 삭제하기 기능 리팩토링, 제출합니다. #446

Conversation

loop-study commented Mar 31, 2021 • edited

wheejuni left a comment

Choose a reason for hiding this comment

wheejuni Mar 31, 2021

Choose a reason for hiding this comment

loop-study Apr 1, 2021

Choose a reason for hiding this comment

wheejuni Mar 31, 2021

Choose a reason for hiding this comment

loop-study Apr 1, 2021

Choose a reason for hiding this comment

wheejuni Mar 31, 2021

Choose a reason for hiding this comment

loop-study Apr 1, 2021

Choose a reason for hiding this comment

wheejuni Mar 31, 2021

Choose a reason for hiding this comment

loop-study commented Apr 1, 2021 • edited

wheejuni left a comment

Choose a reason for hiding this comment

loop-study commented Apr 1, 2021

loop-study commented Mar 31, 2021 •

edited

loop-study commented Apr 1, 2021 •

edited