[MVC 구현하기 2단계] 마코(이규성) 미션 제출합니다. #458

aak2075 · 2023-09-17T14:21:32Z

안녕하세요 주드~

2단계 미션으로 Annotation 기반 컨트롤러와 수동으로 등록하는 컨트롤러 둘 다 동작하도록 구현 했습니다.

ManualHandlerMappingAdapter는 HandlerMapping을 구현하는 어댑터이기 때문에 혹시나 HandlerAdapter와 헷갈리지 않길 바랍니다

동작 흐름

kevstevie

안녕하세요 마코 주드임다
추상화 단계 그림 그려주신거 굉장히 감동적이네요
덕분에 쉽게 이해할 수 있었습니다.
코멘트 확인해주시고 의견 나누면 좋을 것 같아요. 다음 리뷰는 더 빨리 해드릴게요.

코멘트 남긴 것 외에 요구사항으로 register, register/view는 어노테이션 방식으로 변경하라는 요구 사항이 있었습니다! 이 부분을 중점으로 테스트 짜주시면 좋을 것 같아요

kevstevie · 2023-09-18T04:29:55Z

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

+        final var manualHandlerMappingAdapter = new ManualHandlerMappingAdapter();
+        final var manualHandlerAdapter = new ManualHandlerAdapter();
+
+        handlerMapping = new CompositeHandlerMapping(manualHandlerMappingAdapter, annotationHandlerMapping);


varargs 사용한건가요? 좋네요

kevstevie · 2023-09-18T07:05:04Z

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

