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 구현하기 - 3단계] 여우(조승현) 미션 제출합니다 #618

Merged
merged 10 commits into from
Sep 27, 2023

Conversation

BackFoxx
Copy link

안녕하세요 제이미!
제 코드 완전 빵구같아요
그래도 잘 부탁드려요

Copy link
Member

@JJ503 JJ503 left a comment

Choose a reason for hiding this comment

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

여우 이번 코드도 잘 봤습니다!
역시 여우네요 🦊 👍

그런데 제가 지난번에 놓쳤던 부분과 이번 미션에서 추가로 작성해 주신 부분들 중 궁금한 내용이 있어 질문드립니다!
해당 부분들만 확인해 주시면 바로 머지하도록 하겠습니다 ㅎㅎ

이번 미션도 고생 많으셨습니다~!

final String viewPath = handlerAdapter.invoke(handler, request, response);
move(viewPath, request, response);
final ModelAndView modelAndView = handlerAdapter.invoke(handler, request, response);
modelAndView.getView().render(modelAndView.getModel(), request, response);
Copy link
Member

Choose a reason for hiding this comment

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

view와 model을 가져오지 않고 ModelAndView에서 render를 수행해주는 방향으로 가면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

오,, 캡슐화!

@Override
public void render(final Map<String, ?> model, final HttpServletRequest request, HttpServletResponse response) throws Exception {
public void render(final Map<String, ?> model, final HttpServletRequest request, HttpServletResponse response)
Copy link
Member

Choose a reason for hiding this comment

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

저는 해당 결과가 아래와 같이 출력되네요... 🤔
image

Copy link
Author

Choose a reason for hiding this comment

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

앗 model 안에 값이 하나 있을 때랑 여러 개 있을 때의 분기처리가 중복으로 발생했네요! 수정했습니다

import webmvc.org.springframework.web.servlet.mvc.HandlerAdapter;
import webmvc.org.springframework.web.servlet.mvc.tobe.AnnotationExceptionHandlerMapping;
import webmvc.org.springframework.web.servlet.mvc.tobe.AnnotationHandlerMapping;
import webmvc.org.springframework.web.servlet.mvc.tobe.HandlerExecutionHandlerAdapter;
import webmvc.org.springframework.web.servlet.view.JspView;

public class DispatcherServlet extends HttpServlet {
Copy link
Member

Choose a reason for hiding this comment

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

DispatcherServlet에서 필요한 필드인 exceptionHandlerMappings, handlerMappings, handlerAdaptersDispatcherServletInitializer에서 주입 시켜주시는 것에 대해 어떻게 생각하시나요?

Copy link
Member

Choose a reason for hiding this comment

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

추가적으로 미션 2 힌트를 보셨을까요?
해당 미션 힌트에는HandlerAdapterRegistryHandlerMappingRegitry가 있었는데, 적용하지 않은 이유가 있을까요?
적용한다면 findHandler, findHandlerAdaptor 역할을 넘길 수 있을지도..?

Copy link
Author

Choose a reason for hiding this comment

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

2단계 진행 도중 힌트가 삭제돼서 Registry의 존재를 잊고 있었네요!
Registrty들을 사용해서 HandlerMapping들과 HandlerAdapter에 대한 관리 책임을 외부로 옮겼습니다.
혹시 DispatcherServlet 입장에서는
본인이 사용하는 객체에 대한 책임이 외부로 옮겨지는 것이니
이런 걸 IOC라고 부르는 걸까요? IOC가 무엇인지 개념이 잘 안 잡혀 있어서.. 🥹

Copy link
Member

Choose a reason for hiding this comment

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

IOC 중 DI에 대한 이야기였습니다.
사실 의견을 여쭤본 이유가 예전 레벨 1, 2 때는 보통 객체의 필드들을 외부에서 생성해 의존성 주입을 시켜줬던 기억이 있어 저는 그렇게 진행하게 되었습니다.
그런데 여러 pr을 보다보니 저와 비슷하게 진행한 크루들도 있고 그렇지 않은 크루들도 있는 것 같아 여우의 의견도 궁금해 질문드리게 되었습니디!

Comment on lines +20 to +25
if (returnValue instanceof String) {
return new ModelAndView(new JspView((String) returnValue));
}
if (returnValue instanceof ModelAndView) {
return (ModelAndView) returnValue;
}
Copy link
Member

Choose a reason for hiding this comment

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

제 경우 ModelAndView가 아닌 String으로 Controller에서 전달되는 것은 레거시 코드라 이해했는데 여우의 경우 String도 가능하도록 해주셨네요!
혹시 이에 대한 이유가 있으셨을까요?

Copy link
Author

Choose a reason for hiding this comment

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

이게 사실 어떻게 해야 할지 많이 헷갈린 부분인데요,
우리가 스프링 부트에서 컨트롤러를 만들 때는 String을 반환하기도 하고 막 ResponseEntity를 반환하기도 하고 그래도 핸들러어댑터에서 적절하게 변환 작업을 해주잖아요!
그래서 String을 반환하는 컨트롤러는 레거시이니 삭제를 해주어야 할지, 아니면 String과 ModelAndView 두 가지 중 어떤 걸 반환해도 정상 작동하도록 호환해주어야 할지 고민을 많이 했어요.
그러다 결정한 게 위 코드이구요!
그런데 if문 두개로 좌자작 되어있는 게 많이 못생겨보여서,
어떻게 바꾸어야 더 깔끔할지는 앞으로 더 고민해보아야 할 것 같습니다.

Copy link
Author

@BackFoxx BackFoxx left a comment

Choose a reason for hiding this comment

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

정성스러운 피드백 남겨주셔서 정말 고마워요 제이미!
뭔가 스스로 보기에도 고칠 부분이 엄청 많아 보여서 대공사를 하고 싶었지만,
일정이.. 일정이 너무너무 쌓여있어요 🥹
그래서 피드백에서 짚어주신 포인트만 딱 고치고 다시 제출하게 되었습니다.
마지막 리뷰 잘 부탁드려요!

Copy link
Member

@JJ503 JJ503 left a comment

Choose a reason for hiding this comment

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

여우 답변 감사합니다!

이번 3단계도 요구 사항을 모두 만족하였기에 이만 머지합니다!
3단계 동안 여우의 코드를 보며 많이 배울 수 있었습니다.
여우 짱! 🦊

바쁜 기간동안 고생 많으셨습니다.
다음 미션도 파이팅하세요!

@JJ503 JJ503 merged commit 3b4440e into woowacourse:backfoxx Sep 27, 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