[MVC 구현하기 - 1단계] 매튜(김재연) 미션 제출합니다. #424

kpeel5839 · 2023-09-14T05:25:17Z

hello 헤이나!

헤이나에게 코드리뷰를 받을 수 있다니 이것은 너무나도 행복한 일입니다.

너무 한 게 없어서 따로 설명드릴 게 없네요!

하하하하하하!

리뷰 잘 부탁드립니다~!

hyena0608

매튜매튜 1단계 고생하셨어요!

코드 보면서 제 코드를 수정해야겠다는 생각이 들었어요 ㅋㅋㅋㅋ
바로 머지해도 되지만!! 매튜의 맛있는 의견을 들어보고 싶네요 👍

hyena0608 · 2023-09-14T06:37:33Z

study/src/test/java/reflection/Junit4TestRunner.java

 import org.junit.jupiter.api.Test;

 class Junit4TestRunner {

    @Test
    void run() throws Exception {
        Class<Junit4Test> clazz = Junit4Test.class;
+        Junit4Test junit4Test = clazz.getConstructor().newInstance();


저는 처음에 생성자를 불러오는 방법을 몰라서 직접 생성해서 테스트 했었는데!! 매튜매튜 굿
매튜 코드보고 나니까 오히려 제 PR 수정할게 보이네요 👍

알러뷰에용..

hyena0608 · 2023-09-14T06:42:48Z

study/src/test/java/reflection/Junit3TestRunner.java

+        for (Method method : clazz.getDeclaredMethods()) {
+            if (method.getName().startsWith("test")) {
+                method.invoke(junit3Test);
+            }
+        }


test로 시작하는 메서드가 startsWith을 의도 했던거군요!! 👍

hyena0608 · 2023-09-14T06:53:05Z

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

+    private final Object instance;
+    private final Method method;


오호 매튜는 Object(Controller)를 생성자를 주입받으셨네요~

저는 AnnotationHandlerMapping에 try-catch가 생겨서 HandlerExecution의 handle 메서드에서 throw 해주는 방식으로 해봤어요!

public ModelAndView handle(final HttpServletRequest request, final HttpServletResponse response) throws Exception { final Object controller = method.getDeclaringClass() .getDeclaredConstructor() .newInstance(); return (ModelAndView) method.invoke(controller, request, response); }

제 코드는 이런 느낌이었는데 try-catch가 외부에서 사리진 대신에 HandlerExecution의 handle이 호출될 때 리플랙션으로 클래스를 정보를 가져와 생성해서 진행하는 부분이 마음에 걸리더라고요. 🥲

현재 AnnotationHandlerMapping이 이후에 계속해서 생성되는 코드라면 매튜나 저의 코드가 비슷한 비용을
내겠지만 AnnotationHandlerMapping이 단 한 번만 생성된다면 매튜의 코드가 더 효율적이겠네요!

매튜는 HandlerExecution에 Object(Controller)를 가지고 계신 이유가 뭔지 궁금하네요~

긴 질문 정말 감사합니다!

애초에 method 에서 다시 instance 를 추출 해낼 수 있는지도 몰랐구요, 별 생각이 없이 그렇게 하기도 했습니다.

다만 조금 생각해보니, 헤이나가 말씀해주신 방법대로 하게 되면 말씀해주신 것처럼 handle 메서드가 호출될 때마다 새로운 instance 가 생성된다는 단점이 있을 것 같아요.

하지만 try-catch 를 initilize 메서드에서 사용하지 않아도 된다는 점이 굉장히 매력적으로 느껴지네요... 어떻게 해야할까요?

ㅋㅋㅋㅋ 저는 매튜 방식으로 코드 수정하게 됐습니다. 😎
저는 try-catch를 잘 사용하지 않았는데 (예외 핸들링을 무조건 해줘야할거 같아서)
이번 미션 진행하면서 try-catch로 예외 발생 지점을 명확하게 표현하는게 더 낫겠다고 생각하게 됐어요!

제가 커멘트 남긴 방식은 결국 HandlerExecution에 handle 메서드단에서 throws 하고 있어서 결국에 어디선가 예외를 잡아야하고
어떤 예외였는지 빠르게 확인하기도 어려울거 같아요.

괜한 커멘트를 달은건가 싶어서 죄송하네요 🥲
그래서 저는! 매튜매튜의 기존 방식이 좋은거 같다고 생각해요~

알러뷰 헤이나

hyena0608 · 2023-09-14T06:54:57Z

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

+        Set<Class<?>> classes = new Reflections(basePackages)
+                .getTypesAnnotatedWith(Controller.class);
+
+        parseClasses(classes);


내부에서 핸들러 정보를 put하는데 parseClasses라는 메서드 이름만 본다면 눈치채지 못할 수도 있을거 같다고 생각해요!

네이밍이 정말 힘들어부러요... 감사합니다

hyena0608 · 2023-09-14T07:05:08Z

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

+    public Object getHandler(final HttpServletRequest request) {
+        HandlerKey key = new HandlerKey(
+                request.getRequestURI(),
+                RequestMethod.valueOf(request.getMethod())


잘못된 HttpMethod가 들어올 수도 있으니 예외 처리를 해주면 더 좋을거 같다는 생각입니다!

맞네요 감사합니다! 헤나!

hyena0608 · 2023-09-14T07:09:28Z

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

+    private void registerHandlerExecution(Object controller, List<Method> declaredMethods) {
+        for (Method method : declaredMethods) {
+            putHandlerExecution(controller, method);
+        }
+    }


취향의 차이라고 생각하지만 registerHandlerExecution, putHandlerExecution 등 비슷한 메서드명이 많아서 헷갈릴 수도 있지 않을까?라는 생각이 들었어요.
물론 매튜가 어떤 이유로 이런 식으로 나뉘게 되었는지 그리고 어떤 작업을 하고 있는지 잘 읽힙니다!

맞아요 사실 이 부분을 메서드 분리하면서 네이밍이 되게 애매하다는 생각이 들었어요...

네이밍 능력이 너무 떨어져서.. 혹시 추천 해주실 네이밍이 있을까요?

진짜 네이밍 굉장히 어렵네요 ㅠㅠ 😫

제가 감히 한 번 열심히 생각해봤을 때
컨트롤러를 기준으로 보는 putHandlerExecutionsByController와
메서드를 기준으로 보는 putHandlerExectionByMethod라고 할까라는 생각이 들었어요..!!

단순히 제가 생각한 메서드명일 뿐이니 매튜매튜가 이것말고도 다른 메서드명으로 수정하셔도 혹은 하지 않으셔도 좋을거 같습니다 👍👍👍

알러뷰 헤이나

hyena0608 · 2023-09-14T07:14:28Z

study/src/test/java/reflection/ReflectionTest.java

+        final Constructor<?> firstConstructor = questionClass.getConstructor(
+                String.class,
+                String.class,
+                String.class
+        );
+        final Constructor<?> secondConstructor = questionClass.getConstructor(
+                String.class,
+                String.class,
+                String.class
+        );


secondConstructor가 Question의 두 번째 생성자가 아니었을까하는 생각입니다!
근데 검증 부분은 첫 번째 생성자로 충분해서 테스트 자체가 어떤 의도였는지는 저도 헷갈리네요 🤣

저도 사실 처음에는 두번째 생성자로 작성했었는데, 필요가 없어서 바꿨었어요!

테스트 코드까지 봐주시다니... 그저 빛 헤이나..

hyena0608

매튜매튜 1단계 고생하셨어요!

코드 보면서 제 코드를 수정해야겠다는 생각이 들었어요 ㅋㅋㅋㅋ
바로 머지해도 되지만!! 매튜의 맛있는 의견을 들어보고 싶네요 👍

(request changes해야 알림가는데 comment로 해서 다시 달았어요 🥲)

kpeel5839 · 2023-09-14T16:34:22Z

헤이나 꼼꼼한 리뷰 너무 감사해요!

감동 받아 부라쓰요!

이번에도 잘 부탁드립니다!

헤이헤이나

hyena0608

매튜매튜~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
피드백 반영해주시고 커멘트도 하나하나 다 읽고 이모지까지 눌러주셔서 감사합니다! 😊
고민해주신 부분이 잘 느껴져서 리뷰하는 동안 정말 재밌었네요.

살짝 커멘트 남겼는데 읽어주시면 감사하겠습니다!

1단계 미션은 머지하겠습니다!!
다음 2단계도 화이팅이에요 매튜매튜~ 🚀

hyena0608 · 2023-09-14T23:34:37Z

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

+        } catch (Exception exception) {
+            log.error("예외 발생 {0}", exception);
+        }


예외 로깅 굿굿굿~ 이후에 핸들러 매핑 정보를 주입하다가 예외가 발생하면 어떤 이유인지 분석하기 좋겠네요!
생각해보니 등록 과정에서 예외가 발생하면 해당 애플리케이션이 띄어지면 안될거 같다는 생각이 들었어요 🤔

hyena0608 · 2023-09-14T23:35:12Z

mvc/src/main/java/web/org/springframework/web/bind/annotation/RequestMethod.java

+    public static RequestMethod from(String input) {
+        try {
+            return valueOf(input);
+        } catch (IllegalArgumentException illegalArgumentException) {
+            throw new IllegalArgumentException("잘못된 Method 입니다.");
+        }
+    }


valueOf 가 NPE도 나올 수 있더라고요!

hyena0608 · 2023-09-14T23:35:58Z

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

+        Set<Class<?>> classes = new Reflections(basePackages)
+                .getTypesAnnotatedWith(Controller.class);
+
+        putHandlerExecutionsByController(classes);


저의 의견을 반영해주시다니 🥹🥹🥹
감사합니다 맽유맽유~

kang-hyungu and others added 7 commits September 11, 2023 15:29

패키지 위치 변경 및 코드 정리

52ee778

서블릿 학습 테스트 코드 개선

e7c21ab

test : reflection 학습 테스트 작성

23228a7

test : Servlet, Filter 학습 테스트 완료

526a4b3

feat : HandlerExecution 에 instance 와 method 를 인스턴스로 추가

e83697d

feat : 요청으로 들어온 Request URI 와 Method 에 따라 그에 맞는 Handler 를 반환하는 기능 추가

665d272

refactor : class 추출 로직 변경 및 메서드 분리

9de59d6

kpeel5839 requested a review from hyena0608 September 14, 2023 05:25

kpeel5839 self-assigned this Sep 14, 2023

hyena0608 reviewed Sep 14, 2023

View reviewed changes

hyena0608 requested changes Sep 14, 2023

View reviewed changes

kpeel5839 added 3 commits September 15, 2023 01:29

refactor : 잘못된 Http Method 가 들어오는 경우 예외 추가

262e67a

refactor : handlerExecutions 예외 발생 방식 수정

22a1870

refactor : Method 네이밍 수정

e5b8038

hyena0608 approved these changes Sep 14, 2023

View reviewed changes

hyena0608 merged commit 3a07351 into woowacourse:kpeel5839 Sep 14, 2023
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MVC 구현하기 - 1단계] 매튜(김재연) 미션 제출합니다. #424

[MVC 구현하기 - 1단계] 매튜(김재연) 미션 제출합니다. #424

kpeel5839 commented Sep 14, 2023

hyena0608 left a comment

hyena0608 Sep 14, 2023

kpeel5839 Sep 14, 2023

hyena0608 Sep 14, 2023

kpeel5839 Sep 14, 2023

hyena0608 Sep 14, 2023

kpeel5839 Sep 14, 2023

hyena0608 Sep 14, 2023

kpeel5839 Sep 14, 2023

hyena0608 Sep 14, 2023

kpeel5839 Sep 14, 2023

hyena0608 Sep 14, 2023

kpeel5839 Sep 14, 2023

hyena0608 Sep 14, 2023

kpeel5839 Sep 14, 2023

hyena0608 Sep 14, 2023

kpeel5839 Sep 14, 2023

hyena0608 Sep 14, 2023

kpeel5839 Sep 14, 2023

hyena0608 left a comment

kpeel5839 commented Sep 14, 2023 •

edited

Loading

hyena0608 left a comment •

edited

Loading

hyena0608 Sep 14, 2023

hyena0608 Sep 14, 2023

hyena0608 Sep 14, 2023

[MVC 구현하기 - 1단계] 매튜(김재연) 미션 제출합니다. #424

[MVC 구현하기 - 1단계] 매튜(김재연) 미션 제출합니다. #424

Conversation

kpeel5839 commented Sep 14, 2023

hyena0608 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

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

hyena0608 left a comment

Choose a reason for hiding this comment

kpeel5839 commented Sep 14, 2023 • edited Loading

hyena0608 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

kpeel5839 commented Sep 14, 2023 •

edited

Loading

hyena0608 left a comment •

edited

Loading