[Tomcat 구현하기 3단계] 리건(이한길) 미션 제출합니다. by hangillee · Pull Request #625 · woowacourse/java-http

hangillee · 2024-09-11T15:53:28Z

안녕하세요, 릴리! 👋
일정 관리가 여간 어려운게 아니네요.. 😢
릴리는 미션과 프로젝트 잘 병행하고 있나요?

최대한 열심히 리팩토링 해봤습니다!
리뷰 잘 부탁드려요!

lilychoibb

안녕하세요 리건 🤗!
리팩토링 잘 해주셨네요 :) 고생 많으셨습니다
몇 가지 코멘트 남겼는데, 확인 부탁드려요!

레벨 4가 되면서 다들 시간 관리에 대해서 고민이 많아진 것 같아요.
저도 미션과 프로젝트 병행을 아주 벅차게... 하고 있답니다 😇
그래도 같이 파이팅 해보아요 💪🏻💪🏻💪🏻

lilychoibb · 2024-09-12T02:30:43Z

tomcat/src/main/java/com/techcourse/controller/RequestMapping.java

+    public RequestMapping() {
+        requestMap.put(Request.ofGet("/login"), new LoginController());
+        requestMap.put(Request.ofPost("/login"), new LoginController());
+        requestMap.put(Request.ofGet("/register"), new RegisterController());
+        requestMap.put(Request.ofPost("/register"), new RegisterController());
+    }


Get 과 Post 요청을 분리해서 put 하는 이유가 궁금합니다!
get 을 하면 어차피 같은 Controller 를 반환해서 service 하는 것 같아서요

각 요청에 따라 컨트롤러를 분리해놨다가 리팩토링하면서 놓쳤네요.
고쳐보겠습니다!

lilychoibb · 2024-09-12T02:33:02Z

tomcat/src/main/java/com/techcourse/controller/Request.java

