🚀 1단계 - 레거시 코드 리팩터링 #790

chanani · 2025-11-26T06:39:23Z

안녕하세요 ! 레거시 코드 리팩터링 1단계 리뷰 요청드립니다 ! 🙇‍♂️

먼저 호성님의 목표 설정과 체크리스트를 보고 저만의 학습 목표를 작성하여 이번 과정에서 놓쳐서는 안될 부분들을 의식하며, 과제를 진행했습니다.

아직은 기존 코드를 리팩터링하는 것이 서투르고, 어색하게 느껴지지만 이 과정을 통해 많은 것을 배웠으면 좋겠습니다.

… 권한 체크 메서드 생성 및 테스트 코드 작성

…werOwner 메서드 호출

…어떻게 처리하면 좋을지 고민중) 및 테스트 코드 작성

javajigi

드디어 현장과 비슷한 코드인 레거시 코드 리팩터링 미션 진행했군요. 👍
몇 가지 고민해보면 좋겠다 생각하는 부분에 대해 피드백 남겼으니 한번 고민해 보면 좋겠습니다.

javajigi · 2025-11-26T10:47:18Z

docs/01-refactoring.md

+## 코드 리뷰
+> PR 링크 :
+
+## 나의 학습 목표


javajigi · 2025-11-26T10:47:31Z

docs/check-list.md

@@ -0,0 +1,12 @@
+# ✔️ PR 전 확인해야될 목록


javajigi · 2025-11-26T10:50:35Z

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

+            throw new CannotDeleteException("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다.");
+        }
+    }
+


현재는 valid와 updateDeleted가 분리되어 구현되어 있음.
이 둘을 하나로 구현하는 아래와 비교했을 때 어느 방식으로 구현하는 것이 더 좋을까?

Suggested change

public void delete(NsUser loginUser) throws CannotDeleteException {

// 유효성 체크와 삭제 상태로 변경

}

이 피드백 반영했는지 확인 필요

javajigi · 2025-11-26T10:50:48Z

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

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


일급 콜렉션 적용 👍

javajigi · 2025-11-26T10:52:37Z

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

+        }
+    }
+
+    public List<DeleteHistory> delete(long questionId) {


Suggested change

public List<DeleteHistory> delete(long questionId) {

public List<DeleteHistory> delete() {

questionid는 Question이 가지고 있음.
굳이 필요없지 않을까?

javajigi · 2025-11-26T10:54:18Z

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

+        }
+    }
+
+    public List<DeleteHistory> delete(long questionId) {


Suggested change

public List<DeleteHistory> delete(long questionId) {

public void delete(NsUser loginUser) {

Answer 피드백과 같이 현재 둘로 분리되어 있는 구현과 위와 같이 삭제 권한 체크여부와 deleted 상태 변경 로직을 함께 구현하는 위 접근 방식 중 어느 접근 방식이 좋을지 고민해 보면 어떨까?

javajigi · 2025-11-26T10:55:54Z

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

+
+        return deleteHistories;
+    }
+


현재는 delete()에서 상태 변경과 List 생성 후 반환하는 현재 구현과 delete()와 toDeleteHistories()와 같이 상태 변경과 List 반환하는 메서드 두 개로 분리하는 것 중 어느 접근 방식이 좋을까?

javajigi · 2025-11-26T10:56:32Z

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

+    }
+
+
+


Answers 일급 콜렉션에 대한 단위 테스트도 추가하면 어떨까?

상태를 변경하는 메서드와 삭제 내역을 남기는 history를 분리

refactor : answer 테스트 코드 추가, Question 테스트 코드 내부 수정

chanani · 2025-11-26T13:49:05Z

리뷰해 주신 것과 같이 수정해봤습니다 !

한 메서드는 하나의 역할만 해야된다는 생각으로 검증하는 로직과 상태를 변경하는 로직이 Service(deleteQuestion)에서 각각 호출 되어야 한다고 생각했습니다.

delete() 호출 후 해당 메서드 내부에서 검증 로직 따로, 상태 변경 로직 따로 진행하게 되면, 말씀하신 것과 같이 delete라는 목적에 알맞다고 생각합니다 !

추가로 피드백 전 수정해 놓은 내용 있는데 확인 못하셨을 것 같아 말씀드립니다 !
공용으로 사용되는 createdDate, updatedDate, deleted를 BaseModel 클래스로 분리하여 상속 받는 방향으로 진행해 봤는데 효율적인건지 그리고 public으로 선언해 놨는데 괜찮은지 궁금합니다 !

javajigi

BaseModel 분리 인상적이네요. 👍
단, 이 같은 추상 클래스를 분리했을 때 고려해야할 부분 피드백 남겼으니 확인해 보세요.

javajigi · 2025-11-26T21:12:24Z

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

        return this;
    }

+    public void updateDeleted() {


삭제 상태 변경에 따른 검증을 하는 것을 고려했을 때 private으로 구현해야 안전하지 않을까?

javajigi · 2025-11-26T21:12:40Z

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

+            throw new CannotDeleteException("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다.");
+        }
+    }
+


이 피드백 반영했는지 확인 필요

javajigi · 2025-11-26T21:14:51Z

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

