[Step3] 질문 삭제하기 리팩터링 #93

LenKIM · 2021-06-01T11:39:55Z

안녕하세요 : ) 벌써 3단계 질문 삭제하기 리팩터링이네요.

아래 TODO 리스트를 작성하고 최대한 맞쳐서 개발하고자 했습니다.

혹시라도 궁금하신 부분은 코멘트 달아주시면 최대한 빨리 답변드리겠습니다.

감사합니다 😌

slamdunk7575

리팩토링 잘해주셨네요 👍
몇가지 개선해보면 좋을 부분에 코멘트 남겨드렸는데 확인해보시면 좋겠어요.
끝까지 화이팅 입니다!

slamdunk7575 · 2021-06-02T00:52:13Z

src/main/java/qna/service/QnaService.java

        Question question = findQuestionById(questionId);
-        if (!question.isOwner(loginUser)) {
-            throw new CannotDeleteException("질문을 삭제할 권한이 없습니다.");
-        }
-
-        List<Answer> answers = answerRepository.findByQuestionIdAndDeletedFalse(questionId);
-        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.deleted(true);
-            deleteHistories.add(new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now()));
-        }
+        DeleteHistories deleteHistories = question.deletedBy(loginUser);
        deleteHistoryService.saveAll(deleteHistories);


깔끔하게 잘해주셨네요 👍

감사합니다.

slamdunk7575 · 2021-06-02T00:54:55Z

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

+    @OneToMany(mappedBy = "question",
+        fetch = FetchType.LAZY,
+        cascade = CascadeType.ALL,
+        orphanRemoval = true
+    )
+    private List<Answer> answers = new ArrayList<>();


정의한 Answers를 이용해보면 어떨까요?

@Embedded private Answers answers;

참고로 @OneToMany는 기본 타입이 LAZY라서 별도로 지정하지 않으셔도 됩니다 :)

slamdunk7575 · 2021-06-02T00:57:10Z

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

+            throw new CannotDeleteException("질문을 삭제할 권한이 없습니다.");
+        }
+        this.setDeleted(true);
+        DeleteHistory deleteHistory = new DeleteHistory(ContentType.QUESTION, getId(), user, LocalDateTime.now());


DeleteHistory 내에 아래와 같은 정적 팩토리 메소드를 만들어보면
인자수도 줄면서 코드의 가독성을 높여줄 수 있겠네요 :)

public static DeleteHistory ofQuestion(Long contentId, User deletedBy) { return new DeleteHistory(ContentType.QUESTION, contentId, deletedBy); } public static DeleteHistory ofAnswer(Long contentId, User deletedBy) { return new DeleteHistory(ContentType.ANSWER, contentId, deletedBy); }

습관적으로 정적팩토리 메소드를 사용하다 보니, 왜 사용해야되는가? 에 대한 목적을 고민했습니다. 그래서 이번에는 한번 사용해보지 않았습니다.
사용했던 목적을 고민해보면, 가독성이라는 게 가장 크고, 최근 로또 프로젝트에서 인스턴스 캐싱도 정적팩토리 메소드로 해결하면서, 정적 메소드에 대해서 조금더 깊은 고찰에 맞닥뜨리게 될 수 있었습니다. 주신 피드백을 통해서, 다시한번 정적 팩토리 메소드를 사용하는 이유를 집어주셨네요. 감사합니다 : )

slamdunk7575 · 2021-06-02T00:57:50Z

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

+        if (Objects.isNull(this.answers)) {
+            this.answers = new ArrayList<>();
+        }
+        Answers answers = new Answers(this.answers);


위의 피드백을 반영해본다면 제거할 수 있겠네요! #93 (comment)

어썸 x 123512 입니다!!

slamdunk7575 · 2021-06-02T01:03:29Z

src/main/java/qna/domain/question/Title.java

