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 구현하기 - 1단계] 무민(박무현) 미션 제출합니다. #350

Merged
merged 7 commits into from
Sep 14, 2023

Conversation

parkmuhyeun
Copy link
Member

@parkmuhyeun parkmuhyeun commented Sep 13, 2023

안녕하세요 영남회장님 만나뵙게 되어 반갑습니다.
이번 미션 잘 부탁드려요 🙇🏻‍♂️

새로운 구구 커밋을 제외한 순수 제 커밋 범위는 다음과 같습니다!

이번에 바꾼거로는 두 개의 클래스 밖에 없습니다!

  • AnnotationHandlerMapping
  • HandlerExecution

AnnotationHandlerMapping의 init flow는 다음과 같습니다.

  1. basePackage를 기반으로 Reflections을 생성
  2. Controller 애노테이션이 붙은 클래스를 가져온다.
  3. 해당 클래스의 @RequestMapping이 붙은 메서드를 필터해서 가져온다.
  4. 각 메서드들의 path와 method 정보를 이용해 HandlerExecution init

미리 리뷰 감사합니다~ 열정! 열정! 열정! 🙌

Copy link

@BGuga BGuga left a comment

Choose a reason for hiding this comment

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

안녕하세요 무민 ㅎㅎ
리뷰어 푸남회장입니다 👍
이번 미션 한번 열정있게 같이 해봐용 🫡
잘 구현해 주셔서 몇가지 질문만 남겼습니다.
수고 많으셨습니다 열정열정~!

Comment on lines +60 to +63
return method.getDeclaringClass().newInstance();
} catch (InstantiationException | IllegalAccessException e) {
throw new IllegalArgumentException(e);
}
Copy link

Choose a reason for hiding this comment

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

method 에서 Class 만드는 흐름이 상당히 직관적이네요 👍
클래스들을 먼저 추출하고 해당 클래스에 대한 등록 작업도 가능할 것 같은데 혹시 고려해보셨는지 궁금합니다 ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

쉽고 가독성 좋은 것이 베스트라 생각해서 우선 생각나는대로 짜봤습니다. 해당 방법이 더 좋다고 느껴지면 해당 방법도 고려해볼 만한 것 같은데 예시가 있을까요

Copy link

@BGuga BGuga Sep 17, 2023

Choose a reason for hiding this comment

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

클래스를 먼저 추출한 후 클래스의 RequestMapping 메서드를 추출하는 예시 코드는 아래와 같은 흐름일 것 같네요!

Set<Class<?>> controllers = reflections.getTypesAnnotatedWith(Controller.class);
Map<Class<?>, Object> instances = makeInstances(controllers); // Class 별 인스턴스 만들기
for (Class<?> targetClass : controllers.keySet()) {
    enrollHandler(targetClass, controllers.get(targetClass))
}

이런 흐름으로 간다면
하나의 클래스에 RequestMapping 메서드가 5개 있을 경우 똑같은 Class를 한번만 Instance 화 할 수 있네요 ㅎㅎ

.collect(Collectors.toList());
}

private void putHandlerExecutions(List<Method> requestMappingMethods) {
Copy link

Choose a reason for hiding this comment

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

void 반환으로 위임 방식으로 메서드를 작성해주셨네요 ㅎㅎ
메서드들이 반환 값을 갖도록 만들어 상위 호출 메서드에서 합쳐주는 방식도 있었을 것 같은데 요 방식은 어떻게 생각하시나요 ?ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

만약 지금 작성한 것보다 이해하기 쉽다면 그 방식도 고려해볼 것 같습니다.

Copy link

@BGuga BGuga Sep 17, 2023

Choose a reason for hiding this comment

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

예시를 첨가하자면!

private Map<HandlerKey, HandlerExecution> extractHandlerFromMethod(Method method, Object handler);

저는 클래스 내부의 메서드에 이런식으로 반환 값을 갖는 것을 선호합니다 ㅎㅎ

  1. 이렇게 작성하면 메서드 디버깅할 때 해당 메서드를 직접 호출해보면서 테스트하기 편리했습니다!

void 값일 경우에는 handlerExecutions 한번 본 다음에 메서드 호출하고 또 다시 조회해서 변화를 찾아내야 겠죵?

  1. 객체 분리가 조금 더 수월하다.

객체가 너무 커져서 분리를할 때 void 로 선언되어 있다면 해당 클래스에 handlerExecutions 을 넘기는 형태로 구현할 수는 없으니 리팩터링이 발생합니다.
반환 타입이 있을 경우Class 를 가져오는 것은 ClassScanner, Map<HandlerKey, HandlerExecutor> 를 만들어주는 것은 HandlerExtractor 와 같이 책임 별 분리가 좀 더 수월해지더라고요 ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

오.. 좋은거 같아요 👍🏻👍🏻 혹시 이렇게 리턴했을 때의 단점도 있을까여?

Copy link

Choose a reason for hiding this comment

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

흠..
내부 메서드를 하나의 작은 클래스의 메서드의 호출이라 상상하고 메서드를 작성하는 과정인데...
큰 단점을 아직까지 느끼진 못했던 것 같네요🤔

@BGuga BGuga merged commit 674e092 into woowacourse:parkmuhyeun Sep 14, 2023
1 check failed
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.

3 participants