[MVC 구현하기 2단계] 성하(김성훈) 미션 제출합니다. #443
Conversation
| public HandlerAdapter getHandlerAdapter(final Object handler) { | ||
| return handlerAdapters.stream() | ||
| .filter(handlerAdapter -> handlerAdapter.isSupport(handler)) | ||
| .findFirst() |
There was a problem hiding this comment.
findFirst()와 findAny() 메소드 중 findFirst() 메소드를 사용하신 이유가 있을까요?
There was a problem hiding this comment.
지원하는 HandlerAdapter를 찾을 때, 여러 개의 HandlerAdapter가 지원되는 경우가 있을텐데
해당 경우에 일관된 HandlerAdapter를 사용하기 위해 findAny()가 아닌 findFirst로
처음 찾는 HadlerAdapter가 반환되도록 우선순위를 설정하여 일관성을 부여했습니다!
그래서 HandlerAdapter를 추가할 때도 AnnotationHandlerAdapter과 ManualHandlerAdapter 중
우선순위가 높다고 생각하는 AnnotationHandlerAdapter을 먼저 추가하여 findFirst()에서 먼저 찾게 구현하였습니다!
There was a problem hiding this comment.
디테일한 부분이 숨겨져 있었군요!
테스트를 할때 일관적인 결과를 받을 수 있겠네요! 👍
| public void init() throws Exception { | ||
| for (HandlerMapping handlerMapping : handlerMappings) { | ||
| handlerMapping.initialize(); | ||
| } | ||
| } |
There was a problem hiding this comment.
init()을 하며, 발생할 수 있는 에외를 처리하지 않고 바로 던지는군요!
혹시, 예외를 처리하지 않고, 밖으로 예외를 던지는 이유가 있을까요?
There was a problem hiding this comment.
생각없이 예외를 던진 것 같네요 ㅠㅠ 꼼꼼한 리뷰 감사합니다!! 👍🏻👍🏻
다시 보니까 HandlerMapper 인터페이스에서 메소드 선언부에 throws Exception으로 예외를 던지고 있는데,
구현체인 AnnotationHandlerMapper와 ManualHandlerMapper 중에
AnnotationHandlerMapper만 예외 처리를 해줘야하는 것을 확인했습니다!
그래서 추상화 측면에서도 좋지 않은 것 같아서 AnnotationHandlerMapper에서 예외를 처리하도록 수정했습니다!!
There was a problem hiding this comment.
저는 주로 Checked 예외일 경우 처리할 수 있으면 바로 처리하거나, 처리할 필요가 없거나 불분명하면 Unchecked 예외로 반환하여 던지는 것을 선호하는 편이에요.
checked 예외를 계속 던지게 된다면 어디서 예외가 발생하는지 헷갈리기도 하고 매번 throw exception을 메소드에 적어줘야 하니 변경의 요소가 전파가 되더라구요!
| private void addInitHandlerMappings() { | ||
| handlerMapper.addHandlerMapping(new AnnotationHandlerMapping(TECH_COURSE_BASE_PACKAGES)); | ||
| handlerMapper.addHandlerMapping(new ManualHandlerMapping()); | ||
| } | ||
|
|
||
| private void addInitHandlerAdapters() { | ||
| handlerAdapters.addHandlerAdapter(new AnnotationHandlerAdapter()); | ||
| handlerAdapters.addHandlerAdapter(new ManualHandlerAdapter()); | ||
| } |
There was a problem hiding this comment.
해당 메소드를 호출하면 핸들러와 핸들러 어뎁터가 추가되겠군요!
하지만 해당 메서드를 여러 번 호출하게 되면 어떻게 될까요?
메서드 이름과 행동이 맞지 않는 것 같기도 하네요...
또한 구체적인 타입을 알고 있을 필요도 없을 것 같아요!
There was a problem hiding this comment.
이 부분은 어떻게 리팩토링할지 감이 안 잡혀서 글렌 코드좀 염탐했습니다,, 죄송합니다 ㅎㅎㅎ;;;
DispatcherSevletInitializer 클래스가 존재했군요!! 있는지조차 몰랐습니다,, 대단하십니다 👍🏻👍🏻👍🏻
해당 클래스에 초기 Handler, Adapter 설정 역할을 위임하니 DispatcherServlet이 확실히 더 책임 분리가 된 느낌이라
좋았습니다!!
감사합니다 글렌 ㅎㅎㅎ
There was a problem hiding this comment.
👍👍 인터페이스를 사용한다면 외부에서 의존성을 주입을 받게 하는 편이 더 유연한 설계를 가질 수 있게 하는 것 같아요!
| @RequestMapping(value = "/register", method = RequestMethod.POST) | ||
| public ModelAndView save(final HttpServletRequest request, final HttpServletResponse response) { | ||
| final var user = new User(2, | ||
| request.getParameter("account"), | ||
| request.getParameter("password"), | ||
| request.getParameter("email")); | ||
| InMemoryUserRepository.save(user); | ||
|
|
||
| return new ModelAndView(new JspView("redirect:/index.jsp")); | ||
| } |
There was a problem hiding this comment.
기존에 구현되어 있던 execute() 메서드와 동일한 코드가 계속 중복적으로 발생하네요!
@RequestMapping 메서드에서 추가적인 로직이 없다면 execute() 메소드를 그대로 사용해도 됬을 것 같은데...
중복 코드를 작성해주신 이유가 있을까요?
There was a problem hiding this comment.
현재는 미션 상으로 컨트롤러 인터페이스 기반 MVC 프레임워크와 @MVC 프레임워크가 공존하게 되었는데요!
공존하긴 하지만 그래도 독립적인 어노테이션 기반 메소드라고 생각해서
컨트롤러 인터페이스 기반 MVC 프레임워크 메소드와 로직은 같지만 의존적으로 해당 메소드의 로직을 재사용하지 않고
독립적으로 작성했습니다!
There was a problem hiding this comment.
중복을 피해야 하는 것은 맞지만, 이유가 있다면 굳이 피할 필요가 없다고 생각되네요!
| import webmvc.org.springframework.web.servlet.mvc.tobe.exception.HandlerNotFoundException; | ||
| import webmvc.org.springframework.web.servlet.mvc.tobe.handler.HandlerMapping; | ||
|
|
||
| public class HandlerMapper { |
There was a problem hiding this comment.
HandlerAdpater의 일급 컬렉션의 이름은 HandlerAdpaters던데, HandlerMapping의 일급 컬렉션은 HandlerMappings가 아니네요!
다음과 같이 HandlerMapper라고 이름을 지어주신 이유가 있을까요?
There was a problem hiding this comment.
Adapters 네이밍은 해당 클래스가 주체적으로 Adapt를 해주는 느낌이 들었는데,
HandlerMappings 네이밍은 단순 일급 컬렉션 느낌이 들어서 Mapper로 하게 되었습니다!
그런데 일급 컬렉션이라는 의미로 복수형으로 하는게 맞는 것 같아서 HandlerMappers로 하고,
HandlerMapping도 HandlerMapper로 변경하도록 하겠습니다!
감사합니다!!
There was a problem hiding this comment.
ㅎㅎㅎ 처음에 HandlerMapping의 일급 컬렉션이 어딘가 있을텐데 찾다가 HandlerMappings 클래스가 없는 것을 보고 갸우뚱 했네요.
이런 상황이 있기 때문에 항상 일관적인 이름을 가지게 하는 것이 중요하다고 생각해요!
이름만 보고 유추할 수 있기 때문에, 클래스 이름과 내용이 다르게 된다면 혼란이 올 수 있거든요!
|
글렌 꼼꼼한 리뷰 감사합니다!! |
안녕하세요 글렌!! 성하입니다!! ㅎㅎㅎ 1단계 리뷰 너무 잘해주셔서 감사합니다! 2단계도 잘부탁드려요!
2단계 요구사항이
컨트롤러 인터페이스 기반 MVC 프레임워크와 @MVC 프레임워크가 공존하도록 만들자.였는데요!그래서 컨트롤러에 공존하도록 구현하였습니다! (레거시와 공존하는게 재밌네요 ㅎㅎㅎ)
요청 시 흐름은 다음과 같습니다!
구현하느라 힘들어서 좀 놓친 부분이 있을 수도 있을 것 같아요 ㅠㅠ 미리 죄송합니다 ㅎㅎ,,,
잘부탁드려요!! 🙇🏻♂️