-
Notifications
You must be signed in to change notification settings - Fork 299
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단계] 짱수(장혁수) 미션 제출합니다. #840
base: zangsu
Are you sure you want to change the base?
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.
짱수 안녕하세요
구현 너무 잘해주셨네요👍👍
코멘트 남겼으니 확인해주세요!
mvc/src/test/java/com/interface21/webmvc/servlet/mvc/framework/DispatcherServletTest.java
Show resolved
Hide resolved
mvc/src/main/java/com/interface21/webmvc/servlet/view/JsonView.java
Outdated
Show resolved
Hide resolved
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.
|
||
@RequestMapping(value = "/api/user", method = RequestMethod.GET) | ||
public ModelAndView show(HttpServletRequest request, HttpServletResponse response) { | ||
final String account = request.getParameter("account"); |
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.
"account" parameter가 2개 이상이면 어떻게 될까요?🤔
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.
그런 경우 첫번째로 전달된 파라미터의 값만 확인해주고 있네요.
그런데, 또 API 자체가 하나의 account 파라미터를 지원한다고 명시했을 때 여러개의 쿼리 파라미터 요청을 고려해 주는 것이 맞을지,,, 고민은 됩니다.
log.debug("Method : {}, Request URI : {}", request.getMethod(), requestURI); | ||
|
||
try { | ||
HandlerAdapter handlerAdapter = getHandlerAdapter(request); |
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.
어댑터 패턴을 사용했을 때 어떠한 장점이 있었나요?
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.
장점... 이라기 보단 어댑터 이외의 방법을 찾지 못했다 에 가까운 것 같아요 ㅎㅎㅎ
ManualXXX
와 AnnotationXXX
와 함께 사용하기 위해선 추상화가 필요했는데요.
레거시였던 ManualXXX
의 코드를 변경하지 않기 위해서는 인터페이스를 implements
할 수 없었고, 때문에 어댑터 패턴을 사용해 동일한 인터페이스를 구현할 수 있었습니다!
HandlerAdapter handlerAdapter = getHandlerAdapter(request); | ||
ModelAndView modelAndView = handlerAdapter.handle(request, response); | ||
modelAndView.getView().render(modelAndView.getModel(), request, response); | ||
} catch (Throwable e) { |
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.
Exception
, Error
, Throwable
의 차이에 대해 알고 계신가요??
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.
Throwable
은 Error
와 Exception
을 포함하는 상위 개념이군요!
Error
는 시스템 레벨에서 발생하며 개발자가 조치할 수 없는 수준의 문제를, Exception
은 로직상에서 발생하며 다른 방식으로 처리가 가능한 문제를 말합니다!
지금은 로직 상에서 문제가 발생하는 Exception
에 대한 처리가 필요한 부분인 것 같네요.
처리 로직도 런타임 예외로 래핑하여 밖으로 던지고 있으므로 해당 부분 변경해 두겠습니다!
.map(handlerMappingAdapter -> handlerMappingAdapter.getHandler(request)) | ||
.filter(Objects::nonNull) | ||
.findFirst() | ||
.orElseThrow(() -> new NoMatchedHandlerException(request)); |
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.
처리할 핸들러가 없을 때 상태코드 500와 에러페이지를 보여 주고 있습니다.
사용자가 알 수 있도록 올바른 상태코드와 페이지를 보여주는 것이 어떨까요?
JspView view = UserSession.getUserFrom(req.getSession()) | ||
.map(user -> { | ||
log.info("logged in {}", user.getAccount()); | ||
return new JspView("redirect:/index.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
이름에서 충분히 유추 가능하기 때문에 �확장자까지 주입할 필요는 없을거 같아요!
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.
파라미터로 전달되는 viewName
의 값을 말하시는거죠?
하지만, 이 부분은 "파일 이름" 을 전달하는 부분이라 확장자가 없는 것이 더 어색할 것 같은데,,, 어떻게 생각하실까요?? 🤔
String key = model.keySet().stream() | ||
.findFirst() | ||
.get(); | ||
return objectMapper.writeValueAsString(model.get(key)); |
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.
values()
를 사용하면 조금 더 깔끔할 거 같아요!
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.
values()
를 사용하더라도 첫번째 원소를 가지고 오는 로직이 동일하더라고요!
그래서 이 부분은 현상 유지 하였습니다!
handlerMappingAdapters = Stream.of(new AnnotationHandlerMappingAdapter(basePackage)) | ||
.map(adapter -> { | ||
adapter.initialize(); | ||
return (HandlerMappingAdapter) adapter; | ||
}).toList(); |
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.
handlerMappingAdapters = Stream.of(new AnnotationHandlerMappingAdapter(basePackage))
.peek(AnnotationHandlerMappingAdapter::initialize)
.map(adapter -> (HandlerMappingAdapter) adapter)
.toList();
요렇게 표현해볼 수도 있을 거 같아요!
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.
와! peek()
!!!
너무 저에게 필요하던 메서드에요 😭
알려주셔서 감사합니닷...!
종이, 안녕하세요!
마지막 미션 제출이네요!!!
files changed 가 조금 많이 잡혔는데, 패키지 이동, 파일 삭제가 많아서 그렇습니다...ㅎㅎ
이번 리뷰도 잘 부탁드려요!!!!