[MVC 구현하기 - 2단계] 제이미(임정수) 미션 제출합니다. #492

JJ503 · 2023-09-18T14:55:13Z

안녕하세요 민트.
지난 리뷰 감사합니다!
지난 리뷰에 대한 답변은 해당 pr 코멘트로 작성했습니다. (지난 pr 바로가기)

아직 저는 디스패처 서블릿 구조를 잘 모르기에 2단계 요구 사항에만 맞추어 개발을 진행해 보았습니다.
이번 리뷰도 잘 부탁드립니다!

yujamint

안녕하세요 제이미 리뷰가 너무 늦었죠 다시 한 번 죄송합니다..!
2단계 미션 구현하느라 고생하셨습니다.
몇 가지 리뷰 남겼으니, 확인 부탁드려요.
혹시나 궁금한 부분이 있거나, 이야기 하고 싶은 부분이 있다면 언제든 편하게 말씀해 주세요.

yujamint · 2023-09-21T16:57:15Z

mvc/src/main/java/webmvc/org/springframework/web/servlet/mvc/tobe/HandlerExecution.java

 private final Method method;

- public HandlerExecution(final Class<?> clazz, final Method method) {
- this.clazz = clazz;
+ public HandlerExecution(final Object instance, final Method method) {


저는 이렇게 개선된 것도 좋은 것 같습니다! 그렇게 생각한 이유는 지난 PR 코멘트에 남겼습니다! 굿

yujamint · 2023-09-21T16:58:38Z

app/src/main/java/com/techcourse/ManualHandlerMapping.java

+ if (controllers.containsKey(requestURI)) {
+ return controllers.get(requestURI);
+ }
+
+ return new ForwardController("/404.jsp");


Map의 getOrDefault() 메서드를 활용하면 어떨까요?

좋은 팁 감사합니다!
바로 적용해 보았습니다~

yujamint · 2023-09-21T17:00:38Z

app/src/main/java/com/techcourse/ManualHandlerMapping.java

- controllers.put("/login", new LoginController());
- controllers.put("/login/view", new LoginViewController());
- controllers.put("/logout", new LogoutController());
- controllers.put("/register/view", new RegisterViewController());
- controllers.put("/register", new RegisterController());


기능 요구 사항은 다음과 같이 작성됐습니다.

컨트롤러 인터페이스 기반 MVC 프레임워크와 @MVC 프레임워크가 공존하도록 만들자.
예를 들면, 회원가입 컨트롤러를 아래처럼 어노테이션 기반 컨트롤러로 변경해도 정상 동작해야 한다.

즉 두 방식이 공존해야 하는 것 같습니다. 하지만 현재는 @MVC 프레임워크만 동작할 것 같네요. 공존하도록 수정해보면 어떨까요??

해당 부분까지는 생각하지 못했네요..!
다시 원래대로 돌려둔 후 제대로 돌려둔 후 공존을 제대로 하는지 확인을 위해 요구 사항처럼 register와 관련된 컨트롤러만 @MVC 프레임워크를 사용하도록 수정해 보았습니다.

JJ503 · 2023-09-24T07:40:40Z

안녕하세요 민트!
테코톡, 프로젝트 등 바쁜 와중에 좋은 리뷰들 감사합니다.

미션을 처음에 진행했을 때는 dispatcher sevelt의 구조, 동작 과정 등을 모르기도 했고, 제대로 공부하지 않았기에 정말 돌아가기 많 나 코드를 보여드리게 되어 민망하네요..
힌트가 나온 뒤로 민트의 리뷰와 힌트를 함께 보며 다시 리팩토링 해보았습니다.

그런데 아직 고민 중인 사항 한 가지 있습니다.
코드를 보시면 HandlerAdapterRegistry와 HandlerMappingRegistry에서는 예외를 반환하고 있는 상태인 것을 확인하실 수 있을 것입니다.
사실 해당 부분은 예외 처리를 해보고 싶어 만든 Custom Exception입니다.
처음에 예외 처리를 Dispatcher Servlet의 try-catch 문에서 진행을 할까?라고 생각을 했지만, 적절한지 잘 모르겠어 일단 진행하지 않았습니다.
민트의 의견도 궁금합니다!

yujamint

안녕하세요 제이미. 주말에 2단계 리뷰 반영하느라 고생하셨습니다!
리뷰 잘 반영해 주셨네요. 2단계는 이만 머지하도록 하겠습니다!
코멘트 남긴 부분들에 대해서는 3단계 진행하며 반영하셔도 좋을 것 같습니다.
3단계도 파이팅입니다~

+) 예외 처리에 대한 의견을 남기지 않았네요.
먼저, 고민의 포인트가 어떤 부분인지 궁금합니다.

mvc 모듈에서 예외 처리 vs DispatcherServlet에서 예외 처리
app 모듈 내에서 처리하는 것은 확정 -> DispatcherServlet에서 예외 처리 vs DispatcherServlet에서 처리하지 않고 예외 흘려 보냄. 더 깊은 곳에서 예외 처리

1번

일단 1번의 경우를 가정하겠습니다.

현재 구조에서는 DispatcherServlet이 mvc 모듈의 코드를 사용하고 있습니다. 즉, DispatcherServlet이 mvc 모듈의 클라이언트라고 할 수 있습니다.

이러한 구조에서는 mvc 모듈에서 던지는 예외를 클라이언트가 알맞게 핸들링 하는 것이 적절해 보입니다.
즉, 제이미가 생각하신대로 DispatcherServlet이 처리하는 것이 자연스럽다고 생각합니다.

우리가 스프링을 사용하며 발생하는 예외를 직접 핸들링해서 비즈니스에 맞는 커스텀 예외로 변환하듯이요.

2번

이어서 2번에 대해서 고민하신 거라면,

현재 DispatcherServlet은 mvc 모듈을 통해 크게 다음 두 가지를 수행하고 있는 것 같습니다.

app 모듈에 존재하는 레거시 컨트롤러도 mvc 모듈에 적용될 수 있도록 함
요청을 처리할 수 있는 알맞은 핸들러를 호출, 결과(ModelAndView) 반환

저는 두 가지 모두 컨트롤러까지 예외를 흘릴 필요 없이, DispatcherServlet이 핸들링 해야 한다고 생각합니다.

만약 DispatcherServlet에서 처리하지 않는다면 어디서 처리하는 것이 좋을지에 대해서도 생각이 나지 않는 것 같네요.

아무튼 제 결론은 DispatcherServlet에서 예외 처리하는 것이 적절해 보입니다.
근데 제가 질문의 의도를 잘 파악한 것인지 궁금하네요..!
저도 이번 미션, DispatcherServlet에 대해 깊은 이해를 가지고 있지 않아서 제가 잘못 생각한 부분이 있을 수도 있습니다.
혹시나 제가 잘못 이해한 부분이 있거나, 다르게 생각하는 부분이 있다면 얘기해 주시면 감사하겠습니다!

yujamint · 2023-09-24T07:54:40Z

mvc/src/main/java/webmvc/org/springframework/web/servlet/mvc/HandlerMappingRegistry.java

+
+public class HandlerMappingRegistry {
+
+ final private List<HandlerMapping> handlerMappings = new ArrayList<>();


private과 final의 컨벤션 놓치신 것 같습니다!

으악... 저 구조는 뭐죠... 😱
캐치해 주셔서 감사합니다!

yujamint · 2023-09-24T07:54:59Z

mvc/src/main/java/webmvc/org/springframework/web/servlet/mvc/HandlerAdapterRegistry.java

+
+public class HandlerAdapterRegistry {
+
+ final private List<HandlerAdapter> handlerAdapters = new ArrayList<>();


여기도요~ (private, final)

yujamint · 2023-09-24T07:55:53Z

app/src/main/java/com/techcourse/DispatcherServletInitializer.java

 public void onStartup(final ServletContext servletContext) {
- final var dispatcherServlet = new DispatcherServlet();
+ final HandlerAdapterRegistry handlerAdapterRegistry = initializedHandlerAdapterRegistry();
+ final HandlerMappingRegistry handlerMappingRegistry = initializedHandlerAdapater();
+
+ final var dispatcherServlet = new DispatcherServlet(handlerAdapterRegistry, handlerMappingRegistry);


Registry 클래스들의 생성 책임을 Initializer에게 전가한 것이 좋네요~ 제 코드도 이렇게 수정해야겠습니다 ㅋㅋㅋ

yujamint · 2023-09-24T07:57:04Z

app/src/main/java/com/techcourse/DispatcherServlet.java

 }

 @Override
 public void init() {
- manualHandlerMapping = new ManualHandlerMapping();
- manualHandlerMapping.initialize();
+ handlerMappingRegistry.initialize();


생성은 DispatcherServletInitializer에서 하고, handlerMappingRegistry.initialize()는 DispatcherServlet에서 하는 이유가 있을까요??

큰 이유는 없었고 단순하게, 원래 DispatcherServlet에서 초기화를 진행하기도 했고, 밖에서 초기화하게 되면 했는지 안 했는지 헷갈리지 않을까라는 생각에서 DispatcherServlet에서 진행하게 되었습니다.
추가적으로 DispatcherServletInitializer로 생성을 넘긴 이유는 의존성을 주입받기 위해서기 때문에, 해당 부분만 해결하도록 했습니다..!

yujamint · 2023-09-24T07:57:44Z

app/src/main/java/com/techcourse/DispatcherServletInitializer.java

+
+ private static HandlerMappingRegistry initializedHandlerAdapater() {
+ final HandlerMappingRegistry handlerMappingRegistry = new HandlerMappingRegistry();
+ handlerMappingRegistry.addHandlerMapping(new AnnotationHandlerMapping("com.techcourse.controller"));


사소하지만 패키지 경로를 상수화하면 어떨까요??

요즘 미션을 하면서 상수로 하면 좋을 것들과 아닌 것들에 대한 고민이 있는 상태입니다.
첫 번째 미션을 통해 파일의 경로와 같은 경우는 상수화했을 때 오히려 헷갈릴 수 있을 것 같다는 생각을 했습니다.
그리고 이번 패키지 경로도 마찬가지였습니다.
현재는 경로가 하나지만, basePackage가 더 많아진다면 헷갈리지 않을까라는 생각에 따로 상수 처리를 해주지 않았습니다.
혹시 민트는 이에 대해 어떻게 생각하시나요?

그런 의미에서라면 상수화를 하지 않아도 괜찮을 것 같네요! 그런 고민들을 거친 결과라면 저는 제이미의 생각에 동의합니다. 일리 있네요~

yujamint · 2023-09-24T08:01:51Z

app/src/main/java/com/techcourse/DispatcherServlet.java

+ final Object handle = handlerMappingRegistry.getHandler(request);
+ final HandlerAdapter handlerAdapter = handlerAdapterRegistry.getHandlerAdapter(handle);
+ final ModelAndView modelAndView = handlerAdapter.handle(handle, request, response);
+ modelAndView.getView().render(modelAndView.getModel(), request, response);


ModelAndView 내에 model을 인스턴스 변수로 가지고 있으니, 한 층 더 추상화하여 ModelAndView를 통해서 render()를 하면 어떨까요??

만약 그렇게 수정한다면, 디미터의 법칙을 지키면서 ModelAndView 객체가 model을 가지고 있다는 내부적인 내용을 숨길 수 있을 것 같습니다.

오 좋은 방법이네요!
해당 방법으로 바로 수정했습니다 👍

yujamint · 2023-09-24T08:04:22Z

mvc/src/main/java/webmvc/org/springframework/web/servlet/mvc/HandlerAdapter.java

@@ -0,0 +1,12 @@
+package webmvc.org.springframework.web.servlet.mvc;


mvc / asis/ tobe 패키지에 클래스들이 위치하도록 하셨네요. 그렇게 패키지 구조를 결정하신 이유를 설명해 주실 수 있을까요??

제이미의 생각이 궁금해서 여쭤봅니다!

저는 개인적으로 DispatcherServlet도 app에 있는 것이 맞나?라는 생각을 하게 되었습니다.
현재 mvc 패키지가 따로 있음에도, DispatcherServlet 과 관련된 클래스들은 실제 서비스 로직과 함께 있는 것을 확인할 수 있었습니다.
그래서 저는 오히려 관련된 클래스들을 mvc 패키지로 이동해야 할까 고민 중이었기에, 그 외의 클래스들을 일단 mvc 패키지에 둔 채로 보류해 두었습니다.
실제 Spring 프레임워크 구조를 모르기도 하고, 힌트에서 아래 형태인 것을 확인했습니다.

하지만, 해당 구조가 완벽히 이해가 되지 않아 현재 구조가 되었는데, 이와 관련해 민트의 의견과 민트는 어떻게 진행하셨는지 궁금하네요!

저도 우선 DispatcherServlet이 app 모듈에 존재하는 것이 부자연스럽다는 것에 동의합니다!
스프링 MVC가 서블릿을 기반으로 클라이언트의 요청을 처리하는 모듈이라고 봤을 때, 우리의 요청을 핸들링 해주는 DispatcherServlet은 mvc 모듈에 있는 것이 마땅하다고 생각합니다.

저도 스프링의 구조에 대해 잘 몰라서 힌트에 나온 구조가 완벽하게 이해는 되지 않더라고요 ㅎㅎ;;
제가 이해한 바는, 요청을 핸들링 하는 DispatcherServlet을 각기 다른 인터페이스를 가진 핸들러들에게 지원하기 위해, HandlerMapping과 HnalderAdapter라는 추상화된 인터페이스를 지원합니다.
그렇기 때문에 우리의 app 모듈에 존재하는 레거시 컨트롤러도, 어노테이션 컨트롤러도 모두 인터페이스를 통해 DispatcherServlet에 등록될 수 있는 것이고요!

그래서 핵심은 DispatcherServlet, HandlerMapping, HnalderAdapter 관련 클래스이고 mvc 패키지 내에 속하는 것이 적절하다고 생각했습니다.
그리고 app 내의 레거시 컨트롤러를 지원하기 위한 클래스들은 asis, 새로운 어노테이션 컨트롤러를 지원하기 위한 클래스들은 tobe에 속한다고 생각했습니다.

이미 아시는 내용일 것 같지만, 저에게는 새로웠던 내용들이라 한 번 정리한다고 생각하고 적어봤습니다 ㅋㅋㅋ

@MVC

* feat: Legacy MVC와 @MVC 통합하기 * feat: url을 찾을 수 없는 경우의 예외 처리 추가 * refactor: 메서드 명을 명확하게 수정 * refactor: handler의 인스턴스를 주입 받도록 수정 * refactor: static 제거 * refactor: 기존 코드로 돌려 놓기 * refactor: 특정 컨트롤러만 어노테이션 기반 컨트롤러로 변경 * feat: 핸들러 어뎁터 추가 * feat: 핸들러 매핑 추가 * feat: DispatcherServlet과의 연결 * rename: 패키지 위치 변경 * refacotr: map의 getOrDefault()를 사용하도록 수정

JJ503 added 5 commits September 18, 2023 23:54

feat: Legacy MVC와 @MVC 통합하기

bcbfef2

feat: url을 찾을 수 없는 경우의 예외 처리 추가

ad40d4a

refactor: 메서드 명을 명확하게 수정

8da710b

refactor: handler의 인스턴스를 주입 받도록 수정

87aa6d0

refactor: static 제거

c41f4ee

JJ503 self-assigned this Sep 18, 2023

yujamint requested changes Sep 21, 2023

View reviewed changes

JJ503 added 7 commits September 23, 2023 23:42

refactor: 기존 코드로 돌려 놓기

bb1fac6

refactor: 특정 컨트롤러만 어노테이션 기반 컨트롤러로 변경

8002d0c

feat: 핸들러 어뎁터 추가

9e98267

feat: 핸들러 매핑 추가

3675453

feat: DispatcherServlet과의 연결

83ae783

rename: 패키지 위치 변경

544a95f

refacotr: map의 getOrDefault()를 사용하도록 수정

cf1e67e

yujamint approved these changes Sep 24, 2023

View reviewed changes

yujamint merged commit 7347ffc into woowacourse:jj503 Sep 24, 2023
1 check failed

JJ503 mentioned this pull request Sep 25, 2023

[MVC 구현하기 - 3단계] 제이미(임정수) 미션 제출합니다. #593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MVC 구현하기 - 2단계] 제이미(임정수) 미션 제출합니다. #492

[MVC 구현하기 - 2단계] 제이미(임정수) 미션 제출합니다. #492

JJ503 commented Sep 18, 2023

yujamint left a comment •

edited

Loading

yujamint Sep 21, 2023

yujamint Sep 21, 2023

JJ503 Sep 24, 2023

yujamint Sep 21, 2023

JJ503 Sep 24, 2023

JJ503 commented Sep 24, 2023

yujamint left a comment •

edited

Loading

yujamint Sep 24, 2023

JJ503 Sep 25, 2023 •

edited

Loading

yujamint Sep 24, 2023

yujamint Sep 24, 2023

yujamint Sep 24, 2023

JJ503 Sep 25, 2023

yujamint Sep 24, 2023

JJ503 Sep 25, 2023

yujamint Sep 25, 2023

yujamint Sep 24, 2023

JJ503 Sep 25, 2023

yujamint Sep 24, 2023

JJ503 Sep 25, 2023

yujamint Sep 25, 2023


		public class HandlerMappingRegistry {

		final private List<HandlerMapping> handlerMappings = new ArrayList<>();


		public class HandlerAdapterRegistry {

		final private List<HandlerAdapter> handlerAdapters = new ArrayList<>();

		@@ -0,0 +1,12 @@
		package webmvc.org.springframework.web.servlet.mvc;

[MVC 구현하기 - 2단계] 제이미(임정수) 미션 제출합니다. #492

[MVC 구현하기 - 2단계] 제이미(임정수) 미션 제출합니다. #492

Conversation

JJ503 commented Sep 18, 2023

yujamint left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JJ503 commented Sep 24, 2023

yujamint left a comment • edited Loading

Choose a reason for hiding this comment

1번

2번

Choose a reason for hiding this comment

JJ503 Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujamint left a comment •

edited

Loading

yujamint left a comment •

edited

Loading

JJ503 Sep 25, 2023 •

edited

Loading