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단계] 도기(김동호) 미션 제출합니다. #487

Merged
merged 16 commits into from
Sep 24, 2023

Conversation

kdkdhoho
Copy link

@kdkdhoho kdkdhoho commented Sep 18, 2023

안녕하세요 두둠! 2단계 미션 제출합니다.
전에 스프링 MVC 구조에 대해 정말 간단히 공부하고 넘어가서 그런지, 이번 미션이 너무 어렵게 느껴졌네요 😢
그래서 다시 공부하고 적용해봤습니다. 많이 부족할텐데 따끔한 리뷰 부탁드립니다. 감사합니다 !

공유드리고 싶은 사항

  • HandlerMappingRegistry(핸들러 매핑), HandlerAdapterRegistry(핸들러 어댑터) 이 두 객체를 만들어 DispatcherServlet가 필드로 가지도록 했습니다.
  • 테스트 코드로 미션 요구 사항을 만족했는지 확인했어요.
    직접 실행시켜 동작하는지 확인하고 싶었지만, JSP는 3단계 요구사항인 것 같아 남겨두었습니다.
    특히, controller 패키지 아래에 AnnotatedControllerForTest를 두어 핸들러 매핑이 컨트롤러를 잘 가져오는지 테스트도 해봤습니다.
  • 이번 미션을 진행하며 예외 처리때문에 너무너무 힘들었어요.
    특히, 핸들러 매핑에 등록된 핸들러들 중에서 requestUri가 중복되는 경우 404 페이지로 보내고 싶었는데요. 이 부분을 어디서 어떻게 처리하는 게 좋을까 고민이 너무 많았습니다. 그러다보니 구현에 진전이 없어 일단 예외 처리는 나중에 생각하기로 결정하고 일단 구현했어요.
    이 부분에 대해서 두둠은 어떻게 하실건지 궁금합니다!

@kdkdhoho kdkdhoho self-assigned this Sep 18, 2023
Copy link

@younghoondoodoom younghoondoodoom left a comment

Choose a reason for hiding this comment

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

안녕하세요 도기~! 리뷰가 늦어서 죄송합니다ㅠㅠ

이번 미션을 진행하며 예외 처리때문에 너무너무 힘들었어요.
특히, 핸들러 매핑에 등록된 핸들러들 중에서 requestUri가 중복되는 경우 404 페이지로 보내고 싶었는데요. 이 부분을 어디서 어떻게 처리하는 게 좋을까 고민이 너무 많았습니다. 그러다보니 구현에 진전이 없어 일단 예외 처리는 나중에 생각하기로 결정하고 일단 구현했어요.
이 부분에 대해서 두둠은 어떻게 하실건지 궁금합니다!

저는 requestUri가 중복된다면 애초에 어플리케이션이 실행되면 안된다고 생각합니다. 그래서 HandlerMappingRegistry 생성 시 중복되는 requestUri를 체크하는 로직이 있으면 해결 될 것 같습니다.

최대한 기존의 코드를 건드리시지 않고 구현하신게 인상적이네요! 테스트도 꼼꼼하게 짜신 것 같습니다. 구조도 제가 했던 방식과 거의 비슷해서 읽기 쉬웠던 것 같아요!
코드가 짧아서 리뷰 드릴 내용이 많지는 않네요! 참고만 부탁드립니다~! 고생하셨습니다 도기🙇‍♂️

public ModelAndView handle(Object handler, HttpServletRequest request, HttpServletResponse response) throws Exception {
Controller controller = (Controller) handler;
String viewName = controller.execute(request, response);
return new ModelAndView(new JspView(viewName));

Choose a reason for hiding this comment

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

controller는 JspView만 사용한다는 가정인것 같아요! 좀 더 유연하게 처리할 수 있을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

말씀하신 부분 완전 이해가서 개선해보려고 여러 방법을 적용해봤는데요.

ViewAdapter를 따로 두어 viewName으로 View 구현체를 만들어 반환하는 식으로 구현했어요.
하지만, 다양한 view 구현체가 늘어남에 따라 ViewAdapter도 수정이 필요하기에 보완이 필요한 방법임을 인지하고 있는데요.

리플렉션을 이용해서 유연하게 생성하고 싶었는데, JspView와 JsonView를 생성하는 방법에 있어 차이가 또 있더라구요,,
제 짧은 지식으로는 현재 구조에서 최선의 방법이 생각이 나지 않는데, 두둠이라면 위 부분을 어떻게 개선하실건지 여쭤보고 싶습니다!

Choose a reason for hiding this comment

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

저라면 View 인터페이스에 support 같은 메소드를 만들고, ControllerHandlerAdaptor에 View 리스트를 di 받을 것 같습니다! 특정 View의 support가 true라면 그 View를 사용하도록 할 것 같아요!

Copy link
Author

@kdkdhoho kdkdhoho Sep 25, 2023

Choose a reason for hiding this comment

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

그렇게 되면 support하는 기준은 확장자명이 될까요?
그렇다면 viewName에 확장자명이 없을 땐 어떻게 되나요? 그리고 응답이 페이지가 아닌 문자열이라면 그냥 텍스트일지, json일지 어떻게 구분하죠??

스프링 MVC 구현 굉장히 어렵네요 😂

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

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

46.4% 46.4% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

Copy link

@younghoondoodoom younghoondoodoom 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단계에서 뵙겠습니다!!🙇‍♂️

@younghoondoodoom younghoondoodoom merged commit 0811383 into woowacourse:kdkdhoho Sep 24, 2023
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