[MVC 구현하기 - 1단계] 제이(이재윤) 미션 제출합니다. #348
Conversation
| return method -> method.isAnnotationPresent(RequestMapping.class); | ||
| } | ||
|
|
||
| private void putHandlerExecutionsByMethod(final Method method) { |
There was a problem hiding this comment.
findHandlerKeys로 인해 같은 연산이 두 번 반복되는 것 같아요
requestMapping이 갖는 메서드들을 순회하면서 바로 HandlerKey를 만드시는 것은 어떠신가요?
제가 이해하기에는
- findHandlerKey -> requestMapping의 메서드 순회하면서 HandlerKey 생성
- 생성한 HandlerKey를 순회하며 Map에 put
이런 연산인데 한번에 할 수 있지 않을까요??
There was a problem hiding this comment.
불필요하게 계속 RequestMapping requestMapping = handlerMethod.getAnnotation(RequestMapping.class);와 같은 연산이 들어가네요.
하나의 메서드에서 처리할 수 있도록 리팩토링 하겠습니다.
|
|
||
| public Object getHandler(final HttpServletRequest request) { | ||
| String requestURI = request.getRequestURI(); | ||
| return handlerExecutions.get(new HandlerKey(requestURI, RequestMethod.valueOf(request.getMethod()))); |
There was a problem hiding this comment.
오,, Enum 찾을 때 직접 코드를 작성해줬는데 valueOf라는게 있네요
하나 배우고 갑니다
| Reflections reflections = new Reflections(basePackage); | ||
|
|
||
| for (final Class<?> controller : getClassesWithControllerAnnotated(reflections)) { | ||
| putHandlerByController(controller); |
There was a problem hiding this comment.
연산들을 하나의 의미있는 메서드로 묶어주는 습관 너무 좋아보입니다.
하지만 너무 자주 메서드를 묶게 되면 가독성을 되려 해칠 수 있다고 생각합니다.
스트림에서 연산을 보여줘도 될 것 같은데, 메서드를 extract를 함으로써 보는 사람 입장에서 한번 더 메서드를 확인해야합니다.
연관있는 연산들을 묶어서 하나의 메서드로 묶는 것은 어떠신가요?
이 부분은 개인적인 의견이라,, 제이의 생각을 여쭤보고 싶어서 남겨봅니다!
There was a problem hiding this comment.
오.. 한 번도 생각해본적이 없는 좋은 고민이네요.
리뷰 보고 곰곰히 생각해봤습니다.
현재는 stream 연산에서 filter, forEach 부분에서 사용되는 로직 모두 메서드로 분리했는데요.
forEach에서 사용되는 putHandlerExecutionsByMethod(...)와 같은 메서드는 로직이 길기 때문에 충분히 분리해도 된다는 생각입니다.
다만 filter에서 사용되는 hasAnnotatedWithRequestMapping(..)메서드는 Method 객체에서 isAnnotationPresent(...)라는 뜻이 담긴 좋은 네이밍의 메서드가 제공 돼서 굳이 필요하지 않을 수도 있다고 생각합니다.
오히려 hasAnnotatedWithRequestMapping(...) 메서드와 같이 메서드 내부에서 값을 설정하고 반환을 void로 하는 것보다 매개변수로 어노테이션을 넘겨주고, 반환으로 boolean과 같은 값을 넘겨준다면 충분히 메서드로 분리해서 뜻을 줄 수 있다고 생각은합니다.
메서드를 좋은 네이밍으로 분리하는 것 자체는 좋은 행위가 될 수 있지만 불필요하게 분리를 하면 오히려 많은 메서드로 가독성이 저해될 수도 있겠네요.
차라리 불필요하게 나누기 보다는 합칠 수 있는 메서드들을 합치고, 합친 메서드 명을 조금 더 상세하게 적는 것도 좋은 선택이 될 것 같습니다.
다시 기준을 세워볼 수 있게되는 좋은 리뷰 감사합니다 👍
안녕하세요 우르~
이번 1단계 미션에서는 크게
AnnotationHandlerMapping,HandlerExecution를 바꾸었습니다.리뷰 잘 부탁드립니다!
구구의 커밋 내역이 같이 있어서 리뷰할 때 헷갈리실 수 있습니다.
제가 작업한 커밋 범위는 다음과 같습니다.
플로우
basePackage를 기준으로 Reflections를 만듭니다.@Controller어노테이션이 있는 클래스들을 찾아옵니다3-1. 클래스 메서드들을 꺼내서 먼저
@RequestMapping어노테이션이 붙은 메서드만 있도록 필터링을 합니다.3-3. 필터링 된 메서드들을 기준으로
HandlerExecutions를 채워줍니다.AnnotationHandlerMapping 클래스의getHandler()메소드를 통해HandlerExecution객체를 반환 받고HandlerExecution.handle()메서드를 실행하게 됩니다.