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 구현하기 - 2단계] 후디(조동현) 미션 제출합니다. #256

Merged
merged 8 commits into from
Sep 26, 2022

Conversation

devHudi
Copy link

@devHudi devHudi commented Sep 26, 2022

안녕하세요, 찬! 🙌
지난번 PR에 좋은 피드백 많이 달아주셨는데, 데모데이 발표 준비 때문에 미션에 시간 할애를 많이 못해서 답변을 못드렸네요 🥲
이번 2단계 미션에 작성해주신 피드백까지 모두 반영해서 올릴게요.
이번 2단계도 잘 부탁드립니다! 😄

@devHudi devHudi requested a review from kimchan123 September 26, 2022 07:48
@devHudi devHudi self-assigned this Sep 26, 2022
@devHudi
Copy link
Author

devHudi commented Sep 26, 2022

#163 (comment)
이 부분에 남겨주신 코멘트가 잘 이해가 가지 않아서 더 자세히 설명해주실 수 있으실까요??

Comment on lines -71 to -76
private void move(final String viewName, final HttpServletRequest request, final HttpServletResponse response)
throws Exception {
if (viewName.startsWith(JspView.REDIRECT_PREFIX)) {
response.sendRedirect(viewName.substring(JspView.REDIRECT_PREFIX.length()));
return;
}
Copy link
Author

Choose a reason for hiding this comment

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

나중에 이부분도 DispatcherServlet이 아닌 View 쪽으로 로직을 넘기면 좋을 것 같아요!

이번 PR에서 로직을 모두 옮겼습니다 😄

Comment on lines +23 to +24
this.url = request.getRequestURI();
this.requestMethod = RequestMethod.valueOf(request.getMethod());
Copy link
Author

Choose a reason for hiding this comment

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

이 부분 inline 처리하면 어떨까요?

반영하였습니다!

}

return handlerExecution;
return handlerExecutions.get(handlerKey);
}
Copy link
Author

Choose a reason for hiding this comment

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

request가 들어오면 DispatcherServlet에 있는 모든 HandlerMapping을 돌며 handle이 가능한지 여부를 확인할텐데 이렇게 특정 HandlerMapping에서 NotFoundException을 터트려버리면 다른 HandlerMapping에서 해당 request를 처리할 수 있는 여지를 남길 수 없을 것 같아요. 모든 HandlerMapping을 전부 순회했는데도 불구하고 찾지 못한다면 그 때 ControllerNotFoundException을 터트리는 것이 어떨까요?

남겨주신 부분이 맞는 것 같습니다! ControllerHandlerMapping 과 동일하게 존재하지 않으면, null 을 반환하게 하고 예외처리는 HandlerMappingRegistry 에서 처리했습니다!

@@ -25,11 +25,12 @@ public void render(final Map<String, ?> model, final HttpServletRequest request,
}

final var requestDispatcher = request.getRequestDispatcher(viewName);
requestDispatcher.forward(request, response);

model.keySet().forEach(key -> {
log.debug("attribute name : {}, value : {}", key, model.get(key));
request.setAttribute(key, model.get(key));
});
Copy link
Author

Choose a reason for hiding this comment

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

model에 저장되어 있는 값을 request에 setting 한 후에 forward가 일어나야 할 것 같아요!

순서 수정했습니다! 실수했네요 😅

Copy link

@kimchan123 kimchan123 left a comment

Choose a reason for hiding this comment

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

후디 이해안된다고 하신 제가 남긴 커멘트는 제가 잘못 생각하고 있었던 부분이에요.. 넘어가주시면 감사하겠습니다 ㅋㅋㅋ ㅠㅠ 2단계 진행하시면서 리팩토링 너무 잘해주셔서 딱히 드릴 피드백이 없네요.. 테스트 코드만 나중에 추가해주시면 좋을 것 같네요! 고생하셨습니다. 3단계가 내일 까지니 빠르게 머지할게요 ㅎㅎ

return new ModelAndView(jspView);
}

@RequestMapping(value = "/register/view", method = RequestMethod.GET)

Choose a reason for hiding this comment

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

이 부분도 /register/view가 아닌 /register로 변경하는 것이 어떨까요? ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

3단계 미션에서 반영했습니다! :)

@kimchan123 kimchan123 merged commit 87030d9 into woowacourse:devhudi Sep 26, 2022
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