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단계] 루쿠(백경환) 미션 제출합니다. #621

Merged
merged 11 commits into from
Sep 30, 2023

Conversation

aiaiaiai1
Copy link

@aiaiaiai1 aiaiaiai1 commented Sep 25, 2023

안녕하세요 베로!
기능 요구사항에 명시되어있는 내용으로
jsp,json 뷰를 구현하고 레거시mvc컨트롤러를 어노테이션 기반 컨트롤러로 리펙터링 해주었습니다!
미션에 신경을 많이 못써 기능 요구사항 중심적으로 해보았습니다 리뷰 감사합니다!

2단계 PR
2단계 리뷰 반영 커밋
3단계 커밋

Copy link

@Cyma-s Cyma-s left a comment

Choose a reason for hiding this comment

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

안녕하세요, 루쿠!
미션 수고 많으셨어요 👍
고칠 부분이라기보다는 루쿠의 의견이 궁금한 부분이 몇 곳 있어서 Request Change 드렸습니다 :)
다음 리뷰 때 머지하도록 할게요! 시간 되실 때 반영해주세요 😊

}

private String login(final HttpServletRequest request, final User user) {
private ModelAndView tryLogin(final HttpServletRequest request, final User user) {
Copy link

Choose a reason for hiding this comment

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

메서드 오버로딩을 사용하셨는데, 따로 메서드로 분리하지 않고 오버로딩을 하신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

logintryLogin을 서로 메서드 명을 바꾸어 준다는 것이 한 곳만 수정이 되었네요! 수정하였습니다!

@RequestMapping(value = "/login", method = RequestMethod.POST)
public ModelAndView tryLogin(final HttpServletRequest request, final HttpServletResponse response) throws Exception {
if (UserSession.isLoggedIn(request.getSession())) {
return new ModelAndView(new JspView("redirect:/index.jsp"));
Copy link

Choose a reason for hiding this comment

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

저는 JspView 에서는 JSP 파일로 이동시켜주는 역할만 해야 한다고 생각해서 redirect 는 다른 뷰로 분리하는 게 좋다고 생각해서 분리했었습니다! 이 부분에 대해 루쿠는 어떻게 생각하시나요? 😄

Copy link
Author

Choose a reason for hiding this comment

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

저도 redirect의 역할 분리를 위해 클래스를 생성 하는게 좋을거 같다는 쪽의 의견입니다!
근데 JspRedirectView, JsonRedirectView 와 같은 클래스 분리를 떠올렸었는데
이러면 품이 좀 들거같다는 생각이 들어서 고냥 이대로 진행하게 되었습니다~

import webmvc.org.springframework.web.servlet.view.JsonView;

@Controller
public class UserController {
Copy link

Choose a reason for hiding this comment

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

UserController 를 추가해주셨네요!
컨트롤러를 따로 분리한 이유가 궁금해요!

Copy link
Author

Choose a reason for hiding this comment

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

미션 힌트를 바탕으로 해당 컨트롤러를 추가해서 어노테이션 mvc가 정상적으로 동작하는지 테스트하기 위해 추가하였습니다~!


public class JsonView implements View {

@Override
public void render(final Map<String, ?> model, final HttpServletRequest request, HttpServletResponse response) throws Exception {
private final ObjectMapper objectMapper;
Copy link

Choose a reason for hiding this comment

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

현재로서는 ObjectMapper 가 매번 생성되고 있는데, ObjectMapper 를 재사용하면 어떻게 될까요? 재사용하게 되면 어떤 일이 발생할 수 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

생성 비용이 비싸서 성능 이유가 생길 수 있고 또한 Thread-safe라 또 매번 생성할 필요가 없었군요!
몰랐던 내용인데 감사합니다! 👍 👍

Comment on lines +70 to +74
Object handler = handlerMappings.stream()
.filter(handlerMapping -> handlerMapping.isMatch(request))
.findAny()
.orElseThrow(() -> new IllegalArgumentException("지원하지 않는 요청입니다."))
.getHandler(request);
Copy link

Choose a reason for hiding this comment

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

👍👍👍👍

@@ -28,12 +26,10 @@ public DispatcherServlet() {

@Override
public void init() {
handlerMappings.add(new ManualHandlerMapping());
handlerMappings.add(new AnnotationHandlerMapping("com.techcourse.controller"));
handlerMappings.add(new AnnotationHandlerMapping(ANNOTATION_BASE_PACKAGE));
Copy link

Choose a reason for hiding this comment

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

스프링에서는 리플렉션을 사용해서 클래스들을 가져오는 일을 BeanFactory 가 수행해주는데요, 스프링과 비교했을 때 루쿠가 구현하신 구조는 어떤 차이점이 있고, 어떤 부분에서 좀 더 장점을 갖는다고 생각하세요? (물론 현재 구조와 프로젝트 규모에서는 매우 큰 차이가 있지만, 그런 부분은 차치하고 생각했을 때의 질문입니다! 😄 저도 현재 규모에서는 리플렉션을 AnnotationHandlerMapping 에서 수행하는 것이 오버엔지니어링 하지 않는 가장 좋은 방법이라고 생각해요!)

Copy link
Author

@aiaiaiai1 aiaiaiai1 Sep 29, 2023

Choose a reason for hiding this comment

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

먼저 구조의 차이점에는 책임 분리가 되어있느냐 안되어있느냐 그 차이 인것 같아요
결국엔 책임 분리가 코드의 재사용성으로 이어지고 유지보수까지 영향을 미치게 된다고 생각해요
그래서 스프링과 비교했을때 스프링은 구조적이고 분리가 잘 되어있지만 구현하기엔 리소스가 굉장히 많이 듦.
제가 구현한 것은 분리는 잘 안되어있어 유지보수가 별로지만 구현이 단순하고 품이 적게 든다
이런 차이가 있다고 생각하네요

Copy link

@Cyma-s Cyma-s left a comment

Choose a reason for hiding this comment

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

안녕하세요 루쿠! 제가 확인한다고 확인 했었는데 머지를 안 눌렀을 줄은 꿈에도 몰랐네요...
루쿠 의견 확인했습니다! 어프루브 & 머지하겠습니다 🦅

@Cyma-s Cyma-s merged commit b702ff3 into woowacourse:aiaiaiai1 Sep 30, 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.

3 participants