+import org.apache.coyote.http11.HttpMethod;
+import org.apache.coyote.http11.request.HttpRequest;
+
+public class Request {


RequestLine 객체와 같은 역할을 하는 것 같은데 추가하신 이유가 궁금해요 🙂
그리고 http 요청을 처리하는 클래스가 controller 패키지에 있는 느낌이 들었어요!

지금 확인해보니 역할이 동일하네요.. 하나로 통일하겠습니다.
http11 패키지에 의존성을 갖네요. 이것도 고쳐보겠습니다!

lilychoibb · 2024-09-12T04:08:44Z

tomcat/src/main/java/org/apache/coyote/http11/response/HttpResponse.java

+    public void addCookies(final HttpCookie cookie) {
+        headers.addFirst(HttpHeader.setCookie(cookie));


21 에서 addFirst 가 나왔군요 👍 덕분에 알아가요!

lilychoibb · 2024-09-12T04:13:16Z

tomcat/src/main/java/org/apache/coyote/http11/HttpCookie.java

+    public static HttpCookie from(final String cookies) {
+        if ("".equals(cookies)) {
+            return new HttpCookie();
+        }
+        return new HttpCookie(cookies.strip());


strip() 으로 세심하게 처리해주셨네요👍

lilychoibb · 2024-09-12T04:18:43Z

tomcat/src/main/java/org/apache/coyote/http11/response/HttpResponse.java

-                EMPTY_LINE,
-                responseBody
-        ).getBytes();
+    private String statusLine = "";


statusLine 의 Http 버전과 statusCode 는 분리를 안해도 될까요?
추후 버전이 다른 http 를 쓸 경우에 재사용을 할 수 있을 것 같아서요!
리건의 의견이 궁금합니다🙂

재사용성을 두고 보니 릴리의 의견에 동의합니다. 이 역시 리팩토링 해보겠습니다.

lilychoibb · 2024-09-12T04:29:46Z

tomcat/src/main/java/org/apache/coyote/http11/HttpMethod.java

+    public HttpMethod from(final String method) {
+        return HttpMethod.valueOf(method);
+    }


이 메서드는 사용되고 있지 않은 것 같은데 미리 만들어두신 이유가 있을까요?
static 도 붙어있지가 않아 조금 어색하네요 🙂

놓친 부분이네요. 제거하겠습니다!

lilychoibb · 2024-09-12T04:34:30Z

tomcat/src/test/java/com/techcourse/controller/FrontControllerTest.java

+        final String requestString = String.join("\r\n",
+                "GET /login HTTP/1.1 ",
+                "Host: localhost:8080 ",
+                "Connection: keep-alive ",


해당 헤더는 테스트 하는데 꼭 필요한 헤더인가요?
저도 잘 몰라서 여쭤봐요!

앗! 이것도 놓쳤네요.. 중복 제거하겠습니다..

lilychoibb · 2024-09-12T04:43:13Z

tomcat/src/test/java/support/HttpRequestFixture.java

+import java.io.IOException;
+import org.apache.coyote.http11.request.HttpRequest;
+
+public class HttpRequestFixture {


해당 fixture 덕분에 테스트 읽기가 수월했어요 👍

lilychoibb

안녕하세요 리건 🙂
리뷰 잘 반영해주셨네요!
몇 가지 코멘트 더 남겼는데 꼭 확인해주시고 4단계에 함께 반영해주세요!!
4단계도 파이팅입니다 👍

lilychoibb · 2024-09-12T14:33:04Z

tomcat/src/main/java/com/techcourse/controller/AbstractController.java

@@ -0,0 +1,28 @@
+package com.techcourse.controller;


해당 패키지에 있는 Login, Register Controller 와 나머지 Controller 의 역할이 조금 다른 것 같은데 패키지를 분리하시는 건 어떻게 생각하시나요? 🙂

이번 미션하면서 패키지 구분이 정말 어려운 것 같습니다.
이름은 같은 Controller이지만 확연하게 하는 역할이 달라 패키지를 어떻게 구성해야 할지 정말 고민됩니다..

조금 더 고민해보겠습니다!

lilychoibb · 2024-09-12T14:35:42Z

tomcat/src/main/java/com/techcourse/controller/ResourceController.java

+        if ("/".equals(url)) {
+            buildIndexUrlResponse(response);
+        }
+        if (url.endsWith(HTML_SUFFIX)) {
+            buildResourceResponse(response, FileType.HTML, url);
+        }
+        if (url.endsWith(CSS_SUFFIX)) {
+            buildResourceResponse(response, FileType.CSS, url);
+        }
+        if (url.endsWith(JS_SUFFIX)) {
+            buildResourceResponse(response, FileType.JAVASCRIPT, url);
+        }


MIME types 를 조금 더 단순하게 가져올 순 없을까요? 🤔

조금 더 고민해보겠습니다..
당장 떠오르는 건 FileType 클래스가 직접 본인의 미디어 타입을 결정하도록 책임을 넘길 수 있겠네요..

lilychoibb · 2024-09-12T14:39:43Z

tomcat/src/main/java/com/techcourse/controller/LoginController.java

+    private boolean isLogin(final HttpRequest request, final HttpResponse response) {
+        final var cookies = request.getCookies();
+        if (cookies.containsSession() && hasSession(cookies)) {
+            final var session = SessionManager.findSession(cookies.getSessionCookie());


위의 createSession 메서드에서는 Session session 인데 여기선 var 이네요
통일해주는 건 어떨까요?
전체적으로도 통일이 필요할 것 같아요!

var 키워드를 사용했을 때 코드 가독성이 더 떨어지는 상황이 발생하네요.
전반적으로 코드 검토하고 한 방향으로 통일하겠습니다!

lilychoibb · 2024-09-12T14:45:01Z

tomcat/src/main/java/org/apache/coyote/http11/request/HttpRequest.java

@@ -68,14 +75,17 @@ public Map<String, String> getHeaders() {

    public HttpCookie getCookies() {
        final String cookieString = headers.getOrDefault("Cookie", "");


여기랑 from 메서드에 헤더 문자열이 있는데, 이것도 HttpHeaderName 을 사용하는 건 어떠신가요?

반영하겠습니다!

lilychoibb · 2024-09-12T14:48:03Z

tomcat/src/main/java/org/apache/coyote/http11/request/RequestLine.java

        this.path = path;
    }

    public static RequestLine of(final String requestLine) {


of 라고 네이밍 해주신 이유가 궁금해요!
다른 곳은 from 으로 되어있는 것 같아서요 🙂

단일 파라미터에 대한 정적 팩토리 메소드는 from으로 통일했는데 여기서 놓쳤네요!
꼼꼼한 리뷰 감사합니다!

lilychoibb · 2024-09-12T14:50:31Z

tomcat/src/test/java/support/HttpRequestFixture.java

+        final String requestString = String.join("\r\n",
+                "POST /login HTTP/1.1 ",
+                "Host: localhost:8080 ",
+                "Connection: keep-alive ",
+                "Content-Length: 30",
+                "Content-Type: application/x-www-form-urlencoded ",
+                "Accept: */*",
+                "",
+                "account=gugu&password=password ");


requestString 에서 테스트 하는 데 필요한 header 만 추려보면 어떨까요?
HttpRequestFixture 에 있는 requestString 모두를 점검해보면 좋을 것 같아요!

꼭 필요한 헤더만 추려보겠습니다!

hangillee added 28 commits September 4, 2024 13:10

test: 학습 테스트 풀이

c89a99a

feat: 1단계 html 및 css 응답 기능 구현

c654717

feat: Query String 파싱 및 로그인 기능 구현

94de84e

feat: 로그인 기능 구현

c0d7e91

feat: 쿠키 구현

85ed52f

feat: Session 구현

8c08271

feat: 학습 테스트 풀

0364abc

feat: 로그인 구현 및 3단계 1, 2구현

9d15358

Merge branch 'hangillee' into step1

9d48438

fix: 중복 설정 제거

c289a32

refactor: 중복 코드 개선 및 객체 책임 분리

b7a1957

refactor: 파일 패키지 변경

2108252

test: resources 읽고 반환하는 기능 테스트 작성

03c062c

feat: Http Header, Http Method 관리하는 클래스 분리

3720cd7

feat: Controller 인터페이스를 통해 각 클래스 간 역할 분리

f1ddfaa

refactor: 중복 아이디 예외 처리

08708ef

refactor: 코드 가독성 개선 및 누락된 리다이렉트 추가

bc87e45

test: HTTP 프로세서 테스트 작성

2c7ebf8

test: 잘못된 테스트 데이터 수정

e7136b7

fix: 쿠키가 없을 때 비어있는 맵을 반환하도록 수정

74a8dd3

refactor: HttpHeader 클래스의 정적 팩토리 메소드가 자기 자신을 반환하도록 개선

d768186

test: FrontController 테스트 작성

a5c377b

refactor: 쿠키 값의 불필요한 공백 제거

0754344

refactor: 사용하지 않는 메소드 제거

1666d9d

feat: HTTP 응답 바디 반환 메소드 추가

62ffca6

feat: 테스트를 위한 HTTP 요청 fixture 추가

0945e06

test: 로그인과 회원가입 컨트롤러 테스트 작성

503bdd8

test: 테스트 데이터 수정

247bfe6

hangillee requested a review from lilychoibb September 11, 2024 15:53

hangillee self-assigned this Sep 11, 2024

Merge branch 'hangillee' into step3

0d0f318

lilychoibb requested changes Sep 12, 2024

View reviewed changes

hangillee added 3 commits September 12, 2024 16:12

test: 테스트 리팩토링

9a40687

refactor: HTTP 응답 Status Line 분리

a1ace21

refactor: 사용하지 않는 메소드 제거

e020241

hangillee requested a review from lilychoibb September 12, 2024 07:54

lilychoibb approved these changes Sep 12, 2024

View reviewed changes

lilychoibb merged commit 5d8ca42 into woowacourse:hangillee Sep 12, 2024

		public void addCookies(final HttpCookie cookie) {
		headers.addFirst(HttpHeader.setCookie(cookie));

		@@ -68,14 +75,17 @@ public Map<String, String> getHeaders() {

		public HttpCookie getCookies() {
		final String cookieString = headers.getOrDefault("Cookie", "");

Conversation

hangillee commented Sep 11, 2024

Uh oh!

lilychoibb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lilychoibb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants