[톰캣 구현하기 - 1,2단계] 폴로 미션 제출합니다. #357

green-kong · 2023-09-04T08:16:09Z

너무 힘드네요.
최선을 그래도 최선을 다했습니다.
리뷰 잘 부탁드릴게요!

JJ503

안녕하세요 폴로!
코드가 너무 깔끔하고 테스트 코드까지 완벽해 이해하기 정말 좋은 구조였습니다 👍👍
폴로의 커밋을 따라가며 코드를 이해했는데 커밋 단위도 완벽하네요... ✨

그런데 폴로의 의견이 궁금한 점이 몇 가지 있어 해당 답변들을 듣고 싶어 일단 request changes로 두었습니다. (하지만 정말 간단한 질문들...ㅎㅎ)
그 외에는 잘못된 url 요청 시 아무 페이지도 뜨지 않거나 컨벤션적인 부분에 대한 리뷰이니 가볍게 봐주시면 될 것 같습니다!

폴로의 코드 리뷰하며 여러 방면에서 배울 수 있는 시간이었습니다!

JJ503 · 2023-09-05T04:17:41Z

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

+    GET,
+    POST,
+    PUT,
+    PATCH,
+    DELETE,
+    HEAD,
+    OPTION,
+    CONNECT,
+    TRACE;


모든 요청 메서드에 대해 처리해 주셨군요! 👍

그냥 있는거 다 갖다 박았습니다~ㅋㅋ

JJ503 · 2023-09-05T06:34:51Z

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

+        Map<String, String> header = bufferedReader.lines()
+                .takeWhile(line -> !line.isEmpty())


항상 while문을 통해 readline()을 수행해 BufferedReader의 내용들을 가져왔는데 이런 방법도 있군요!
lines()를 통해 stream()으로 가져올 수 있다니 배웠습니다 🙇‍♀️

JJ503 · 2023-09-05T06:38:08Z

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

+    private static final int VERSION_INDEX = 2;
+    private final HttpMethod httpMethod;


static 변수들과 non-static 변수들 사이에 개행이 있다면 더 좋을 것 같네요!

꼼꼼하시네요! 감사합니다.

JJ503 · 2023-09-05T06:51:47Z

tomcat/src/main/java/org/apache/coyote/http11/common/HttpVersion.java

