[톰캣 구현하기 - 3, 4단계] 저문(유정훈) 미션 제출합니다. #459

jeomxon · 2023-09-10T17:01:15Z

안녕하세요 조이, 저문입니다 :)
이전에 남겨주신 리뷰를 통해서 많이 배웠습니다!
지난 리뷰에 대한 답변은 여기에 남겨두었습니다.

이번은 3단계 리팩터링을 진행하는데 꽤나 오랜시간이 걸렸던 것 같아요.
4단계는 아직 공부해야할 내용이 산더미네요..ㅎㅎ

크게 변한건 없지만 Controller가 생기면서 로직이 훨씬 알아보기 편해진 것 같아요.
그만큼 조이가 리뷰하시기 수월했으면 하네요.

리뷰 잘 부탁드립니다🙇🏻‍♂️🙇🏻‍♂️🙇🏻‍♂️

yeonkkk

안녕하세요 저문! 조이입니다🍀

이번에도 구현을 정말 잘 해주셨네요..
코드가 너무 깔끔해서 리뷰하기도 너무 편했고, 정말 많이 배웠습니다🙂
남길 코멘트가 거의 컨벤션 관련된 것 뿐이네요 ㅎㅎ 역시 고수,,👀⭐️

지난 PR과 이번 PR에 모두 코멘트 남겨두었으니, 편하실 때 확인 부탁드려요 ㅎㅎ
고생하셨습니다!

yeonkkk · 2023-09-11T11:19:49Z

tomcat/src/main/java/org/apache/coyote/http11/controller/RegisterController.java

+ @Override
+ protected void doGet(final HttpRequest request, final HttpResponse response) {
+ response.setHttpStatus(HttpStatus.OK);
+ response.setPath(REGISTER_PAGE);
+ }


요구사항은 아니지만, 로그인한 상태로 /register 를 누르면 회원가입 창으로 가네요!
로그인한 사용자 입장에서 조금 부자연스러울수도 있을 것 같아요🧐

저문은 어떻게 생각하시나요?

당연히 막아야하는 부분이라는 생각이 드는데 제가 인지하지 못했네요.
수정하겠습니다!

yeonkkk · 2023-09-11T11:24:59Z

tomcat/src/main/java/org/apache/coyote/http11/controller/RequestMapping.java

