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단계] 짱수(장혁수) 미션 제출합니다. #840

Open
wants to merge 17 commits into
base: zangsu
Choose a base branch
from

Conversation

zangsu
Copy link

@zangsu zangsu commented Sep 30, 2024

종이, 안녕하세요!
마지막 미션 제출이네요!!!

files changed 가 조금 많이 잡혔는데, 패키지 이동, 파일 삭제가 많아서 그렇습니다...ㅎㅎ

이번 리뷰도 잘 부탁드려요!!!!

@zangsu zangsu self-assigned this Sep 30, 2024
Copy link
Member

@dwax1324 dwax1324 left a comment

Choose a reason for hiding this comment

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

짱수 안녕하세요
구현 너무 잘해주셨네요👍👍
코멘트 남겼으니 확인해주세요!

Copy link
Member

@dwax1324 dwax1324 left a comment

Choose a reason for hiding this comment

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

짱수 안녕하세요!
코드가 전체적으로 많이 깔끔해지고 가독성이 좋아진게 느껴져요. 👍
점진적으로 리펙토링 되는 걸 보니 뭔가 뿌듯하네요. ☺️

(아래 테스트들이 통과하지 않고 있으니 확인 부탁드립니다!)
Screenshot 2024-10-08 at 20 37 51


@RequestMapping(value = "/api/user", method = RequestMethod.GET)
public ModelAndView show(HttpServletRequest request, HttpServletResponse response) {
final String account = request.getParameter("account");
Copy link
Member

Choose a reason for hiding this comment

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

"account" parameter가 2개 이상이면 어떻게 될까요?🤔

log.debug("Method : {}, Request URI : {}", request.getMethod(), requestURI);

try {
HandlerAdapter handlerAdapter = getHandlerAdapter(request);
Copy link
Member

Choose a reason for hiding this comment

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

어댑터 패턴을 사용했을 때 어떠한 장점이 있었나요?

HandlerAdapter handlerAdapter = getHandlerAdapter(request);
ModelAndView modelAndView = handlerAdapter.handle(request, response);
modelAndView.getView().render(modelAndView.getModel(), request, response);
} catch (Throwable e) {
Copy link
Member

Choose a reason for hiding this comment

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

Exception, Error, Throwable 의 차이에 대해 알고 계신가요??

.map(handlerMappingAdapter -> handlerMappingAdapter.getHandler(request))
.filter(Objects::nonNull)
.findFirst()
.orElseThrow(() -> new NoMatchedHandlerException(request));
Copy link
Member

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");
Copy link
Member

Choose a reason for hiding this comment

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

JspView이름에서 충분히 유추 가능하기 때문에 �확장자까지 주입할 필요는 없을거 같아요!

Comment on lines +36 to +39
String key = model.keySet().stream()
.findFirst()
.get();
return objectMapper.writeValueAsString(model.get(key));
Copy link
Member

Choose a reason for hiding this comment

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

values()를 사용하면 조금 더 깔끔할 거 같아요!

Comment on lines +33 to +37
handlerMappingAdapters = Stream.of(new AnnotationHandlerMappingAdapter(basePackage))
.map(adapter -> {
adapter.initialize();
return (HandlerMappingAdapter) adapter;
}).toList();
Copy link
Member

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();

요렇게 표현해볼 수도 있을 거 같아요!

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