[톰캣 구현하기 1, 2단계] 하디(전동혁) 미션 제출합니다. #323
Conversation
There was a problem hiding this comment.
안녕하세요 하디!
처음에 Pull Request 본문에 제 이름을 언급 안해주셔서 서운해서 리뷰 못할 뻔 했어요..
다행히도 수정해주셨네요 감사합니다.
코드가 정말 환상적이네요.
호랑이 냄새가 진하게 납니다.
본론으로 돌아와서 제가 남긴 모든 코멘트들은 강요가 아닌 의견입니다.
이해가 가지 않는 부분 혹은 동의가 되지 않는 부분은 꼭 말씀해주세요
진하게 의논해봅시다.
마지막으로 좋은 코드 리뷰할 수 있게 해주셔서 정말 감사합니다.
꼼꼼히 읽어보면서 정말 많이 배웠어요.
앞으로도 잘 부탁드립니다! 어흥
| boolean haveToReadBody = false; | ||
| int contentLength = 0; | ||
| while (!(header = bufferedReader.readLine()).isBlank()) { | ||
| String[] splitHeader = header.split(": "); |
There was a problem hiding this comment.
오! trim() 사용하지 않기 위해서 : 뒤에 공백도 포함해주셨군요!
너무 천재적인 발상입니다.
하지만, 개인적인 생각으로는 공백을 포함시켜 : 로 Split 해주는 느낌보다는 딱 : 하나의 문자로 Split 해준다는 느낌이 더 직관적일 수도 있을 것 같은데 하디 생각은 어떠신가요?!
There was a problem hiding this comment.
": " 로 하는것이 나중에 Response의 Header를 만들 때도 편할 것 같아서 이렇게 했습니다 !
tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java
Outdated
Show resolved
Hide resolved
| return handleUnAuthorizedPage(httpRequest); | ||
| } | ||
| try { | ||
| Path path = Paths.get(getClass().getClassLoader().getResource("static" + httpRequest.uri() + ".html").getPath()); | ||
| byte[] bytes = Files.readAllBytes(path); | ||
| String body = new String(bytes); | ||
| HttpResponse response = new HttpResponse(); | ||
| response.setHttpVersion(HttpVersion.HTTP11.value()); | ||
| response.setHttpStatus(HttpStatus.OK); | ||
| response.setHeader(HttpHeader.CONTENT_TYPE, ContentType.TEXT_HTML.value()) | ||
| .setHeader(HttpHeader.CONTENT_LENGTH, body.getBytes().length + " "); | ||
| response.setBody(body); | ||
| return response; | ||
| } catch (IOException e) { | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private HttpResponse handlePostMapping(HttpRequest httpRequest) { | ||
| String body = httpRequest.body(); | ||
| if (body != null) { | ||
| String account = parseAccount(body); | ||
| String password = parsePassword(body); | ||
| Optional<User> user = InMemoryUserRepository.findByAccount(account); | ||
| if (user.isPresent() && user.get().checkPassword(password)) { | ||
| log.info(ACCOUNT + ": " + account + ", " + PASSWORD + ": " + password); | ||
| return handleRedirectPage(user.get()); | ||
| } | ||
| return handleUnAuthorizedPage(httpRequest); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private String parseAccount(String quiryString) { | ||
| String[] accountInfo = quiryString.substring(0, quiryString.indexOf('&')).split("="); | ||
| if (ACCOUNT.equals(accountInfo[0])) { | ||
| return accountInfo[1]; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private String parsePassword(String quiryString) { | ||
| String[] passwordInfo = quiryString.substring(quiryString.indexOf('&') + 1).split("="); | ||
| if (PASSWORD.equals(passwordInfo[0])) { | ||
| return passwordInfo[1]; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private boolean isExistUser(String account, String password) { | ||
| if (account == null || password == null) { | ||
| return false; | ||
| } | ||
| Optional<User> user = InMemoryUserRepository.findByAccount(account); | ||
| if (user.isPresent() && user.get().checkPassword(password)) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| private HttpResponse handleUnAuthorizedPage(HttpRequest httpRequest) { | ||
| return new ExceptionHandler(HttpStatus.UNAUTHORIZED).handle(httpRequest); | ||
| } | ||
|
|
||
| private HttpResponse handleRedirectPage(User user) { | ||
| UUID uuid = UUID.randomUUID(); | ||
| Session session = new Session(uuid.toString()); | ||
| session.setAttribute("user", user); | ||
| new SessionManager().add(session); |
There was a problem hiding this comment.
SessionManager 를 LoginHandler 가 가지고 있는 것은 어떨까요?
현재로서는 LoginHandler 만이 SessionManger 를 필요로하니까요.
물론 추후에 다른데서도 SessionManager 를 필요로 할 수 있지만요 호호호호
There was a problem hiding this comment.
SessionManager를 필드로 가질지 메서드내 지역변수로 가질지 고민했는데요.
필드로 갖고있기보다는 세션을 사용하는 메서드에서 지역변수로 갖는건 어떻게 생각하세요 ?
| return null; | ||
| } | ||
|
|
||
| private HttpResponse handleGetMapping(HttpRequest httpRequest) { |
There was a problem hiding this comment.
1단계 미션에서는 Login 버튼을 눌렀을 때 Get 요청을 보내도록 하였지만, 추후 2단계에서 Login 요청을 Post 요청으로 보내도록 바뀌었잖아요?
저는 그 때 그래서 GetMapping 로그인 요청은 딱 Redirection, login page 제공의 역할만을 하도록 하였거든요.
이에 대한 하디의 생각은 어떠신가요 ??
There was a problem hiding this comment.
아 코드를 지우지 못했네요 ㅜ
Get으로 로그인하는 경로를 코드에서 삭제하겠습니다 !
| new AbstractMap.SimpleEntry<HandlerMatcher, Handler>(new HandlerMatcher(HttpMethod.GET, "/"), new HelloHandler()), | ||
| new AbstractMap.SimpleEntry<HandlerMatcher, Handler>(new HandlerMatcher(HttpMethod.GET, "/index.html"), new HtmlHandler()), | ||
| new AbstractMap.SimpleEntry<HandlerMatcher, Handler>(new HandlerMatcher(HttpMethod.GET, "/assets/chart-area.js"), new JsHandler()), | ||
| new AbstractMap.SimpleEntry<HandlerMatcher, Handler>(new HandlerMatcher(HttpMethod.GET, "/assets/chart-bar.js"), new JsHandler()), | ||
| new AbstractMap.SimpleEntry<HandlerMatcher, Handler>(new HandlerMatcher(HttpMethod.GET, "/assets/chart-pie.js"), new JsHandler()), | ||
| new AbstractMap.SimpleEntry<HandlerMatcher, Handler>(new HandlerMatcher(HttpMethod.GET, "/js/scripts.js"), new JsHandler()), | ||
| new AbstractMap.SimpleEntry<HandlerMatcher, Handler>(new HandlerMatcher(HttpMethod.GET, "/css/styles.css"), new CssHandler()), | ||
| new AbstractMap.SimpleEntry<HandlerMatcher, Handler>(new HandlerMatcher(HttpMethod.GET, "/login"), new LoginHandler()), |
There was a problem hiding this comment.
현재 Handle 부분에서 정적 파일제공이 많은 비중을 차지하는 것 같아요.
실제로 CssHandler, HtmlHadnler, JsHandler 코드를 보면 거의 동일하구요!
이 부분을 해결하기 위해 request uri 를 파싱하여 확장자를 통해 동적으로 처리할 수 있지 않을까요??
There was a problem hiding this comment.
넵 이번에 예외 핸들러, 정적파일 핸들러로 구분해서 생성했습니다 !!
| private String parseEmail(String quiryString) { | ||
| String[] emailInfo = quiryString.substring(quiryString.indexOf('&') + 1, quiryString.lastIndexOf('&')).split("="); | ||
| if (EMAIL.equals(emailInfo[0])) { | ||
| return emailInfo[1]; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private String parsePassword(String quiryString) { | ||
| String[] passwordInfo = quiryString.substring(quiryString.lastIndexOf('&') + 1).split("="); | ||
| if (PASSWORD.equals(passwordInfo[0])) { | ||
| return passwordInfo[1]; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
이 부분은 LoginHandler 의 코드와 중복되네요!
그렇기 떄문에, 이 부분에서도 말씀드렸던 것과 같은 예외가 발생할 여지가 있을 것 같습니다.
| public enum ContentType { | ||
|
|
||
| TEXT_HTML("text/html;charset=utf-8 "), | ||
| TEXT_CSS("text/css;charset=utf-8 "), | ||
| TEXT_JS("text/javascript;charset=utf-8 "); | ||
|
|
||
| private String value; | ||
|
|
||
| ContentType(String value) { | ||
| this.value = value; | ||
| } | ||
|
|
||
| public String value() { | ||
| return value; | ||
| } | ||
| } |
|
안녕하세요 매튜 ~ 리팩터링해서 다시 왔어요 ! 리뷰 반영했습니다 ㅎㅅㅎ 큰 구조는 비슷하고 객체 분리만 좀 했어요 ! 잘부탁드립니다 ! |
|
Kudos, SonarCloud Quality Gate passed!
|
| public HttpVersion httpVersion() { | ||
| return statusLine.httpVersion(); | ||
| } | ||
|
|
||
| public HttpStatus httpStatus() { | ||
| return statusLine.httpStatus(); | ||
| } | ||
|
|
||
| public Header header() { | ||
| return this.header; | ||
| } | ||
|
|
||
| public Body body() { | ||
| return this.body; | ||
| } | ||
| } |
There was a problem hiding this comment.
저는 개인적으로 this 를 사용하지 않아도 되는 상황에서 사용하지 않는 것을 선호해요!
this 를 사용하지 않아도 되는 상황에서 this 를 사용하다보면 일관성이 깨지는 경우도 다소 존재하더라고요
하디의 생각은 어떠신가요?
There was a problem hiding this comment.
좋아요 ㅎㅎ 섞어 쓰니까 일관성이 깨진다고 볼 수도 있겠어요
| .add(httpResponse.httpVersion().value()) | ||
| .add(String.valueOf(httpResponse.httpStatus().statusCode())) | ||
| .add(httpResponse.httpStatus().statusText()); | ||
| return statusLineMaker.toString() + LINE_DELIMITER; |
There was a problem hiding this comment.
return statusLineMaker.toString() + LINE_DELIMITER 에서 toString 은 빼더라도 잘 동작하는 것 같네요!
There was a problem hiding this comment.
LINE_DELIMITER랑 +연산을 하면서 변환이 되나 보군요 감사합니다
|
|
||
| public class HttpResponseWriter { | ||
|
|
||
| private final static String LINE_DELIMITER = " "; |
| return bufferedReader.readLine(); | ||
| } | ||
|
|
||
| private static String readHeader(BufferedReader bufferedReader, List<Integer> contentLengthStorage) throws IOException { |
There was a problem hiding this comment.
라인 길이가 너무 길어지면 가독성이 떨어질 것 같아요!
이럴 때에는 개행을 해주시는 것은 어떨까요?
| List<Integer> contentLengthStorage = new ArrayList<>(); | ||
| String startLine = readStartLine(bufferedReader); | ||
| String header = readHeader(bufferedReader, contentLengthStorage); | ||
| String body = ""; |
There was a problem hiding this comment.
사용하지 않는 값이네요!
개개인의 선호가 있긴 하겠지만 String body 은 어떠신가요
| private static boolean canHandleByStaticHandler(String requestTarget) { | ||
| return Arrays.stream(ContentType.values()) | ||
| .map(ContentType::extension) | ||
| .anyMatch(requestTarget::endsWith); | ||
| } | ||
| } |
| } | ||
|
|
||
| private HttpResponse handleAlreadyLogin(HttpRequest httpRequest) { | ||
| Session session = httpRequest.session(); |
There was a problem hiding this comment.
별거 아니긴 하지만, handleAlreadyLogin 은 무조건 session 을 get 한 이후에 호출되니 request 와 함께 파라미터로 넣어주는 것은 어떨까요??
| if (account != null && email != null && password != null && !isDuplicateAccount(account)) { | ||
| InMemoryUserRepository.save(new User(account, password, email)); | ||
| return handleRedirectPage(httpRequest); | ||
| } | ||
| return handleRedirectPage(httpRequest); |
| String[] splitStartLine = startLine.split(START_LINE_DELIMITER); | ||
|
|
||
| try { | ||
| HttpMethod httpMethod = HttpMethod.valueOf(splitStartLine[0]); |
There was a problem hiding this comment.
여기서 HttpMethod.findByValue 를 사용하는 것은 어떨까요?
|
|
||
| public class Body { | ||
|
|
||
| public static Body EMPTY = new Body(""); |
안녕하세요 ~ 매튜^_^
구현이 막막하네요...
나름 리팩토링을 해봤으나 아직 더 할게 남은 것 같습니다 !(3단계도 리팩터링이더라구요)
CssHandler,HtmlHandler,JsHandler등을StaticHandler로 통합하기현재 구조에 대한 피드백을 마구마구 해주시면 감사하겠습니다!
로직의 흐름은
Http11Processor.process()->HandlerMapping에서 요청에 맞는 핸들러 가져오기 -> 핸들러 실행시켜서Response받기 -> 응답값을 아웃풋스트림에 쓰고 플러시입니다.
코드에 이해가 안가는 부분이 있으시면 언제든 물어보셔도 됩니다 !!
잘 부탁드려용~