[톰캣 구현하기 - 1,2단계] 에단(김석호) 미션 제출합니다. #372

cookienc · 2023-09-04T08:39:17Z

안녕하세요 무민! 리뷰이 에단입니다.
시간이 촉박해서 깔끔하게 짜지는 못했습니다.
다음번에 리팩토링 할 때 더 예쁘게 해보겠습니다. 리뷰 미리 감사드립니다!

parkmuhyeun

안녕하세요 에단!

톰켓 구현하시느라 고생 많으셨습니다. 브랜치 step2는 노리신 건가요!? 😀
이번에 어려웠던거 같은데 FrontHandler, HandlerMapping 등 잘 구현해주셨네요 ㅎㅎ
특히 support 부분 인상 깊었습니다 👍🏻👍🏻

잘 구현해주셨지만 좀 더 개선할 점이 보여 커멘트 남겼습니다. 그리고 Mapping 부분들에 공통적으로 반영했으면 좋겠는 부분이 있어서 Mapping 뒤쪽에는 따로 코멘트를 안 적었고 여기 남기겠습니다!
다른 객체로 역할을 분리하거나 메서드를 분리하면 Mapping쪽의 코드들이 많이 줄고 하드 코딩들도 줄어서 가독성도 더 좋아질거 같습니다.

그리고 다음번에 PR할 때는 테스트코드도 있으면 더 좋을 거 같아요!
+없는 path로 들어오면 404.html 띄워주는 것도 해보면 어떨까요??

parkmuhyeun · 2023-09-04T14:39:01Z

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


            outputStream.write(response.getBytes());
            outputStream.flush();
        } catch (IOException | UncheckedServletException e) {
            log.error(e.getMessage(), e);
        }
    }
