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단계] 푸우(백승준) 미션 제출합니다. #344

Merged
merged 10 commits into from
Sep 14, 2023

Conversation

BGuga
Copy link

@BGuga BGuga commented Sep 13, 2023

안녕하세요 주노 !! 🫡
이번에 리뷰 받게된 푸우입니다 ㅎㅎ
1단계 미션에서 만든 AnnotationHandlerMapping 에서 Map<HandlerKey, HandlerExecution> 를 만들기 위한 계층은 다음과 같습니다.

basePackage -> class -> method -> RequestMethod

여기서 가장 아래 계층인 RequestMethod 가 Map<HandlerKey, HandlerExecution> 만드는 단위 입니다 ㅎㅎ
만약 중복된 HandlerKey가 있다면 예외를 발생시키도록 만들었습니다 👍

리뷰 감사합니다 주노 ㅎㅎ ☺️

Copy link
Member

@Choi-JJunho Choi-JJunho 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 +33 to +38
private void validateBasePackage(Object[] basePackage) {
for (Object o : basePackage) {
if (!(o instanceof String)) {
throw new IllegalArgumentException("basePackage 는 String 으로 입력해야 합니다");
}
}
}
Copy link
Member

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.

감사합니다 ㅎㅎ

.reduce(new HashMap<>(), migrateHandler());
}

private Object makeClass(Class<?> targetClass) {
Copy link
Member

Choose a reason for hiding this comment

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

makeClass라는 네이밍에서 클래스를 생성한다..? 혹은 리플렉션을 이용해 클래스를 찾아오는구나~ 하고 생각하게 되었습니다!
메소드 명을 클래스를 받아서 인스턴스화 시킨다라는 의미를 나타낼 수 있도록 구성해보는건 어떤가요?

생각나는 예시로는 toInstance() 같은 네이밍이 생각나네요~

Copy link
Author

Choose a reason for hiding this comment

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

toInstance 힙하네요!


private Map<HandlerKey, HandlerExecution> extractHandlerFromClass(Class<?> targetClass) {
Object handler = makeClass(targetClass);
return Arrays.stream(targetClass.getMethods()).filter(this::haveRequestMapping)
Copy link
Member

Choose a reason for hiding this comment

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

method의 isAnnotationPresent() 메소드를 활용해볼 수 있지 않을까요?

Suggested change
return Arrays.stream(targetClass.getMethods()).filter(this::haveRequestMapping)
return Arrays.stream(targetClass.getMethods()).filter(method -> method.isAnnotationPresent(RequestMapping.class))

Comment on lines +84 to +90
private BinaryOperator<Map<HandlerKey, HandlerExecution>> migrateHandler() {
return (originHandler, migrateHandler) -> {
checkDuplication(originHandler, migrateHandler);
originHandler.putAll(migrateHandler);
return originHandler;
};
}
Copy link
Member

Choose a reason for hiding this comment

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

오.. 생소한 방식이네요!
이렇게 Map을 합쳐가는 방식을 고려해볼 수도 있겠군요 👍

Comment on lines 104 to 107
Optional<HandlerKey> findHandler = handlerExecutions.keySet().stream()
.filter(handlerKey -> handlerKey.canHandle(request))
.findAny();
return findHandler.map(handlerExecutions::get).orElseGet(null);
Copy link
Member

Choose a reason for hiding this comment

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

HandlerKey에 equals, hashcode가 재정의되어있어 handlerExecutions.get(new HandlerKey(...))로도 충분히 값을 가져올 수 있다고 생각하는데 위와같이 구성하신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

추후에 equals 로 처리하기 힘든 상황이 있을 것 같아서 미리 메서드를 만들었던 것 같아요 ㅎㅎ
힘든 상황에 대한 설명은 아래 질문에서 답변하겠습니다!!

Comment on lines +18 to +21
public boolean canHandle(HttpServletRequest httpServletRequest){
return httpServletRequest.getMethod().equals(requestMethod.name()) &&
httpServletRequest.getRequestURI().equals(url);
}
Copy link
Member

Choose a reason for hiding this comment

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

equals가 재정의되어있는데 해당 메소드를 다시 정의한 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

저는 HandlerKey 이 RequestMapping 가 객체화됨으로써 요청으로 전달된 HttpRequest 가 Mapping 되는지 확인하는 역할로 생각했습니다 ㅎㅎ
그래서 든 생각이 @RequestMapping 에 만약에 consumes 과 같은 column이 추가되어서

@RequestMapping(consumes = { 
    "application/json",
    "text/plain"
})

요 상황에서 만약

HttpRequest
~
Content-Type: application/json

이런게 오면 해당 Request 로 만든 HandlerKey 로 equals 연산을 못하겠다고 생각해서
HandlerKey 가 해당 요청을 처리할 수 있는지 판단하게 canHandle 을 만들자! 하고 생각했습니다 ㅎㅎ
지금은 equal 로 정의할 수 있지만 의존을 미리 만들어두면 나중에 처리하기 힘들 것 같아서 똑같은 메서드라도 재정의 해뒀습니다 ㅎㅎ

Copy link
Member

Choose a reason for hiding this comment

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

오 contentType까지 고려한 로직이였군요!
확장성을 고려한 설계라면 동의합니다 👍

Copy link
Member

@Choi-JJunho Choi-JJunho left a comment

Choose a reason for hiding this comment

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

반영해주신 내용 확인했습니다~!
1단계에서 요구하는 내용은 모두 만족한 것 같으니 2단계로 넘어가시죠!

@Choi-JJunho Choi-JJunho merged commit 8097ab6 into woowacourse:bguga 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