[step1] 레거시 코드 리팩터링 #468
[step1] 레거시 코드 리팩터링 #468
Conversation
| @@ -0,0 +1,40 @@ | |||
| package nextstep.qna.domain; | |||
There was a problem hiding this comment.
질문 도메인에 삭제하는 조건이 들어가는게 맞을까요?
질문을 삭제하려면 답변의 작성자를 확인해야합니다. 데이터베이스에서 모두 답변을 조회 후 확인하는 형태가 맞을까요?
현재 Question이 Answers를 가지고 있는 형태로 구성되어 있습니다. 일반적으로 JPA를 사용할 것이라 Lazy 로딩으로 가져온 Answers가 다 조회된 것이라 가정하고 작성하셔도 무방하고, 그게 아니라면 직접 AnswerRepository를 통해 조회한 후 Question의 Answers에 값이 할당된 상태로 인스턴스를 만들어주시면 될 것 같습니다.
| return isEmpty || isAllOwner; | ||
| } | ||
|
|
||
| public void setAllDeleted(boolean deleted){ |
There was a problem hiding this comment.
set이 아닌 deleteAll 등의 동사 형태로 표현해도 무방할 것 같아요.
| public List<Answer> getAll(){ | ||
| return this.answerList; |
There was a problem hiding this comment.
외부에 참조가 그대로 노출되고 있네요. 외부에서 추가 및 삭제를 할 수 있지 않을까요? Collections.unmodifiableList를 사용하시거나 Stream의 toList 등을 활용해보시면 어떨까요?
| deleteHistories.addQuestion(question); | ||
| answers.setAllDeleted(true); | ||
| deleteHistories.addAnswers(answers); | ||
|
|
||
| deleteHistoryService.saveAll(deleteHistories); |
There was a problem hiding this comment.
QnAService는 질문과 답변에 대한 처리를 하고 있습니다. 생성, 수정 등에는 DeleteHistoryService가 사용되지 않음에도 의존을 하고 있는 응집도가 낮은 상태라고 할 수 있겠네요.
이 부분을 이벤트 리스너 등을 통해 QnAService와 DeleteHistoryService간의 결합을 끊어볼 수 있을까요?
| deleteHistories.add(new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now())); | ||
| } | ||
| deleteHistories.addQuestion(question); | ||
| answers.setAllDeleted(true); |
There was a problem hiding this comment.
항상 true가 넘어갈 것 같은데 외부에서 프로퍼티를 받을 필요가 있을까요?
| private void addDeleteHistory(DeleteHistory deleteHistory){ | ||
| this.deleteHistories.add(deleteHistory); | ||
| } | ||
|
|
||
| public void addQuestion(Question question){ | ||
| DeleteHistory deleteHistory = new DeleteHistory(ContentType.QUESTION, question.getId(), question.getWriter(), LocalDateTime.now()); | ||
| this.addDeleteHistory(deleteHistory); | ||
| } | ||
|
|
||
| public void addAnswer(Answer answer){ | ||
| DeleteHistory deleteHistory = new DeleteHistory(ContentType.ANSWER, answer.getId(), answer.getWriter(), LocalDateTime.now()); | ||
| this.addDeleteHistory(deleteHistory); | ||
| } | ||
|
|
||
| public void addAnswers(Answers answers){ | ||
| for(Answer answer : answers.getAll()){ | ||
| this.addAnswer(answer); | ||
| } | ||
| } |
There was a problem hiding this comment.
가변 List에 question, answers에 따라 삭제 이력을 더해주는 형태로 구성되어 있네요. 코드 안에서 DeleteHistories 인스턴스를 사용하다 보면 Question, Answers가 어디까지 저장된 상태인지 알기 어려워 보여요. 정적 팩터리 등을 통해 명확하게 Answers, Question 삭제 이력에 대한 DeleteHistories를 분리해서 생성하는 방법도 고려해볼 수 있을까요?
| if (!answers.isDeletable(loginUser)) { | ||
| throw new CannotDeleteException("다른 사람이 쓴 답변이 있어 삭제할 수 없습니다."); | ||
| } | ||
|
|
There was a problem hiding this comment.
QnAService에 삭제 예외 처리 로직이 있네요. 이 부분을 도메인(Answers)로 옮겨보면 좋겠습니다.
| } | ||
|
|
||
| List<DeleteHistory> deleteHistories = new ArrayList<>(); | ||
| DeleteHistories deleteHistories = new DeleteHistories(); | ||
| question.setDeleted(true); |
There was a problem hiding this comment.
false로 설정될 일이 없을 것 같은데 delete라는 동사형 이름으로 짓고 파라미터를 받지 않는건 어떨까요?
| @@ -30,20 +30,17 @@ public void deleteQuestion(NsUser loginUser, long questionId) throws CannotDelet | |||
| throw new CannotDeleteException("질문을 삭제할 권한이 없습니다."); | |||
| } | |||
There was a problem hiding this comment.
질문 삭제 권한과 관련된 부분도 Question 도메인으로 옮겨주시면 좋겠습니다.
도메인에서 예외를 발생시키고 변환이 필요하다면 서비스 레이어에서 예외를 잡아서 서비스 레이어에 맞는 예외 클래스로 변환해서 사용하시면 될 것 같아요. 불필요하다면 도메인의 예외를 컨트롤러까지 전파시킨 뒤 그 응답에 맞게 Response를 구성하시면 될 것 같구요. |
기존 규칙대로 할 수 있는 부분만 우선 변경하고 PR요청 드립니다.
궁금한 점
질문 도메인에 삭제하는 조건이 들어가는게 맞을까요?
질문을 삭제하려면 답변의 작성자를 확인해야합니다. 데이터베이스에서 모두 답변을 조회 후 확인하는 형태가 맞을까요?
질문 도메인에 삭제하는 조건이 있다면, 예외 발생은 어디서 해야할까요?
서비스 레이어에서 예외를 �관리를 해야 한다고 생각했는데 비즈니스 로직이 질문 도메인으로 가면 처리를 어떻게 해야 할지 모르겠습니다.
피드백 주시면 감사하겠습니다.