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단계] 연어(황재현) 미션 제출합니다. #607

Merged
merged 7 commits into from
Sep 26, 2023

Conversation

nuyh99
Copy link

@nuyh99 nuyh99 commented Sep 25, 2023

안녕하세요 로이스!
최근에 너무 바빠서 퀄리티가 낮은 상태로 제출하는 것 같네요ㅠㅠ

구현한 내용은 아래와 같습니다.

  • View 구현(JSP는 이전 단계에서 해둬서 JSON 구현)
  • 기존 컨트롤러 모두 어노테이션 기반으로 변경
  • DispatcherServlet mvc 패키지로 변경
    • 기존에는 SpringContainer 어쩌고에서 DispatcherServlet을 초기화시키고 있었는데 해당 부분 삭제 후 DispatcherServlet을 바로 초기화

잘 부탁드립니다...! 🥹

@nuyh99 nuyh99 self-assigned this Sep 25, 2023
Copy link
Member

@TaeyeonRoyce TaeyeonRoyce left a comment

Choose a reason for hiding this comment

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

연어, 바쁘셨을텐데 요구사항를 만족하며 코드를 잘 작성하셨네요!
구조에 대해서도 잘 이해하고 계신 것 같고, 잘 작성해주셔서 거청하게 리뷰 드릴게 없네요..!

몇가지 코멘트와 질문 남겨두었는데, 이 부분만 확인해주시면 될 것 같습니다!

})
.orElse("/login.jsp");
.orElse(new ModelAndView(new JspView("redirect:/login.jsp")));
Copy link
Member

Choose a reason for hiding this comment

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

redirect가 필요한가요?

Copy link
Author

Choose a reason for hiding this comment

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

현재 동작상으로는 /login.jsp로 바로 보내도 동일하게 동작합니다!

하지만 리다이렉트를 했을 때 다시 필터나 인터셉터단을 통과시키도록 하려면 분기점이 필요할 것 같아서요.
제거하는 것이 더 장점일 지 모르겠어서 놔뒀습니다!

Copy link
Member

Choose a reason for hiding this comment

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

현재 JspView가 모두 redirect를 통해 응답하는 이유가 있었군요! 좋은 것 같습니다!

Comment on lines 23 to 28
final List<?> values = new ArrayList<>(model.values());
if (values.size() == 1) {
objectMapper.writeValue(writer, values.get(0));
}
if (values.size() > 1) {
objectMapper.writeValue(writer, model);
Copy link
Member

Choose a reason for hiding this comment

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

early return이 좋아보여요

public class DispatcherServletInitializer implements WebApplicationInitializer {
import java.util.Set;

public class DispatcherServletInitializer implements ServletContainerInitializer {
Copy link
Member

Choose a reason for hiding this comment

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

기존 WebApplicationInitializerServletContainerInitializer는 어떤 차이가 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

미션에서는 스프링처럼 SpringServletContainerInitializer에서 WebApplicationInitializer를 핸들링하는 방식으로 처리하고 있더라구요.

현재의 구조에서는 필요할까? 싶어서 DispatcherServletInitializer를 톰캣이 바로 초기화시켜주도록 했습니다!
resources/META-INF/services에서 바로 초기화시켜주도록 클래스를 설정했습니다.

@@ -38,6 +38,7 @@ public void initialize() {
controllerMethods.forEach(method -> addHandlerExecutions(method, controller));
}

handlerExecutions.keySet().forEach(handlerKey -> log.info("{}", handlerKey));
Copy link
Member

Choose a reason for hiding this comment

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

👍 등록이 잘 되었는지 쉽게 파악 할 수 있겠네요

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability B 1 Vulnerability
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

70.1% 70.1% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@nuyh99
Copy link
Author

nuyh99 commented Sep 26, 2023

생각해볼 만한 부분 남겨주셔서 도움이 많이 됐어요!!

아, Initializer 부분은 사실 DispatcherServletInitializer를 내렸을 때 런타임에 찾지를 못해서 바로 등록시킨 것도 있습니다.
혹시 좋은 방법이 있다면...알려주실 수 있을까요??

매번 좋은 리뷰 감사히 먹고 있습니다.
로이스 최고에요👍👍👍

Copy link
Member

@TaeyeonRoyce TaeyeonRoyce left a comment

Choose a reason for hiding this comment

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

남겨주신 DispatcherServletInitializer 등록과 관련하여 더 좋은 방법은 모르지만, 만약 기존 인터페이스를 유지한채로 옮긴다면 수업내용처럼 조금 더 작업이 필요할 것 같습니다..!
TomcatStarterWEBAPP/WEB-INF와 연관이 있어보이더라구요..?

우선 런타임에 잘 동작하기 위해 바로 등록 하신 방법도 괜찮습니다!

@TaeyeonRoyce
Copy link
Member

연어, 빠르게 리뷰 적용해주셨네요.
처음 작성해주셨던 코드가 좋았어서 크게 코멘트 드리지 않았어요!

질문에 답변남겨주셔서 감사드리고, 이만 머지하도록 하겠습니다!! 고생하셨어요👍👍

@TaeyeonRoyce TaeyeonRoyce merged commit bf7807c into woowacourse:nuyh99 Sep 26, 2023
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