+import java.util.Map;
+import org.apache.coyote.http11.request.HttpRequest;
+
+public class RequestMapping {


사용자 입장에서 /jeomxon/login, /jeomxon/register, /index.htm 등 정의하지 않은 URI에 접근하면 404.html 화면을 보여주는게 보다 자연스러울 것 같아요!
저문은 어떻게 생각하시나요?!

사용자가 어떤 행동을 할지 예측할 수 없으니 좋은 것 같아요
반영했습니다!

yeonkkk · 2023-09-11T11:40:18Z

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

+
+public class HttpResponseHeader {
+
+ private final Map<String, String> headers = new HashMap<>();


Map<String, String> headers 가 사용되지 않고 있네요.
사용하지 않는데 제거하지 않은 이유가 있나요?🤔

(아마 사용하지 않고 있는 이유는 response header들을 response에서 넣어줘서 그런 것 같네요?!)

제거하는 방향으로 변경했습니다!

yeonkkk · 2023-09-11T12:10:11Z

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

+ return makeRedirectResponse();
+ }
+
+ final URL resource = HttpResponse.class.getClassLoader().getResource("static" + path);


"static" 도 상수화해주면 어떨까요?

yeonkkk · 2023-09-11T12:11:42Z

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

+
+ public String get() throws IOException {
+ final String contentType = ContentType.getByPath(path) + CHARSET_UTF_8;
+ if (path.equals("/")) {


"/" 도 상수화하면 의미하는 바가 명확해질 것 같아요!

yeonkkk · 2023-09-11T12:38:12Z

tomcat/src/main/java/org/apache/coyote/http11/controller/LoginController.java

+ if (session == null) {
+ throw new IllegalArgumentException("존재하지 않는 세션입니다.");
+ }


메서드로 분리해서 의미를 명확하게 하면 어떨까요? (depth도 줄어서 좋을 것 같아요 ㅎㅎ)

좋은 것 같아요 분리하겠습니다~

yeonkkk · 2023-09-11T12:44:05Z

tomcat/src/main/java/org/apache/coyote/http11/controller/AbstractController.java

+ return;
+ }
+ response.setHttpStatus(HttpStatus.FOUND);
+ response.sendRedirect("/404.html");


"/404.html" 이 것도 상수처리 해주면 좋을 것 같네용🥹

yeonkkk · 2023-09-11T13:03:16Z

tomcat/src/main/java/org/apache/coyote/http11/controller/LoginController.java

@@ -0,0 +1,62 @@
+package org.apache.coyote.http11.controller;


변경을 요청드리기보다는 의견을 여쭤보고 싶은 부분인데요, 혹시 controller의 위치(패키지)에 대해 고민해보신 부분이 있으실까요?
(그냥 궁금해서 드리는 질문입니다ㅎ)

따로 공부해보지 않았는데 어떤 키워드로 학습하면 좋을까요?
어떤 의존관계를 가지고 설계해야할지 감이 잘 안잡히네요

저는 tomcat 의 coyote, caltalina 의 역할을 고려하셨는지 궁금해서 여쭤봤어요! ㅎㅎ

yeonkkk · 2023-09-11T13:05:37Z

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

+ }
+
+ final URL resource = HttpResponse.class.getClassLoader().getResource("static" + path);
+ this.body = new String(Files.readAllBytes(new File(resource.getFile()).toPath()));


resource.getFile() 부분에서 파일이 없을 경우 NPE가 발생할 수 있을 것 같아요!

그렇네요.
수정했습니다!

yeonkkk · 2023-09-11T13:13:09Z

tomcat/src/main/java/org/apache/coyote/http11/auth/SessionManager.java

 public SessionManager() {
 }


혹시 기본 생성자를 작성해주신 이유가 있을까요?!

이유 없습니다. 삭제했습니다.

sonarcloud · 2023-09-14T05:01:39Z

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

yeonkkk

저문, 조이 입니다!

질문에 답변도 다 해주시고, 리뷰 잘 반영해주신 것도 확인했습니다 ㅎㅎ👍
정말 처음부터 끝까지 코드가 깔끔해서 리뷰하기 너무 너무 편했습니다.
마지막까지 최고네여 ~~

저문의 코드를 리뷰할 수 있어서 너무 영광이었어요 ~~✨
이제 approve & merge 하겠습니다.
고생하셨습니다 ~

yeonkkk · 2023-09-14T04:33:11Z

tomcat/src/main/java/org/apache/coyote/http11/controller/LoginController.java

@@ -0,0 +1,62 @@
+package org.apache.coyote.http11.controller;


저는 tomcat 의 coyote, caltalina 의 역할을 고려하셨는지 궁금해서 여쭤봤어요! ㅎㅎ

yeonkkk · 2023-09-14T04:55:26Z

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

- return Arrays.stream(body.split("&"))
- .map(info -> Arrays.asList(info.split("=")))
+ return Arrays.stream(body.split(USER_INFO_DELIMITER))
+ .map(info -> Arrays.asList(info.split(USER_INFO_SEPARATOR)))
 .collect(Collectors.toMap(infos -> infos.get(0), infos -> infos.get(1), (a, b) -> b));


상수 네이밍에 소요되는 시간이 많다는 것에 공감합니다..🥲
저는 개인적으로 정말 불필요하다고 생각되는 경우를 제외하고 가급적이면 상수화를 하는 편이긴 합니다.
(모든 경우에 하지는 않습니다)

이건 제가 상수화를 하는 목적과 관계가 있는데요, 제가 생각하는 상수화의 목적은 이렇습니다.

수정 상황이 발생하는 경우 고려

여러 번 사용되는 값을 상수화 하면, 이후에 값을 변경해야 하는 상황이 오면 상수 처리한 값만 변경하면 되기 때문에 더 효율적이고, 수정하면서 발생할 수 있는 실수(오타 등)을 줄일 수 있습니다.

다른 개발자가 코드를 볼 때, 코드만으로 이해할 수 있도록 하기 위함 (가독성)

단순 문자, 숫자가 아니라 네이밍을 한 상수로 코드를 작성하면 코드를 처음 보는 사람의 이해도를 높일 수 있다고 생각합니다.
이해도도 높이고 보는 사람의 시간도 절약할 수 있겠네요.
(협업의 관점에서 보면 설명하는 시간도 줄수도 있겠어여 ~)

저문의 말대로 코드를 보는 사람은 대부분 개발자이고 map이 key-value를 대부분 알겠죠🤔 그런데 모두가 그렇다고 확신할 수 있나요?
그리고 (코드 작성자는 당연히 알고 있겠지만) 위 코드를 보고 key-value라고 생각할 수 있으려면 body, info의 구조를 알고 있어야할 것 같아요. 하지만 이름으로 명시해주면 굳이 구조를 확인하지 않아도 key-value 구조라고 생각할수도 있을 것 같아요.

상수화를 하는게 저문 말대로 네이밍하는데 시간도 걸리고 꼭 필요한 작업은 아니기 때문에 선택적인 부분이라고 생각해요!
제 상수화의 기준을 물어봐주시니, 저도 정리하는 시간을 가지게 돼서 좋네요 ㅎㅎ 감사해용.

(여담으로 말하자면 지금 보니 KEY_INDEX, VALUE_INDEX 라고 지은건 네이밍이 구리네요 ..^^..)

yeonkkk · 2023-09-14T04:55:50Z

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

+ private static final String QUERY_STRING_DELIMITER = "&";
+ private static final String QUERY_STRING_SEPARATOR = "=";


오호 하나 알게 되었네요 감사합니다 ㅎㅎ

jeomxon added 9 commits September 8, 2023 21:32

refactor: HttpRequest관련 클래스 상수화, 변수명 수정

109ed43

refactor: HttpResponse클래스 역할 분리 및 Controller 적용

bc52458

refactor: 네이밍 수정

4f9e7d5

refactor: sendRedirect메서드로 역할 통합

0fc7f2f

refactor: 상수화

a1a1fd5

refactor: 없는 세션에 대한 예외처리 추가

9db9345

refactor: Executors로 Thread Pool 적용 및 SessionManager 동시성 컬렉션 추가

da79b67

refactor: get, post이외의 요청이 왔을 경우 404로 redirect

6b294dc

refactor: 패키지 구조 변경 및 상수 분리

d02ff61

jeomxon requested a review from yeonkkk September 10, 2023 17:01

jeomxon assigned jeomxon and unassigned jeomxon Sep 10, 2023

jeomxon added 2 commits September 11, 2023 02:04

refactor: 분기 처리에 대한 return 추가

7fe69af

refactor: HttpResponse의 HttpVersion을 통일된 변수를 사용하도록 변경

5f5bcb1

yeonkkk requested changes Sep 11, 2023

View reviewed changes

jeomxon added 4 commits September 13, 2023 11:57

refactor: 상수화

0ccfc56

refactor: 로그인 완료 시 회원가입 페이지로 이동하지 못하도록 수정

7417df7

refactor: 지정되지 않은 요청이 왔을 때 404페이지로 이동시키도록 변경

f4e1821

refactor: 사용하지 않는 클래스 제거

363ba00

yeonkkk approved these changes Sep 14, 2023

View reviewed changes

yeonkkk merged commit 11d8cf2 into woowacourse:jeomxon Sep 14, 2023
2 checks passed

jeomxon deleted the step2 branch September 14, 2023 05:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 3, 4단계] 저문(유정훈) 미션 제출합니다. #459