+import org.apache.coyote.http11.exception.InvalidHttpVersionException;
+
+public enum HttpVersion {
+    HTTP_1_1("HTTP/1.1");


HTTP 버전에 대한 확인을 왜 했는지 의문이 들어 간단히 조사해 보았는데 버전별로 요청과 응답 형식이 달랐군요..!
해당 이유가 맞는지 궁금합니다. 혹은 다른 이유도 있을까요?

네네 맞습니다!
저도 이번 미션진행하면서 알게됐네요 ㅎㅎ

JJ503 · 2023-09-05T06:56:31Z

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

+    }
+
+    public String parse() {
+        return String.join(" ", httpVersion.getVersion(), statusCode.getStatus(), "");


" "에 대해서도 상수로 처리해 주면 어떨까요?

전체적으로 보니 문자열에 대해 상수로 만든 것도 있고 아닌 부분들이 혼용되어 있는데 기준이 있는 걸까요?

ㅋㅋ 기준은 없고, 막판에 시간에 쫒겨서 신경못쓴 부분들이 많아요ㅋㅋㅋ
3단계 진행하면서 수정해볼게요~

JJ503 · 2023-09-05T08:05:06Z

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

+    public User parseToUser() {
+        final String account = bodies.get("account");
+        final String password = bodies.get("password");
+        final String mail = bodies.get("email");
+        return new User(account, password, mail);
+    }


parseToUser()가 RequestBody()에 있는 이유가 궁금합니다.
현재까지의 조건과 몇 가지 상황에 대해 생각해 봤을 때 대부분 User에 대한 정보가 있을 것 같긴 합니다.
하지만, 정말 모든 RequestBody()에 User 정보가 있을지 의문이 듭니다.
또한, RequestBody는 그저 전달된 정보를 key-value 형태의 map으로 만들어줄 뿐 user객체를 만들어 반환해 주는 역할이 적절한지 잘 모르겠네요..!

오. 백프로 동의합니다 이부분도 참고하여 3단계 리팩토링 진행하며 수정하도록 할게요~

JJ503 · 2023-09-05T08:15:27Z

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

+        this.request = request;
+    }
+
+    public Response generateResponse() {


만약 api가 추가된다면 해당 메서드에 계속해 추가되겠군요..!
요청 메서드 혹은 uri를 사용한 분기처리 등을 통해 메서드를 분리하거나 각 요청 별로 핸들러를 만들면 어떨까요?

음.. 우선은 어떻게 처리를 해도 api가 추가된다면, 코드의 수정이 이뤄지는건 어쩔수 없다고 생각하는데요.

현재 이 메서드가 이쁘지 않다는 것은 백프로 동의합니다ㅠㅠ
사실 이 분기들을 어떻게 이쁘게 처리할지 고민이 많았는데요..
메서드 별로 먼저 분기처리를 한 뒤에 uri별로 분기처리를 하는 방식으로 하면 조금 더 깔끔해지려나요....
조금 더 고민 해보도록 할게요!

JJ503 · 2023-09-05T08:27:27Z

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

+    public Session getSession(final boolean isNew) {
+        if (isNew) {
+            final Session session = Session.generate();
+            sessionManager.add(session.getId(), session);
+            return session;
+        }
+        final String jsessionid = cookie.findByKey("JSESSIONID");
+        return sessionManager.getById(jsessionid);
+    }


session이 존재하는지 여부에 대해 외부에서 주입받는 방식이 어렵다고 생각합니다.
왜냐하면 외부에서 주입받게 된다면 해당 코드를 작성하는 시점에 세션이 만들어져 있을까에 대해 고민해야 하기 때문입니다.
그런데 이에 대한 검증을 cookie에 위임하면 어떨까요?
당장은 sessionManager.getById(jsessionid);을 사용하거나 혹은 isExistKey(jsessionid)와 같은 메서드를 생성해 존재 여부를 확인하는 방법이 생각나네요!

session이 존재하는지 여부에 대해 외부에서 주입받는 방식이 어렵다고 생각합니다.

사실 이부분이 원래 하려던 방법이 있었는데 시간이 촉박해서 가장 구현이 쉬운 방법으로 급선회해버린 코드인데...
정말 귀신같이 알아내셨네요ㅋㅋㅋㅋ

이 부분도 3단계진행하며 수정해보도록 하겠습니다!

JJ503 · 2023-09-05T09:01:36Z

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

+            return register();
+        }
+
+        return getStaticPateResponse(requestPath, OK);


requestPath에 저희가 설정한 path 외의 uri가 들어갔을 때에 대한 처리가 필요할 것 같아요.
현재는 http://localhost:8080/test와 같은 url로 요청했을 때 아무 페이지도 나오지 않네요..!

엇... 전혀 생각해보지 못한거네요ㅋㅋㅋ
static에 404.html이 있던거 같은데 그 파일을 서빙하는 식으로 바꿔볼게요~

등록되지 않은 URI로 요청하는 경우 404.html을 반환하도록 수정했습니다!
404.html에 svg 파일이 포함되어있었는데, ContentType enum에 해당하는 값이 없다보니
ContentType.HTML을 반환하다보니 이미지가 정상적으로 표시되지 않는 이슈가 있었습니다.

해당 부분 ContentType에 SVG 추가해주었더니 해결되었습니다!
제이미 덕에 svg파일의 ContentType을 어떤식으로 반환해야 하는지 알게되었네요!
https://css-tricks.com/snippets/htaccess/serve-svg-correct-content-type/

JJ503 · 2023-09-05T09:06:58Z

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

+        final Map<String, String> bodies = Arrays.stream(requestBody.split(DELIMITER))
+                .map(keyValue -> keyValue.split(KEY_VALUE_DELIMITER))
+                .collect(Collectors.toMap(
+                        splitKeyValue -> splitKeyValue[KEY_INDEX],
+                        splitKeyValue -> splitKeyValue[VALUE_INDEX]
+                ));


만약 로그인 페이지에서 아무 값 없이 로그인 버튼을 누르면 현재 로그인 로직을 실행하고 있습니다.
그런데 데이터가 없다 보니 java.lang.ArrayIndexOutOfBoundsException에 대한 예외가 발생하네요..!
이에 대한 처리가 필요해 보입니다.

