-
Notifications
You must be signed in to change notification settings - Fork 308
1단계 - 레거시 코드 리팩터링 #810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
1단계 - 레거시 코드 리팩터링 #810
Conversation
javajigi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
레거시 코드 리팩터링 미션 넘 구현 잘 했네요. 💯
특별히 피드백할 내용은 없지만 한번 고민해 보면 좋겠다는 생각이 들어 피드백 남겨 봅니다.
그리 오래 시간이 걸리지는 않을 것 같으니 빠르게 피드백 반영하고 다음 단계 진행해 보시죠.
|
|
||
| public NsUser getWriter() { | ||
| return writer; | ||
| public List<DeleteHistory> deleteWithHistory(NsUser user) throws CannotDeleteException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
객체의 삭제 상태를 변경하는 책임과 DeleteHistory 생성하고 반환하는 책임
위와 같이 두 가지 책임을 가지고 있는 느낌이 드는데 어떻게 생각하나?
https://vimeo.com/1137582691 영상에서 추천하는 메서드 이름 짓기 원칙을 따라 이름을 짓고 메서드 분리가 필요하다면 메서드 분리해 보면 어떨까?
| import nextstep.qna.CannotDeleteException; | ||
| import nextstep.users.domain.NsUser; | ||
|
|
||
| public class Answers { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일급 콜렉션 👍
| answers.add(answer); | ||
| } | ||
|
|
||
| public List<DeleteHistory> deleteAll(NsUser user) throws CannotDeleteException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
앞의 Answer 피드백과 같은 피드백 남겨봄
객체의 삭제 상태를 변경하는 책임과 DeleteHistory 생성하고 반환하는 책임
위와 같이 두 가지 책임을 가지고 있는 느낌이 드는데 어떻게 생각하나?
https://vimeo.com/1137582691 영상에서 추천하는 메서드 이름 짓기 원칙을 따라 이름을 짓고 메서드 분리가 필요하다면 메서드 분리해 보면 어떨까?
|
|
||
| import nextstep.users.domain.NsUser; | ||
|
|
||
| public abstract class BaseEntity extends BaseTimeEntity { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
특히 BaseTimeEntity까지 분리한 점
|
|
||
| private NsUser writer; | ||
|
|
||
| private boolean deleted = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
soft delete로 삭제하는 컨텐츠의 경우 상속하는 의미로 BaseEntity보다 SoftDeletableBaseEntity와 같은 이름으로 구현하는 것은 어떨까?
| return writer; | ||
| } | ||
|
|
||
| public void markAsDeleted() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public void markAsDeleted() { | |
| protected void markAsDeleted() { |
👍
단, 접근 제어자를 protected로 구현하는 것은 어떨까?
|
|
||
| private LocalDateTime updatedDate; | ||
| public class Question extends BaseEntity { | ||
| private QuestionContent content; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
javajigi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피드백 반영 👍
1단계 미션을 통해 레거시 코드 리팩터링하는데 영감을 받았기를 바랍니다.
2단계는 기존의 개발 방식과 달리 db 먼저가 아니라 도메인 객체 설계인데요.
도전해 보면서 기존과는 다른 경험을 맛볼 수 있기를 바랍니다.
안녕하세요.
1단계 - 레거시 코드 리팩터링 PR 입니다.