[톰캣 구현하기 1 & 2단계] 제이미(임정수) 미션 제출합니다 #353
Conversation
| outputStream.write(response.getBytes()); | ||
| outputStream.flush(); | ||
| } | ||
|
|
||
| if ("POST".equals(method)) { | ||
| final var responseBody = getPostResponse(bufferedReader, request, uri); | ||
| final String response = getContent(uri, Objects.requireNonNull(responseBody)); | ||
|
|
||
| outputStream.write(response.getBytes()); | ||
| outputStream.flush(); |
There was a problem hiding this comment.
outputStream.write()와 outputStream.flush() 작업이 중복됩니다. 분기에 따라 응답을 만든 후에 해당 응답을 한 번에 내보내는 것은 어떨까요?
| private static Map<String, String> getRequestHeaders(final BufferedReader bufferedReader, String request) throws IOException { | ||
| Integer contentLength = null; | ||
| while (request != null && !"".equals(request)) { | ||
| request = bufferedReader.readLine(); | ||
| if (request.contains("Content-Length")) { | ||
| request = request.replaceAll(" ", ""); | ||
| contentLength = Integer.parseInt(request.split(":")[1]); | ||
| } | ||
| } | ||
|
|
||
| char[] buffer = new char[contentLength]; | ||
| bufferedReader.read(buffer, 0, contentLength); | ||
| String requestBody = URLDecoder.decode(new String(buffer), StandardCharsets.UTF_8); | ||
|
|
||
| return Arrays.stream(requestBody.split("&")) | ||
| .map(query -> query.split("=")) | ||
| .collect(toMap(query -> query[0], query -> query[1])); | ||
| } |
There was a problem hiding this comment.
메서드명은 getRequestHeaders() 이지만 코드를 보면 request body를 읽어오는 것 같습니다.
| return new Response(HttpStatus.OK, "login.html"); | ||
| } | ||
|
|
||
| InMemoryUserRepository.save(new User(queries.get(ACCOUNT), queries.get("password"), queries.get("email"))); |
There was a problem hiding this comment.
"password"와 "email"은 상수화를 하지 않은 이유가 무엇인가요?
| return InMemoryUserRepository.findByAccount(queries.get(ACCOUNT)) | ||
| .filter(user -> user.checkPassword(queries.get("password"))) | ||
| .map(this::loginSuccess) | ||
| .orElseGet(() -> new Response(HttpStatus.UNAUTHORIZED, "/401.html")); |
There was a problem hiding this comment.
login.html이나 index.html은 "/"를 포함하지 않은 경로를 입력해주는데 401.html은 "/"를 포함한 경로를 입력해주는 이유가 무엇인가요?
| return null; | ||
| } |
| private String getContent(final String uri, final Response responseBody) throws IOException { | ||
| if (responseBody.getCookies().isEmpty()) { | ||
| final URL resource = ClassLoader.getSystemClassLoader().getResource("static/" + responseBody.getContent()); | ||
| final File file = new File(URLDecoder.decode(Objects.requireNonNull(resource) | ||
| .getPath(), StandardCharsets.UTF_8)); | ||
| final String content = new String(Files.readAllBytes(file.toPath())); | ||
|
|
||
| return String.join("\r\n", | ||
| "HTTP/1.1 " + responseBody.getHttpStatus().getCode() + " " + responseBody.getHttpStatus() | ||
| .name() + " ", | ||
| getContentType(uri), | ||
| getContentLength(content), | ||
| "", | ||
| content); | ||
| } | ||
|
|
||
| return getContentWithCookie(uri, responseBody); | ||
| } | ||
|
|
||
| private String getContentWithCookie(final String uri, final Response responseBody) throws IOException { | ||
| final URL resource = ClassLoader.getSystemClassLoader().getResource("static/" + responseBody.getContent()); | ||
| final File file = new File(URLDecoder.decode(Objects.requireNonNull(resource) | ||
| .getPath(), StandardCharsets.UTF_8)); | ||
| final String content = new String(Files.readAllBytes(file.toPath())); | ||
|
|
||
| return String.join("\r\n", | ||
| "HTTP/1.1 " + responseBody.getHttpStatus().getCode() + " " + responseBody.getHttpStatus().name() + " ", | ||
| getCookies(responseBody.getCookies()), | ||
| getContentType(uri), | ||
| getContentLength(content), | ||
| "", | ||
| content); | ||
| } |
There was a problem hiding this comment.
if문 안의 코드와 getContentWithCookie의 코드가 한 줄 말고는 동일한 것 같습니다
| return String.join("\r\n", | ||
| "HTTP/1.1 " + responseBody.getHttpStatus().getCode() + " " + responseBody.getHttpStatus() | ||
| .name() + " ", | ||
| getContentType(uri), | ||
| getContentLength(content), | ||
| "", | ||
| content); |
There was a problem hiding this comment.
Response의 각 요소(start line, response headers, response body)에 대한 객체를 만들고 해당 객체가 자신의 출력 형태(?)를 관리하도록 하는 것은 어떨까요?
| return getContentWithCookie(uri, responseBody); | ||
| } | ||
|
|
||
| private String getContentWithCookie(final String uri, final Response responseBody) throws IOException { |
There was a problem hiding this comment.
헤더에 쿠키, content type, content length 말고 다른 정보도 추가된다면 어떻게 될까요? 그때마다 메서드를 따로따로 만들어야 한다면 불편할 것 같습니다.
|
|
||
| private final String id; | ||
|
|
||
| private final Map<String, Object> values = new HashMap<>(); |
There was a problem hiding this comment.
ConcurrentHashMap에 대해선 처음 알게 되었네요...!
멀티 스레드 환경에서는 HashMap보다 성능이 좋고 동시성 향상으로 인한 스레드의 안전성을 보장해 주다니 신기하네요.
덕분에 좋은 거 하나 알고 갑니다~
| if ("/register".equals(uri)) { | ||
| return new Response(HttpStatus.OK, "register.html"); |
There was a problem hiding this comment.
Response 객체 생성자의 두 번째 인자는 리소스 파일 이름이 들어갑니다.
그런데 Response 객체의 필드 이름은 content라고 되어 있고, 위에 getContent()나 getContentWithCookie() 메서드 이름에서 content는 응답을 http 형식에 맞게 바꾼 문자열을 말하고 있습니다. content라는 단어에 대해 두 가지 의미로 사용하고 있어서 혼란이 되는 것 같습니다!
|
Kudos, SonarCloud Quality Gate passed!
|
|
안녕하세요 엔초! 코드가 많이 더러웠을 텐데 지난 리뷰 감사합니다. 사실 아직도 시간 상의 문제로 완벽히 해결하지 못한 것들이 많은 상태입니다.
코드가 너무 많이 바뀌게 되어 리뷰를 다시 부탁드리는 것 같아 죄송한 마음뿐이네요… |
| final int id = database.size() + INCREMENT_BY_ONE; | ||
| final User savedUser = new User( | ||
| Integer.toUnsignedLong(id), |
There was a problem hiding this comment.
디비를 맵으로 안 쓰다 보니까 id를 직접 할당해줘야 한다는 사실을 까먹고 있었네요! 제 코드에도 적용해야겠어요. 감사합니다~
| public static final String ROOT_PATH = "/"; | ||
| private static final String STATIC_PATH = "static"; | ||
| private static final String LOGIN_PATH = "/login"; | ||
| private static final String REGISTER_PATH = "/register"; | ||
| private static final String ACCOUNT_KEY = "account"; | ||
| private static final String PASSWORD_KEY = "password"; | ||
| private static final String EMAIL_KEY = "email"; | ||
| private static final String JSESSIONID = "JSESSIONID"; | ||
| private static final String SESSION_USER_KEY = "user"; |
| outputStream.write(response.getBytes()); | ||
| outputStream.flush(); |
There was a problem hiding this comment.
inputStream를 BufferedReader로 래핑해서 사용하면 읽기 속도가 더 빨라지듯이, outputStream도 래핑해서 사용하면 데이터 전송 속도가 빨라진다고 합니다!
| public static String generate(final Request request) throws IOException { | ||
| if (ROOT_PATH.equals(request.getPath())) { | ||
| final Response response = getDefaultResponse(); | ||
| return response.toMessage(); | ||
| } | ||
| if (LOGIN_PATH.equals(request.getPath())) { | ||
| final Response response = getLoginResponse(request); | ||
| return response.toMessage(); | ||
| } | ||
| if (REGISTER_PATH.equals(request.getPath())) { | ||
| final Response response = getRegisterResponse(request); | ||
| return response.toMessage(); | ||
| } |
There was a problem hiding this comment.
request에서 path를 꺼내와서 비교하는 것보다 request한테 이 path가 맞는지 물어보는 것은 어떤가요?
| if (HttpMethod.GET.equals(request.getMethod())) { | ||
| return getLoginResponseGetMethod(request); | ||
| } |
There was a problem hiding this comment.
이 부분도 request한테 해당 HttpMethod인지 물어보는 것은 어떤가요?
| public static Cookies from(final String request) { | ||
| final Map<String, String> cookies = Arrays.stream(request.split(COOKIE_SEPARATOR)) | ||
| .map(value -> value.split(KEY_VALUE_SEPARATOR)) | ||
| .collect(toMap(value -> value[0], value -> value[1])); |
There was a problem hiding this comment.
여기 인덱스도 Header에서처럼 NAME_INDEX와 VALUE_INDEX로 분리하면 통일성이 있을 것 같아요
| bufferedReader.lines() | ||
| .takeWhile(line -> !line.isEmpty()) |
|
|
||
| private static Cookies getCookies(final Headers requestHeaders) { | ||
| return requestHeaders.getHeaders().stream() | ||
| .filter(header -> header.getName().equals("Cookie")) |
|
|
||
| public boolean hasHeaderBy(final String name) { | ||
| return headers.getHeaders().stream() | ||
| .anyMatch(header -> header.getName().equals(name)); |
There was a problem hiding this comment.
anyMatch()라는 것도 처음 봤네요! 새로운 코드 감사합니다
| final Request request = Request.from(new BufferedReader(new InputStreamReader(inputStream))); | ||
| final String response = ResponseGenerator.generate(request); |
There was a problem hiding this comment.
서버 내부에서 Exception이 발생했을 때는 어떤 반환을 해줄 수 있을까요?
There was a problem hiding this comment.
해당 부분도 시간 관계상 진행하지 못했습니다.
3, 4 단계에서 함께 진행하도록 하겠습니다.
안녕하세요 엔초. 이렇게 만나게 되네요 ㅎㅎ
그런데 이번 미션은 급박하게 구현만 진행해 코드 정리가 전혀 되어 있지 않는 상태입니다. 😭
이런 코드라 너무 부끄럽네요...
빠른 시일 내로 리팩토링을 진행할 예정이긴 하지만, 그 외적으로 조언 혹은 궁금한 점에 대해 함께 이야기해 보면 좋을 것 같습니다!
현재 코드는 보기 많이 어려우실 것 같아 최대한 빨리 리팩토링 진행할 수 있도로 하겠습니다!
잘 부탁드립니다~