[MVC 구현하기 - 2단계] 카피(김상혁) 미션 제출합니다. #750
Conversation
kelly6bf
left a comment
kelly6bf
left a comment
There was a problem hiding this comment.
카피! 리뷰가 늦어 죄송합니다! 잘 작성해주셔서 리뷰 드릴 부분이 크게 없네요! 다음 리뷰 요청은 빛의 속도로 받겠습니다!
| import jakarta.servlet.http.HttpServletResponse; | ||
| import java.util.List; | ||
|
|
||
| public class HandlerAdapters { |
There was a problem hiding this comment.
1급 객체를 사용해서 HandlerAdapter 객체들을 관리하네요! 덕분에 어떤 장점을 얻으셨는지 궁금합니다!
There was a problem hiding this comment.
기존에는 알맞은 handlerAdapter를 찾아 ModelAndView를 반환하는 기능이 DispatcherServlet안에 private메서드로 존재했습니다! 그래서 여러 책임이 dispatcherSerlet에 존재했었고 테스트하기에도 용이하지 않았습니다.
그래서 일급 컬렉션을 사용해서 책임 분리와 쉽게 테스트할 수 있다는 장점을 얻었습니다!
| this.handlerAdapters = handlerAdapters; | ||
| } | ||
|
|
||
| public ModelAndView handle(HttpServletRequest request, HttpServletResponse response, Object handler) |
There was a problem hiding this comment.
작업 처리의 역할도 HandlerAdapters에게 위임하셨네요? 전 개인적으로 일급 객체인 HandlerAdapters는 조건에 맞는 HandlerAdapter를 반환하는 책임만 가지고 그에대한 테스트만 작성해주면 좋겠다 생각하는데 카피의 HandlerAdapters는
- 조건에 맞는 어뎁터를 찾는다.
- 발견한 어뎁터에 작업을 위임한다.
라는 2개의 책임이 생겨버려서 테스트할 영역이 늘어났다는 느낌이 들어요..! 단순히 필요한 어뎁터만 찾아서 반환해주고 작업은 그 어뎁터에만 시키면 될 거 같다고 생각하는데 카피는 다른 생각을 가지고 계신지 궁금합니다!
There was a problem hiding this comment.
켈리의 의견에 동의해요!
지금 코드는 HandlerAdapters가 두가지 역할을 하고 있네요!
HandlerAdapters는 어뎁터를 찾는 역할만 하도록 수정했습니다!
| ) | ||
| public HandlerExecution findHandler(HttpServletRequest request) { | ||
| return handlerExecutions.get( | ||
| new HandlerKey(request.getRequestURI(), RequestMethod.valueOf(request.getMethod())) |
There was a problem hiding this comment.
개인적으로 HandlerKey 인스턴스를 생성하는 로직은 위로 분리하면 가독성이 더 좋아질거라 생각합니다!
| public Object mapToHandler(HttpServletRequest request) throws ServletException { | ||
| for (HandlerMapping handlerMapping : handlerMappings) { | ||
| Object handler = handlerMapping.findHandler(request); | ||
| if (handler != null) { |
There was a problem hiding this comment.
1, 2 레벨에서 늘 연습했던 객체지향 생활체조의 한 메서드에 오직 한 단계의 들여쓰기만 한다.를 지켜볼까요? 람다 & 스트림을 활용하면 해결할 수 있겠네요!
| manualHandlerMapping.initialize(); | ||
| annotationHandlerMapping.initialize(); | ||
| handlerMappings = new HandlerMappings( | ||
| List.of(new ManualHandlerMapping(), new AnnotationHandlerMapping("com.techcourse.controller")) |
There was a problem hiding this comment.
지금의 DispatcherServlet은 더이상 HandlerMapping의 구체 클래스들을 몰라 좋은데 아래 List.of()에 구체 클래스들이 명시되어 있는게 너무 아쉽네요..!
HandlerMappings와 HandlerAdapters를 초기화해주는 책임을 다른곳에 분리해서 DispatcherServlet이 더이상 구체 클래스에 의존하지 않게 해보는건 어떨까요?
이건 SOLID 원칙중 어디에 해당될까요? 🤔
There was a problem hiding this comment.
DIP원칙에 해당되는 것 같네요! 현재는 dispatcherServlet이 구체 클래스에 의존하고 있는데, 이를 각각 initializer를 생성해 외부에서 주입하도록 코드를 변경했습니다.
DispatcherServletInitializer가 DispatcherSevlet을 새로 생성할 때 주입해주도록 했습니다!
kelly6bf
left a comment
kelly6bf
left a comment
There was a problem hiding this comment.
카피! 제가 요청드린 부분은 모두 적용해주신거 같아 빠른 Approve 드리겠습니다! 3단계 진행해주세요!
2 participants
안녕하세요 켈리~
이것저것 바빠가지고 제출이 늦어졌네요😅
2레벨 힌트 나오기전에 작성했어서 아직 미흡한 부분도 있을 것 같아요!
리뷰 주고받으면서 차근차근 리팩토링 해볼게요!
잘부탁드립니다!