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 구현하기 2단계] 포이(김보준) 미션 제출합니다. #446

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

poi1649
Copy link

@poi1649 poi1649 commented Sep 16, 2023

이오 안녕하세요!
먼저 이전 풀리퀘스트에서 있었던 질답을 이어서 적고 가겠습니다.

reflection이 어떤 방식으로 사용되었냐에 따라 다르겠지만, reflection을 사용해 어떤 값을 얻고자 하는 해당 클래스에서 처리해야 한다고 생각합니다. 이번 미션의 코드를 예로 들면 AnnotationHandlerMapping이 TestController에서 reflection을 사용해 존재하지 않는 필드를 찾아내려 시도한다면, AnnotationHandlerMapping 클래스에서 예외를 처리해 줘야 한다고 생각해요.
질문의 의도가 이게 맞으신지 모르겠네요😂😂 포이는 어떻게 생각하시나요?

이오가 답변해주신 부분이 정확하게 여쭤보고 싶었던 내용입니다! 다만 여기서 터뜨려도 사용자가 잘 확인할 수 있을까가 궁금해서 예외를 여기저기서 터뜨려봤는데 500 페이지에 어떤 예외인지도 잘 나오네요ㅋㅋㅋ... 답변 감사합니다!

아마 HandlerExecution을 말씀하신 것 같아요! 저는 HandlerExecution은 외부에서 들어오는 값이 Controller 클래스인지 아닌지와 상관 없이 동작한다고 생각합니다. Controller가 아니더라도 사용 가능성이 열려있고, 따라서 Controller를 모르는 편이 관심사를 분리하는 데에 적절하다고 느껴요. 즉 현재의 구조가 좋다고 생각합니다! 👍

이 부분은 이번 단계를 진행하며 확실히 느끼게 됐네요! 다른 형태를 가지는 handler에 대해서 처리하기 위해서는 확실히 클래스를 국한하지 않는게 좋을 것 같습니다! 👍


이번 단계에서 구성한 흐름은 다음과 같습니다.

sequenceDiagram
    Participant User as 사용자
    Participant DS as DispatcherServlet
    Participant HM as HandlerMapping
    Participant HA as HandlerAdapter
    Participant Controller as 컨트롤러

    User->>DS: 요청
    DS->>HM: 해당 요청의 핸들러 조회
    HM-->>DS: 핸들러 정보 전달
    DS->>HA: 핸들러 실행 요청
    HA->>Controller: 해당 핸들러 실행
    Controller-->>HA: 응답 생성
    HA-->>DS: 응답 반환
    DS-->>User: 최종 응답 전송
Loading

이 과정에서 아직 RequestDispatcher 를 사용하는 기존 코드는 유지하였습니다.
정확히는 이를 어떻게 수정해야할지 감이 안와서 놔뒀습니다.. 3단계에 뷰에 대한 책임을 나누는 요구사항이 있던데 이 과정을 진행하면서 알아봐야할 것 같아요!
패키지 또한 3단계 요구사항에 분리요청이 있어 적당히 배치해놨습니다!
그래서 이번 리뷰에서는 HandlerMapping과 Adapter의 생성, 사용위치, 추상화에 대해 집중적으로 이야기해보고 싶습니다!
또 글이 매우 길어졌네요..ㅋㅋㅋ

오늘도 좋은 하루 보내세요~~🫡

@LJW25
Copy link

LJW25 commented Sep 18, 2023

안녕하세요 포이!
제가 오늘 근로와 회의등이 있는 관계로, 오늘 저녁에 리뷰가 가능할 것 같습니다.
10시 쯔음 예정인데, 더 늦어진다면 다시 말씀드리겠습니다. 😃

Copy link

@LJW25 LJW25 left a comment

Choose a reason for hiding this comment

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

안녕하세요 포이!
미션 구현하느라 수고하셨습니다 👏👏

추상화를 굉장히 잘 하셨고, 구조도를 남겨주셔서 이해하기에 편했던 것 같아요.
구조적으로는 부족한 점이 느껴지지 않아 자잘한 컨벤션(final)과 궁금한 부분을 남겨놓았습니다.