+        }
+    }
+
+    public void addDeleteAnswerHistory(List<DeleteHistory> deleteHistories) {


Suggested change

public void addDeleteAnswerHistory(List<DeleteHistory> deleteHistories) {

public List<DeleteHistory> toDeleteAnswerHistories() {

List<DeleteHistory> deleteHistories = new ArrayList<>();

개인적으로 List 콜렉션을 인자로 전달해 저장하는 것보다 위와 같이 해당 메서드에서 직접 생성해 반환하는 것이 다른 객체와의 의존 관계가 발생하지 않기 때문에 선호하는 방법임

javajigi · 2025-11-26T21:17:42Z

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

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


👍
단, 프로젝트에 모든 컨텐츠는 성격의 데이터는 soft delete 정책을 따른다와 같은 규칙이 있을 경우 deleted 상태를 포함하는 것이 추상화 측면에서 맞음.
이렇게 될 경우 이 BaseModel은 이 규칙을 따르지 않는 Model은 상속할 수 없기 때문에 객체의 이름을 더 구체적으로 작성할 것을 추천
AI에게 프로젝트의 정책을 설명하고 더 적합한 이름을 추천 받아본다.

javajigi · 2025-11-26T21:18:16Z

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

+    public LocalDateTime createdDate = LocalDateTime.now();
+
+    public LocalDateTime updatedDate;
+
+    public boolean deleted = false;


Suggested change

public LocalDateTime createdDate = LocalDateTime.now();

public LocalDateTime updatedDate;

public boolean deleted = false;

private LocalDateTime createdDate = LocalDateTime.now();

private LocalDateTime updatedDate;

private boolean deleted = false;

모든 인스턴스 변수는 가능하면 private으로 구현

javajigi · 2025-11-26T21:19:35Z

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

+    public LocalDateTime createdDate = LocalDateTime.now();
+
+    public LocalDateTime updatedDate;
+
+    public boolean deleted = false;


Suggested change

public LocalDateTime createdDate = LocalDateTime.now();

public LocalDateTime updatedDate;

public boolean deleted = false;

private boolean deleted = false;

그럼 상태 값 변경은 어떻게 하나?

protected void deleted() { this.deleted = true; }

아래와 같이 protected 메서드를 제공하고 자식 클래스에서 호출할 수 있도록 한다.

javajigi · 2025-11-26T21:19:58Z

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

@@ -0,0 +1,10 @@
+package nextstep.qna.domain;
+
+public class Contents {


이 객체는 값을 포장하는 것 외에 아무 로직도 없는데 포장하는 것이 의미있을까?
Title 동일
단, title과 contents를 인스턴스 변수를 가지는 QuestionBody와 같은 객체를 분리한다면 의미있을까?

chanani added 8 commits November 25, 2025 14:46

docs : 기능 요구사항 문서 작성

c4960b6

docs : 나의 학습 목표, PR전 확인해야할 목록 작성(호성님이 작성한 내용에 감명 받아 저도 관리하려고합니다.)

a117751

refactor : Question 객체에 질문 삭제 권한 체크 메서드 생성 및 테스트 코드 작성, Answer 객체에 삭제…

3cdb2ed

… 권한 체크 메서드 생성 및 테스트 코드 작성

refactor : Question 객체의 인스턴스 변수(answer) 활용해 직접 Answer 객체의 validateAns…

9c9d031

…werOwner 메서드 호출

refactor : 질문, 답변 삭제 로직 Question 객체로 이동(Deleted 값을 변경하는 로직이 포함되어 있어서 …

625a895

…어떻게 처리하면 좋을지 고민중) 및 테스트 코드 작성

refactor : Question 클래스의 List<Answer> 변수 포장

188c9d7

docs : 헷갈리는 내용 Todo 추가

8312ece

refactor : 공동으로 사용되는 createdDate, updatedDate, delted를 BaseModel로 분리

f1b3547

javajigi requested changes Nov 26, 2025

View reviewed changes

chanani added 4 commits November 26, 2025 20:02

refactor : title, contents 객체 포장

54e1f6d

refactor : delete() 메서드 파라미터 제거 및 테스트 코드 수정

c4db9bd

refactor : 검증하는 로직과 상태 변경하는 로직을 함께 구현,

400b772

상태를 변경하는 메서드와 삭제 내역을 남기는 history를 분리

feat : Answers 테스트 코드 생성

73effd2

refactor : answer 테스트 코드 추가, Question 테스트 코드 내부 수정

javajigi requested changes Nov 26, 2025

View reviewed changes

+public void delete(NsUser loginUser) throws CannotDeleteException {
+    // 유효성 체크와 삭제 상태로 변경
+}

	public List<DeleteHistory> delete(long questionId) {
	public List<DeleteHistory> delete() {

	public List<DeleteHistory> delete(long questionId) {
	public void delete(NsUser loginUser) {

	public void addDeleteAnswerHistory(List<DeleteHistory> deleteHistories) {
	public List<DeleteHistory> toDeleteAnswerHistories() {
	List<DeleteHistory> deleteHistories = new ArrayList<>();


		import java.time.LocalDateTime;

		public abstract class BaseModel {

		@@ -0,0 +1,10 @@
		package nextstep.qna.domain;

		public class Contents {

🚀 1단계 - 레거시 코드 리팩터링 #790

Are you sure you want to change the base?

🚀 1단계 - 레거시 코드 리팩터링 #790

Uh oh!

Conversation

chanani commented Nov 26, 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!

Choose a reason for hiding this comment

Uh oh!

chanani commented Nov 26, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants