[톰캣 구현하기 - 1,2단계] 주노(최준호) 미션 제출합니다. #320

Choi-JJunho · 2023-09-04T04:57:53Z

최대한 읽기쉽게 구성하려고 노력했으나 아직 부족한 부분이 많은 것 같습니다.
잘부탁드립니다.

학습테스트를 제외한 Commit에 대한 범위를 지정했습니다.
리뷰 간 아래 링크를 통해 Files chagned 내역을 확인해주세요!

작업내역 - 커밋 지정

Kim0914

안녕하세요 주노 ! 그레이입니다 🌱
같은 팀인데 미션에서도 만나다니 운명인가 봅니다^^

구조를 분리하지 않고 작성해주셨는데도 너무너무 잘 읽혀서 리뷰하기 편했어요 :)
컨벤션이나 상수화같은 부분은 잘 아실거라 생각하고 별도의 코멘트 남기지 않았습니다 !
또한 3단계가 리팩터링이기 때문에 구조같은 부분도 코멘트 남기지 않았습니다 !

궁금한 부분 몇 가지만 코멘트로 남겼습니다 ~ 확인부탁드려요 ㅎㅎ

Kim0914 · 2023-09-04T11:21:58Z

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

+    public String getType() {
+        return String.format("%s;%s", type, DEFAULT_UTF8);
+    }


CharSet을 이렇게 보여줄 수도 있군요 👍

Kim0914 · 2023-09-04T11:22:27Z

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

+    private static final String DEFAULT_UTF8 = "charset=utf-8";
+    private final String type;


전역 변수와 필드 변수 라인으로 구분해도 좋을 것 같아요 ㅎㅎ

Kim0914 · 2023-09-04T11:38:48Z

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

+            bufferedWriter.write(header + httpResponse.getBody());
+            bufferedWriter.flush();


OutputStream을 사용할 수도 있는데 BufferedWriter 객체를 사용해주신 이유가 궁금합니다 !!

String으로 가공한 자료를 Byte 로 한번 더 치환하여 출력하는 상황이 비즈니스 로직 상에서 불필요한 행위라고 생각하여 String 데이터를 write할 수 있는 bufferedWriter를 사용했습니다!

Kim0914 · 2023-09-04T11:44:27Z

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

+    }
+
+    private HttpResponse routePost(String uri, RequestHeader requestHeader, RequestBody requestBody) throws IOException {
+


여기 한 줄 공백이 숨어 있네요 !

Kim0914 · 2023-09-04T11:58:37Z

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

+        String query = requestBody.getItem();
+        if (query.isBlank()) {
+            return ViewResolver.resolveView(LOGIN_URI);
+        }


로그인을 완료한 상태에서 GET /login으로 접근하면 리다이렉트 302로 반환하지 않고 200으로 반환할 것 같은데요 !
이 부분은 의도해주신 부분인지 궁금합니다 ㅎㅎ

로그인된 상태에서 /login 페이지에 HTTP GET method로 접근하면 이미 로그인한 상태니 index.html 페이지로 리다이렉트 처리한다.

앗 리다이렉트 처리라는 구문에 집중하지 못했던 것 같네요!
이렇게 처리된다면 클라이언트 측에서 POST 요청 후 페이지 로딩으로 이뤄지니 정상적인 흐름이라고 보기에는 어려울 수 있겠네요...😅
리다이렉트하도록 수정했습니다!

Kim0914 · 2023-09-04T12:07:43Z

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

+    }
+
+
+    public static HttpResponse routePath(String path, String query) throws IOException {


query파라미터는 사용하고 있지 않는 것 같아요 !!

Kim0914 · 2023-09-04T12:09:38Z

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

+            return new HttpResponse("Hello world!", HttpStatus.OK, TEXT_HTML);
+        }
+
+        return ViewResolver.resolveView(path);


같은 클래스에 있기 때문에 resolveView(path);만 호출해도 될 것 같아요 ~

Kim0914 · 2023-09-04T12:12:49Z

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

