[3단계 - Tomcat 구현하기] 새양(양경호) 미션 제출합니다. #609

geoje · 2024-09-11T04:27:04Z

냥인🐈🧍‍♀️ 안녕하세요!

이번 미션은 리팩토링🔨 이었어요~
살짝 까먹고 살았던 우테코 1, 2 레벨을 생각하며 미션을 했답니다!

그럼 잘 부탁 드립니다! 🤗

진행 사항

분리되어있던 HttpRequest 의 필드의 원시값 포장
불필요한 로깅 제거
프리미티브 타입 값 " " ": " "; " 상수로 분리
메서드 라인을 10줄 이하로 줄이기 위한 함수 분리 및 스트림 사용

리뷰 반영 사항

NullPointException 이 발생할 수 있는 변환 포인트 제거
요청과 응답에서 동시에 사용되는 클래스 common 패키지로 이동
Map<String, String> 으로 관리하던 자료구조 일급 컬렉션으로 분리

추가 사항

GET / 과 GET/ / HTTP/1.1 과 같이 형식에 맞지 않는 requestLine 에 대해 400 으로 응답
성공 케이스 뿐만 아닌 여러 실패 케이스에 대한 테스트 추가 (위와 같은 등등)
컨트롤러, 세션 관리, Http11Processor 등 직접 요청을 보내 응답 테스트 추가

cutehumanS2

새양 ~ 안녕하세요. 냥인입니다. ◠‿◠
바쁘신 와중에 미션 구현하시느라 정말 수고 많으셨어요.

리뷰 열심히 남기다가 뒤늦게 애플리케이션 & 테스트를 실행해 봤는데,
테스트가 다 실패하고, 애플리케이션도 잘 작동하지 않고 있어요 ~
(Status에 문제가 있는 것 같아요.)

확인 부탁드려도 될까요?
한 번 확인해 보시고, 문제가 없다면! 다시 리뷰 요청 주세요.
마저 리뷰 남겨드리겠습니다. ㅎ ㅎ

cutehumanS2 · 2024-09-11T10:40:53Z

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