[톰캣 구현하기 - 3, 4단계] 저문(유정훈) 미션 제출합니다. #459

jeomxon commented Sep 10, 2023

yeonkkk left a comment

yeonkkk Sep 11, 2023

jeomxon Sep 13, 2023

yeonkkk Sep 11, 2023

jeomxon Sep 14, 2023

yeonkkk Sep 11, 2023

jeomxon Sep 14, 2023

yeonkkk Sep 11, 2023

yeonkkk Sep 11, 2023

yeonkkk Sep 11, 2023

jeomxon Sep 13, 2023

yeonkkk Sep 11, 2023

yeonkkk Sep 11, 2023

jeomxon Sep 13, 2023

yeonkkk Sep 14, 2023

yeonkkk Sep 11, 2023

jeomxon Sep 13, 2023

yeonkkk Sep 11, 2023

jeomxon Sep 13, 2023

sonarcloud bot commented Sep 14, 2023

yeonkkk left a comment

yeonkkk Sep 14, 2023

yeonkkk Sep 14, 2023

yeonkkk Sep 14, 2023


		public class HttpResponseHeader {

		private final Map<String, String> headers = new HashMap<>();

		@@ -0,0 +1,62 @@
		package org.apache.coyote.http11.controller;

		private static final String QUERY_STRING_DELIMITER = "&";
		private static final String QUERY_STRING_SEPARATOR = "=";

[톰캣 구현하기 - 3, 4단계] 저문(유정훈) 미션 제출합니다. #459

[톰캣 구현하기 - 3, 4단계] 저문(유정훈) 미션 제출합니다. #459

Conversation

jeomxon commented Sep 10, 2023

yeonkkk 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

sonarcloud bot commented Sep 14, 2023

yeonkkk 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