+import static org.apache.coyote.http11.ContentType.CSS;
+import static org.apache.coyote.http11.ContentType.TEXT_HTML;
+
+public class ViewResolver {


현재 ViewResolver에서 HttpResponse를 모두 반환하고 있는데요 !
ViewResolver에게 HttpResponse 생성 책임을 부여해주신 이유가 궁금합니다 ㅎㅎ

String 타입의 뷰 내용을 그대로 반환하고 Http11Processor의 createHttpResponse 내부에서 뷰 내용을 받아서 HttpResponse를 만들어주는건 어떤지 제안드려봅니다 !

확실히 Http Status Code, ContentType 모두 View Resolver에서 관리하는게 과한 책임이 든다고 생각되네요!

하지만 저는 되려 ViewResolver에서 HttpResponse를 반환하는 것이 되려 자연스럽다는 생각입니다!

응답해야하는 파일이 html, css처럼 정적으로 제공해야하는 파일인지 json 형태의 데이터 형태의 파일인지는 현재 구현되어있는 process 과정에서 다음과 같은 조건을 만족해야한다고 생각합니다.

path를 알아야한다.

method를 알아야한다.

위에서 결정된 path, method로부터 반환되는 타입을 결정짓는다.

위와같은 고려사항이 있다보니 View에 해당하는 파일인지는 특정 path, method를 처리하는 함수 내부에서 결정짓는다고 생각했습니다.
예를들면 /login, GET 요청에 도달했을 경우 view파일을 반환한다 가 대표적인 예가 되겠네요.

그레이가 제안해주신 배경은 충분히 공감하나 현재 로직의 흐름 상으로 view 내용을 그대로 반환받아 로직에서 이용하게 된다면 해당 값이 ResponseBody의 내용인지, view의 내용인지 판별해야하는 추가적인 검증이 필요하기 때문에 현재 상태를 유지하는것이 더 적은 검증을 수행하며 안정적으로 view를 반환할 수 있는 방향이라고 생각합니다.

따라서 ViewResolver에서 HttpResponse를 조합하고 반환하는 방향을 유지하고자 합니다~!

코멘트로 남겨주신 글을 읽어보니 이해가 되네요 ㅎㅎ

"뷰 페이지를 반환하는 경우 일괄적으로 ViewResolver를 통해 반환한다" 라고 할 수 있겠네요 !

좋습니다 ~

Kim0914 · 2023-09-04T12:13:51Z

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

+        }
+        return Arrays.stream(string.split(" "))
+                .map(line -> line.split("="))
+                .collect(collectingAndThen(


collectingAndThen 사용 👍👍

Kim0914 · 2023-09-04T12:15:28Z

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

+        if (account.isBlank() || password.isBlank() || email.isBlank()) {
+            return ViewResolver.resolveView(REGISTER_URI);
+        }


입력이 잘못 들어온 경우 다시 입력받을 수 있도록 해주는 센스 좋네요 ^0^

sonarcloud · 2023-09-04T14:09:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 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

Kim0914

주노 ~~ 리뷰 반영 확인했습니다 ㅎㅎ

아주 사소한 부분은 다음 단계 리팩토링 진행하면서 해주시면 될 것 같아요 !
고생하셨습니다 :)

Kim0914 · 2023-09-05T04:56:29Z

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

+        if (LOGIN_URI.equals(path)
+                && cookie != null
+                && sessionManager.findSession(cookie.getJSessionId(false)) != null) {
+            return ViewResolver.resolveView(INDEX_URI);
+        }


이부분 리팩토링하면서 진행해주시면 될 것 같네요 😉

Choi-JJunho added 14 commits September 3, 2023 11:11

test: IOStream 학습테스트

b298b80

test: File 학습테스트

b0e2503

feat: GET /index.html 응답하기

a3cda7d

feat: CSS 지원하기

715bb4b

feat: Query String 파싱

001d50d

test: 로그인 페이지 테스트 추가

588c23f

test: css 테스트 추가

0f5bbf4

refactor: HttpStatus, ContentType 분리

4510d66

feat: HTTP Status Code 302 구현

ace86ae

feat: 회원가입, 로그인 POST 구현

42da21e

feat: 로그인 시 Session 쿠키 저장

0b7002d

feat: 유효한 Session으로 Login 페이지 접근 시 index로 리다이렉트

e8aabbd

style: System.out -> log

71b0a5c

test: 테스트 수정

e6b841f

Kim0914 requested changes Sep 4, 2023

View reviewed changes

Choi-JJunho added 3 commits September 4, 2023 21:46

style: 코드 포맷팅

59a8419

refactor: ContentType 파일명으로부터 구분

2a7e641

test: login post 시 redirect로 변경

4f18be7

Kim0914 approved these changes Sep 5, 2023

View reviewed changes

Kim0914 merged commit 958ba79 into woowacourse:choi-jjunho Sep 5, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 1,2단계] 주노(최준호) 미션 제출합니다. #320

[톰캣 구현하기 - 1,2단계] 주노(최준호) 미션 제출합니다. #320

Choi-JJunho commented Sep 4, 2023 •

edited

Loading

Kim0914 left a comment

Kim0914 Sep 4, 2023

Kim0914 Sep 4, 2023

Kim0914 Sep 4, 2023

Choi-JJunho Sep 4, 2023

Kim0914 Sep 4, 2023

Kim0914 Sep 4, 2023

Choi-JJunho Sep 4, 2023

Kim0914 Sep 4, 2023

Kim0914 Sep 4, 2023

Kim0914 Sep 4, 2023

Choi-JJunho Sep 4, 2023

Kim0914 Sep 5, 2023

Kim0914 Sep 4, 2023

Kim0914 Sep 4, 2023

sonarcloud bot commented Sep 4, 2023

Kim0914 left a comment

Kim0914 Sep 5, 2023

		private static final String DEFAULT_UTF8 = "charset=utf-8";
		private final String type;

		bufferedWriter.write(header + httpResponse.getBody());
		bufferedWriter.flush();

		}

		private HttpResponse routePost(String uri, RequestHeader requestHeader, RequestBody requestBody) throws IOException {

		}


		public static HttpResponse routePath(String path, String query) throws IOException {

[톰캣 구현하기 - 1,2단계] 주노(최준호) 미션 제출합니다. #320

[톰캣 구현하기 - 1,2단계] 주노(최준호) 미션 제출합니다. #320

Conversation

Choi-JJunho commented Sep 4, 2023 • edited Loading

Kim0914 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

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

Choose a reason for hiding this comment

sonarcloud bot commented Sep 4, 2023

Kim0914 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choi-JJunho commented Sep 4, 2023 •

edited

Loading