Skip to content

4주차 미션 / 서버 2조 송민서#6

Open
MinseoSONG wants to merge 7 commits intoKonkuk-KUIT:mainfrom
MinseoSONG:minseo
Open

4주차 미션 / 서버 2조 송민서#6
MinseoSONG wants to merge 7 commits intoKonkuk-KUIT:mainfrom
MinseoSONG:minseo

Conversation

@MinseoSONG
Copy link
Copy Markdown
Member

No description provided.

@MinseoSONG MinseoSONG self-assigned this Apr 9, 2025
Copy link
Copy Markdown
Contributor

@kisusu115 kisusu115 left a comment

Choose a reason for hiding this comment

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

코드 전체적으로 잘 작성해주신 것 같습니다.
코드 리뷰와 예시 코드 참고하시면서 생각해보시면 더 이상적인 코드가 나올 것 같아요!

추가적인 생각해볼 점들만 남겨보겠습니다.

resp.sendRedirect(view.substring("redirect:".length()));
} else {
RequestDispatcher rd = req.getRequestDispatcher(view);
rd.forward(req, resp);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

이 부분에서는 if-else 대신 early return 방식도 고려해볼만 한 것 같습니다!
예외적인 케이스인 redirect를 먼저 처리하고, 핵심 로직인 forward 로직이 더 가독성 있게 보일 것 같습니다.

추가적으로, 특정한 목적을 가진 "redirect:" 문자열도 상수로 정의된다면 더 좋을 것 같습니다!


@Override
protected void service(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
String path = req.getRequestURI().substring(req.getContextPath().length());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

비록 이 프로젝트에서는 ContextPath가 빈문자열이지만, 더 정확하게 경로를 추출하도록 구현해주신 것 같아요!

RequestDispatcher rd = req.getRequestDispatcher("/home.jsp");
rd.forward(req, resp);

return null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DispatcherServlet에서 RequestDispatcher의 생성과 포워딩을 처리해주고 있기에, 이 컨트롤러에서는 "/home.jsp" 문자열만 return 해주어도 정상적으로 처리될 것 같습니다! 그게 DispatcherServlet의 생성 및 책임 분리 측면에서도 더 이상적일 것 같아요.


if (sessionUser == null) {
return "redirect:/user/login.jsp";
}
Copy link
Copy Markdown
Contributor

@kisusu115 kisusu115 Apr 13, 2025

Choose a reason for hiding this comment

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

조건에 따라 분기를 나누어 처리해주신 부분 너무 좋습니다!

RequestDispatcher rd = req.getRequestDispatcher("/user/updateForm.jsp");
rd.forward(req, resp);

return null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

HomeController에 대한 comment와 같은 맥락으로, null 대신 "/user/updateForm.jsp" 문자열을 return 해준다면 더 좋을 것 같습니다! 이 컨트롤러의 책임은 해당 문자열을 반환해주는 것이라 생각해요.

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