[1 -2단계 - Tomcat 구현하기] 릴리(최윤서) 미션 제출합니다. #528
Conversation
tomcat/src/main/java/org/apache/coyote/http11/HttpResponse.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/com/techcourse/controller/AbstractController.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/com/techcourse/controller/LoginController.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/com/techcourse/controller/RequestMapping.java
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,4 @@ | |||
| package org.apache.coyote.http; | |||
|
|
|||
| public record StatusCode(String statusCode) { | |||
There was a problem hiding this comment.
StatusCode를 enum이 아닌 record 타입으로 선언한 이유가 궁금해요!
There was a problem hiding this comment.
VO 와 순간 헷갈렸던 것 같아요 😅 수정했습니다!
| @@ -0,0 +1,4 @@ | |||
| package org.apache.coyote.http; | |||
|
|
|||
| public record HttpVersion(String version) { | |||
| } | ||
|
|
||
| @Override | ||
| public void add(Session session) { |
There was a problem hiding this comment.
몇몇 메서드 인자에 final 키워드가 사용되고 있는데, 통일이 필요할 것 같아요 🥲
| StringBuilder responseBuilder = new StringBuilder(); | ||
|
|
||
| responseBuilder.append(httpVersion.version()).append(" ") | ||
| .append(statusCode.statusCode()).append(" ").append("\r\n"); | ||
|
|
||
| for (Map.Entry<String, String> header : headers.entrySet()) { | ||
| responseBuilder.append(header.getKey()).append(": ").append(header.getValue()).append(" ").append("\r\n"); | ||
| } | ||
|
|
||
| responseBuilder.append("\r\n"); | ||
|
|
||
| if (responseBody != null && !responseBody.isEmpty()) { | ||
| responseBuilder.append(responseBody); | ||
| } | ||
|
|
||
| return responseBuilder.toString().getBytes(StandardCharsets.UTF_8); |
There was a problem hiding this comment.
| StringBuilder responseBuilder = new StringBuilder(); | |
| responseBuilder.append(httpVersion.version()).append(" ") | |
| .append(statusCode.statusCode()).append(" ").append("\r\n"); | |
| for (Map.Entry<String, String> header : headers.entrySet()) { | |
| responseBuilder.append(header.getKey()).append(": ").append(header.getValue()).append(" ").append("\r\n"); | |
| } | |
| responseBuilder.append("\r\n"); | |
| if (responseBody != null && !responseBody.isEmpty()) { | |
| responseBuilder.append(responseBody); | |
| } | |
| return responseBuilder.toString().getBytes(StandardCharsets.UTF_8); | |
| appendStatusLine(httpVersion, statusCode, statusMessage); | |
| appendHeaders(headers); | |
| appendBody(responseBody); | |
| return responseBuilder.toString().getBytes(StandardCharsets.UTF_8); |
메서드로 추출해보면 어떨까요?
| private String getContentType(String resource) { | ||
| if (resource.endsWith(".css")) { | ||
| return "text/css"; | ||
| } else if (resource.endsWith(".js")) { | ||
| return "application/javascript"; | ||
| } else if (resource.endsWith(".ico")) { | ||
| return "image/x-icon"; | ||
| } else if (resource.endsWith(".html")) { | ||
| return "text/html"; | ||
| } else { | ||
| return "text/plain"; | ||
| } |
There was a problem hiding this comment.
자바 MimeTypesFileTypeMap 라는 클래스 사용을 고민해봐도 좋을 것 같아요 !
There was a problem hiding this comment.
구두논의 후 Files.probeContentType 사용
| private String getParameter(String requestBody, String key) { | ||
| return Arrays.stream(requestBody.split("&")) | ||
| .filter(param -> param.startsWith(key + "=")) | ||
| .map(param -> param.split("=")[1]) | ||
| .findFirst() | ||
| .orElse(""); | ||
| } |
There was a problem hiding this comment.
content-type이 application/x-www-form-urlencoded 인 경우 Body의 형태가 key value 쌍으로 온다고 하더라구요!
따라서 key value 파싱 또한 현재 컨트롤러가 아닌 Request 파싱 단계에서 이루어져야 할 것 같아요!
There was a problem hiding this comment.
Request 에서 처리하도록 변경하였습니다!
| handleSuccessfulLogin(response, account); | ||
| } | ||
|
|
||
| if (!findUserByInfo(account, password)) { | ||
| handleFailedLogin(response); | ||
| } |
There was a problem hiding this comment.
| handleSuccessfulLogin(response, account); | |
| } | |
| if (!findUserByInfo(account, password)) { | |
| handleFailedLogin(response); | |
| } | |
| handleSuccessfulLogin(response, account); | |
| return; | |
| } | |
| handleFailedLogin(response); |
early return을 해보면 불필요한 조건문을 제거할 수 있을 것 같아요!
| @@ -0,0 +1,24 @@ | |||
| package com.techcourse.controller; | |||
There was a problem hiding this comment.
/com.techcourse.controller 에 위치해 있는데, 이 클래스는 해당 서비스가 다뤄야할 부분은 아닌 것 같다는 생각이 들어요
현재 패키지 구조에서 /com/techcourse/에는 해당 애플리케이션의 고유한 비즈니스 플로우가 담겨야 할 것 같다는 생각인데요, 🤔
릴리는 어떻게 생각하시는지 궁금합니당
There was a problem hiding this comment.
저도 같은 생각입니다!
/org.apache.catalina.controller 에 두었습니다
| @@ -1,14 +1,13 @@ | |||
| package org.apache.coyote.http11; | |||
There was a problem hiding this comment.
Test를 추가해볼까요?
모든 도메인에 대한 단위테스트까진 아니더라도 통합 테스트라도 작성해보면 어떨까요?
안녕하세요 몰리!~ ⍢
코드리뷰 잘 부탁드려요 :)
리뷰 하시다가 궁금한 점 있으시면 언제든 DM 주셔도 좋아요 🤗