1단계 레거시 코드 리펙터링 #791
1단계 레거시 코드 리펙터링 #791
Conversation
|
PR을 보내고나서 이전단계 미션에서 피드백받았던것들이 몇개 보여 추가 수정하였습니다. |
javajigi
left a comment
javajigi
left a comment
There was a problem hiding this comment.
이 미션을 쉽게 접근했다니 다행이네요. 👍
이 미션은 생각보다 간단한 미션일 수 있지만 이 미션을 통해 많은 것을 느껴봤으면 좋겠어요.
우리가 흔히 현장에서 만나는 코드인데요.
로직을 어디에 구현하느냐에 따라 TDD 가능 여부와 OOP의 맛을 제대로 느낄 수 있음을 알 수 있습니다.
고민해 보면 좋겠다 생각하는 부분에 대해 피드백 남겨봤어요.
| public DeleteHistory delete(){ | ||
| this.deleted = true; | ||
| return new DeleteHistory(ContentType.ANSWER, id, writer, LocalDateTime.now()); | ||
| } |
There was a problem hiding this comment.
객체의 삭제 상태를 변경하는 책임과 DeleteHistory 생성하고 반환하는 책임
위와 같이 두 가지 책임을 가지고 있는 느낌이 드는데 어떻게 생각하나?
https://vimeo.com/1137582691 영상에서 추천하는 메서드 이름 짓기 원칙을 따라 이름을 짓고 메서드 분리가 필요하다면 메서드 분리해 보면 어떨까?
| this.question = question; | ||
| } | ||
|
|
||
| public DeleteHistory delete(){ |
There was a problem hiding this comment.
현재 구현한 방식과 아래와 같이 글쓴이 인지 여부를 이 메서드가 판단하도록 구현하는 것 중 어느 방식으로 구현하는 것이 좋을지 고민해 본다.
위 두 가지 방식 중 선택한 방법으로 구현하는 것이 좋은 이유에 대해 피드백으로 남겨본다.
public void delete(NsUser loginUser){
// 글쓴이 인지를 판단하는 로직 검증 후 삭제 상태로 변경
}| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class Answers { |
| public void checkDeletable(NsUser loginUser) throws CannotDeleteException { | ||
| if (answers.stream().anyMatch(answer -> !answer.isOwner(loginUser))) { | ||
| throw new CannotDeleteException("다른 사람의 답변이 존재하여 삭제할 수 없습니다."); | ||
| } | ||
| } | ||
|
|
||
| public List<DeleteHistory> delete() { |
There was a problem hiding this comment.
check하는 부분과 delete가 분리되어 있음.
Answer 피드백의 delete 구현 부분의 결정에 따라 리팩터링해본다.
There was a problem hiding this comment.
Answers의 delete는 검증없이 삭제를 해서 checkDeletable을 호출해야만 하네요
| return writer.equals(loginUser); | ||
| } | ||
|
|
||
| public Question setDeleted(boolean deleted) { |
There was a problem hiding this comment.
setter 메서드 필요없지 않을까?
모든 도메인 객체의 setter를 제거해 본다.
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class Question { |
There was a problem hiding this comment.
Question 객체는 우리가 현장에서 흔하게 접하는 인스턴스 변수가 많은 객체의 모습이다.
객체 지향 생활 체조 원칙의 다음 두 가지 원칙에 지키기 위해 노력해 본다.
- 규칙 6: 모든 엔티티를 작게 유지한다.
- 규칙 7: 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.
특히 규칙 7의 경우 지키기 힘들다 하더라도 인스턴스 변수의 수를 줄이기 위해 도전해 본다.
힌트: 상속을 통한 인스턴스 변수 줄이기, 관련 있는 인스턴스 변수를 새로운 객체로 분리하기 등 활용
|
id필드는 BaseEntity로 분리를 잘 안했던 것 같은데 규칙 7을 맞춰보려다가 분리하게되었네요. |
javajigi
left a comment
javajigi
left a comment
There was a problem hiding this comment.
피드백 반영하느라 수고했어요. 👍
BaseEntity와 QuestionContents 분리 👍
피드백 남겼는데요. 확인하고 2단계 진행할 때 함께 반영해 보세요.
| @@ -0,0 +1,18 @@ | |||
| package nextstep.qna.domain; | |||
|
|
|||
| public class QuestionContent { | |||
| answer.setDeleted(true); | ||
| deleteHistories.add(new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now())); | ||
| } | ||
| List<DeleteHistory> deleteHistories = question.delete(loginUser); |
There was a problem hiding this comment.
| List<DeleteHistory> deleteHistories = question.delete(loginUser); | |
| question.delete(loginUser); | |
| List<DeleteHistory> deleteHistories = question.deleteHistories(); |
이와 같이 삭제 메시지와 deleteHistories 생성을 분리하는 것과 현재와 같이 구현하는 것 중 어느 접근 방식이 좋을까?
각각의 장단점은?
|
|
||
| import java.time.LocalDateTime; | ||
|
|
||
| public abstract class BaseEntity { |
2 participants
안녕하세요 이번 미션을 너무 쉽게만 생각하고 진행해버렸는지 금방 끝나버린 느낌이네요.(기존 레거시코드가 깔끔해서 그런것같은 느낌도..)
QuestionTest를 작성하다보니 기존에 상수로 등록되어있는 Q1을 활용했었는데 그러다보니 오히려 다른 테스트에서 기존 데이터가 남아있어서 그런지 다른 사람의 답변이 존재하여 삭제할 수 없습니다.라고 발생하더라구요.
테스트를 작성하면서 테스트 격리를 신경써야할 것 같아요