+import org.apache.coyote.http11.response.HttpResponse;

 public abstract class AbstractController implements Controller {



10~15 라인에 해당하는 내용입니다.
Method Enum 클래스를 만드셨으니, 이를 활용해 볼 수 있겠군요 ~ 😸

아래 코드는 예시입니다.

if (request.method() == Method.GET) { doGet(request, responseBuilder); } if (request.method() == Method.POST) { doPost(request, responseBuilder); }

request.method()를 직접 꺼내지 않고, request 안에 Method가 일치하는지 묻는 메서드(ex. request.matchesMethod(Method.GET))를 만들 수도 있을 것 같아요~
새양이 원하는 방식으로 바꿔보시죠 !

네! 해당 부분을 수정하였습니다.
코멘트에 버그가난다는 것을 보고 후다닥 달려와 수정했을 때 이 부분이 문제더라구요!
Enum 과 String 을 비교하려해서 항상 false 가 된다는 경고 문구를 제거 무시해버린 사태였습니다 ㅠㅠ

수정하여 더욱 좋게 바꾸었습니다~ isMethod 메서드!

cutehumanS2 · 2024-09-11T10:42:45Z

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

+    public String getValue() {
+        return value;
+    }


쓰이지 않는 메서드군요.
지워도 될 것 같아요. ㅎ ㅎ

제거하였습니다!!

cutehumanS2 · 2024-09-11T10:44:44Z

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

+    public static Method from(String value) {
+        return Arrays.stream(Method.values())
+                .filter(method -> method.value.equals(value))
+                .findAny()
+                .orElse(null);


인자로 받은 value를 찾지 못했을 때, 예외가 아닌 null을 리턴한 이유가 궁금해요 ~ ✦‿✦

다시 보니 문제가 있어 보입니다! 해당 부분은 예외를 던지도록 변경하였습니다!
.orElseThrow(() -> new IllegalArgumentException("HTTP Method not found: " + value))

cutehumanS2 · 2024-09-11T10:48:49Z

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

@@ -0,0 +1,27 @@
+package org.apache.coyote.http11.request;


ProtocolVersion은 response에서도 쓰이는데, request 패키지에 두셨군요 !
위치 조정이 필요해 보여요.

네! request 와 response 모두가 사용하는 상위단으로 이동하였습니다!

cutehumanS2 · 2024-09-11T10:49:27Z

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

+    public String getValue() {
+        return value;
+    }


여기도 쓰이지 않는 메서드는 지우면 좋을 것 같아요 ~ 😸

앗, 이 부분은 리팩토링을 하면서 사용하게 되었습니다!
이전 request 쪽 Method 는 단순히 읽는 곳에서만 사용되어서 필요가 없었는데
현재 ProtocolVersion 쪽에서는 String 값을 HTTP Response Message 에 작성해야 하므로 사용하게 되었습니다!

cutehumanS2 · 2024-09-11T11:34:09Z

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

        String path,
        Map<String, String> parameters,
        Map<String, String> headers,
        Map<String, String> cookies,


parameters, headers, cookies 일급컬렉션을 만드는 건 어떠신가요?
셋의 타입이 똑같아 HttpRequest 생성자에 주입할 때 순서가 바뀌어도 컴파일 타임에 에러를 감지할 수 없을 수도 있겠네요. 😅

// 맞게 주입한 경우 return new HttpRequest(method, path, parameters, headers, cookies, protocolVersion, body); // 잘못 주입한 경우 return new HttpRequest(method, path, headers, parameters, cookies, protocolVersion, body);

새양은 어떻게 생각하시나요 ?!

저도 적용을 심히 고민을 했다만 고민만 하고 하지 않았네요 ㅋㅋ
이번 기회에 확실히 적용하였습니다!
geoje@8631162

cutehumanS2 · 2024-09-11T11:35:43Z

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

+                .collect(Collectors.toMap(entry -> entry[0], entry -> entry[1]));
    }

    private static Map<String, String> extractCookies(String cookieMessage) {


이 메서드는 스트림으로 변경하시지 않았네요. 이유가 궁금합니다 ~ ㅎ ㅎ
그리고 새양이 보기에 스트림과 for문 중 어떤 게 더 가독성이 좋다고 생각하시나요?

이게 indent 1 이라는 요구사항을 만족하려다 보니 스트림을 사용하게 되었네요.

이 메서드에서의 경우 endHeaderIndex 를 구할 때 IntStream 을 한 번 돌리고 다시 헤더 추출을 한다는 것이 불필요해 보이긴 합니다.

하지만 스트림이 가독성이 안좋을 것은 아닌 것 같아요!
물론 IDEA를 사용해야 하지만 아래와 같이 변환되는 부분의 타입이 Stream<String[]> 과 같이 보이니 오히려 더 좋더라구요!

cutehumanS2 · 2024-09-11T11:37:26Z

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

-                }
-            }
+    private static String extractBody(List<String> lines) {
+        if (lines.size() > 1 && lines.get(lines.size() - 2).isEmpty()) {


의미있는 메서드로 만들면 가독성이 좋아질 것 같아요. 😸

네! 해당 부분을 existBodySeparatorEmptyLine(lines) 로 변경하여 가독성을 높여주었어요~

cutehumanS2 · 2024-09-11T12:08:07Z

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

+import org.apache.coyote.http11.response.HttpResponse;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;



17 라인에 대한 내용입니다.

보통 상수명은 upper snake case로 쓰곤 하잖아요.
구글 자바 스타일 가이드 문서에 따르면, 상수를 의도하고 선언했다 하더라도, 변경될 가능성이 있다면 엄밀하게는 상수가 아니라고 하네요. 따라서 upper snake case로 쓰지 않는다고 합니다.
같은 의미에서 바로 위 log도 static final이지만 upper case로 쓰지 않았다고 볼 수 있어요. ㅎ ㅎ
더 상세한 내용은 링크된 문서에서 확인해 보시죠 ~

반영은 새양 자유입니다. 😄

오!!! 저도 이 부분에 대해서 견해를 개인적으로 가지고 있었는데 보통 다 그렇게 하는군요.
완벽히 동일한 의견입니다 ㅎㅎ
레퍼런스를 알려주셔서 감사합니다!

requestMapping 으로 변경하였습니다~

cutehumanS2 · 2024-09-11T12:19:29Z

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

+        if (SessionManager.getInstance().getSession(request) != null) {
+            processSessionLogin(responseBuilder);


LMS의 "2단계 - 로그인 구현하기" 중 2. POST 방식으로 회원가입에 아래와 같은 내용이 있습니다.

로그인 페이지도 버튼을 눌렀을 때 GET 방식에서 POST 방식으로 전송하도록 변경하자.

doPost()가 있어야 할 것 같은 느낌이 들지 않나요?! ㅎ ㅎ

이럴수가 LMS 를 제대로 읽지 않는 저의 실수군요!
너무 나도 바꾸고 싶었던 부분이었는데 반영하였습니다!!

cutehumanS2

그리고 IOStreamTest와 GreetingControllerTest의 메서드 일부도 실패하는데
확인 부탁드려요. 😿

cutehumanS2 · 2024-09-11T12:40:09Z

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

+    private boolean isValidBody(Map<String, String> body) {
+        return !body.containsKey("account") ||
+                !body.containsKey("password") ||
+                !body.containsKey("email");
+    }


메서드명 변경합시다!
검증 내용을 보니 isInvalidBody()여야 할 것 같군요.

엇! 이전 코멘트를 반영하다가 저도 확인해보니 이상함을 느끼고 후다닥 바꾸었습니다.
예리하시군요... 이것까지 들키다니 😶

cutehumanS2 · 2024-09-11T12:44:03Z

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

+                !body.containsKey("email");
+    }
+
+    private boolean existAccount(String account) {


Suggested change

private boolean existAccount(String account) {

private boolean existsAccount(String account) {

저엉말 사소한 내용이지만 보통 이렇게 쓰는 것 같아요...ㅎ ㅎ

이외에도 몇몇 exist 가 있어서 전부 exists 로 변경하였습니다!
영어 강의는 언제나 환영입니다!!

cutehumanS2 · 2024-09-11T12:45:40Z

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

+    }

-        InMemoryUserRepository.save(new User(account, body.get("password"), body.get("email")));
+    private void postProcess(HttpResponse.Builder responseBuilder, Map<String, String> body) {


메서드명을 조오금 더 잘 지어볼 수 있을 것 같아요!
새양의 센스를 발휘해 주세요. 😸

창작은 제가 가장 힘들어 하는 부분이라 오랜 시간 끝에 나온 메서드명입니다!
saveUserAndRedirectHome

…ass collection

cutehumanS2

새양 반영하시느라 고생 많으셨습니다. ◠‿◠
이번 단계는 여기서 머지할게요.
4단계 얼른 하러 갑시다. ㅎ ㅎ

geoje added 12 commits September 10, 2024 15:29

refactor: use try with resources statement to prevent memory leak

0a73d91

refactor: rearrange method order on request

7ca7dcf

refactor: remove unused method while adding method of major header

f323126

refactor: use checking parameters exists with stream

92bceb3

refactor: remove logging on request with session

c657b7f

refactor: move responsibility to create session to manager

2a81262

refactor: move responsibility to create session to manager

db5648a

refactor: simplify method lines and extract methods

b049dbf

refactor: move and add enum to encapsulate primitive value

2300bef

refactor: add method enum for request encapsulation fields

f0e2106

refactor: extract constant on request and message

9890bd4

refactor: use stream to reduce indent

38ae266

geoje self-assigned this Sep 11, 2024

cutehumanS2 requested changes Sep 11, 2024

View reviewed changes

geoje added 15 commits September 12, 2024 01:15

fix: use value of enum to fix message

a9feb02

test: fix failed test about cache

4414499

refactor: remove excluding path

5cabc8d

fix: complete cache busting of static resources

ddbd91c

fix: add etag filter

f1b6c55

refactor: remove unused method

cf327d2

refactor: move package to common position

6d68afb

refactor: optimize new line

c6513c9

refactor: change new line style

02da74a

refactor: extract all constant at http response

190f098

refactor: extract filtering method on http request

6a468c1

refactor: change map located on http request and response to first cl…

8631162

…ass collection

refactor: extract to improve readability

004010c

refactor: change variable name to camelcase

ebb02c0

feat: login with post method

e5d5d3c

geoje added 5 commits September 12, 2024 16:20

refactor: fix typo exist to exists

adbb49f

refactor: rename method to readable

b57fad1

refactor: remove unused method on http request

b745832

feat: set default protocol version to http1.1

fdc5194

test: write for controller

8fe93a4

geoje force-pushed the step2 branch from 85c885c to 8fe93a4 Compare September 13, 2024 01:30

geoje added 6 commits September 13, 2024 10:40

test: view main page

65fdab9

fix: remove space on delimiter of semicolon on response

f9de082

test: use containsSequence correctly

c19b564

test: reflect for using POST method on login

a9d8a29

test: find and get session

100bcaa

test: response 400 for invalid http request

23963bb

cutehumanS2 approved these changes Sep 13, 2024

View reviewed changes

cutehumanS2 merged commit 3af18e0 into woowacourse:geoje Sep 13, 2024

		import org.apache.coyote.http11.response.HttpResponse;

		public abstract class AbstractController implements Controller {

		if (SessionManager.getInstance().getSession(request) != null) {
		processSessionLogin(responseBuilder);

	private boolean existAccount(String account) {
	private boolean existsAccount(String account) {

[3단계 - Tomcat 구현하기] 새양(양경호) 미션 제출합니다. #609

[3단계 - Tomcat 구현하기] 새양(양경호) 미션 제출합니다. #609

Uh oh!

Conversation

geoje commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

진행 사항

리뷰 반영 사항

추가 사항

Uh oh!

cutehumanS2 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!

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!

cutehumanS2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoje Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

cutehumanS2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

geoje commented Sep 11, 2024 •

edited

Loading

geoje Sep 12, 2024 •

edited

Loading