-
Notifications
You must be signed in to change notification settings - Fork 305
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 푸우~🐻
꼼꼼한 검증이 인상적이네요 👍
몇가지 궁금한 부분에 대해 코멘트 남겼습니다! 확인 부탁드릴게요~
private void validateBasePackage(Object[] basePackage) { | ||
for (Object o : basePackage) { | ||
if (!(o instanceof String)) { | ||
throw new IllegalArgumentException("basePackage 는 String 으로 입력해야 합니다"); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
꼼꼼한 검증 좋네요! 👍
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makeClass
라는 네이밍에서 클래스를 생성한다..? 혹은 리플렉션을 이용해 클래스를 찾아오는구나~ 하고 생각하게 되었습니다!
메소드 명을 클래스를 받아서 인스턴스화 시킨다
라는 의미를 나타낼 수 있도록 구성해보는건 어떤가요?
생각나는 예시로는 toInstance() 같은 네이밍이 생각나네요~
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method의 isAnnotationPresent() 메소드를 활용해볼 수 있지 않을까요?
return Arrays.stream(targetClass.getMethods()).filter(this::haveRequestMapping) | |
return Arrays.stream(targetClass.getMethods()).filter(method -> method.isAnnotationPresent(RequestMapping.class)) |
private BinaryOperator<Map<HandlerKey, HandlerExecution>> migrateHandler() { | ||
return (originHandler, migrateHandler) -> { | ||
checkDuplication(originHandler, migrateHandler); | ||
originHandler.putAll(migrateHandler); | ||
return originHandler; | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오.. 생소한 방식이네요!
이렇게 Map을 합쳐가는 방식을 고려해볼 수도 있겠군요 👍
Optional<HandlerKey> findHandler = handlerExecutions.keySet().stream() | ||
.filter(handlerKey -> handlerKey.canHandle(request)) | ||
.findAny(); | ||
return findHandler.map(handlerExecutions::get).orElseGet(null); |
There was a problem hiding this comment.
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(...))
로도 충분히 값을 가져올 수 있다고 생각하는데 위와같이 구성하신 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추후에 equals 로 처리하기 힘든 상황이 있을 것 같아서 미리 메서드를 만들었던 것 같아요 ㅎㅎ
힘든 상황에 대한 설명은 아래 질문에서 답변하겠습니다!!
public boolean canHandle(HttpServletRequest httpServletRequest){ | ||
return httpServletRequest.getMethod().equals(requestMethod.name()) && | ||
httpServletRequest.getRequestURI().equals(url); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
equals가 재정의되어있는데 해당 메소드를 다시 정의한 이유가 있을까요?
There was a problem hiding this comment.
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 로 정의할 수 있지만 의존을 미리 만들어두면 나중에 처리하기 힘들 것 같아서 똑같은 메서드라도 재정의 해뒀습니다 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 contentType까지 고려한 로직이였군요!
확장성을 고려한 설계라면 동의합니다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반영해주신 내용 확인했습니다~!
1단계에서 요구하는 내용은 모두 만족한 것 같으니 2단계로 넘어가시죠!
안녕하세요 주노 !! 🫡
이번에 리뷰 받게된 푸우입니다 ㅎㅎ
1단계 미션에서 만든 AnnotationHandlerMapping 에서 Map<HandlerKey, HandlerExecution> 를 만들기 위한 계층은 다음과 같습니다.
여기서 가장 아래 계층인 RequestMethod 가 Map<HandlerKey, HandlerExecution> 만드는 단위 입니다 ㅎㅎ
만약 중복된 HandlerKey가 있다면 예외를 발생시키도록 만들었습니다 👍
리뷰 감사합니다 주노 ㅎㅎ☺️