[MVC 구현하기 - 1단계] 홍실(홍혁준) 미션 제출합니다. by hong-sile · Pull Request #410 · woowacourse/java-mvc

hong-sile · 2023-09-14T01:11:49Z

안녕하세요 하디

mvc 미션 1단계 리뷰 요청드립니다!

하디의 리뷰 기대하고 있어요!

잘 부탁드립니다!!

구구가, 코드 정리한 부분에서, FileChanaged가 많이 발생하더라고요.

미션 관련해서 구현한 부분은 커밋하나 라서 해당 부분만 확인해주시면 될 것 같아요.
이해 안 되시는 부분은 편하게 질문 주세요!

미션 구현부분 코드

jundonghyuk

안녕하세요 홍실 !

리뷰어 하디입니다.

홍실 코드를 리뷰하게 되어 영광입니다.

워낙 변경 클래스가 적어 구조적인 측면보다는 홍실의 생각을 듣고싶은 부분 위주로 리뷰를 남겼습니다 ㅎㅎ

고생하셨어요 !

이해가 안되는 리뷰가 있다면 DM 언제든지 주세요~

jundonghyuk · 2023-09-14T13:22:37Z

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

 public class HandlerExecution {

-    public ModelAndView handle(final HttpServletRequest request, final HttpServletResponse response) throws Exception {
-        return null;
+    private final Method method;
+
+    public HandlerExecution(final Method method) {
+        this.method = method;
+    }
+
+    final ModelAndView handle(final HttpServletRequest request, final HttpServletResponse response)
+        throws Exception {
+        final Object object = method.getDeclaringClass().getConstructor().newInstance();
+        return (ModelAndView) method.invoke(object, request, response);


HandlerExecution은 AnnotaionHandlerMapping의 initialize가 호출 되었을 때 초기화 되는 것 같아요.

일반적인 상황이라면 initialize는 딱 1번 호출이 될 것으로 예상이되는데요.

HandlerExecution은 그럼 딱 1번만 생성이 돼서 돌려쓰기가 가능할 것 같습니다.

하지만 현재 handle 메서드를 보면 주입받은 method를 통해 호출될 때 마다 DeclaringClass의 instance를 생성하고 있는
데요 ! 물론 GC가 잘잡아줄것같지만 ..

처음 HandlerExecution을 생성할 때 해당 메서드의를 선언한 클래스의 instance까지 필드로 갖고 있었다면, 매번 생성을 안해줘도 될 것 같아요 !

물론 해당 class의 instance는 한 번만 생성되어 호출되기 때문에 상태를 가지면 안될것 같다는 입장입니다!

제가 생각하지 못한 부분이 있을 수도 있어서 홍실의 의견 또한 듣고 싶어요 ㅎㅎ

오 생각을 못한 부분이네요. 하디 말처럼 DeclaringClass의 instance를 필드로 잡아주면 더 좋을 것 같습니다.

아무리 GC가 잘 정리해준다고 하더라도, 최적화할 것이 보이는데 안 할 이유도 없고, 단순 클래스 객체기 때문에 상태가 변경될 일도 없고요.

필드로 갖고있는 방향으로 리팩터링 해보겠습니다.

jundonghyuk · 2023-09-14T13:26:25Z

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

+
+        final Map<HandlerKey, HandlerExecution> handlerExecutions = new Reflections(basePackage)
+            .getTypesAnnotatedWith(Controller.class)
+            .stream()
+            .map(Class::getMethods)
+            .flatMap(Arrays::stream)
+            .filter(method -> Objects.nonNull(method.getAnnotation(RequestMapping.class)))
+            .map(method -> entry(method, method.getAnnotation(RequestMapping.class)))
+            .flatMap(this::mapExecutions)
+            .collect(Collectors.toMap(Entry::getKey, Entry::getValue));
+
+        this.handlerExecutions.putAll(handlerExecutions);


스트림 고수시군요... 대단하십니다.

조금은 분리할걸 그랬네요 ... ㅋㅋ

jundonghyuk · 2023-09-14T13:34:03Z

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

+        final String path = requestMapping.value();
+        final RequestMethod[] methods = requestMapping.method();


requestMapping.value()의 default 값은 빈 문자열이고, requestMapping.method()의 default는 빈 배열이라고 정의되어 있는데요 !

만약 @RequestMapping 어노테이션을 사용한 사용자가 둘 중 하나를 까먹고 안쓰면 어떻게 될까용 ?

예를들어 path attribute를 까먹고 명시를 안하면 이걸 가지고 HandlerKey를 생성해 그대로 넣어야할까요 아니면 예외처리를 해야할까요 ?

홍실의 의견을 듣고싶습니다.

path와 다르게 method는 명시를 안해도 빈배열으로 나와 정상적인 흐름으로 가겠네요.

로직에는 문제가 없을 것 같으나, 사용자로 하여금 method를 명시해달라는 예외를 던져도될것같구요 .. ?

구현하기 나름인것 같습니다 ㅋㅋ ㅠ

말씀해주신 것처럼 지금 상황에서는, 스프링 서버가 뜰 때 문제가 생기진 않지만 실제로 해당 ReuqestMapping이 동작하진 않을 것 같네요.

서버가 동작할 때 부터 명시적으로 띄워주는 게 더 좋을 것 같습니다.

jundonghyuk · 2023-09-14T13:42:15Z

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

    public Object getHandler(final HttpServletRequest request) {
-        return null;
+        final String servletPath = request.getRequestURI();
+        final RequestMethod requestMethod = RequestMethod.valueOf(request.getMethod());


request.getMethod()는 다음과 같은 설명이 적혀져있는데요.

Returns the name of the HTTP method라는데, 내부에서 POST, GET등 검증을 해주는걸까요 ? 아무래도 해주겠죠 ?

GETT와 같은 잘못된 문자열은 뱉어주지 않겠죠 ? (진짜로 궁금함..)

이와 같은 질문을 드린 이유는 RequestMethod.valueOf() 때문인데요.

문자열에 매칭되는 ENUM이 있는지 찾는거라 IllegalArgumentException이 날 수도 있을것같아요 !

이 경우 처리를 해줘야한다고 생각하시나요 ?

Returns 문에 있는 설명을 보면 특정한 문자열만 반환해준다고 하는 걸 보니, GETT과 같은 잘못된 문자열을 반환할 것 같진 않네요.

하지만, 별도로 처리를 해주긴 해야 할 것 같아요.(내부적으로 또 어떻게 구현될지 모르니까요.)

이것도 반영해보겠습니다.

jundonghyuk · 2023-09-14T13:50:04Z

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

+        final Map<HandlerKey, HandlerExecution> handlerExecutions = new Reflections(basePackage)
+            .getTypesAnnotatedWith(Controller.class)
+            .stream()
+            .map(Class::getMethods)
+            .flatMap(Arrays::stream)
+            .filter(method -> Objects.nonNull(method.getAnnotation(RequestMapping.class)))
+            .map(method -> entry(method, method.getAnnotation(RequestMapping.class)))
+            .flatMap(this::mapExecutions)
+            .collect(Collectors.toMap(Entry::getKey, Entry::getValue));


basePackages와 Annotiation을 받아서 해당 어노테이션을 가진 클래스를 뱉어주는 Scanner와 같은 것을 구현해봐도 재밌겠네요 ! (그냥 제안입니다..ㅋㅋ)

워낙 깔끔하게 스트림을 구현해주셨지만 ! 여러가지 일을 하고 있는 것 같아서요 !

반영은 자유입니다 ~!

저 스트림이 읽기 힘들다는 것에 동의해요. 어느정도는 메서드 분리가 필요해보입니다.
(어거지로 entry를 쓴 부분이 있어서 그 부분도 수정해보도록 하겠습니다.)

hong-sile

하디 말씀해주신 부분에 대해서 반영해봤습니다.
엄청 꼼꼼하게 봐주시더군요. 👍

리뷰 다시 요청드려요!

jundonghyuk

홍실 리뷰어 하디입니다 !

리뷰를 남긴 방향대로 잘 고쳐 주신 것 같아요 이만 머지하겠습니다 !

2단계 때 뵈어요 !! 고생하셨습니다

jundonghyuk · 2023-09-18T08:13:14Z

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

    private final Method method;
+    private final Object object;


jundonghyuk · 2023-09-18T08:15:55Z

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

+        try {
+            final RequestMethod requestMethod = RequestMethod.valueOf(request.getMethod());
+            final HandlerKey handlerKey = new HandlerKey(servletPath, requestMethod);
+            return handlerExecutions.get(handlerKey);
+        } catch (final IllegalArgumentException e) {
+            throw new RequestMethodNotValidException();


jundonghyuk · 2023-09-18T08:17:12Z

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

-            throw new
+            throw new HandlerExecutionNotInitializeException();


이전 커밋 amend 했으면 더 완전한 커밋이 되었을 것 같아요 ㅎㅅㅎ 고생하셨어용

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

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

52ee778

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

e7c21ab

test: Reflection 학습테스트 진행

db46c80

test: Servlet 학습테스트 진행

4969421

feat: step1 구현

2870d7e

chore: sonar 명령어 추가

ddf01ac

hong-sile requested a review from jundonghyuk September 14, 2023 01:11

hong-sile self-assigned this Sep 14, 2023

refactor: 필터 리팩터링

f894cb3

jundonghyuk requested changes Sep 14, 2023

View reviewed changes

hong-sile added 4 commits September 18, 2023 13:19

refactor: object를 인스턴스로 선언하도록 리팩터링

1ec558f

refactor: stream 코드 리팩터링

9de3f56

feat: RequestMethod가 적절하지 않은경우 예외처리

f7a29ac

feat: HandlerExecution이 초기화 될 때 정상적으로 초기화 되지 않는 경우에 대한 예외처리

a7545cc

hong-sile commented Sep 18, 2023

View reviewed changes

jundonghyuk approved these changes Sep 18, 2023

View reviewed changes

jundonghyuk merged commit 56d2b08 into woowacourse:hong-sile Sep 18, 2023

		final String path = requestMapping.value();
		final RequestMethod[] methods = requestMapping.method();

		throw new
		throw new HandlerExecutionNotInitializeException();

Conversation

hong-sile commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jundonghyuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hong-sile Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hong-sile Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hong-sile Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hong-sile left a comment

Choose a reason for hiding this comment

Uh oh!

jundonghyuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hong-sile commented Sep 14, 2023 •

edited

Loading

hong-sile Sep 18, 2023 •

edited

Loading

hong-sile Sep 18, 2023 •

edited

Loading

hong-sile Sep 18, 2023 •

edited

Loading