Skip to content

[MVC 구현하기 - 1단계] 케빈(박진홍) 미션 제출합니다. #17

Merged
MyaGya merged 6 commits intowoowacourse:xlffm3from
xlffm3:step1
Sep 13, 2021
Merged

[MVC 구현하기 - 1단계] 케빈(박진홍) 미션 제출합니다. #17
MyaGya merged 6 commits intowoowacourse:xlffm3from
xlffm3:step1

Conversation

@xlffm3
Copy link

@xlffm3 xlffm3 commented Sep 7, 2021

안녕하세요 마갸!
리뷰 잘 부탁드립니다 ^_^

Spring MVC의 경우 ResponseBody를 반환하느냐, ViewName을 반환하느냐에 따라 처리하는 HandlerMapping 및 Adapter가 다른데요.

  • 있는 경우
    • HandlerMapping : RequestMappingHandlerMapping.
    • HandlerAdapter : RequestMappingHandlerAdapter.
  • 없는 경우
    • HandlerMapping : BeanNameUrlHandlerMapping.
    • HandlerAdapter : SimpleControllerHandlerAdapter.

이에 착안하여 HandlerMapping 및 Adapter를 Controller 인터페이스 구현 or 애너테이션 기반으로 세분화하고 DispatcherServlet에 등록했습니다.

레거시 MVC 코드와 애너테이션 기반의 MVC가 모두 공존할 수 있도록 하라는 요구사항이 있었는데요. 기존 레거시 요청을 잘 수행하면서, 애너테이션 기반의 코드 (테스트)가 통과하도록 코드를 구성했습니다.

Copy link

@MyaGya MyaGya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다 케빈! 코드를 꼼꼼히 읽어 보았지만 두 군데 정도밖에 코멘트를 남길 부분이 없었던 것 같아요. 리뷰가 늦어져서 죄송합니다.
피드백은 무조건 수정하실 필요는 없어요. adapter 패턴에 대해서는 모르지만 어떤 역할을 한다라는 개념만 잡은 상태로 남긴 피드백입니다!

Comment on lines +59 to +71
private Object getHandler(HttpServletRequest request) {
return handlerMappings.stream()
.map(handlerMapping -> handlerMapping.getHandler(request))
.filter(Objects::nonNull)
.map(Controller.class::cast)
.findFirst()
.orElseThrow();
.map(handlerMapping -> handlerMapping.getHandler(request))
.filter(Objects::nonNull)
.findAny()
.orElseThrow(IllegalStateException::new);
}

private void move(String viewName, HttpServletRequest request, HttpServletResponse response) throws Exception {
if (viewName.startsWith(JspView.REDIRECT_PREFIX)) {
response.sendRedirect(viewName.substring(JspView.REDIRECT_PREFIX.length()));
return;
}

final RequestDispatcher requestDispatcher = request.getRequestDispatcher(viewName);
requestDispatcher.forward(request, response);
private HandlerAdapter getHandlerAdapter(Object handler) {
return handlerAdapters.stream()
.filter(handlerAdapter -> handlerAdapter.supports(handler))
.findAny()
.orElseThrow(IllegalStateException::new);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 값들은 모두 handlerMappingshandlerAdapters 두 값에 종속된 기능인데요. 일급 컬렉션으로 만들어 줄 수 있지 않을까요?

Comment on lines 21 to +24
dispatcherServlet.addHandlerMapping(new ManualHandlerMapping());

dispatcherServlet.addHandlerMapping(new AnnotationHandlerMapping(BASE_PACKAGE));
dispatcherServlet.addHandlerAdapter(new SimpleHandlerAdapter());
dispatcherServlet.addHandlerAdapter(new AnnotationHandlerAdapter());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handler와 adapter를 넣어주고 있는데
AnnotationHandlerMapping를 사용하기 위해 AnnotationHandlerAdapteadapter 로 존재한다고 읽혀집니다
그런데 ManualHandlerMapping 을 사용하기 위해서는 SimpleHandlerAdapter 를 호출해야 할까요?
네이밍이 조금 모호한 것 같아요
SimpleHandlerAdapter 대신 ManualHandlerAdapter 는 어떠실까요?

@@ -1,7 +1,15 @@
package nextstep.mvc.controller.tobe;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 위치가 @Controller 를 잡아서 처리해주는 어노테이션 기반 기능인 것 같아요. 그런데 이 기능을 확인해 볼 수 있는 페이지나 테스트가 있을까요?

Copy link
Author

@xlffm3 xlffm3 Sep 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트의 package nextstep.mvc.controller.tobe;의 AnnotationHandlerMappingTest입니다.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그렇군요! 런타임에서 확인할 수 있는 페이지가 없어서 확인차 물어봤습니다!

@xlffm3
Copy link
Author

xlffm3 commented Sep 13, 2021

안녕하세요 마갸~ 피드백 반영해서 다시 재요청보냅니다!

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link

@MyaGya MyaGya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인했습니다! 고생많으셨습니다. 다음 리펙토링 미션에서 봐요

@MyaGya MyaGya merged commit 0155bad into woowacourse:xlffm3 Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants