[MVC 구현하기 - 1단계] 허브(방대의) 미션 제출합니다. #404
Conversation
|
SonarCloud Quality Gate failed.
|
| public Set<Method> toHttpMappingMethods(final Set<Class<?>> types) { | ||
| return types.stream() | ||
| .flatMap(type -> Arrays.stream(type.getDeclaredMethods())) | ||
| .filter(this::isHttpMappingAnnotationPresent) | ||
| .collect(toSet()); | ||
| } |
There was a problem hiding this comment.
해당 메서드는 이 클래스 내부에서만 사용되는 것으로 private으로 두는 것은 어떻게 생각하시나요?
해당 메서드는 @Controller가 붙어있는 타입들에서 @RequestMapping가 붙어있는 메서드를 가져온다는 메서드인 것 같습니다.
하지만 Object.toString()과 같이 to 로 시작하는 메서드는 어떤 것을 다른 것으로 변환하거나 할 때 많이 사용되는 메서드명인 것 같은데,
지금은 private 메서드로 매개변수로 넘어가는 값에서 어떤 값들을 추출하는 것 같아서 약간 그런 동사를 포함한 메서드 명으로 바꿔보는 것에 대해서는 어떧ㅎ게 생각하세용?
There was a problem hiding this comment.
내부에서만 사용하기 때문에 private으로 하는거 너무 좋은 것 같아요! 👍 👍 👍 👍 👍 👍 👍 👍
제가 메서드 네이밍을 너무 못해서 2단계 하면서 고민 열심히 해보겠습니다!!
| import org.slf4j.LoggerFactory; | ||
| import web.org.springframework.web.bind.annotation.RequestMapping; | ||
|
|
||
| public class ControllerScanner { |
There was a problem hiding this comment.
이 ControllerScanner가 매우 감동적이였답니다!
처음에는 무슨 객체지 싶었는데, @controller로 설정한 클래스를 특정하고 클래스 레벨에서 @RequsetMapping에 대한 프리픽스를 정한다...
책임이 명확한 객체인 것 같아서 매우 감동적이였습니다
There was a problem hiding this comment.
저도 루카의 리뷰가 매우 감동적이네요.
꼼꼼하게 봐주셔서 정말 감사합니다 :)
| final Set<Method> methods = toHttpMappingMethods(controllers.keySet()); | ||
|
|
||
| for (final Method method : methods) { | ||
| final String prefix = uriPrefixes.get(method.getDeclaringClass()); | ||
| final Object instance = controllers.get(method.getDeclaringClass()); | ||
| final List<HandlerKey> handlerKeys = handlerKeyGenerator.generate(prefix, method); | ||
| final HandlerExecution handlerExecution = new HandlerExecution(instance, method); | ||
| handlerKeys.forEach(handlerKey -> handlerExecutions.put(handlerKey, handlerExecution)); | ||
| } | ||
|
|
||
| log.info("Initialized AnnotationHandlerMapping!"); | ||
| } | ||
|
|
||
| public Set<Method> toHttpMappingMethods(final Set<Class<?>> types) { | ||
| return types.stream() | ||
| .flatMap(type -> Arrays.stream(type.getDeclaredMethods())) | ||
| .filter(this::isHttpMappingAnnotationPresent) | ||
| .collect(toSet()); | ||
| } | ||
|
|
||
| private boolean isHttpMappingAnnotationPresent(final Method method) { | ||
| final boolean isHttpMappingAnnotationPresent = Arrays.stream(method.getDeclaredAnnotations()) | ||
| .anyMatch(annotation -> annotation.annotationType().isAnnotationPresent(RequestMapping.class)); | ||
| return method.isAnnotationPresent(RequestMapping.class) || isHttpMappingAnnotationPresent; | ||
| } |
There was a problem hiding this comment.
위의 Controller 레벨 스캔은 ControllerScanner에게 역할 부여돼서 잘 이루어지고 있는데요!
이렇게 @RequestMapping(@GetMapping,...)을 스캔하는 역할도 다른 객체에 부여할 수 있지 않을까요?
There was a problem hiding this comment.
루카가 말씀해주신대로 RequestMapping을 스캔만 담당하는 클래스를 하나 만들어봐도 좋을 것 같네요! 👍
그러면 toHttpMappingMethods <- 이 메서드의 네이밍도 해결 될 것 같아요 ㅋㅋㅋ
| public List<HandlerKey> generate(final String prefix, final Method method) { | ||
| final Annotation annotation = getHttpMappingAnnotation(method); | ||
| final String uri = httpMappingExtractor.extractRequestUri(annotation); | ||
| final RequestMethod[] requestMethods = httpMappingExtractor.extractRequestMethod(annotation); | ||
| return Arrays.stream(requestMethods) | ||
| .map(requestMethod -> new HandlerKey(prefix + uri, requestMethod)) | ||
| .collect(toList()); | ||
| } |
There was a problem hiding this comment.
이 부분에서 이미 클래스 레벨에 대한 스캔을 할 때 RequestMapping 어노테이션을 읽어서 prefix를 만들어 놓은 것을 파라미터로 받는데요.
prefix(컨트롤러 레벨에 걸려진 요청 매핑)을 컨트롤러 레벨 스캔 시 스캔 해놓는 것도 보고 너무 감동했지만
제 생각엔 RequestMapping 정보 자체는 이 키를 생성할 때 더 집중해보면 어떨까 싶습니다!!
그래서 이 단계에서 @controller만 먼저 스캔하고 그에 대한 @RequestMapping이 붙은 메서드만 스캔해고
이 메서드를 호출해서 그 메서드의 클래스 레벨에 다시 올라가서 @RequestMapping 정보가 있는지 체크해본다 이런 느낌으로 가면 어떨까요?
There was a problem hiding this comment.
해당 부분은 트레이드오프가 있을만한 부분인 것 같아요!
예를 들어 메서드마다 클래스 레벨의 정보가 있는지 체크를 하는 방식으로 간다면 클래스에 메서드가 있는 만큼 다시 클래스에 정의된 RequestMapping을 찾아야합니다!
따라서 RequestMapping의 정보를 획득하는 것에 대한 응집도 vs 호출 횟수 줄이기가 될 것 같아요!
이 부분은 제가 코드를 수정해보면서 말해주신 방법과 함께 더 좋은 방법이 있는지 생각을 더 해본다음 레벨 2 PR과 함께 제출해보도록 할게요 👍
너무 좋은 리뷰 감사힙니다 🙇
|
|
||
| @Test | ||
| void post() throws Exception { | ||
| @CsvSource({"/request, GET", "/get, GET", "/post, POST", "/patch, PATCH", "/put, PUT", "/delete, DELETE"}) |
There was a problem hiding this comment.
안녕하세요 허브🌿🪴!
리뷰어 루카입니다!!
먼저 빠른 리뷰를 못 드린 점 죄송합니다 🙇♂️
커밋 내역을 차례대로 타고 올라오며 허브가 코드를 어떻게 리팩토링 해나갔었는지 파악해보았습니다!
코드에서 타인에 대한 허브의 배려심이 느껴지더라구요. 책임들이 명확하게 분리되어 있는 누가봐도 읽기 편안한 코드일 것 같습니다!!
부족한 실력이지만 여러모로 얘기 나눠볼 것이 있는지 꼼꼼히 살펴 보았는데, 따로 트집(?) 잡을 만한 내용은 많이 못찾았습니다.
일단 매우 잘 짜신 코드인 것으로 느껴져서 해당 PR은 어프로브 하겠습니다!!
저도 미션 최대한 음미해보고 2~3단계 진행하면서 더 많은 대화 나눠보면 좋을 것 같습니다!!
There was a problem hiding this comment.
안녕하세요 허브🌿🪴!
리뷰어 루카입니다!!
먼저 빠른 리뷰를 못 드린 점 죄송합니다 🙇♂️
제가 코드를 읽는 속도가 좀 느린편이라 ㅠㅠㅠ
커밋 내역을 차례대로 타고 올라오며 허브가 코드를 어떻게 리팩토링 해나갔었는지 파악해보았습니다!
코드에서 타인에 대한 허브의 배려심이 느껴지더라구요. 책임들이 명확하게 분리되어 있는 누가봐도 읽기 편안한 코드일 것 같습니다!!
부족한 실력이지만 여러모로 얘기 나눠볼 것이 있는지 꼼꼼히 살펴 보았는데, 따로 트집(?) 잡을 만한 내용은 많이 못찾았습니다.
일단 매우 잘 짜신 코드인 것으로 느껴져서 해당 PR은 어프로브 하겠습니다!!
저도 미션 최대한 음미해보고 이해도를 많이 높여서 오겠습니다 ㅠㅠ
2~3단계 진행하면서 더 많은 대화 나눠보면 좋을 것 같습니다!!
안녕하세요!!! 루카 :dooboo🍪 (두부 이모지 없음)
루카의 리뷰가 기대되네요. 후추한테 자랑해야지 😄
AnnotationHandlerMappingTest 테스트를 통과시키려고 노력했습니다!
그리고 추가로 RequestMapping이 클래스에 적용(경로만)이 되도록 했습니다 👍
그리고 GetMapping과 같은 Mapping 애너테이션도 추가해봤어요!
구조는 아래와 같습니다!
ControllerScanner: 컨트롤러 애너테이션 적용된 클래스 반환, RequestMapping이 컨트롤러 레벨에 적용된 경우 RequestMapping의 value를 가져오는 클래스입니다.HandlerKeyGenerator: Handler를 가져올 때 HandlerKey를 사용하게 될텐데, 해당 키를 생성해주는 클래스입니다.HttpMappingExtractor: annotation의 필드랑 HttpMethod 정보를 가져오는 클래스입니다.리뷰 범위는 다음과 같습니다!
리뷰 범위
그럼 1단계 리뷰 잘 부탁드립니다 😄 (천천히 리뷰 남겨주셔도 됩니다!)