[MVC 구현하기 1단계] 키아라(김도희) 미션 제출합니다 #353
Conversation
There was a problem hiding this comment.
안녕하세요 키아라!🦁 디노입니다~🦖
리뷰 요청 일찍 보내주셨는데 제가 약속드린 시간보다 조금 늦었습니다..
전체적으로 깔끔하게 작성해 주셔서 이해하기 수월한 코드였어요..!
1단계 요구사항도 만족해 바로 approve & merge 하겠습니다ㅎㅎ
제가 코멘트 남긴 부분은 다음 스텝 pr에서 답변 주셔도 되구 그냥 내일 말로 해주셔도 됩니다..!
그냥 정말 제가 궁금한 부분만 달아 봤어요.
제가 식견이 얕아 이상한 질문을 했을 수도 있는데 그런건 그냥 무시해 주십쇼.
그럼 다음 스텝에서 만나요~🙋
| log.info("Initialized AnnotationHandlerMapping!"); | ||
| putHandlerExecutions(); | ||
| } |
There was a problem hiding this comment.
키아라와 의견 나눠보고 싶어서 적어 봅니다.!
현재 함수 진입 시 바로 로그를 출력하도록 되어 있는데요.
"AnnotationHandlerMapping이 Initialized 되었다"는 의미의 로그는
해당 함수가 완벽히 종료되는 시점에 적어야 하지 않을까? 하는 생각이 들었습니다.
키아라는 어떠한 이유에서 putHandlerExecutions 메서드 앞 단에 로깅을 하도록 구현하셨는지 궁금합니다.!
There was a problem hiding this comment.
아하, 저는 이 로그를 안읽어보고 단지 초기화 하기 위해 그 아래에 로직을 넣어줬었습니다.
예리하시네용👍
| final Constructor<?> firstConstructor = questionClass.getDeclaredConstructors()[0]; | ||
| final Constructor<?> secondConstructor = questionClass.getDeclaredConstructors()[1]; |
| } | ||
|
|
||
| public ModelAndView handle(final HttpServletRequest request, final HttpServletResponse response) | ||
| throws NoSuchMethodException, InvocationTargetException, InstantiationException, IllegalAccessException { |
There was a problem hiding this comment.
원래 Exception이었던 걸 NoSuchMethodException, InvocationTargetException, InstantiationException, IllegalAccessException 로 나눈건 sonarLint가 냄새 맡아서 바꿔준건가요?
There was a problem hiding this comment.
예...맞아요. 혹시 그냥 Exception으로 하는게 더 나아보일까요?
| private static HandlerKey makeHandlerKey(Method method) { | ||
| RequestMapping annotation = method.getAnnotation(RequestMapping.class); | ||
| String url = annotation.value(); | ||
| RequestMethod requestMethod = annotation.method()[0]; |
There was a problem hiding this comment.
0 매직넘버 처리 or 배열로 가져오는 방식 이외에 다른 로직으로 구현하는 방법
둘 중에 하나 진행하면 좋을 것 같습니다..!
There was a problem hiding this comment.
아이구, 이거 수정했는데 코드 범위에 추가되지 않았네요...! 배열로 가져오는 것으로 수정했으니 step2때 다시 봐주십셩!
| public void initialize() { | ||
| log.info("Initialized AnnotationHandlerMapping!"); | ||
| putHandlerExecutions(); | ||
| } | ||
|
|
||
| private void putHandlerExecutions() { | ||
| Reflections reflections = new Reflections(basePackage); | ||
| reflections.getTypesAnnotatedWith(Controller.class) | ||
| .forEach(this::processControllerClass); | ||
| } | ||
|
|
||
| private void processControllerClass(Class<?> aClass) { | ||
| Arrays.stream(aClass.getDeclaredMethods()) | ||
| .filter(method -> method.isAnnotationPresent(RequestMapping.class)) | ||
| .forEach(this::processControllerMethod); | ||
| } | ||
|
|
||
| private void processControllerMethod(Method method) { | ||
| HandlerKey handlerKey = makeHandlerKey(method); | ||
| HandlerExecution handlerExecution = new HandlerExecution(method); | ||
| handlerExecutions.put(handlerKey, handlerExecution); | ||
| } | ||
|
|
||
| private static HandlerKey makeHandlerKey(Method method) { | ||
| RequestMapping annotation = method.getAnnotation(RequestMapping.class); | ||
| String url = annotation.value(); | ||
| RequestMethod requestMethod = annotation.method()[0]; | ||
| return new HandlerKey(url, requestMethod); |
There was a problem hiding this comment.
(진짜 그냥 궁금해서 물어보는거임)
현재 initialize 메서드에서 파생되는 연관 메서드가 총 4개가 되도록 메서드 추출을 진행했는데요.
이렇게 메서드를 분리하는 키아라만의 기준이 있을까요?
저는 이렇게 분리를 할 때 depth가 점점 깊어지면 오히려 로직을 이해하기 어렵더라구요.
어느 단위로 분리하고, 최대 허용 깊이에 대한 본인만의 기준이 있는지 궁금합니다.!
There was a problem hiding this comment.
이렇게 분리하지 않으면 이중 for문에 조건문이 들어있어 depth가 길어지니 최대한 2-depth를 넘어가지 않기위해 메서드를 분리하고 있습니다.
메서드가 많다고 로직 이해가 어려워진다는건 아무래도 제가 메서드 이름을 명확하게 짓지 않았고 한 메서드에서 하는 일을 여러가 정의해서 그런거겠죠..ㅎㅎ 만약 메서드의 역할과 이름이 명확했다면 메서의의 개수와 상관없이 잘 이해되셨을 겁니다🫠
step2에선 이런 부분 고려해서 리펙터링 하겠습니다!
안녕하세요 디노🦖 키아라입니다🦁
AnnotationHandlerMapping을 Reflection을 사용해 구현해보았어요.
학습테스트를 제외하면
AnnotationHandlerMapping이랑 HandlerExecution 클래스만 변경했는데 이게 맞는걸까요..?
리뷰 잘 부탁 드려요ㅎㅎ
요기로