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단계] 그레이(김동욱) 미션 제출합니다. #359

Merged
merged 7 commits into from
Sep 14, 2023

Conversation

Kim0914
Copy link

@Kim0914 Kim0914 commented Sep 13, 2023

안녕하세요 아벨 ! 그레이입니다 😀

리뷰 잘 부탁드려요 🙇🏻

Copy link

@tjdtls690 tjdtls690 left a comment

Choose a reason for hiding this comment

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

안녕하세요 그레이! 아벨이에요. 이번 미션동안 잘 부탁 드려요 :)

AnnotationHandlerMappingTest 테스트를 통과하는 것을 기준으로 보고 리뷰했습니다. 나머지 file change가 일어난 건 저번 구구가 말했 fork할 때 최신화 문제로 보고 리뷰할 때 참고하지 않았습니다.

리플렉션을 정말 잘 사용해주었네요. 질문 하나 남겼는데, 머지 후에 답변 달아주세요! 일단 머지하도록 할게요!

Comment on lines +52 to +53
HandlerKey handlerKey = new HandlerKey(url, requestMethod);
handlerExecutions.put(handlerKey, new HandlerExecution(getObject(aClass), method));
Copy link

@tjdtls690 tjdtls690 Sep 14, 2023

Choose a reason for hiding this comment

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

HandlerKey를 따로 변수로 뺀 다음에 handlerExecutions.put 에 전달할거면 HandlerExecution도 같이 변수로 먼저 뺀 다음에 그 변수를 HandlerExecution.put 에 전달해주는 식으로 바꿔주면 일관성과 가독성을 좀 더 올릴 수 있을 것 같아요!

Suggested change
HandlerKey handlerKey = new HandlerKey(url, requestMethod);
handlerExecutions.put(handlerKey, new HandlerExecution(getObject(aClass), method));
HandlerKey handlerKey = new HandlerKey(url, requestMethod);
HandlerExecution handlerExecution = new HandlerExecution(getObject(aClass), method);
handlerExecutions.put(handlerKey, handlerExecution);

Copy link
Author

Choose a reason for hiding this comment

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

앗 놓쳤던 부분이네요 ㅎㅎ

꼼꼼하게 봐주셔서 감사합니다 :)
2단계 진행하며 반영하도록 하겠습니다 !

@tjdtls690 tjdtls690 merged commit 98b718b into woowacourse:kim0914 Sep 14, 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.

3 participants