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단계] 애쉬(정설희) 미션 제출합니다. #598

Merged
merged 12 commits into from
Sep 27, 2023

Conversation

xxeol2
Copy link

@xxeol2 xxeol2 commented Sep 25, 2023

안녕하세요 썬샷☀️
3단계는 별다른 수정사항이 없네요!
대신 legacy -> MVC로 변환시키는 과정에서 파일체인지만 잔뜩인 것 같은..
중요한 변경점은 DispatcherServlet을 mvc 모듈로 이동시켰습니다!
기존에 DispatcherServlet안에서 HandlerMapping과 HandlerAdatper를 생성해줬는데
그 대신 DispatcherServletInitializer의 onStartup 메서드에서 생성해주고 init해주었습니다!
이번에도 좋은 리뷰 부탁드립니다 🙇🏻‍♀️

Copy link

@Ohjintaek Ohjintaek left a comment

Choose a reason for hiding this comment

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

3단계도 수고하셨습니다 애쉬!!
mvc 모듈에서 app 모듈을 모르도록 잘 구현해주신 점이 인상적이었습니다.😆😆
mvc.asis 패키지에 있었던 FrontController가 사라지면서 http://localhost:8080으로 진입했을 때 아무것도 응답하지 않는 부분과 몇몇 HTTP 요청이 제대로 매핑되지 않는 부분이 있어 확인해주셨으면 합니다
리뷰 요청 마지막 날이라 최대한 빨리 리뷰하려고 했는데 속도가 좀 느리네요... 죄송합니다

@@ -18,11 +21,13 @@ public class DispatcherServletInitializer implements WebApplicationInitializer {
@Override
public void onStartup(final ServletContext servletContext) {
final var dispatcherServlet = new DispatcherServlet();

dispatcherServlet.addHandlerMapping(new AnnotationHandlerMapping(getClass().getPackage().getName()));

Choose a reason for hiding this comment

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

mvc 모듈에 있는 DispatcherServlet이 app 모듈에 있는 요소를 모르도록 신경써서 잘 구현해주신 것 같아요!!
저도 어떻게 Controller들이 구현된 패키지를 전달할지 고민했었는데 우연히AnnotationHandlerMapping에 아무 것도 넣지 않았는데도 작동하더라구요???Reflection의 생성자에 아무런 값도 전달되지 않았을 때 ClassLoader에서 읽어오는 방식인 것 같은데 신기했습니다

Copy link
Author

Choose a reason for hiding this comment

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

오 진짜 그렇네요... 너무 신기해요 ㅋㅋㅋ
덕분에 개념 하나 더 배워갔어요 감사해요 👍


private static final Logger log = LoggerFactory.getLogger(LoginController.class);

@Override
public String execute(final HttpServletRequest req, final HttpServletResponse res) throws Exception {
@RequestMapping(method = RequestMethod.GET)

Choose a reason for hiding this comment

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

주어진 정적 파일에서 로그인 페이지를 얻는 HTTP 요청과 매핑이 안 되서 호출되지가 않아요

Copy link
Author

Choose a reason for hiding this comment

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

직접 localhost:8080/login으로 접속해 테스트하고, 대시보드에 로그인/로그아웃 버튼이 있는걸 인지하지 못해서 해당 부분을 신경못썼네요 🥲
반영하겠습니다!!

.orElse(convertToView("/login.jsp"));
}

private ModelAndView convertToView(String viewName) {

Choose a reason for hiding this comment

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

메서드 분리 좋아요👍👍

public String execute(final HttpServletRequest req, final HttpServletResponse res) throws Exception {
public class LogoutController {

@RequestMapping(method = RequestMethod.GET, value = "/view")

Choose a reason for hiding this comment

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

요것도 주어진 정적 파일에서 로그아웃 하는 HTTP 요청이랑 매핑이 안 되서 작동하지 않네요😅

Copy link
Author

Choose a reason for hiding this comment

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

#598 (comment)

이 부분도 수정했습니다!!


dispatcherServlet.addHandlerMapping(new AnnotationHandlerMapping(getClass().getPackage().getName()));
dispatcherServlet.addHandlerAdapter(new HandlerExecutionHandlerAdapter());
dispatcherServlet.init();

Choose a reason for hiding this comment

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

여기서 dispatcherServlet의 init()을 호출해주는데 Servlet Container(ex. tomcat)가 Servlet의 생성 주기를 관리하니까 호출해주지 않아도 되지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

그렇네요! DispatcherServlet도 Servlet이니 ServletContainer에서 생명주기를 관리하겠군요!!
꼼꼼하게 봐주셔서 감사합니다!!

public String getName() {
return null;
}
private String convertToJson(Map<String, ?> model) throws JsonProcessingException {

Choose a reason for hiding this comment

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

이터레이터를 이용해서 요소 하나만 빼내는 걸 멋지게 구현해주셨네요👍👍
하지만 render에서 직접 사용해주지 않아서 요구사항이 충족이 되지 않고 있어요!!
(사실 이 부분 요구사항이 정확히 어떤 의미인지 저도 잘 모르겠어서 현재 방식도 괜찮을 것 같긴해요)

Copy link
Author

Choose a reason for hiding this comment

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

리팩토링 해놓고 stash를 해버렸네요... 🥹 다시 pop해서 푸시했습니다!
그런데 현재 UserController에서 Model에 데이터가 2개 이상인 경우가 언제 있는지 잘 모르겠어요..
혹시 썬샷은 알고 계신가요?!

Choose a reason for hiding this comment

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

현재는 없긴 한데 구구가 강의 때 Json 출력 형식을 맞추기 위한 요구사항이라고 해주셨어요. 저는 Json 요소가 여러 개일 때와 한 개일 때 괄호의 개수를 조정해주기 위한 거라고 이해했어요.

@xxeol2
Copy link
Author

xxeol2 commented Sep 27, 2023

꼼꼼하게 직접 실행까지 하면서 리뷰해주셔서 감사합니다 👍
모두 반영 완료했습니다!

Copy link

@Ohjintaek Ohjintaek left a comment

Choose a reason for hiding this comment

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

mvc 미션 수고하셨어요~ 애쉬👍👍
요구사항대로 다 잘 작동하고 제가 생각해기에 미션에서 의도한 것들을 다 잘 구현해주셔서 Approve하겠습니다!! 다음 미션도 화이팅입니다.
내일부터 긴 연휴인데 행복한 추석 보내세요😆

@Ohjintaek Ohjintaek merged commit a026beb into woowacourse:xxeol2 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