step1 - 레거시 코드 리팩토링 리뷰요청 드립니다. #812
Conversation
javajigi
left a comment
javajigi
left a comment
There was a problem hiding this comment.
바쁜 일정에도 1단계 미션 진행하느라 넘 수고했습니다. 👍
요즘 여러 면에서 힘든 일이 많은 것으로 알고 있어요.
그로 인해 학습에 집중하기 힘들 것 같은데요.
그런 때 일수록 학습에 몰입함으로써 힘든 일을 잊는 시간으로 만들어 봐도 좋을 것 같아요.
피드백 남겼으니 한번 고민해 보는 시간을 가져보면 좋겠습니다.
| return "Answer [id=" + getId() + ", writer=" + writer + ", contents=" + contents + "]"; | ||
| } | ||
|
|
||
| public DeleteHistory delete() { |
There was a problem hiding this comment.
현재 구현한 방식(삭제 가능 여부와 삭제를 분리함)과 아래와 같이 글쓴이 인지 여부를 이 메서드가 판단하도록 구현하는 것 중 어느 방식으로 구현하는 것이 좋을지 고민해 본다.
위 두 가지 방식 중 선택한 방법으로 구현하는 것이 좋은 이유에 대해 피드백으로 남겨본다.
public void delete(NsUser loginUser){
// 글쓴이 인지를 판단하는 로직 검증 후 삭제 상태로 변경
}There was a problem hiding this comment.
피드백 받은 후 곰곰히 생각해보았습니다
결론적으로는 이 메소드가 판단하도록 구현하는것이 좋다고 생각했습니다
글쓴이 인지 여부를 판단하는 건, 삭제를 위해서 이기 때문에 같이 관리하는게 좋을거 같습니다.
(예를들면, 출금을 하기전 잔액을 체크하는 로직을 생각해보았습니다.
이 경우 출금과 잔액 체크를 나누는게 어색하다고 생각했습니다)
그래서 크게 delete()와 history()로 나누는게 좋을거 같다는 생각이 듭니다.
| import java.util.List; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class Answers { |
| public void updateDeleted() { | ||
| this.deleted = true; | ||
| } |
There was a problem hiding this comment.
| public void updateDeleted() { | |
| this.deleted = true; | |
| } | |
| private void delete() { | |
| this.deleted = true; | |
| } |
외부에서 삭제 상태를 바꿀 수 없도록 위와 같이 구현하는 것은 어떨까?
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class Question { |
There was a problem hiding this comment.
Question 객체는 우리가 현장에서 흔하게 접하는 인스턴스 변수가 많은 객체의 모습이다.
객체 지향 생활 체조 원칙의 다음 두 가지 원칙에 지키기 위해 노력해 본다.
- 규칙 6: 모든 엔티티를 작게 유지한다.
- 규칙 7: 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.
특히 규칙 7의 경우 지키기 힘들다 하더라도 인스턴스 변수의 수를 줄이기 위해 도전해 본다.
힌트: 상속을 통한 인스턴스 변수 줄이기, 관련 있는 인스턴스 변수를 새로운 객체로 분리하기 등 활용
| return title; | ||
| } | ||
|
|
||
| public Question setTitle(String title) { |
There was a problem hiding this comment.
불필요한 setter 메서드는 모두 제거하면 어떨까?
| List<DeleteHistory> deleteHistories = question.delete(loginUser); | ||
| deleteHistoryService.saveAll(deleteHistories); |
There was a problem hiding this comment.
👍
| List<DeleteHistory> deleteHistories = question.delete(loginUser); | |
| deleteHistoryService.saveAll(deleteHistories); | |
| question.delete(loginUser); | |
| List<DeleteHistory> deleteHistories = question.toDeleteHistories(); | |
| deleteHistoryService.saveAll(deleteHistories); |
단, 위와 같이 분리해 보는 방안도 고민해 보면 어떨까?
|
전체적으로 지난 피드백을 반영해보았습니다. 삭제 조건 체크와 삭제를 하나의 기능으로 묶으려다 보니, 전체적인 변경이 필요했습니다. 피드백 주시면 확인해보겠습니다. |
javajigi
left a comment
javajigi
left a comment
There was a problem hiding this comment.
피드백 반영 넘 잘 했네요. 💯
pr 본문에 남겼듯이 여유를 가지고 진행한 점은 칭찬하고 싶네요.
급할 것은 없으니 여유를 가지고 2단계도 진행해 보면 좋겠습니다.
| return "Answer [id=" + getId() + ", writer=" + writer + ", contents=" + contents + "]"; | ||
| } | ||
|
|
||
| public void delete(NsUser loginUser) throws CannotDeleteException { |
| return id; | ||
| } | ||
|
|
||
| protected void delete() { |
2 participants
안녕하세요!
레거시 코드 리팩토링 진행해보았습니다
서비스에 있던 로직을 도메인 객체 안으로 옮기고, 그것들을 합쳐서 캡슐화하는 과정으로 진행했습니다.
퇴근이 늦어..조금 자고 새벽에 작업을 하는 바람에..
Question 객체에 있는 변수를 분리해보고 싶은데, 시간이 부족해서 우선 리뷰요청했습니다.
이게 분리되면, Question 객체의 deleteQuestion의 new로 리턴하는 부분을 정적 팩토리 메소드로 바꿔보는건 괜찮은 방법일까요?
피드백 주시면, 확인해보겠습니다!
감사합니다.