오.. 이것도 확인하지 못한 부분인데..

꼼꼼하게 체크해주셔서 감사합니다 🙇‍♂️

public static RequestBody convert(final BufferedReader bufferedReader, int contentLength) throws IOException { char[] buffer = new char[contentLength]; bufferedReader.read(buffer, 0, contentLength); String requestBody = new String(buffer); final Map<String, String> bodies = Arrays.stream(requestBody.split(DELIMITER)) .map(keyValue -> keyValue.split(KEY_VALUE_DELIMITER)) .collect(Collectors.toMap( splitKeyValue -> splitKeyValue[KEY_INDEX], getValue() )); return new RequestBody(bodies); } private static Function<String[], String> getValue() { return splitKeyValue -> { if (splitKeyValue.length < 2) { return ""; } return splitKeyValue[VALUE_INDEX]; }; }

위와 같이 변경했습니다! 근데 람다 내부에서 if 쓰는게 너무 꼴뵈기 싫어서 람다 리턴하는 메서드를 분리해주긴 했는데요..
조금 나아진거지 꼴보기 싫은건 마찬가지네요ㅠㅠㅠ

이런 경우엔 그냥 삼항연산자 쓰는건 어떻게 생각하시나요?
삼항연산자쓰면 퇴출 당하려나요?ㅋㅋㅋㅋ

public static RequestBody convert(final BufferedReader bufferedReader, int contentLength) throws IOException { char[] buffer = new char[contentLength]; bufferedReader.read(buffer, 0, contentLength); String requestBody = new String(buffer); final Map<String, String> bodies = Arrays.stream(requestBody.split(DELIMITER)) .map(keyValue -> keyValue.split(KEY_VALUE_DELIMITER)) .collect(Collectors.toMap( splitKeyValue -> splitKeyValue[KEY_INDEX], splitKeyValue -> splitKeyValue.length < 2 ? "" : splitKeyValue[VALUE_INDEX] )); return new RequestBody(bodies); }

오히려 깔끔하지 않나요..?

저는 위와 같은 경우 삼항연산자를 사용해도 괜찮다고 봅니다.
또한, 폴로가 말씀해 주신 대로 해당 코드는 삼항 연산자가 더 깔끔해 보이네요!
삼항연산자의 경우 남용되거나 너무 복잡한 조건에 사용되었을 때 가독성을 낮추는 것으로 알고 있는데 위 경우는 간결한 조건인만큼 오히려 가독성에 도움을 준다고 생각합니다.

다만, 현재는 개인 프로젝트라 상관없다고 생각하지만 만약 팀 컨벤션으로 삼항연산자를 사용하지 말자라고 정한 경우에만 아예 사용하면 안 된다고 생각하고 있습니다.

404.html에 svg파일이 포함되어 있었는데, ContentType Enum에 SVG가 없어서 추가해주었음.

기존에 account=; password= 이런식으로 들어오면 arrayindexoutofbound 터졌는데, split의 결과의 length가 2 이하인 경우, ""(빈스트링)을 value로 설정하도록 수정.

green-kong · 2023-09-05T11:50:59Z

#357 (comment)
#357 (comment)

우선은 위 두개의 리뷰내용만 반영하였습니다 ㅎㅎ
제이미의 소중한 다른 리뷰들은 3단계 리팩토링 하며 진행해보려 합니다~

꼼꼼한 리뷰덕에 미처 확인 하지 못한 부분들을 캐치할 수 있었네요.
고마워요 제이미!

sonarcloud · 2023-09-05T11:58:55Z

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

JJ503

리뷰 반영하신 내용 잘 봤습니다.
폴로 덕분에 저도 배워가네요!
그리고 프로젝트와 병행하면서 미션 시간을 만드는 게 아직 어려운데 폴로는 꼼꼼한데 빠르기까지..! 대단하시네요 👍

더 이상의 수정 사항은 보이지 않아 1, 2 단계는 머지하도록 하겠습니다.
3, 4 단계에서 뵙겠습니다!

green-kong added 24 commits September 3, 2023 14:34

feat: File Test

96ad702

