3주차 미션 / 서버 4조 최재현 #8
Conversation
|
|
||
| public class ForwardController implements Controller{ | ||
| public ForwardController() { | ||
| } |
There was a problem hiding this comment.
기본 생성자는 커스텀 생성자가 없을 경우 컴파일러가 자동으로 생성해주기 때문에 굳이 명시적으로 생성해주지 않아도 됩니다!
| @Override | ||
| public void execute(HttpRequest httpRequest, HttpResponse httpResponse) { | ||
| System.out.println("ForwardController"); | ||
| System.out.println("webapp" + httpRequest.getUrl()); |
There was a problem hiding this comment.
실무에서는 System.out을 사용하지 않아요! 지금부터라도 log 사용에 익숙해 지시는건 어떨까요?
| public void execute(HttpRequest httpRequest, HttpResponse httpResponse) { | ||
| boolean logined = httpRequest.isLogined(); | ||
| if (logined) { | ||
| httpResponse.forward(LIST_HTML.getUrl()); |
There was a problem hiding this comment.
early return 을 사용했다면 아래 조건문이 생략 가능했을 것 같네요.
| Repository repository = MemoryUserRepository.getInstance(); | ||
| User user = repository.findUserById(inputUserId); | ||
| if (user != null) { | ||
| if (inputPassword.equals(user.getPassword())) { |
There was a problem hiding this comment.
두 조건식은 한 조건문 안에서 서술해도 좋을 것 같습니다!
| String queryString = httpRequest.getBody(); | ||
| Map<String, String> queryMap = HttpRequestUtils.parseQueryParameter(queryString); | ||
|
|
||
| httpResponse.redirect(HOME.getUrl()); |
There was a problem hiding this comment.
아래 회원가입에 대한 비지니스 로직까지 완료된 이후에 리다이렉트를 응답하는게 좋을 것 같네요. 회원가입 로직에서 오류가 발생한다면 클라이언트에게 해당 오류를 전달하기 어렵지 않을까요?
| try { | ||
| body = IOUtils.readData(br, requestContentLength); | ||
| } catch (IOException e) { | ||
| log.log(Level.SEVERE, e.getMessage()); |
There was a problem hiding this comment.
IOException은 클라이언트 코드인 RequestHandler에서 같은 방식으로 처리해주고 있어요. 예외 처리 방식과 타입이 같다면 한 군데에서 예외처리를 맡는게 코드 응집도가 높지 않을까요?
| private final HttpBody httpBody; | ||
|
|
||
|
|
||
| private HttpRequest(BufferedReader br) { |
There was a problem hiding this comment.
생성자는 private로 클래스 내부에서만 접근 가능하게 설정한다면, 클래스 필드를 인자로 받는 식으로 단순히 객체 초기화의 역할을 가지도록 하는게 좋아보입니다!
| } | ||
|
|
||
| public boolean isLogined() { | ||
| return httpHeader.isLogined(); |
There was a problem hiding this comment.
로그인 여부를 파악하는게 HttpRequest의 책임일까요..?
|
|
||
| public class RequestMapper { | ||
|
|
||
| private Map<String, Controller> controllerMap; |
There was a problem hiding this comment.
참고로 static 영역에서 controllers를 관리하면 멀티쓰레드 환경에서 인스턴스를 공유할 수 있어서 자원을 보다 효율적으로 활용할 수 있습니다! 물론 그래서 발생할 수도 있는 문제가 있는데 일단 우리 코드에서 문제되는 부분은 없어서 static으로 관리해도 괜찮을 것 같습니다.
이 부분은 싱글톤을 키워드로 검색해보시는걸 추천드립니다!
| return; | ||
| } | ||
| if (method.equals(GET.getMethod()) && url.endsWith(".html")) { | ||
| controller = new ForwardController(); |
There was a problem hiding this comment.
ForwardController는 정적 자원에 대한 요청에서 사용하면 좋을 것 같아요. url.endsWith(".html")을 없애주면 css나 기타 자원에 대한 응답을 할 수 있을 것 같아요.
기능 구현에 급급해서 내부로직이나 리팩토링은 조금 아쉬움이 남네요... 시간을 가지고 조금 더 고쳐보겠습니다.