Conversation
kisusu115
left a comment
kisusu115
left a comment
There was a problem hiding this comment.
코드 전체적으로 잘 작성해주신 것 같습니다!
요구사항에 대해 신경써서 작성해주신 것 같아요.
추가적인 생각해볼 점들만 남겨보겠습니다!
| if (controller == null) { | ||
| resp.setStatus(HttpServletResponse.SC_NOT_FOUND); | ||
| return; | ||
| } |
There was a problem hiding this comment.
요구사항에 없었던 404 활용도 좋습니다!
| if (viewPath.startsWith("redirect:")) { | ||
| resp.sendRedirect(viewPath.substring("redirect:".length())); | ||
| } else { | ||
| req.getRequestDispatcher(viewPath).forward(req, resp); |
There was a problem hiding this comment.
이 부분에서는 if-else 대신 early return 도 고려해볼만 한 것 같습니다!
예외적인 케이스인 redirect를 먼저 처리하고, 핵심 로직인 forward가 더 가독성 있게 눈에 들어올 것 같아요.
| public static RequestMapper getInstance() { | ||
| if(requestMapper == null){ | ||
| requestMapper = new RequestMapper(); | ||
| } |
There was a problem hiding this comment.
getInstance 방식으로 구현하신 것 좋다고 생각합니다!
불필요한 객체 생성 없이 싱글톤 방식의 RequestMapper도 좋은 것 같네요.
| <c:otherwise> | ||
| <a href="/user/login.jsp" type="button" class="btn btn-outline-primary me-2">Login</a> | ||
| <a href="/user/form.jsp" type="button" class="btn btn-primary">Sign-up</a> | ||
| </c:otherwise> |
There was a problem hiding this comment.
LoginFormController / SignUpFormController를 모두 구현해주셨기에, 이 부분의 url도
RequestMapper에서 정의해주신 대로 "/user/loginForm"과 "/user/signUpForm"로 수정되면 좋을 것 같습니다!
다른 jsp 파일의 url도 마찬가지로 수정되면 좋을 것 같아요. 현재 코드로는 직접적인 jsp 파일로의 접근이 이루어지고 있습니다!
| @Override | ||
| public String handleRequest(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { | ||
| return "/user/form.jsp"; | ||
| } |
There was a problem hiding this comment.
LoginFormController / SignUpFormController
이 두개의 컨트롤러는 중간 처리없이 단순히 파일 정보를 반환하는 공통된 로직을 가지고 있기에, 묶어서 일괄적으로 처리해도 좋을 것 같습니다! 예시 코드 4week/step2-mvc 브랜치의 ForwardController 클래스를 참고하시면 좋을 것 같아요.
2 participants
No description provided.