feat: IOStream Test

81ee2a5

feat: HttpMethod 구현

df44ae2

feat: RequestLine 구현

61bd0b6

feat: RequestHeader 구현

df313ca

feat: Request 구현

31b2dc6

fix: requestVersion -> HttpVersion

812ce7e

feat: ResponseLine 구현

5bb5a11

feat: ResponseHeader구현

aeeffff

feat: Respone 구현

56d7dca

feat: 정적파일 서빙 구현

076d8a8

feat: registeredUri 구현

c289fbe

feat: 쿼리스트링 구현

6e16fcf

feat: 쿼리스트링 적용

5059e59

feat: 존재하는 회원인 경우 로그 출력

764b59b

feat: 로그인 성공하는 경우 index.html 로 리다이렉트, 실패하는 경우 401.html로 이동하는 기능 구현

2f56ce8

feat: /register 로 GET 요청시, register.html 보여주는 기능 구현

f5d6059

feat: RequestBody 구현

f48ed1d

feat: POST로 회원가입 기능 구현

d92e00a

feat: POST로 로그인 기능 구현

10b237d

feat: login시 쿠키 저장기능 구현

a068784

feat: login 시 세션 저장 가능

2287c0b

feat: 세션을 통한 재로그인 기능 구현

c15d406

fix: 테스트 코드 수정

ab54687

green-kong self-assigned this Sep 4, 2023

JJ503 self-requested a review September 4, 2023 10:14

JJ503 requested changes Sep 5, 2023

View reviewed changes

green-kong added 2 commits September 5, 2023 20:27

fix: 등록되지 않은 uri로 요청시 404.html 보여주도록 수정

3f25cda

404.html에 svg파일이 포함되어 있었는데, ContentType Enum에 SVG가 없어서 추가해주었음.

fix: RequestBody 생성로직 수정

47fad0c

기존에 account=; password= 이런식으로 들어오면 arrayindexoutofbound 터졌는데, split의 결과의 length가 2 이하인 경우, ""(빈스트링)을 value로 설정하도록 수정.

refactor: 람다 분리

901b6ab

JJ503 approved these changes Sep 5, 2023

View reviewed changes

JJ503 merged commit 3004eda into woowacourse:green-kong Sep 5, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 1,2단계] 폴로 미션 제출합니다. #357

[톰캣 구현하기 - 1,2단계] 폴로 미션 제출합니다. #357

green-kong commented Sep 4, 2023

JJ503 left a comment

JJ503 Sep 5, 2023

green-kong Sep 5, 2023

JJ503 Sep 5, 2023

JJ503 Sep 5, 2023

green-kong Sep 5, 2023

JJ503 Sep 5, 2023

green-kong Sep 5, 2023

JJ503 Sep 5, 2023

JJ503 Sep 5, 2023

green-kong Sep 5, 2023

JJ503 Sep 5, 2023

green-kong Sep 5, 2023

JJ503 Sep 5, 2023

green-kong Sep 5, 2023

JJ503 Sep 5, 2023

green-kong Sep 5, 2023

JJ503 Sep 5, 2023

green-kong Sep 5, 2023

green-kong Sep 5, 2023

JJ503 Sep 5, 2023

green-kong Sep 5, 2023

green-kong Sep 5, 2023

green-kong Sep 5, 2023

JJ503 Sep 5, 2023 •

edited

Loading

green-kong commented Sep 5, 2023

sonarcloud bot commented Sep 5, 2023

JJ503 left a comment •

edited

Loading

		Map<String, String> header = bufferedReader.lines()
		.takeWhile(line -> !line.isEmpty())

		private static final int VERSION_INDEX = 2;
		private final HttpMethod httpMethod;

[톰캣 구현하기 - 1,2단계] 폴로 미션 제출합니다. #357

[톰캣 구현하기 - 1,2단계] 폴로 미션 제출합니다. #357

Conversation

green-kong commented Sep 4, 2023

JJ503 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

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

JJ503 Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

green-kong commented Sep 5, 2023

sonarcloud bot commented Sep 5, 2023

JJ503 left a comment • edited Loading

Choose a reason for hiding this comment

JJ503 Sep 5, 2023 •

edited

Loading

JJ503 left a comment •

edited

Loading