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단계] 우르(김현우) 미션 제출합니다 #349

Merged
merged 4 commits into from
Sep 14, 2023

Conversation

java-saeng
Copy link

안녕하세요 메리 !

이번 1단계 미션에서는 AnnotationHandlerMapping, HandlerExecution를 중점적으로 수정했습니다.

리뷰 잘 부탁드립니다 !

@java-saeng java-saeng self-assigned this Sep 13, 2023
Copy link

@swonny swonny left a comment

Choose a reason for hiding this comment

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

안녕하세요 우르!

코드가 간결하고 깔끔해서 리뷰하기 정말 수월했습니다!

미션 구현 후 몇 가지 궁금한 부분이 생겨서 우르의 의견도 궁금해 간단하게 코멘트 몇 가지 남겨두었는데,
2단계 진행하시면서 우르의 의견 남겨주시면 감사하겠습니다
approve 하겠습니다. 고생하셨습니다!

TRACE
;

public static RequestMethod find(final String method) {
Copy link

Choose a reason for hiding this comment

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

valueOf()가 해당 역할을 해줄 수 있을 것 같은데 해당 메서드를 직접 정의해서 사용하신 이유가 있으신가요?

Copy link
Author

Choose a reason for hiding this comment

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

몰랐습니다,, ㅋㅋㅋ 바로 반영할게요 감사합니다 !!

return null;
}
private final Method method;
private final Object target;
Copy link

Choose a reason for hiding this comment

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

Class<?>로 클래스 타입을 저장하고 handle() 메서드가 호출될 때 인스턴스를 생성할 수 있을 것 같은데
핸들러 매핑 시 인스턴스를 생성하신 이유가 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

제가 잘 이해를 못한 것 같은데, 생성된 인스턴스를 HandlerExecution에 넘겨줘서 handle 메서드를 실행하고 있는데

Class<?> 타입을 어디에 지정하는걸까요??


Arrays.stream(methods)
.filter(method -> method.isAnnotationPresent(RequestMapping.class))
.forEach(method -> putHandlerExecutions(method, instance));
Copy link

Choose a reason for hiding this comment

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

이 부분에서 메서드 분리가 되어 읽기 편하다고 느꼈습니다!👍🏻

reflections.getTypesAnnotatedWith(Controller.class);

for (Class<?> controllerAnnotationClass : controllerAnnotationClasses) {
final Object instance = controllerAnnotationClass.getDeclaredConstructor().newInstance();
Copy link

Choose a reason for hiding this comment

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

다른 코드 리뷰를 보면서 알게된 부분인데요,
기본 생성자가 없는 경우에 대한 에러를 해당 코드에서 던진다고 합니다.
이러한 예외들은 어떤 클래스에서 처리해주는 것이 좋은지에 대한 우르의 의견이 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

무조건 해당 클래스에 기본 생성자가 있어야한다면 에러 트레이스를 하기 쉽게 어느 클래스에 기본 생성자가 없다고 에러 로깅을 찍고, 처리는 AnnotationHandlerMapping 에서 해줘도 되지 않을까? 라는 생각이 듭니다.

Annotation을 사용하면서 발생한 일이기 떄문이에요.

@swonny swonny merged commit d2be789 into woowacourse:java-saeng 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.

2 participants