한가지 걸렸던 ModelAndViewviewName을 넣고 이후 렌더링때 다시 꺼내오는 구조는, 현재 3단계를 피하면서 2단계를 구현하기 위해서는 최선의 구조였다고 생각합니다. 🫠

추가로 시간 여유가 되신다면 테스트도 구현해보면 어떨까 싶어요!

이번 리뷰에서 이야기해보고 싶은 부분이 있다고 하셔서, 혹여 추가로 주고받을 의견이 있을까 싶어 request change 하겠습니다.
바쁜 기간 화이팅 하시고 좋은 하루 보내세요 😊

Comment on lines 24 to 26
public DispatcherServlet(List<HandlerMapping> handlerMappings, List<HandlerAdapter> handlerAdapters) {
this.handlerMappings = handlerMappings;
this.handlerAdapters = handlerAdapters;
Copy link

Choose a reason for hiding this comment

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

#컨벤션 통일한다면 여기 매개변수에도 final을 붙이는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

배우고 싶을 정도로 꼼꼼하시네요😎 감사합니다!!!

app/src/main/java/com/techcourse/DispatcherServlet.java Outdated Show resolved Hide resolved
Comment on lines 16 to 22
@RequestMapping(value = "/register", method = RequestMethod.GET)
public ModelAndView show(final HttpServletRequest req, final HttpServletResponse res) {
return new ModelAndView(new JspView("/register.jsp"));
}

@RequestMapping(value = "/register", method = RequestMethod.POST)
public ModelAndView save(final HttpServletRequest req, final HttpServletResponse res) {
Copy link

Choose a reason for hiding this comment

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

AnnotationHandlerMapping에서 클래스 단위의 경로인 parentPath까지 구현 하신걸로 알고있는데, 이를 활용해보는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

엇.. 왜 제가 구현해놓고 까먹었네요ㅋㅋㅋㅋㅋㅋㅋ 바로 등록했습니다 ㅎㅎ..

Comment on lines 9 to 17
@Override
public boolean supports(Object handler) {
return handler instanceof HandlerExecution;
}

@Override
public ModelAndView handle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception {
return ((HandlerExecution) handler).handle(request, response);
}
Copy link

Choose a reason for hiding this comment

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

#컨벤션 여기도 매개변수에 final 을 사용해보는건 어떨까요?

  • SimpleControllerHandlerAdapter도 동일합니다.

Copy link
Author

Choose a reason for hiding this comment

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

앗... 프로젝트 때문에 기본 설정을 바꿔놨더니 자주 놓치게 되네요.. ㅠㅠ 감사합니다!

Comment on lines 20 to 22
final var handlerMappings = HandlerMappingFactory.getHandlerMappings();
final var handlerAdapters = HandlerAdapterFactory.getHandlerAdapters();
final var dispatcherServlet = new DispatcherServlet(handlerMappings, handlerAdapters);
Copy link

Choose a reason for hiding this comment

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

DispatcherServlet에서 init 메서드가 존재하는데, init에서 handlerMappingshandlerAdapters를 생성하지 않고 외부에서 주입해주는 이유가 궁금해요! 😄

Copy link
Author

Choose a reason for hiding this comment

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

스터빙을 통해 테스트를 쉽게 하려는 의도였습니다! (테스트 0개)
아무래도 스프링의 구조처럼 beanFactory처럼 bean을 프로필마다 다르게 불러올 수 있다면 테스트에도 문제가 없겠지만, 이를 구현하지 않으면 테스트가 어려워질 것 같았습니다.

그렇지만 확실히 DispatcherServlet에서만 사용하는 클래스들을 밖에서 주입하는 것도 조금 이상하게 느껴지긴 하네요! 이를 basePackage를 주입받아 초기화하는 식으로 변경하였습니다!

Copy link

@LJW25 LJW25 left a comment

Choose a reason for hiding this comment

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

안녕하세요 포이!

피드백 드린 부분 모두 잘 반영해주셨네요! 어프루브 하겠습니다. 😊

다음 단계에서 다시 뵙겠습니다 :)

@LJW25 LJW25 merged commit c828142 into woowacourse:poi1649 Sep 20, 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