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단계] 마코(이규성) 미션 제출합니다. #345

Merged
merged 6 commits into from
Sep 13, 2023

Conversation

aak2075
Copy link

@aak2075 aak2075 commented Sep 13, 2023

Hi~ 주드 👋

AnnotationHandlerMappingTest가 통과하도록 구현했습니다.

구현한 방법은 다음과 같습니다.

  1. basePacakge에 있는 @Controller 애너테이션이 붙은 클래스들을 찾음.
  2. 클래스들의 인스턴스를 생성하고 @RequestMapping이 붙은 메서드들을 찾음.
  3. 핸들러와 메서드로 HandlerKeyHandlerExecution을 생성하여 Map<HandlerKey, HandlerExecution>에 삽입.

잘 부탁드립니다~🙏

@aak2075 aak2075 self-assigned this Sep 13, 2023
Copy link

@kevstevie kevstevie left a comment

Choose a reason for hiding this comment

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

안녕하세요 마코
빠르고 깔끔하게 구현해주셔서 코멘트달게 별로 없었어요 (진짜로)
추가 수정사항이나 리팩터링은 2단계라서 그대로 진행해주시면 좋을 것 같습니다
2단계가 훨씬 방대한 걸로 알고 있는데 기대하겠습니다~

Arrays.stream(methods)
.filter(this::supportParameters)
.filter(method -> method.isAnnotationPresent(RequestMapping.class))
.forEach(method -> putHandlerExecutions(handler, method));

Choose a reason for hiding this comment

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

https://tecoble.techcourse.co.kr/post/2020-05-14-foreach-vs-forloop/

foreach에 로직이 들어가면 안된다는데 어떻게 생각하시나요
근데 저도 foreach로 map에 넣는 작업을 수행하긴 했습니다

Copy link
Author

Choose a reason for hiding this comment

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

좋은 아티클 공유 감사합니다👍

for-loop보다 짧은 코드로 해결하려고 애써 외면하고 이 정도는 괜찮겠지 하고 타협했던 문제인데 충분히 로직을 빼고 스트림으로 해결할 수 있을 것 같네요. 2단계에서 리팩토링 해보겠습니다~

log.info("Initialized AnnotationHandlerMapping!");
}

private boolean supportParameters(final Method method) {

Choose a reason for hiding this comment

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

이거 좀 hip하네요 👍

@kevstevie kevstevie merged commit 451a7a8 into woowacourse:aak2075 Sep 13, 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