Conversation
kisusu115
left a comment
kisusu115
left a comment
There was a problem hiding this comment.
코드 전체적으로 잘 작성해주신 것 같습니다.
코드 리뷰 참고하시면서 예시 코드와 비교하며 공부하시면 더 도움이 될 것 같습니다!
추가적인 생각해볼 점들만 남겨보겠습니다.
| private final Map<String, Controller> controllers = new HashMap<>(); | ||
|
|
||
| public RequestMapper() { | ||
| initControllers(); |
There was a problem hiding this comment.
현재 DispatcherServlet의 생성 시 마다, 즉 요청이 들어올때마다 새로운 RequestMapper 객체가 생성되어 반복적으로 init이 일어나고 있는 것 같습니다! static이나 getInstance를 통한 싱글톤 방식의 사용을 고려해보시면 더 좋을 것 같아요.
| if (controller ==null) { | ||
| res.setStatus(HttpServletResponse.SC_NOT_FOUND); | ||
| return; | ||
| } |
There was a problem hiding this comment.
요구사항에 없었던 404 처리 너무 좋습니다!
|
|
||
| req.setAttribute("user", user); | ||
|
|
||
| return "/user/updateForm.jsp"; |
There was a problem hiding this comment.
업데이트 폼을 반환하는 컨트롤러와 실제 업데이트 요청을 처리하는 컨트롤러는 역할이 다르기에 분리하는 것도 좋은 방향이라고 생각합니다!
동일한 URL에 대해 여러 HTTP 메서드를 사용하는 경우, 같은 자원에 대해 다른 행위를 나타내기 위해 쓰이는데, 현재 코드에서 업데이트 폼을 반환하는 로직은 form과 관련된 사항이고, 유저 정보의 수정은 User와 관련된 사항이기에 다른 URL을 사용하는 것이 더 좋을 수도 있겠다는 생각이 듭니다!
물론 정답이 있는 것은 아니기에 상황과 구현 스타일에 따라 달라질 수 있다고 생각합니다.
| <a href="/user/login.jsp" type="button" class="btn btn-outline-primary me-2">Log-In</a> | ||
| <a href="/user/form.jsp" type="button" class="btn btn-primary">Sign-up</a> | ||
| </c:otherwise> | ||
| </c:choose> |
There was a problem hiding this comment.
jsp 파일들에서 이 부분까지, 즉 화면에서 네비게이션 바에 해당하는 코드는 navigation.jspf 파일에 포함되었다면 더 좋을 것 같습니다!
모든 jsp 파일에서 중복적으로 나오는 부분이기에, jspf 파일 안으로 들어갔다면 모든 jsp 파일의 수정 필요없이 jspf 파일 하나만 관리할 수 있어 좀 더 수월하게 작성하셨을 것 같아요.
2 participants
No description provided.