Conversation
There was a problem hiding this comment.
코드 잘 봤습니다!
요구사항이 많고 어려운 미션이라 힘드셨을 부분은 이해하지만, 요구사항에 있었던 상수를 enum으로 관리하는 부분이나 테스트가 없는 부분은 조금 아쉬웠네요. 그리고 추가로 개발하실 때 main 브랜치 같은 중요한 브랜치에서 직접적인 작업은 피하시는게 좋을 것 같습니다. 항상 브랜치를 따로 만들고 코드에 문제가 없는걸 확인 후(팀원과의 코드 리뷰 등) 중요 브랜치로 머지하면서 사용하는게 혼란을 피하는 방법입니다!
코드 리뷰 확인하시고 업로드된 구현 코드와 비교해 보시면 좋을 것 같습니다! 수고 많으셨습니다!
|
|
||
| import java.io.IOException; | ||
|
|
||
| public class CssController implements Controller { |
There was a problem hiding this comment.
정적 자원을 응답하는 ForwardController와 비슷한 역할을 가지는 것 같아요!
| response.addHeader("Content-Type", "text/css"); | ||
| response.forward("/css/styles.css"); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
IOException은 RequestHandler에서 마찬가지로 처리해주고 있어요. 예외 처리 로직이 프로젝트 전반에 퍼져있으면 가독성이 떨어지지 않을까요?
| if (user != null && params.get("password").equals(user.getPassword())) { | ||
| response.addHeader("Set-Cookie", "logined=true; Path=/; Max-Age=30; HttpOnly"); | ||
| response.redirect("/index.html"); | ||
| } else { |
There was a problem hiding this comment.
코드에서 else 예약어가 많이 보이는 것 같아요. 의도적으로 else의 사용을 줄이려고 노력해 보시는게 어떨까요?
| public class HttpRequest { | ||
| private String method; | ||
| private String path; | ||
| private String version; |
There was a problem hiding this comment.
위 값들은 HttpRequestStartLine 클래스로 관리할 수 있지 않을까요?
| private String method; | ||
| private String path; | ||
| private String version; | ||
| private Map<String, String> headers = new HashMap<>(); |
There was a problem hiding this comment.
1주 차에 배웠던 일급 컬렉션을 사용해서 Map을 포장하는 것도 좋을 것 같아요!
|
|
||
| public class HttpResponse { | ||
| private final OutputStream out; | ||
| private final Map<String, String> headers; |
There was a problem hiding this comment.
HttpResponse도 startLine, Header, Body 를 인스턴스 필드로 가지도록 작성해 보는게 어떨까요? 아래에 구현된 코드들도 오히려 HttpResponse 클래스가 자신이 가지고 있어야 할 필드에 대해 상태 관리를 하지 않으니 가독성이 매우 떨어지는 것 같아요.
| public class HttpRequestUtils { | ||
|
|
||
| public static HttpRequest parse(BufferedReader br) throws IOException { |
There was a problem hiding this comment.
음... HttpRequest를 생성하는 책임을 갖는 클래스인가요? 마치 팩토리 클래스와 비슷해 보이는데 명칭을 달리하는게 더 클래스의 역할을 인식하기 좋을 것 같네요.
외부 의존성을 연결하거나 하는 코드가 없어서 HttpRequest 내부 정적 팩토리 메서드에서 관리하는게 더 좋아보이긴 합니다.
| case 404: | ||
| return "HTTP/1.1 404 Not Found \r\n"; | ||
| default: | ||
| return "HTTP/1.1 500 Internal Server Error \r\n"; |
There was a problem hiding this comment.
응답 코드가 추가될 때 마다 이 부분을 수정해 주어야 할 것 같은데.. 벌써부터 유지 보수가 험난할 것 같아요...
위에서 언급했듯이, HttpResponse에서 Http 응답 메시지에 필요한 속성들을 관리했다면 일일히 하드코딩해줄 필요가 없어질 것 같아요. 하드코딩은 유지 보수의 적이라는 거 명심해 주세요!
| } | ||
| } | ||
| System.out.println("현재 경로: " + httpRequest.getPath()); | ||
| System.out.println("헤더: " + httpRequest.getHeaders()); |
There was a problem hiding this comment.
실무에선 System.out을 사용하지 않아요. 지금부터라도 log 사용에 익숙해져 보는게 어떨까요?
| public class RequestMapper { | ||
| private final HttpRequest request; | ||
| private final HttpResponse response; | ||
| private static final Map<String, Controller> controllerMap; |
There was a problem hiding this comment.
상태를 가지지 않는 객체는 static 필드로 지정해 사용하면 싱글톤처럼 사용할 수 있죠! 매 요청마다 생성되지 않아 리소스의 낭비를 줄일 수 있는 좋은 코드입니다 👍
피드백 부탁드립니다!!