[수강신청] 1단계 - 레거시 코드 리팩터링 #666
Conversation
| import nextstep.users.domain.NsUser; | ||
|
|
||
| import java.time.LocalDateTime; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class Question { | ||
| private static final List<QuestionDeletable> deletableConditions = List.of(new QuestionDeletableAnswer(), new QuestionDeletableWriter()); |
There was a problem hiding this comment.
유효성검사를 추상화해서 사용하는건 좋은 방법이라고 생각해요!
하지만 추상화를 하면 필연적으로 직관적이지 못한 코드가 되는데요, 그런 관점에서 지금은 별로 맞지 않는 요소들이 많지 않나 싶어요!
- 유효성검사 체크가 복잡하지 않아요. 삭제 요청 시 간단하게 체크 할 수 있는 수준이라 굳이 별개의 개념으로 만들 필요는 없어보여요
- 실제 삭제가 수행되는건 deleteQuestionAndAnswers인데, 이 부분만 보면 요구사항에 있는 유효성검사는 없는것처럼 보여요
복잡한 비즈니스 정책이 엮인 유효성검사라면 지금같은 방법이 좋다고 생각합니다! 물론 그때는 도메인에선 안 할 것 같긴 하지만요..!
하지만 지금은 그냥 심플하게 가져가는게 위의 이유에서 더 좋다고 생각하는데 재석님은 어떻게 생각하실까요 ?
There was a problem hiding this comment.
처음에는 단순하게 checkDeletable() 안에서 if 문을 몇 개 쓰는 방식으로 구현했습니다.
다만 객체생활 체조 원칙 등에서 if 문을 여러개 쓰지 말라고 했던 것도 있고, 이번 라이브 강의 시간에 추상화를 통해 if 를 for 로 바꾸는 법을 보여주셔서 적용해 봤습니다.
우선 deleteQuestionAndAnswers 는 아래 코멘트와 함께 수정하면서 delete 에 통합하였습니다.
이제 delete 에서 checkDeletable 를 호출하고, checkDeletable 에서 QuestionDeletable 을 확인합니다.
말씀 주신 것처럼 유효성검사 체크가 복잡하진 않지만, 유효성 체크는 쉽게 수정 가능한 조건이라고 생각되어 최대한 확장성 있게 작성해 봤습니다.
(낮은 권한의 유저는 아무것도 삭제할 수 없다던가)
과한 작업이라고 보이실까요?
There was a problem hiding this comment.
취향차이가 좀 있을 수 있는데요! 저는 개인적으로 지금은 과한 작업이라고 생각하긴 합니다~
하지만 바꿔주신 모양에서는 delete의 유효성 검사를 하겠다는 명확한 의도가 함께 보여서 확장성을 고려한 설계라고 인지하고 읽을 수 있어서 나쁘지 않은 것 같아요!
과하다고 생각하는 이유는 위에 말씀드린대로 저는 최대한 직관적으로 드러나는걸 좋아하는 편이라 추상화를 통해 한겹 감싸는건 꼭 필요할 때 해야 한다고 생각하기 때문이구요, 학습의 목적 + 확장 가능한 설계를 위해서 그런 프레임을 잡아나가는 측면에서는 동의합니다! 실제로 저도 실무에서 비즈니스 요구사항이 많은 경우는 재석님이 작성해주신 것처럼 유효성 검사 부분을 추상화해서 사용하고 있어요!
재석님 의견도 충분히 공감되어서 굳이 바꾸실 필요는 없을 것 같아요!
|
|
||
| public List<DeleteHistory> delete(NsUser loginUser) throws CannotDeleteException { | ||
| checkDeletable(loginUser); | ||
| deleteQuestionAndAnswers(); |
There was a problem hiding this comment.
이부분이 역할이 좀 애매하게 나뉘어진 것 같은데, Question과 Answer 각각의 delete에서 DeleteHistory를 리턴하고 결과를 합치게 해보면 어떨까요 ?
| @@ -68,13 +74,52 @@ public void addAnswer(Answer answer) { | |||
| answers.add(answer); | |||
| } | |||
|
|
|||
| private void checkDeletable(NsUser loginUser) throws CannotDeleteException { | |||
There was a problem hiding this comment.
메서드 순서는 public -> private 순으로 해주시면 읽기 좀 편해질 것 같아요..!
| private void checkDeletable(NsUser loginUser) throws CannotDeleteException { | ||
| for (QuestionDeletable condition : deletableConditions) { | ||
| if (!condition.deletable(this, loginUser)) { | ||
| throw new CannotDeleteException(condition.reason()); |
There was a problem hiding this comment.
Checked Exception과 Unchecked Exception의 차이가 뭘까요 ?
There was a problem hiding this comment.
질문의 의도를 이해하지 못했습니다.
왜 CannotDeleteException 을 Checked Exception 으로 구성했냐는 말씀이실까요?
이 부분은 기존에 정의되어 있던 부분이라, 따로 수정하지는 않았습니다.
CannotDeleteException 을 Unchecked Exception 으로 수정하는 것을 권장하려는 의도이신가요?
There was a problem hiding this comment.
넵 그 의도가 맞습니다! 굳이 throws를 해서 이 메서드를 쓰는쪽까지 다 체크하게 만들 필요는 없을 것 같아서요!
| public class QuestionDeletableAnswer implements QuestionDeletable { | ||
|
|
||
| @Override | ||
| public boolean deletable(Question question, NsUser loginUser) { |
There was a problem hiding this comment.
참고차, 요거 어차피 통과 안되면 예외 던지는거라 reason을 나눌 필요 없이 여기서 예외에 메세지 담아서 던지면 될 것 같아요 ~
There was a problem hiding this comment.
'삭제 가능 여부 검사' 와, '삭제 실패 예외 던지기' 를 다른 목적이라고 봐서 분리했는데, 동일한 책임으로 볼 수도 있겠네요.
수정했습니다.
| } | ||
| } | ||
| public List<DeleteHistory> delete(NsUser loginUser) throws CannotDeleteException { | ||
| checkDeletable(loginUser); |
| for (Answer answer : answers) { | ||
| deleteHistories.add(new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now())); | ||
| deleteHistories.add(answer.delete()); |
요구 사항 구현했습니다.
확인 부탁드립니다.