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단계] 다즐(최우창) 미션 제출합니다. #510

Merged
merged 11 commits into from
Sep 24, 2023

Conversation

woo-chang
Copy link
Member

안녕하세요 키아라 !

저는 팀 프로젝트와 미션을 병행하는게 쉽지 않은데, 키아라는 어떠신가요?
그로 인해 1단계 미션이 빠르게 머지됐음에도 2단계 제출이 늦어졌네요 🥲

전체적인 흐름은 아래와 같습니다.

  1. 서블릿 컨테이너가 생성되고, 애플리케이션 컨텍스트가 로드될 때 DispatcherServlet 초기화 작업이 진행됩니다.
    • ManualHandlerMapping, AnnotationHandlerMapping 정보가 등록됩니다.
      • 각각의 핸들러 매핑을 통해 조회되는 핸들러 객체가 다르므로, 핸들러에 알맞은 실행을 처리하는 어댑터 객체가 필요합니다.
    • ControllerHandlerAdapter, HandlerExecutionHandlerAdapter 정보가 등록됩니다.
  2. 요청이 들어오면 HandlerMapping을 순회하며 알맞은 Handler를 조회합니다.
  3. Handler를 바탕으로 HandlerAdapter를 순회하며 알맞은 HandlerAdapter를 조회합니다.
  4. 어댑터 실행을 통해 ModelAndView 결과를 반환하고 이를 클라이언트에게 응답합니다.

미션을 진행하며 몇 가지 고민거리도 존재했습니다!

1. 예외는 어디서 처리하는 게 좋을까?

private Object instantiateClass(Class<?> aClass) {
    try {
        return aClass.getConstructor().newInstance();
    } catch (Exception e) {
        throw new RuntimeException(e);
    }
}

기능을 구현하면서 체크 예외를 처리해야 할 부분들이 종종 있었습니다. 체크 예외를 계속 상위로 넘기다 보면 모든 메서드에 throws Exception이 붙을 것 같아 발생하는 곳에서 런타임 예외로 변환해서 예외를 처리했습니다. 해당 방식을 사용하면 DispatcherServlet의 try-catch에서 걸려 공통으로 ServletException으로 처리되게 됩니다.

키아라는 어떤 방법으로 예외를 처리하셨나요?!

2. DispatcherServlet 패키지 구조

현재 DispatcherServletapp 패키지에 존재합니다. 실제로는 스프링 프레임워크 mvc 패키지에 속해있기에, 현재 패키지 구조에 의문이 들었습니다. 이와 관련해서 같이 얘기해보고 싶네요 🤓


추가적으로 1단계 리뷰 확인 후 커멘트와 반영 완료했습니다! 이번 리뷰도 잘 부탁드립니다 🙇🏻‍♂️

@woo-chang woo-chang self-assigned this Sep 20, 2023
Copy link

@kiarakim kiarakim left a comment

Choose a reason for hiding this comment

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

안녕하세요 다즐🥨

리뷰가 늦어져서 죄송해요ㅠㅠ 구현을 잘 해주셨어요!👍

요구사항엔 없었지만 코드에 처리할 거리를 //todo로 힌트가 주어졌는데 이 부분을 적절하게 채워주면 좋겠어요.
그리고 개인적으로 궁금한 예외던지기..! ControllerScanner와 HandlerExecution에선 RuntimeException을, Mapping과 Adapter Registry에선 IllegalStateException을 던지고 DispatcherServlet에선 ServletException으로 받는 부분이 잘 이해가 가지 않아 이 부분에 대해서 대화를 나눠보면 좋을 것 같습니당~

다즐의 고민거리를 저도 생각해봤어요.

1. 예외는 어디서 처리하는 게 좋을까?

저는 우선 DispatcherServlet에서 try-catch에서 받았어요. 그럼 어디서 던지냐! 다즐과 마찬가지로 HandlerMappingRegistry 와 HandlerAdapterRegistry와 같은 곳에서 ServletException 예외를 통일되게 던져주었어요.

2. DispatcherServlet 패키지 구조

저는 최대한 Spring Framework를 참고하지 않으려고 했어요(귀찮아서 그런거 아니..ㄴ가 암튼) 그래서 이 패키지 고민을 해본 적이 없는데 만약 고민을 해본다면 '스프링이 그렇게 쓰니까'가 이유가 되는 것보단 점진적인 리팩터링을 해보면서 '이러이러한 이유로 DispatcherServlet은 mvc 모듈에 들어가는게 맞다'고 생각하는게 좋을 것 같아요.