+@Embeddable
+public class Title {
+
+	@Column(length = 100, nullable = false)
+	private String value;
+
+	public Title(String value) {
+		this.value = value;
+	}


원시값 포장 좋습니다 👍

입력값 value에 대한 유효성 검사를 해보면 좋을것 같아요.
제가 피드백 드린 부분 외에도 필요하다면 적용해보시면 좋겠네요 😄

아마도 DB 의 제약조건과 객체의 제약조건은 다르기 때문에 따로 명시해주는 거겠죠?? 많은 원시값 포장을 일괄로 하다보니 미쳐 생각하지 못했네요; 코드 수정하겠습니다!

slamdunk7575 · 2021-06-02T01:05:02Z

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

+    @ManyToOne(fetch = FetchType.LAZY, cascade = CascadeType.ALL)
    @JoinColumn(name = "question_id",
        foreignKey = @ForeignKey(name = "fk_answer_to_question"))
    private Question question;


JPA를 배제하고 서비스의 흐름으로 봤을때
일반적으로 질문(Question)이 먼저 생성되고 답변(Answer)이 생성되는 순서가 될 것 같아요.

이부분에서 Answer가 Cascade를 통해 Question의 라이프 사이클을 관리하는 부분은 제거하는게 좋을듯 한데요.
정규님 생각은 어떠신가요? 😄

아래는 참고해보시면 좋을것 같아요.
[JPA] 프록시와 연관관계 관리

미쳐 생각해보지 못한 부분입니다. cascade 행위 자체를 서비스의 흐름으로 생각해보지 못했는데, 리뷰어님의 말씀을 들어보니 공감이 되네요. 해당 코드 수정하겠습니다.

그리고 링크 감사합니다.

slamdunk7575 · 2021-06-02T01:06:51Z

src/main/java/qna/domain/user/User.java

@@ -11,6 +12,7 @@
 import javax.persistence.UniqueConstraint;

 import qna.UnAuthorizedException;
+import qna.domain.BaseEntity;

 @Entity


사용하지 않는 코드(GUEST_USER)는 제거해주세요.

네, 사용하지 않는 코드이므로 제거하겠습니다!

slamdunk7575 · 2021-06-02T01:10:01Z

src/test/java/qna/domain/question/QuestionTest.java

+    @Test
+    void Owner가_아닌_사람이_질문을_삭제하면_예외를_출력() {
+        User notOwnerUser = UserTest.JAVAJIGI;
+
+        assertThat(question.isDeleted()).isFalse();
+        assertThatThrownBy(() -> question.deletedBy(notOwnerUser))
+            .isInstanceOf(CannotDeleteException.class)
+        .hasMessageContaining("질문을 삭제할 권한이 없습니다.");
+    }


단위 테스트하기 어려운 코드와 단위 테스트 가능한 코드를 분리해 단위 테스트 가능한 코드에 대해 단위 테스트를 구현한다.

테스트 하기 어려운 부분을 분리하여 리팩토링 했다면 다른 도메인에 대한 테스트도 해보면 좋겠네요!

도메인에 대한 테스트 코드를 더욱 추가하겠습니다.
리팩토링을 하는 과정에서는 테스트 주도 개발의 리듬을 따라가기가 쉽지 않네요.

테스트 코드 작성 > Fail > Pass > 리팩토링 이런식의 절차인데,

마지막 단계인 리팩토링에서 코드를 수정하다보니, 이 리듬을 금새 까먹어버리는군요 하핳

slamdunk7575

피드백 반영 잘해주셨네요 👍
깔끔하게 마무리하면 좋을것 같아 간단한 코멘트 몇개 남겨드렸습니다.
끝까지 화이팅 입니다!

slamdunk7575 · 2021-06-03T03:47:12Z

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

+    public static DeleteHistory ofQuestion(ContentId contentId, User deletedBy, LocalDateTime createDate){
+        return new DeleteHistory(ContentType.QUESTION, contentId, deletedBy, createDate);
+    }


테스트에만 사용되는 메소드는 제거하는게 좋겠네요.

네ㅎㅎ 덕분에 Caller shortcut 다시한번 상기시킬 수 있었습니다.
Ctr + Option + h 이네요.

또한 테스트에서 호출되는 코드는 프로덕트에 필요한 코드가 아니므로 제거대상이 맞다고 판단되요 ㅎㅎ 피드백 감사합니다.

slamdunk7575 · 2021-06-03T03:49:33Z

src/main/java/qna/domain/question/Title.java

+	public Title(String value) {
+		setValue(value);
+	}
+
+	protected void setValue(String value) {
+		if (Objects.isNull(value) || value.length() > 100){
+			throw new IllegalArgumentException("Invalid Title value");
+		}
+		this.value = value;
+	}
+
+	protected Title() { }


아래처럼 해보면 어떨까요?
기본 생성자는 가장 위에 배치하는게 좋겠네요 :)

protected Title() { } public Title(String value) { validateTitle(value); this.value = value; } private void validateTitle(String value) { if (Objects.isNull(value) || value.length() > TITLE_LIMIT_LENGTH) { throw new IllegalArgumentException("Invalid Title value"); } }

제가 습관적으로 하는 코딩 방식입니다. setter 의 목적을 생각하다보니, 위와 같은 코드를 작성하게 됐네요. protected 가 아니라 private 로 많이 하는데, 저 부분은 protected로 했네요;;

혹시 리뷰어님께서는 위와같이 작성하는 이유가 있을까요??
만약에 생성자 인자가 1개라면 어색하지 않습니다. 그러나, 만약 3개 인자 일경우, 코드의 가독성이 떨어질거라 판단되서요 : )

다시 생각해보니 이게, 그렇게 중요한것인가? 라는 생각도 드네요. 조금더 본질적인 질문으로, 혹시 경중님께서는 코드를 작성하실때 가장 중요하게 여기는 부분이 어디일까요? (ex, 가독성, 유지보수성 ... )

우선 피드백을 드렸던 의도는 메소드내에서 검증과 할당을 분리해보시면 좋을 것 같아 드렸던 거였습니다.
파라미터가 3개 이상 늘어나는 것은 피하는게 좋지만 만약 그렇게 된다면 정규님 말씀대로 검증 메소드가 늘어나 가독성이 떨어질거라 생각되네요. 검증하는 부분을 한곳으로 묶거나 아예 넘어오는 원시값을 포장하는 방향으로 생각을 이어가볼 수도 있겠네요 :)

정규님의 방식이 틀렸다라는 것이 결코 아니니 참고만 해주시면 좋겠습니다 🙇

[Clean Code] 3 : 함수

slamdunk7575 · 2021-06-03T03:53:14Z

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

+@Embeddable
+public class Answers {
+
+	@OneToMany(mappedBy = "question",
+		fetch = FetchType.LAZY,
+		cascade = CascadeType.ALL,
+		orphanRemoval = true
+	)
+	@Column(name = "answers")
+	private List<Answer> value = new ArrayList<>();


피드백 반영 잘해주셨네요 💯

Answers에 대한 테스트를 해보면 좋겠어요 :)

ㅎㅎ 꼼꼼한 테스트 케이스 작성하는게 어렵네요. 네! 테스트 케이스 작성하겠습니다 : )

slamdunk7575 · 2021-06-03T07:24:48Z

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

+    public static DeleteHistory ofAnswer(ContentId contentId, User deletedBy, LocalDateTime createDate){
+        return new DeleteHistory(ContentType.ANSWER, contentId, deletedBy, createDate);
+    }


이부분도 같이 제거해볼 수 있겠네요.

slamdunk7575 · 2021-06-03T07:25:40Z

src/test/java/qna/domain/answer/AnswersTest.java

+    @Test
+    void 복수의_Answers_를_Owner가_아닌_사람이_삭제할_경우() {
+        User notOwnerUser = UserTest.SANJIGI;
+        sut = new Answers(Lists.newArrayList(answer, answer2, answer3));
+
+        assertThatThrownBy(() -> sut.deletedBy(notOwnerUser)).isInstanceOf(CannotDeleteException.class);
+    }


테스트 꼼꼼히 잘해주셨네요 👍

slamdunk7575 · 2021-06-03T07:36:15Z

src/main/java/qna/domain/question/Title.java

+	public Title(String value) {
+		setValue(value);
+	}
+
+	protected void setValue(String value) {
+		if (Objects.isNull(value) || value.length() > 100){
+			throw new IllegalArgumentException("Invalid Title value");
+		}
+		this.value = value;
+	}
+
+	protected Title() { }


우선 피드백을 드렸던 의도는 메소드내에서 검증과 할당을 분리해보시면 좋을 것 같아 드렸던 거였습니다.
파라미터가 3개 이상 늘어나는 것은 피하는게 좋지만 만약 그렇게 된다면 정규님 말씀대로 검증 메소드가 늘어나 가독성이 떨어질거라 생각되네요. 검증하는 부분을 한곳으로 묶거나 아예 넘어오는 원시값을 포장하는 방향으로 생각을 이어가볼 수도 있겠네요 :)

정규님의 방식이 틀렸다라는 것이 결코 아니니 참고만 해주시면 좋겠습니다 🙇

[Clean Code] 3 : 함수

slamdunk7575

마지막까지 피드백 열심히 반영 해주셨네요 👍

저 또한 리뷰를 하면서 많이 배울 수 있었습니다.
제가 드렸던 피드백들이 앞으로 정규님께 조금이라도 도움이 되었으면 좋겠네요!

그동안 고생 많으셨습니다. 감사합니다. 🙇

저와의 미션은 종료되었지만, 궁금한점이나 어려운점 있으면 언제든 편하게 DM 주세요!
남은 미션도 꾸준히 진행하시고 끝까지 완주하시길 응원하겠습니다! 🎉

LenKIM added 13 commits June 1, 2021 20:35

docs(README): 기능 및 프로그래밍 요구사항 정의

9b71c2e

