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단계] 레오(차영호) 미션 제출합니다. #586

Merged
merged 18 commits into from
Sep 26, 2023

Conversation

youngh0
Copy link

@youngh0 youngh0 commented Sep 24, 2023

안녕하세요 오잉! 벌써 주말이 지나갔네요.. 깃이 좀 꼬여서 step2 커밋이 같이 들어와버렸네요.. b2a5d6a (feat: view render 로직 JspView 로 분리) ~ 마지막 까지가 step3 커밋입니다..

테스트 코드가 부족한 것 같아서 추가하려 했지만, 의미있는 테스트를 작성하기가 어렵네요.. 계속 고민하다간 기간 내에 리뷰 요청을 못 보낼 것 같아서 기능 구현은 다하고 리뷰 요청 보냅니다!

추가적으로, DispatcherServletInitializer 패키지 관련해서 질문이 있습니다. DispatchServletInitializer 도 mvc 모듈에 있는게 자연스러워 보여 옮기려고 했으나 실패 했는데요. 이유를 찾아보려 했는데 쉽지 않네요.. 혹시 mvc 모듈로 변경하면 동작하지 않는 원인을 아시나요..?

이번 주도 화이팅입니다 ㅎㅎ👊

@youngh0 youngh0 changed the title Step3 [MVC 구현하기 - 3단계] 레오(차영호) 미션 제출합니다. Sep 24, 2023
Copy link

@hanueleee hanueleee left a comment

Choose a reason for hiding this comment

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

안녕하세요 레오!!
3단계도 순식간에 완성하셨군요 굿굿👨‍🎤

코드 깔끔하게 짜주셔서 읽기 좋았습니다 ㅎㅎ
리뷰는 간단한 코멘트 위주로 남겼어요!

다음 요청 때 바로 merge하도록 하겠습니다!

추가적으로, DispatcherServletInitializer 패키지 관련해서 질문이 있습니다. DispatchServletInitializer 도 mvc 모듈에 있는게 자연스러워 보여 옮기려고 했으나 실패 했는데요. 이유를 찾아보려 했는데 쉽지 않네요.. 혹시 mvc 모듈로 변경하면 동작하지 않는 원인을 아시나요..?

저도 DispatcherSerlvetInitializer를 mvc모듈로 옮기고 싶어서 찾아봤는데요.. 복잡해보여서 포기했습니다 ㅋㅋ ㅜ (spring boot의 내장톰캣 쪽을 자세히 파봐야할 것 같아요..)

(DispatcherSerlvetInitializer 가 구현하고 있는) WebApplicationInitializer인터페이스 관련해서 좀 알아보았는데요!

해당 인터페이스를 구현하는 클래스를 작성해두면, 웹 어플리케이션을 시작할 때 서블릿 컨테이너(ex. Tomcat)가 자동으로 해당 클래스를 찾아 onStartUp메서드를 호출한다고 하네요..!

제 뇌피셜로는 현재 Tomcat관련 설정(TomcatStarter)이 app모듈에 위치한 상태라
DispatcherServletInitializer를 mvc모듈로 옮기게 되면 tomcat이 DispatcherServletInitializer를 못 찾는거 아닐까..?라고 생각합니다.

@@ -2,4 +2,5 @@

public class MediaType {

Choose a reason for hiding this comment

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

enum이 아니라 클래스로 뽑은 이유가 있나용??

Copy link
Author

Choose a reason for hiding this comment

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

이 부분은 기존 코드가 class 로 되어 있어서 PLAIN_TEXT_UTF8_VALUE 상수만 추가한겁니다!

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 {
response.setContentType(MediaType.APPLICATION_JSON_UTF8_VALUE);
PrintWriter writer = response.getWriter();

Choose a reason for hiding this comment

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

PrintWriter를 열고 사용한 후에 닫아주는 처리도 해주면 좋을 것 같아요!
try-with-resources를 사용해보는건 어떨까용?

Copy link
Author

Choose a reason for hiding this comment

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

오 꼼꼼한 리뷰 감사합니다!

response.setContentType(MediaType.APPLICATION_JSON_UTF8_VALUE);
PrintWriter writer = response.getWriter();
if (model.size() == 1) {
response.setContentType(MediaType.PLAIN_TEXT_UTF8_VALUE);

Choose a reason for hiding this comment

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

세심한 처리 👍

PrintWriter writer = response.getWriter();
if (model.size() == 1) {
response.setContentType(MediaType.PLAIN_TEXT_UTF8_VALUE);
for (Map.Entry<String, ?> entry : model.entrySet()) {

Choose a reason for hiding this comment

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

depth 2 👀

Copy link
Author

Choose a reason for hiding this comment

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

하하 들켰네요.. 수정했습니다~

@youngh0
Copy link
Author

youngh0 commented Sep 25, 2023

안녕하세요 오잉~ 빠른 리뷰 감사합니다!!

제 질문에 답변해주시기 위해 그렇게 많은 노력을 하실 줄 몰랐어요.. 정말 감사하면서도 미안한 마음이 드네요 ㅎㅎ 감동받았습니다 😎

오잉의 답변을 보고 구글링 좀 해봤는데 오잉의 뇌피셜이 맞는 것 같아요. 내장 톰캣을 사용하면 모르겠는데 현재 TomcatStarter 를 사용하면서 같은 모듈안에 위치해야 하는 것 같아요.

추가적으로, PrintWriter 의 close 관해서 오늘 저녁에 캠퍼스에서 들은게 있는데 아직 확실하지 않아서 내일 더 알아보고 찾아가겠습니다 ㅎㅎ

리뷰 감사합니다~~

Copy link

@hanueleee hanueleee left a comment

Choose a reason for hiding this comment

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

안녕하세요 레오!
이번 미션 너무너무 고생많으셨습니다ㅎㅎ

더 이상 수정할 사항이 보이지 않아 MVC미션은 여기서 마무리 할게요!
다음 미션도 화이팅팅팅👨‍🎤

@hanueleee hanueleee merged commit d205c7c into woowacourse:youngh0 Sep 26, 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