Step1 - 레거시 코드 리팩터링 #804

ho-jun97 · 2025-12-08T14:27:15Z

안녕하세요! 코드 리뷰 요청합니다. 잘 부탁드립니다.

javajigi

1단계 미션 진행하느라 수고했어요. 👍
코드를 분석할 때 전체를 분석 후에 리팩터링할 부분을(대표적으로 setter) 찾는 습관을 들여보면 어떨까요?
객체 지향 생활 체조 원칙의 다음 두 가지 원칙에 대한 피드백 남겼으니 한번 도전해 볼 것을 추천합니다.

규칙 6: 모든 엔티티를 작게 유지한다.
규칙 7: 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.

javajigi · 2025-12-08T21:06:26Z

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

    }

-    public Answer setDeleted(boolean deleted) {
+    private Answer setDeleted(boolean deleted) {


Suggested change

private Answer setDeleted(boolean deleted) {

private Answer delete() {

this.deleted = true;

}

위와 같이 구현하거나 setter 메서드를 아예 제거하는 것은 어떨까?

javajigi · 2025-12-08T21:06:58Z

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

+    public void delete() {
+        setDeleted(true);
+    }


현재 구현한 방식(삭제 가능 여부와 삭제를 분리함)과 아래와 같이 글쓴이 인지 여부를 이 메서드가 판단하도록 구현하는 것 중 어느 방식으로 구현하는 것이 좋을지 고민해 본다.
위 두 가지 방식 중 선택한 방법으로 구현하는 것이 좋은 이유에 대해 피드백으로 남겨본다.

public void delete(NsUser loginUser){ // 글쓴이 인지를 판단하는 로직 검증 후 삭제 상태로 변경 }

javajigi · 2025-12-08T21:07:59Z

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

+import java.util.ArrayList;
+import java.util.List;
+
+public class Answers {


일급 콜렉션 추가 👍

javajigi · 2025-12-08T21:08:20Z

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

+        this.answers.add(answer);
+    }
+
+    public List<DeleteHistory> delete() {


현재 구현한 방식(삭제 가능 여부와 삭제를 분리함)과 아래와 같이 글쓴이 인지 여부를 이 메서드가 판단하도록 구현하는 것 중 어느 방식으로 구현하는 것이 좋을지 고민해 본다.
위 두 가지 방식 중 선택한 방법으로 구현하는 것이 좋은 이유에 대해 피드백으로 남겨본다.

public void delete(NsUser loginUser){ // 글쓴이 인지를 판단하는 로직 검증 후 삭제 상태로 변경 }

javajigi · 2025-12-08T21:10:01Z

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

+import java.util.stream.Collectors;
+import java.util.stream.Stream;

 public class Question {


Question 객체는 우리가 현장에서 흔하게 접하는 인스턴스 변수가 많은 객체의 모습이다.
객체 지향 생활 체조 원칙의 다음 두 가지 원칙에 지키기 위해 노력해 본다.

규칙 6: 모든 엔티티를 작게 유지한다.

규칙 7: 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.

특히 규칙 7의 경우 지키기 힘들다 하더라도 인스턴스 변수의 수를 줄이기 위해 도전해 본다.
힌트: 상속을 통한 인스턴스 변수 줄이기, 관련 있는 인스턴스 변수를 새로운 객체로 분리하기 등 활용

javajigi · 2025-12-08T21:11:11Z

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

        return title;
    }

    public Question setTitle(String title) {


도메인 객체에서 불필요한 setter 메서드는 모두 제거한다.

javajigi · 2025-12-08T21:11:44Z

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

+        return answers.getAnswers();
+    }
+
+    public List<DeleteHistory> delete(NsUser loginUser) throws CannotDeleteException {


객체의 삭제 상태를 변경하는 책임과 DeleteHistory 생성하고 반환하는 책임
위와 같이 두 가지 책임을 가지고 있는 느낌이 드는데 어떻게 생각하나?
https://vimeo.com/1137582691 영상에서 추천하는 메서드 이름 짓기 원칙을 따라 이름을 짓고 메서드 분리가 필요하다면 메서드 분리해 보면 어떨까?

javajigi · 2025-12-08T21:11:51Z

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

-            deleteHistories.add(new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now()));
-        }
-        deleteHistoryService.saveAll(deleteHistories);
+        deleteHistoryService.saveAll(question.delete(loginUser));


ho-jun97 · 2025-12-10T15:39:17Z

삭제를 하기전에 검증을 했을 때 좋다고 생각하는 점

삭제 행위를 하기전 누군가 항상 검증을 하기 때문에 안전성이 더 보장된다
더 객체지향적이다.

포비님께서 생각하시는 좋은점이 궁금합니다.

객체의 삭제 상태를 변경하는 책임과 DeleteHistory 생성하고 반환하는 책임 분리 과정

deleteHistories를 만드는 책임을 DeleteHistoryService 위임

Question question = questionRepository.findById(questionId).orElseThrow(NotFoundException::new);
 question.delete(loginUser)
 deleteHistoryService.saveAll(deleteHistoryService.createDeleteHistory(question);

처음에는 delete에서 상태변경과 DeleteHistory 생성하고 반환하는 부분을 분리를 하고 DeleteHistoryService에 createDeleteHistory를 만들어서 하려고 했으나 주어진 QnaServiceTest를 고쳐야하는 상황이 와서 Question class안에 만들어서 반환하도록 하였습니다.

상태변화 후 service단에서 다시 createDeleteHistory() 호출

Question question = questionRepository.findById(questionId).orElseThrow(NotFoundException::new);
 question.delete(loginUser)
 deleteHistoryService.saveAll(question.createDeleteHistory());

delete 와 history는 항상 연결지어야 한다고 생각이 들어 아래와 같이 변경

Question question = questionRepository.findById(questionId).orElseThrow(NotFoundException::new);

  deleteHistoryService.saveAll(question.delete(loginUser));

delete안에서 상태변화를 시키고 마지막에 createDeleteHistory()를 호출하였습니다.

   public List<DeleteHistory> delete(NsUser loginUser) throws CannotDeleteException {
    this.markAsDeleted(loginUser);
    answers.delete(loginUser);

    return createDeleteHistories();
}

위와 같은 과정들을 통해서 최종 형태로 만들었는데 어떤 방식이 맞는건지 잘 모르겠습니다. 리뷰 잘 부탁드립니다.

질문

PostContent에 writer, title, contents를 담아두었는데요. 여기서 title과 contents는 String으로 원시값 포장을 하지 않고 그냥 두었습니다.
원시값을 포장하는 것이 더 좋을까요?

javajigi

피드백 반영하느라 수고했어요. 👍
단, 피드백 반영 중 아쉬움이 남는 부분이 있어 피드백 남겨 봅니다.

Q1. PostContent에 writer, title, contents를 담아두었는데요. 여기서 title과 contents는 String으로 원시값 포장을 하지 않고 그냥 두었습니다.
원시값을 포장하는 것이 더 좋을까요?
A1. 원시값, 문자열을 포장했을 때 얻을 수 있는 잇점이 없다면 굳이 포장하지 않아도 된다고 생각해요.

객체의 삭제 상태를 변경하는 책임과 DeleteHistory 생성하고 반환하는 책임 분리 과정에 대해서는 피드백에 남겼으니 한번 고민해 보세요.
저는 둘을 완전히 분리하고 테스트 코드 수정이 필요하다면 수정하는 것도 추천해 봅니다.

javajigi · 2025-12-10T23:02:58Z

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

        this.question = question;
    }

+    public void validateDeletableBy(NsUser loginUser) throws CannotDeleteException {


Suggested change

public void validateDeletableBy(NsUser loginUser) throws CannotDeleteException {

private void validateDeletableBy(NsUser loginUser) throws CannotDeleteException {

private으로 구현해도 되지 않을까?

javajigi · 2025-12-10T23:04:08Z

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

+        List<DeleteHistory> deleteHistories = new ArrayList<>();
+
+        for (Answer answer : answers) {
+            deleteHistories.add(new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now()));


Suggested change

deleteHistories.add(new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now()));

deleteHistories.add(answer.createDeleteHistory());

answer의 값을 꺼내고 있는데 answer가 위와 같이 생성하도록 구현하는 것은 어떨까?

javajigi · 2025-12-10T23:04:34Z

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

+        return deleteHistories;
+    }
+
+    public void validateDeletableBy(NsUser loginUser) throws CannotDeleteException {


이 메서드 제거해도 되지 않을까?

javajigi · 2025-12-10T23:04:53Z

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

+
+import java.time.LocalDateTime;
+
+public abstract class BaseEntity {


javajigi · 2025-12-10T23:05:42Z

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

+public class PostContent {
+    private final NsUser writer;
+    private final String title;
+    private final String contents;


Answer는 title이 없는데 이 객체를 사용해도 될까?
이 객체 또한 분리해야하지 않을까?

javajigi · 2025-12-10T23:06:32Z

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


-    public boolean isDeleted() {
-        return deleted;
+    public List<DeleteHistory> delete(NsUser loginUser) throws CannotDeleteException {


Suggested change

public List<DeleteHistory> delete(NsUser loginUser) throws CannotDeleteException {

public void delete(NsUser loginUser) throws CannotDeleteException {

테스트 때문에 이 메서드 원형을 유지했다면 테스트 코드를 수정하는 것을 추천함

javajigi · 2025-12-10T23:06:44Z

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


-    public List<Answer> getAnswers() {
-        return answers;
+    public List<DeleteHistory> createDeleteHistories() {


ho-jun97 added 2 commits December 8, 2025 22:31

refactor: quest에 delete 메시지 보내기

263e542

refactor: answer 리스트를 answers 에 포장하기 (일급 컬렉션)

b414d7f

javajigi requested changes Dec 8, 2025

View reviewed changes

ho-jun97 added 3 commits December 10, 2025 22:08

refactor: delete 처리 전에 유효한지 체크

06dc6e1

refactor: 공통된 부분 처리

12e7e60

refactor: 삭제 마크 하는 부분과 history 만드는 부분 분리

0c6c63a

javajigi requested changes Dec 10, 2025

View reviewed changes

-    private Answer setDeleted(boolean deleted) {
+    private Answer delete() {
+        this.deleted = true;
+    }

	public void validateDeletableBy(NsUser loginUser) throws CannotDeleteException {
	private void validateDeletableBy(NsUser loginUser) throws CannotDeleteException {

	deleteHistories.add(new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now()));
	deleteHistories.add(answer.createDeleteHistory());


		import java.time.LocalDateTime;

		public abstract class BaseEntity {

	public List<DeleteHistory> delete(NsUser loginUser) throws CannotDeleteException {
	public void delete(NsUser loginUser) throws CannotDeleteException {

Step1 - 레거시 코드 리팩터링 #804

Are you sure you want to change the base?

Step1 - 레거시 코드 리팩터링 #804

Uh oh!

Conversation

ho-jun97 commented Dec 8, 2025

Uh oh!

javajigi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ho-jun97 commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

객체의 삭제 상태를 변경하는 책임과 DeleteHistory 생성하고 반환하는 책임 분리 과정

질문

Uh oh!

javajigi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ho-jun97 commented Dec 10, 2025 •

edited

Loading