refactor: add Question Owner Validation method

e884047

docs: Todo 리스트 작성

f56e757

refactor: getter/setter 제거로 빈약한 도메인 모델 방지

16d1df9

docs: checked checklist

7a95557

refactor: setting @manytoone Fetch config to LazyLoading

2f0146d

refactor: answer 엔티티의 문자열 및 원시값 포장

d7f8df9

refactor: DeleteHistory 엔티티의 문자열 및 원시값 포장

4c2f0a7

refactor: Question 엔티티의 문자열 및 원시값 포장

372bd76

docs: checked DeleteHistory 엔티티의 문자열 및 원시값 포장

b8e8f91

refactor: user 엔티티의 원시값 및 문자열 포장

6acc055

refactor: @Where(clause = "deleted = false") 추가

0d2a6bb

refactor: deleteQuestion method 리팩토링

baa37d0

slamdunk7575 requested changes Jun 2, 2021

View reviewed changes

LenKIM added 7 commits June 2, 2021 10:46

docs: 리팩토링 Todo 정의

9f090c8

refactor: 맵핑관계에서 Answers 사용, DeleteHistory의 정적 팩토리메소드 추가

32bb374

refactor: ValueObject 의 유효성 검사

5f7bbd6

style: remove Contents Variable in Entity

f3883c8

refactor: Answer 의 Cascade 제거

719a6a6

style: remove GUEST_USER

2e811c4

refactor: 분리된 역할에 대한 테스트 코드 추가

5417952

LenKIM requested a review from slamdunk7575 June 2, 2021 11:09

LenKIM closed this Jun 2, 2021

LenKIM reopened this Jun 2, 2021

LenKIM closed this Jun 2, 2021

LenKIM reopened this Jun 2, 2021

slamdunk7575 requested changes Jun 3, 2021

View reviewed changes

LenKIM added 3 commits June 3, 2021 13:56

docs: 리팩토링 대상 Todolist 정의

defc3e1

refactor: 테스트 코드에서만 사용되는 메소드 제거

5cd03a2

refactor: Answers 에 대한 테스트 코드 작성

31cfcab

LenKIM requested a review from slamdunk7575 June 3, 2021 04:56

slamdunk7575 reviewed Jun 3, 2021

View reviewed changes

slamdunk7575 approved these changes Jun 3, 2021

View reviewed changes

slamdunk7575 merged commit f00c229 into next-step:lenkim Jun 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Step3] 질문 삭제하기 리팩터링 #93

[Step3] 질문 삭제하기 리팩터링 #93

LenKIM commented Jun 1, 2021

slamdunk7575 left a comment

slamdunk7575 Jun 2, 2021

LenKIM Jun 2, 2021

slamdunk7575 Jun 2, 2021

slamdunk7575 Jun 2, 2021

LenKIM Jun 2, 2021

slamdunk7575 Jun 2, 2021 •

edited

LenKIM Jun 2, 2021

slamdunk7575 Jun 2, 2021

LenKIM Jun 2, 2021

slamdunk7575 Jun 2, 2021

LenKIM Jun 2, 2021

slamdunk7575 Jun 2, 2021

LenKIM Jun 2, 2021

slamdunk7575 Jun 2, 2021 •

edited

LenKIM Jun 2, 2021

slamdunk7575 left a comment

slamdunk7575 Jun 3, 2021

LenKIM Jun 3, 2021

slamdunk7575 Jun 3, 2021 •

edited

LenKIM Jun 3, 2021

LenKIM Jun 3, 2021

slamdunk7575 Jun 3, 2021 •

edited

slamdunk7575 Jun 3, 2021

LenKIM Jun 3, 2021

slamdunk7575 Jun 3, 2021

slamdunk7575 Jun 3, 2021

slamdunk7575 Jun 3, 2021 •

edited

slamdunk7575 left a comment

[Step3] 질문 삭제하기 리팩터링 #93

[Step3] 질문 삭제하기 리팩터링 #93

Conversation

LenKIM commented Jun 1, 2021

slamdunk7575 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slamdunk7575 Jun 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slamdunk7575 Jun 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slamdunk7575 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slamdunk7575 Jun 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slamdunk7575 Jun 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slamdunk7575 Jun 3, 2021 • edited

Choose a reason for hiding this comment

slamdunk7575 left a comment

Choose a reason for hiding this comment

slamdunk7575 Jun 2, 2021 •

edited

slamdunk7575 Jun 2, 2021 •

edited

slamdunk7575 Jun 3, 2021 •

edited

slamdunk7575 Jun 3, 2021 •

edited

slamdunk7575 Jun 3, 2021 •

edited