Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MVC 구현하기 - 1단계] 파워(송재백) 미션 제출합니다. #396

Merged
merged 9 commits into from
Sep 16, 2023

Conversation

thdwoqor
Copy link

@thdwoqor thdwoqor commented Sep 13, 2023

안녕하세요 이레

AnnotationHandlerMappingTest의 테스트 통과가 요구사항인줄모르고
app 모듈도 건드렸는데 1단계 부분만 리뷰해주셔도됩니다,,,

잘 부탁 드립니다!!

1단계 범위

Copy link

@zillionme zillionme left a comment

Choose a reason for hiding this comment

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

안녕하세요 파워!
리뷰어 이레입니다.

리뷰 요청에 비해 늦은 리뷰를 드리게 되었네요ㅠ
부족한 실력이지만 여러모로 얘기 나눠볼 것이 있는지 꼼꼼히 살펴 보았는데, 1단계에서 특별히 말씀드릴 코드가 없었습니다👍🏻👍🏻
제가 미션 2-3단계도 바로 시작할 것 같지 않은데, 코드 읽는 속도가 빠르지 않아서, 혹시 리팩토링 하시고 싶은 게 있으시다면 하셔도 좋을 것같아요!

Comment on lines +53 to +55
private boolean isReturnType(final Method method) {
return method.getReturnType().equals(ModelAndView.class);
}

Choose a reason for hiding this comment

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

method의 반환 타입이 ModelAndView인지 확인...👍🏻👍🏻

}

private boolean isSameParameter(final Method method) {
method.getReturnType().equals(ModelAndView.class);

Choose a reason for hiding this comment

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

위의 isReturnType과 중복되는 부분 같습니다.

method.getReturnType().equals(ModelAndView.class);
List<Class<?>> parameterTypes = List.of(method.getParameterTypes());
return parameterTypes.containsAll(List.of(HttpServletRequest.class, HttpServletResponse.class));
}

Choose a reason for hiding this comment

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

컨트롤러 메서드의 파라미터 타입을 정해주신 이유가 있나요? 후에 ArgumentResolver이 사용된다면, 컨트롤러 메서드의 매개변수가 HttpServletRequest.class, HttpServletResponse.class는 아닐 수 도 있을 것 같아서요.

Copy link
Author

Choose a reason for hiding this comment

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

만약 TestController에 파라미터를 가지지않는 PUT 메서드가 추가되고

@RequestMapping(value = "/put-test", method = RequestMethod.PUT)
    public ModelAndView post() {
        log.info("test controller put method");
        final var modelAndView = new ModelAndView(new JspView(""));
        return modelAndView;
    }
@Test
    void put() throws Exception {
        final var request = mock(HttpServletRequest.class);
        final var response = mock(HttpServletResponse.class);

        when(request.getAttribute("id")).thenReturn("gugu");
        when(request.getRequestURI()).thenReturn("/put-test");
        when(request.getMethod()).thenReturn("PUT");

        final var handlerExecution = (HandlerExecution) handlerMapping.getHandler(request);
        final var modelAndView = handlerExecution.handle(request, response);
        System.out.println(modelAndView);
    }

테스트를 진행한다면 HandlerExecution.handle 에서는
final HttpServletRequest request, final HttpServletResponse response 해당 파라메터를 가지지만 실제 PUT 메서드는 파라미터를 가지지않아서
wrong number of arguments 예외가 발생합니다.
그래서 HandlerExecution 클래스에서 처리가능한 메서드만 등록하기위해서 파라미터가 동일한지 검증했습니다

Choose a reason for hiding this comment

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

실제 Method 객체가 실제로 매개변수를 갖지 않는 메서드일때,
매개변수를 인자로 넘겨주면 illegalArgumentException이 발생해서 이기 때문이군요!
생각하지 못했던 예외 처리였던것 같습니다. 👍🏻

Copy link

@zillionme zillionme left a comment

Choose a reason for hiding this comment

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

제 질문에 대한 답글 잘 읽었습니다.
남겨주신 테스트를 실제로 돌려보면서 코드를 더 정확히 이해할 수 있었던 것 같아요.
1단계는 여기서 머지하도록 하겠습니다.

method.getReturnType().equals(ModelAndView.class);
List<Class<?>> parameterTypes = List.of(method.getParameterTypes());
return parameterTypes.containsAll(List.of(HttpServletRequest.class, HttpServletResponse.class));
}

Choose a reason for hiding this comment

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

실제 Method 객체가 실제로 매개변수를 갖지 않는 메서드일때,
매개변수를 인자로 넘겨주면 illegalArgumentException이 발생해서 이기 때문이군요!
생각하지 못했던 예외 처리였던것 같습니다. 👍🏻

@zillionme zillionme merged commit 7aea419 into woowacourse:thdwoqor Sep 16, 2023
1 check failed
thdwoqor added a commit to thdwoqor/jwp-dashboard-mvc that referenced this pull request Sep 16, 2023
파워!
고쳐주신 내용과 답변 잘 읽었습니다!
특히, 예시로 들어주신 테스트를 실제로 돌려보면서 코드를 더 정확히 이해할 수 있었던 것 같아요.
1단계는 여기서 머지하도록 하겠습니다.
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.

3 participants