금요일은 데모데이가 있어서 시간이 부족할 것 같은데 조금 더 대화를 해보고자 Request Change 할게요. 만약 해당 부분을 반영하고 step3를 진행하기에 시간이 부족하다고 느껴지시면, 말씀해주세요. Approve 하고 step3에서 다시 대화해보면 되니까요!😊

프로젝트와 미션 병향하느라 고생 많으셨습니당~

}

@Override
public void render(final Map<String, ?> model, final HttpServletRequest request, final HttpServletResponse response) throws Exception {
public void render(Map<String, ?> model, HttpServletRequest request, HttpServletResponse response)
throws Exception {
// todo

Choose a reason for hiding this comment

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

요청에 setAttribute를 하기 전 후에 JspView가 해줄 일은 없을까요?
DispatcherServlet의 move 메서드의 역할을 이쪽으로 갖고오면 DispatcherServlet이 렌더의 책임까지 안고가지 않아도 될 것같은데 어떻게 생각하시나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

DispatcherServlet이 가지고 있던 뷰 렌더링 역할을 옮겼더니,

각각이 가져야하는 역할이 더욱 명확하게 보이는 것 같네요 👍

Comment on lines 65 to 73
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 var requestDispatcher = request.getRequestDispatcher(viewName);
RequestDispatcher requestDispatcher = request.getRequestDispatcher(viewName);
requestDispatcher.forward(request, response);
}

Choose a reason for hiding this comment

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

이 부분이요. JspView 코멘트에 적어놨습니다!

Comment on lines +24 to +32
private Map<Class<?>, Object> instantiateControllers(Set<Class<?>> classes) {
return classes.stream()
.collect(Collectors.toMap(
Function.identity(),
this::instantiateClass,
(exist, replace) -> replace,
HashMap::new
));
}

Choose a reason for hiding this comment

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

엄청나게 멋드러진 스트림이네요.👍

Choose a reason for hiding this comment

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

근데 이해는 못한... 혹시 간단쓰 명료쓰하게 설명해주실 수 있나요?ㅠ

Copy link
Member Author

Choose a reason for hiding this comment

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

Controller 어노테이션이 붙은 클래스 목록을 스트림을 통해 순회화면서 key는 해당 클래스가 되고, value는 해당 클래스의 인스턴스를 가지는 Map을 생성하는 스트림입니다!

toMap의 첫번째 인자가 키를 의미하고 두번째 인자가 값을 의미하게 됩니다. Function.identity()는 스트림의 값을 그대로 사용하는 함수를 뜻합니다. 세번째 인자는 merge function을 의미하는데 동일한 키가 존재하는 경우 새로운 값으로 교체하게 됩니다. 마지막 인자를 통해 어떠한 Map을 생성할지 결정할 수 있습니다.

return handlerAdapters.stream()
.filter(handlerAdapter -> handlerAdapter.support(handler))
.findFirst()
.orElseThrow(() -> new IllegalStateException("핸들러에 알맞은 어댑터가 존재하지 않습니다."));

Choose a reason for hiding this comment

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

IllegalStateException을 던지고 ServletException으로 일괄받는다는 의미였을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

맞습니다 ! DispatcherServelt의 try-catch 구문에서 Throwable로 예외를 잡고 있기 때문에 모든 예외를 잡을 수 있어 위와 같은 고민을 하게 되었습니다 😄

Comment on lines 9 to 11
ModelAndView handle(HttpServletRequest request, HttpServletResponse response, Object handler);

boolean support(Object handler);

Choose a reason for hiding this comment

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

별 건 아니지만 통상 support() 메소드로 이 핸들러를 핸들할 수 있는지 확인하고 handle() 메소드를 진행하니까 이 둘의 순서를 바꾸는 것이 더 자연스러울 것 같아용. 참고참고~

try {
return aClass.getConstructor().newInstance();
} catch (Exception e) {
throw new RuntimeException(e);

Choose a reason for hiding this comment

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

핸들러를 못 찾을 땐 IllegatStateException, 클래스의 생성자로 객체 갖고오는 과정에선 RuntimeException을 던져주시는데 이렇게 다르게 해주신 이유가 있나요? 정말 몰라서 묻는 질문입니다..!ㅋㅋ

Copy link
Member Author

Choose a reason for hiding this comment

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

큰 이유는 존재하지 않고, 어떤 예외를 던져야할지 결정이 어려울 때 RuntimeException을 선택하였습니다 !

전체적으로 통일된 예외를 던지도록 수정하는게 좋을 것 같네요 ㅎㅎ

@kiarakim kiarakim merged commit ee07e93 into woowacourse:woo-chang Sep 24, 2023
1 check failed
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