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단계] 제나(위예나) 미션 제출합니다 #413

Merged
merged 8 commits into from
Sep 14, 2023

Conversation

yenawee
Copy link

@yenawee yenawee commented Sep 14, 2023

안녕하세요 필립
2번째 과제 리뷰어로 만나게 되어서 반갑습니다! 👏

이번 단계에서는 @controller 어노테이션이 붙어있는 클래스들을 스캔하고 @RequestMapping 이 붙어있는 메서드들을 추출해서 웹 요청이 들어오는 URI 와 URI 를 실행해줄 메서드를 묶어주는 기능을 구현하였습니다!!

리뷰해주셔서 감사합니다!! 😊

@yenawee yenawee self-assigned this Sep 14, 2023
Copy link

@pilyang pilyang left a comment

Choose a reason for hiding this comment

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

안녕하세요 제나!!

이렇게 리뷰로 만나게 되어 반가워요 😄

코드 깔끔하게 잘 짜주셔서 리뷰하기 너무 편했어요!! 최고최고!!!

리뷰는 반영하면 좋을것 같은 아주아주 사소한 부분 하나만 있고, 자잘한 코멘트만 있어서 이번 단계에선 Approve 바로 할께요!!
코멘트는 RC), C)로 앞부분에 구분해두었는데, RC)코멘트 1개는 다음 미션 진행하기전에 한번 반영해보고 진행하면 좋을 것 같아요.
나머지 일반 코멘트는 읽어보고 고민만 해봐도 좋을 것 같습니다!

의견 나누고싶거나 궁금하거나 이야기하고싶은 부분 있으면 언제든 편하게 dm주십쇼!

그럼 다음단계도 화이팅해요!!

Comment on lines +8 to +13
public static RequestMethod getRequestMethod(String method) {
return Arrays.stream(values())
.filter(value -> value.name().equalsIgnoreCase(method))
.findFirst()
.orElseThrow(() -> new IllegalArgumentException("잘못된 요청입니다"));
}
Copy link

Choose a reason for hiding this comment

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

RC)

java enum에는 기본으로 valueOf() 메소드가 있습니다!!
해당 메소드는 RequestMethod.valueOf(methodString)을 다시 구현한것같네요. 이미 있는 메소드를 사용해보는게 어떨까 합니다! 😄

참고로 valueOf메소드 또한 없는 value name에 대해서는 제나가 구현해준것과 같이 IllegalArgumentException을 던져줍니다.

Copy link
Author

Choose a reason for hiding this comment

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

해당 메서드의 존재를 모르고 있었어요...!
반영했습니다 !!

Comment on lines +49 to +50
RequestMethod[] requestMethods = requestMappingMethod.method();
RequestMethod requestMethod = requestMethods[0];
Copy link

Choose a reason for hiding this comment

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

C)

RequestMethod가 여러개가 맵핑되는 경우는 없다고 생각하고 하신거겠죠..?
여러개를 맵핑하는 경우가 없다고 생각을 하더라도, @RequestMapping 에서 method를 배열로 가지고 있으니, 여러개가 입력되는 상황에 대해서도 고민을 해보는것이 좋을것 같기는 합니다..ㅎ

입력된경우들을 다 맵핑해준다던가, 여러개가 입력되어도 처음 입력한것만 맵핑된다고 명시적으로 나타내주거나 (주석 java doc을 이용해서..?) 예외처리등을 해준다거나 하는 방식으로요.

이부분 한번 고민해봐도 좋을 것 같습니다!!

Copy link
Author

@yenawee yenawee Sep 17, 2023

Choose a reason for hiding this comment

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

method 가 여러개 들어올 수 있는 상황이 발생할 수 있으니 입력된 경우를 다 매핑해주는 것으로 수정하였습니다!

for 문으로 뎁스가 하나 더 들어가지만 메서드 분리를 해주는 것보다 지금 상태로 두는 것이 가독성이 좋다고 개인적으로 생각해서 따로 메서드 분리는 해주지 않았어요

this.handlerExecutions = new HashMap<>();
}

public void initialize() throws NoSuchMethodException, InvocationTargetException, InstantiationException, IllegalAccessException {
Copy link

Choose a reason for hiding this comment

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

C)

예외는 일단 throws로 추가해주는 방식으로 하셨군요!!

이렇게 하게되면 후에 서버를 실행시킬때 intialize를 해주는 부분에서 해당 예외들에 대한 처리도 고민해보거나, 이 initialize메소드 내에서 예외를 처리해주어서 로그를 통해 컨트롤러가 초기화가 실패한 상황에 대한 정보를 사용자가 알 수 있게 해주는것도 고민해보면 좋을 것 같습니다!!

Copy link
Author

Choose a reason for hiding this comment

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

리플렉션 쓰면서 예외안날리면 에러가 나서 예외를 너무 막날렸네요,,,,!! ㅎㅎ
예외 처리에 대한 고민을 좀 더 해보겠습니다!!

@pilyang pilyang merged commit 1a448dc into woowacourse:yenawee 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