+
+    private static String parseRequestBody(final HttpMethod httpMethod, final HttpHeaders headers, final BufferedReader bufferedReader) throws IOException {


static이 붙어있어용

매의 눈이네요!

parkmuhyeun · 2023-09-04T14:47:19Z

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

@@ -28,20 +40,48 @@ public void run() {
    public void process(final Socket connection) {
        try (final var inputStream = connection.getInputStream();
             final var outputStream = connection.getOutputStream()) {
+            final InputStreamReader inputStreamReader = new InputStreamReader(inputStream);
+            final BufferedReader bufferedReader = new BufferedReader(inputStreamReader);
+            final String firstLine = bufferedReader.readLine();


지금 request의 정보가 다 나눠져있고 나눠져있는 걸 그대로 handle에 넘겨서 또 거기서 나눠지고 있는데
요런 정보들을 RequestParser 같은 객체로 분리해보는 건 어떨까요? 응집도도 높아지고 넘기는 매개변수도 줄어들 수 있을 거 같아요!

넵! 3단계에 httprequest와 httpresponse로 변경하기가 있어서 남겨뒀는데, 이번 단계에서는 httpRequest 만들어서 처리하겠습니다!

parkmuhyeun · 2023-09-04T14:52:40Z

tomcat/src/main/java/org/apache/coyote/handler/FrontHandler.java

+        for (final HandlerMapping mapping : handlerMapping) {
+            final HttpMethod httpMethod = HttpMethod.from(parsedFirstLine[0]);
+            final String requestUri = parsedFirstLine[1];
+            if (mapping.supports(httpMethod, requestUri)) {
+                response = mapping.handle(parsedFirstLine[1], headers, requestBody);
+                break;
+            }
+        }


위처럼 하게되면 메서드로 꺼내 쓸 수 있어서 parseFirstLine[0], parsedFirstLine[1] 이런 것들도 이름이 좀 더 명확해질 거 같아요.

parkmuhyeun · 2023-09-04T15:01:49Z

tomcat/src/main/java/org/apache/coyote/handler/FrontHandler.java

+        for (final HandlerMapping mapping : handlerMapping) {
+            final HttpMethod httpMethod = HttpMethod.from(parsedFirstLine[0]);
+            final String requestUri = parsedFirstLine[1];
+            if (mapping.supports(httpMethod, requestUri)) {


supports 굉장히 깔끔하네요 👍🏻👍🏻👍🏻

감사합니다!

parkmuhyeun · 2023-09-04T15:05:48Z

tomcat/src/main/java/org/apache/coyote/handler/mapping/LoginMapping.java

+        final Map<String, String> bodyParams = Arrays.stream(requestBody.split("&"))
+                .map(param -> param.split("="))
+                .collect(Collectors.toMap(param -> param[0], param -> param[1]));


HttpParser로 객체 분리하게 되면 자연스럽게 분리되어 메서드를 호출하면 되는데 만약에 분리 안하신다면 메서드로 분리하셔서 어떤 연산인지 알려주면 더 가독성이 좋을거 같아요!

parkmuhyeun · 2023-09-04T15:08:55Z

tomcat/src/main/java/org/apache/coyote/handler/mapping/LoginMapping.java

+                    .orElseThrow(() -> new IllegalArgumentException("잘못된 계정입니다. 다시 입력해주세요."));
+
+            if (!user.checkPassword(password)) {
+                throw new IllegalArgumentException("잘못된 비밀번호입니다. 다시 입력해주세요.");


꼼꼼하시네요 bb

parkmuhyeun · 2023-09-04T15:10:35Z

tomcat/src/main/java/org/apache/coyote/handler/mapping/LoginMapping.java

+            return String.join("\r\n",
+                    "HTTP/1.1 302 Found ",
+                    "Location: /401.html ");


response 하는 부분들도 하드코딩 되어있는데 response를 생성해주는 객체로 분리해주면 재사용성과 유지보수에 더 좋을거 같아요!

이걸 보고 httpRequest와 httpResponse를 만들었습니다. 살짝 3단계를 첨가했어요...

parkmuhyeun · 2023-09-04T15:25:55Z

tomcat/src/main/java/org/apache/coyote/handler/mapping/RegisterMapping.java

+        final String password = bodyParams.get("password");
+        final String email = bodyParams.get("email");
+
+        final User user = new User(account, password, email);


음... 이번 톰켓 미션과 크게 관련있는지는 모르겠는데 똑같은 걸로 계속 가입이 되더라구요.
요 부분은 에단이 원하면 반영해주세요!

parkmuhyeun · 2023-09-04T15:27:41Z

tomcat/src/main/java/org/apache/coyote/handler/mapping/StaticFileMapping.java

+    @Override
+    public boolean supports(final HttpMethod httpMethod, final String requestUri) {
+        return GET == httpMethod &&
+                ("/".equals(requestUri) ||


"/"도 StaticFile에 속할 수 있는지 궁금합니다.

잘못 생각했습니다! 지적 감사합니다

parkmuhyeun · 2023-09-04T15:45:14Z

tomcat/src/main/java/org/apache/coyote/handler/mapping/LoginFilter.java

+        return loginManager.isAlreadyLogined(jSessionId);
+    }
+
+    protected void setSession(final String jSessionId, final Map<String, String> sessionData) {


session에 User대신 account(string)를 넣으신 이유가 있나요?

인가가 구현되어 있지 않아 account만으로도 인증만 구별하도록 했습니다

parkmuhyeun · 2023-09-04T16:03:13Z

tomcat/src/main/java/org/apache/coyote/http/SessionManager.java

+        if (Objects.nonNull(SESSIONS.get(id))) {
+            return true;
+        }
+
+        return false;


이 부분 return Objects.nonNull(SESSIONS.get(id)); 한 줄로 줄일 수 있을거 같아요!

sonarcloud · 2023-09-06T15:16:50Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

0.0% Coverage
0.0% Duplication

The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

cookienc · 2023-09-06T15:20:12Z

안녕하세요 무민! 에단입니다. 정성스러운 코드리뷰 감사합니다. 코드리뷰 주신대로 httpRequst와 httpResponse로 Parser를 만들어서 분리해봤습니다. 더 고칠 수 있겠지만, 변경된 사항이 많아서 리뷰하기가 힘드실것 같아서 제출합니다. 테스트는 3 - 4단계에서 진행하겠습니다

parkmuhyeun

이번에 리뷰 할 때 미리 3단계꺼 까지 하도록 해서 약간 죄송했었는데 잘 적용해주셔서 감사합니다!
적용하고 나니 하드코딩이 엄청나게 줄고 역할 분리가 잘 되어서 코드가 매우 깔끔해졌네요 👍🏻

조금의 코멘트 더 남겼고 요건 다음 단계 때 같이 반영해주세요!
아 그리고 테스트 코드까지 추가 해주시면 Best 🙌

고생하셨습니다.

parkmuhyeun · 2023-09-07T01:27:17Z

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

+
+    private final HttpMethod httpMethod;
+    private final RequestUri requestUri;
+


공백이 하나있네요!

parkmuhyeun · 2023-09-07T01:37:44Z

tomcat/src/main/java/org/apache/coyote/handler/mapping/LoginPageMapping.java

            }
        }

-        final String[] parsedRequestUri = requestUri.split("\\?");
-        if (requestUri.contains("?")) {
+        final String[] parsedRequestUri = httpRequest.getRequestUri().getRequestUri().split("\\?");


parsedRequestUri가 아래 if문 안에서만 사용되는 거 같아서 If문 안에 들어가도 될 것 같아요!

parkmuhyeun · 2023-09-07T02:24:20Z

tomcat/src/main/java/org/apache/coyote/handler/mapping/LoginPageMapping.java

-        final String[] parsedRequestUri = requestUri.split("\\?");
-        if (requestUri.contains("?")) {
+        final String[] parsedRequestUri = httpRequest.getRequestUri().getRequestUri().split("\\?");
+        if (httpRequest.getRequestUri().getRequestUri().contains("?")) {
            final Map<String, String> queryStrings = Arrays.stream(parsedRequestUri[1].split("&"))
                    .map(param -> param.split("="))
                    .collect(Collectors.toMap(param -> param[0], param -> param[1]));


이 부분도 HttpRequest안으로 분리해보는 건 어떨까요?

parkmuhyeun · 2023-09-07T02:27:20Z

tomcat/src/main/java/org/apache/coyote/handler/mapping/LoginMapping.java


 public class LoginMapping extends LoginFilter implements HandlerMapping {

+    public static final String TARGET_URI = "login";


접근제어자 private으로 해주면 더 좋을 것 같아요.

parkmuhyeun · 2023-09-07T02:32:03Z

tomcat/src/main/java/org/apache/coyote/http/common/HttpHeader.java

+    SET_COOKIE("Set-Cookie"),
+    UPGRADE_INSECURE_REQUESTS("Upgrade-Insecure-Requests"),
+    USER_AGENT("User-Agent"),
+    ;


와우 👍🏻👍🏻

cookienc added 18 commits September 3, 2023 13:50

test: FileTest 작성

92cba7e

test: IOEStreamTest 작성

d115a52

feat: 정적파일 파싱 기능 구현

2a7c45c

feat: header 파싱 기능 분리

4507b9d

feat: queryString 파싱 기능 구현

cdd1155

feat: login 기능 구현

63eb5f4

refactor: login을 post로 변경

159001a

refactor: fronthandler를 생성해서 정적파일을 처리하도록 변경

0a2fbfd

refactor: fronthandler가 GET을 처리하도록 변경

c1a1e25

refactor: LoginMapping, LoginPageMapping으로 리팩토링

59d8f91

feat: 회원가입 화면 추가

8e5ff25

feat: 회원가입 기능 구현

45180a3

refactor: handle 메서드에 header 추가

7720677

feat: Cookie를 파싱하는 HttpCookie 클래스 구현

e3be3bc

feat: Cookie 추가

3f4bb9e

feat: Session 추가

96e89a9

refactor: 필요없는 throws 제거

0e56b55

refactor: HttpMethod enum 추가

df69091

cookienc requested a review from parkmuhyeun September 4, 2023 08:39

cookienc self-assigned this Sep 4, 2023

cookienc changed the title ~~[톰캣 구현하기 - 1,2단계] 에단 미션 제출합니다.~~ [톰캣 구현하기 - 1,2단계] 에단(김석호) 미션 제출합니다. Sep 4, 2023

refactor: HttpHeaders 로 추가

ded667a

parkmuhyeun requested changes Sep 4, 2023

View reviewed changes

parkmuhyeun reviewed Sep 4, 2023

View reviewed changes

cookienc added 6 commits September 6, 2023 12:49

refactor: 필요없는 static 제거

c839e59

refactor: BufferedReader를 try with resources로 변겨

b406c5e

refactor: HttpRequest로 변경

d0380db

refactor: HttpRequest로 변경

cff2f3b

refactor: session 및 request 패키지 변경

cf9b8bc

refactor: startLine 을 requestLine 으로 변경

fae548b

cookienc added 9 commits September 6, 2023 23:28

refactor: httpResponse 로 리팩토링

3b0c080

refactor: 레거시 코드 제거

01ef6dd

refactor: hello world가 text로 나가도록 변경

59b8ca5

fix: 리다이렉트가 될 때 html이 안보이는 문제 해결

b332054

refactor: isAlreadyLogined 메서드를 한 줄로 변경

aa5d8ec

refactor: "/" url을 homePageMapping으로 변경

8bf5593

refactor: body에 빈 공백 제거

5f5c2cb

refactor: header관련 설정 변경

8db321f

refactor: ContentLength가 잘못나오던 문제 해결

feb7a82

parkmuhyeun approved these changes Sep 7, 2023

View reviewed changes

parkmuhyeun merged commit b6e02c7 into woowacourse:cookienc Sep 7, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 1,2단계] 에단(김석호) 미션 제출합니다. #372

[톰캣 구현하기 - 1,2단계] 에단(김석호) 미션 제출합니다. #372

cookienc commented Sep 4, 2023

parkmuhyeun left a comment •

edited

Loading

parkmuhyeun Sep 4, 2023

cookienc Sep 6, 2023

parkmuhyeun Sep 4, 2023

cookienc Sep 6, 2023

parkmuhyeun Sep 4, 2023

parkmuhyeun Sep 4, 2023

cookienc Sep 6, 2023

parkmuhyeun Sep 4, 2023 •

edited

Loading

parkmuhyeun Sep 4, 2023

parkmuhyeun Sep 4, 2023 •

edited

Loading

cookienc Sep 6, 2023

parkmuhyeun Sep 4, 2023

parkmuhyeun Sep 4, 2023

cookienc Sep 6, 2023

parkmuhyeun Sep 4, 2023

cookienc Sep 6, 2023

parkmuhyeun Sep 4, 2023

sonarcloud bot commented Sep 6, 2023

cookienc commented Sep 6, 2023

parkmuhyeun left a comment

parkmuhyeun Sep 7, 2023

parkmuhyeun Sep 7, 2023

parkmuhyeun Sep 7, 2023

parkmuhyeun Sep 7, 2023

parkmuhyeun Sep 7, 2023


		private final HttpMethod httpMethod;
		private final RequestUri requestUri;


		public class LoginMapping extends LoginFilter implements HandlerMapping {

		public static final String TARGET_URI = "login";

[톰캣 구현하기 - 1,2단계] 에단(김석호) 미션 제출합니다. #372

[톰캣 구현하기 - 1,2단계] 에단(김석호) 미션 제출합니다. #372

Conversation

cookienc commented Sep 4, 2023

parkmuhyeun left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkmuhyeun Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkmuhyeun Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 6, 2023

cookienc commented Sep 6, 2023

parkmuhyeun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkmuhyeun left a comment •

edited

Loading

parkmuhyeun Sep 4, 2023 •

edited

Loading

parkmuhyeun Sep 4, 2023 •

edited

Loading