+        try {
+            final Object handler = handlerMapping.getHandler(request);
+            final ModelAndView modelAndView = handlerAdapter.handle(request, response, handler);
+            modelAndView.getView().render(modelAndView.getModel(), request, response);


modelAndView에서 render()를 하지 않은 특별한 이유가 있나요?

말씀해주신대로 ModelAndView에 책임을 넘기는 것이 캡슐화가 깨지지 않고 더 깔끔하네요.

기존 코드를 건드리지 않고 스프링 코드(스프링에선 예외 처리나 로깅을 하지만 제 코드에선 불필요하네용)를 따라가다 보니 불필요하게 꺼내쓴것 같습니다.

kevstevie · 2023-09-18T08:01:33Z

app/src/main/java/com/techcourse/mvc/ManualHandlerAdapter.java

+                               final Object handler) {
+        try {
+            final var controller = (Controller) handler;
+            final var method = controller.getClass().getMethod("execute", HttpServletRequest.class, HttpServletResponse.class);


handler를 캐스팅하고 execute를 호출하지 않고 리플렉션을 통해 메서드를 사용한 이유가 궁금해요

생각이 짧았습니다

kevstevie · 2023-09-18T08:04:51Z

app/src/main/java/com/techcourse/mvc/ManualHandlerMappingAdapter.java

+public class ManualHandlerMappingAdapter implements HandlerMapping {
+
+    private final ManualHandlerMapping manualHandlerMapping;
+
+    public ManualHandlerMappingAdapter() {
+        manualHandlerMapping = new ManualHandlerMapping();
+    }
+


한번 더 추상화되어야하는 이유가 궁금해요
manualHandlerMapping이 바로 handlerMapping을 구현하는 형태면 생기는 문제가 뭐가 있을까요

레거시 코드에 약간의 변경도 허용하고 싶지 않았습니다. 구현을 한다는 것도 변경이고 이로 인해 getHandler의 반환 타입이 Controller -> Object로 바뀌어야 하는데요, 작은 변경이기는 하지만 다른 코드에서 사용하고 있다면 문제가 생길 수도 있고 사실 다른 프레임워크를 적용했는데 시그니처도 같고 구현만 하면 호환이 된다는 것 자체가 좀 와닿지가 않았습니다.
정리하자면 구현으로 간단하게 해결할 수 있지만 실제로 프레임워크를 바꾼다는 생각으로 어댑터를 추가했습니다.
(미션에서 요구한건 컨트롤러만 변경하지 말라는 것인데 지금 봤네요)

좋네요 이런 생각은 전혀 못햇어요

kevstevie · 2023-09-18T08:25:51Z

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

+    public boolean supports(Object handler) {
+        return getAdapter(handler).isPresent();
+    }
+
+    private Optional<HandlerAdapter> getAdapter(Object handler) {
+        return handlerAdapters.stream()
+                .filter(adapter -> adapter.supports(handler))
+                .findAny();
+    }


이 부분 가독성이 좀 떨어지는 것 같아요.. 서로 순환하는 형태같아 보이기도 하고
CompositeHandlerAdapter는 HandlerAdapter의 구현체가 아니어도 될 것 같아요

@Override public boolean supports(Object handler) { return handlerAdapters.stream() .anyMatch(adapter -> adapter.supports(handler)); }

이렇게 빼버리고 getAdapter 메서드 제거했습니다.

CompositeHandlerAdapter는 HandlerAdapter의 구현체가 아니어도 될 것 같다고 생각하신 이유는 무엇인가요?
제 의도는 디스패처서블릿이 적절한 핸들러어댑터에게 요청할 때 디스패처 서블릿이 찾아서 요청하는 것이 아닌 어댑터 내부에서 알아서 찾아서 요청을 처리하도록 하는 것이 책임 분리가 더 적절하다고 생각했습니다.

일급컬렉션같은 역할을 하고 있어서 컬렉션 내부의 객체들과 같은 인터페이스를 의존하는게 어색하다고 생각했었는데 설명을 들으니 괜찮기도 하네요
그리고 메서드가 정리되니 한결 낫네요!

kevstevie · 2023-09-18T08:26:55Z

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

+    }
+
+    @Override
+    public boolean supports(Object handler) {


혹시 이 메서드 사용되고 있나요?

사용되지 않고 있습니다만 인터페이스 구현 때문에 있는거기도 하고 혹시 나중에 밖에서 에러 처리시 필요할 수도 있을것 같습니다.
만약 구현을 안한다면 지울것 같긴 합니다.

kevstevie · 2023-09-18T08:31:02Z

.../test/java/webmvc/org/springframework/web/servlet/mvc/tobe/AnnotationHandlerAdapterTest.java

+
+        // then
+        assertThat(result.getView())
+                .usingRecursiveComparison()


usingRecursiveComparison 쓰는거 좋네요

왜 getName()이 없는건지....

kevstevie · 2023-09-18T08:35:34Z

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

+
+    @Override
+    public boolean supports(Object handler) {
+        return ((HandlerExecution) handler).getHandler().getClass().isAnnotationPresent(Controller.class);


manual handler adapter와 같이 instanceOf 리플렉션으로 확인하는 방법이 일관성있고 좋지 않을까요?
추후에 확장이 있을 때 더 좋을 것 같아요

@Override public boolean supports(Object handler) { if (handler instanceof HandlerExecution) { return ((HandlerExecution) handler).getHandler().getClass().isAnnotationPresent(Controller.class); } return false; }

이러한 구조를 방식을 말씀하신게 맞나요?
확인 없이 캐스팅하면 예외가 발생할것 같긴 하네요

return handler instanceof HandlerExecution;

형태를 말씀드린거 였어요
manual handler adapter도 같은 형태를 하고 있어서요

천재신가요?

aak2075 · 2023-09-19T17:41:27Z

주드 안녕하세요

피드백 덕분에 많이 개선됐네요! 감사합니다.

코멘트에 대한 답변과 잘 모르겠는 부분은 질문 남겼으니 확인해주세요.

register 쪽 테스트는 디스패처 서블릿 테스트로 추가해봤는데 생각보다 어렵네요

kevstevie

안녕하세요 마코 주드입니다!
마코의 답변과 코드 잘 봤습니다.. 저보다 깊은 생각을 하시는 군요
몇개 간단한 코멘트 남겼습니다
그리고 approve를 하려고 했는데 테스트가 깨지는 부분이 있어서 확인하고 수정해주시면 바로 다음 단계 진행해도 될 것 같습닌다!

kevstevie · 2023-09-20T04:04:08Z

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

+        if (handler instanceof HandlerExecution) {
+            return ((HandlerExecution) handler).getHandler().getClass().isAnnotationPresent(Controller.class);
+        }
+        return false;


instanceof HandlerExecution 만으로 true를 반환할 수 있지 않을까요?

좋습니다

kevstevie · 2023-09-20T04:06:02Z

app/src/main/java/com/techcourse/mvc/ManualHandlerMappingAdapter.java

+public class ManualHandlerMappingAdapter implements HandlerMapping {
+
+    private final ManualHandlerMapping manualHandlerMapping;
+
+    public ManualHandlerMappingAdapter() {
+        manualHandlerMapping = new ManualHandlerMapping();
+    }
+


좋네요 이런 생각은 전혀 못햇어요

kevstevie · 2023-09-20T04:08:30Z

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

+    public boolean supports(Object handler) {
+        return getAdapter(handler).isPresent();
+    }
+
+    private Optional<HandlerAdapter> getAdapter(Object handler) {
+        return handlerAdapters.stream()
+                .filter(adapter -> adapter.supports(handler))
+                .findAny();
+    }


일급컬렉션같은 역할을 하고 있어서 컬렉션 내부의 객체들과 같은 인터페이스를 의존하는게 어색하다고 생각했었는데 설명을 들으니 괜찮기도 하네요
그리고 메서드가 정리되니 한결 낫네요!

kevstevie · 2023-09-20T04:09:52Z

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

+
+    @Override
+    public boolean supports(Object handler) {
+        return ((HandlerExecution) handler).getHandler().getClass().isAnnotationPresent(Controller.class);


return handler instanceof HandlerExecution;

형태를 말씀드린거 였어요
manual handler adapter도 같은 형태를 하고 있어서요

aak2075 · 2023-09-20T04:32:07Z

RegisterController를 ManualHandlerAdapter쪽 테스트에서 사용하고 있었는데 이 컨트롤러를 어노테이션 기반으로 변경했더니 깨졌었네요
테스트용 컨트롤러 추가해서 이걸로 테스트하도록 하여 해결했습니다.

kevstevie

좋습니다 3단계에서 만나요!

aak2075 added 7 commits September 13, 2023 14:47

refactor: stream for each 제거

f29a04b

feat: HandlerMapping 추가

564552d

feat: HandlerAdapter 추가

b83e0e6

refactor: 패키지 이동

2198a32

refactor: 불필요한 import 제거

177260f

refactor: 변수 정리

7046e05

refactor: 레거시 MVC HandlerExecution -> Controller 사용하도록 수정

af60665

aak2075 requested a review from kevstevie September 17, 2023 14:21

aak2075 self-assigned this Sep 17, 2023

kevstevie requested changes Sep 18, 2023

View reviewed changes

aak2075 added 3 commits September 20, 2023 01:47

refactor: 피드백 반영

ccd7119

refactor: RegisterController 어노테이션 기반으로 변경

6706838

test: register 요청에 대한 DispatcherServlet 테스트 추가

dabd9d2

kevstevie requested changes Sep 20, 2023

View reviewed changes

aak2075 added 2 commits September 20, 2023 13:24

test: 깨지는 테스트 수정

bfabcac

refactor: instanceof로 핸들러 지원 판단 로직 수정

9535457

kevstevie approved these changes Sep 20, 2023

View reviewed changes

kevstevie merged commit 74ed737 into woowacourse:aak2075 Sep 20, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MVC 구현하기 2단계] 마코(이규성) 미션 제출합니다. #458

[MVC 구현하기 2단계] 마코(이규성) 미션 제출합니다. #458

aak2075 commented Sep 17, 2023

kevstevie left a comment

kevstevie Sep 18, 2023

kevstevie Sep 18, 2023

aak2075 Sep 19, 2023

kevstevie Sep 18, 2023

aak2075 Sep 19, 2023

kevstevie Sep 18, 2023

aak2075 Sep 19, 2023 •

edited

Loading

kevstevie Sep 20, 2023

kevstevie Sep 18, 2023

aak2075 Sep 19, 2023

kevstevie Sep 20, 2023

kevstevie Sep 18, 2023

aak2075 Sep 19, 2023

kevstevie Sep 18, 2023

aak2075 Sep 19, 2023

kevstevie Sep 18, 2023

aak2075 Sep 19, 2023 •

edited

Loading

kevstevie Sep 20, 2023

aak2075 Sep 20, 2023

aak2075 commented Sep 19, 2023

kevstevie left a comment

kevstevie Sep 20, 2023

aak2075 Sep 20, 2023

kevstevie Sep 20, 2023

kevstevie Sep 20, 2023

kevstevie Sep 20, 2023

aak2075 commented Sep 20, 2023

kevstevie left a comment

[MVC 구현하기 2단계] 마코(이규성) 미션 제출합니다. #458

[MVC 구현하기 2단계] 마코(이규성) 미션 제출합니다. #458

Conversation

aak2075 commented Sep 17, 2023

동작 흐름

kevstevie left a 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

aak2075 Sep 19, 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

aak2075 Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aak2075 commented Sep 19, 2023

kevstevie left a 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

aak2075 commented Sep 20, 2023

kevstevie left a comment

Choose a reason for hiding this comment

aak2075 Sep 19, 2023 •

edited

Loading

aak2075 Sep 19, 2023 •

edited

Loading