-
Notifications
You must be signed in to change notification settings - Fork 305
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
Conversation
There was a problem hiding this 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"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redirect가 필요한가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 동작상으로는 /login.jsp로 바로 보내도 동일하게 동작합니다!
하지만 리다이렉트를 했을 때 다시 필터나 인터셉터단을 통과시키도록 하려면 분기점이 필요할 것 같아서요.
제거하는 것이 더 장점일 지 모르겠어서 놔뒀습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 JspView가 모두 redirect를 통해 응답하는 이유가 있었군요! 좋은 것 같습니다!
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); |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존 WebApplicationInitializer
와 ServletContainerInitializer
는 어떤 차이가 있나요?
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 등록이 잘 되었는지 쉽게 파악 할 수 있겠네요
SonarCloud Quality Gate failed.
|
생각해볼 만한 부분 남겨주셔서 도움이 많이 됐어요!! 아, 매번 좋은 리뷰 감사히 먹고 있습니다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
남겨주신 DispatcherServletInitializer
등록과 관련하여 더 좋은 방법은 모르지만, 만약 기존 인터페이스를 유지한채로 옮긴다면 수업내용처럼 조금 더 작업이 필요할 것 같습니다..!
TomcatStarter
과 WEBAPP/WEB-INF
와 연관이 있어보이더라구요..?
우선 런타임에 잘 동작하기 위해 바로 등록 하신 방법도 괜찮습니다!
연어, 빠르게 리뷰 적용해주셨네요. 질문에 답변남겨주셔서 감사드리고, 이만 머지하도록 하겠습니다!! 고생하셨어요👍👍 |
안녕하세요 로이스!
최근에 너무 바빠서 퀄리티가 낮은 상태로 제출하는 것 같네요ㅠㅠ
구현한 내용은 아래와 같습니다.
잘 부탁드립니다...! 🥹