[톰캣 구현하기 - 1, 2단계] 허브(방대의) 미션 제출합니다. #302

greeng00se · 2023-09-02T12:10:34Z

안녕하세요 디노 🦖
1, 2단계 리뷰 요청드립니다!

3단계를 보니 리팩터링이어서 1, 2단계는 기능 구현에 급급했습니다 😢
그러다보니 Response를 생성해주는 HttpResponseGenerator 클래스가 엄청 비대해졌어요.

전체적인 흐름은 아래와 같습니다.

사용자 요청
process 메서드 에서 requestLine, 헤더, 바디 파싱
handleRequest 메서드에서 login, register, 그 외 라우팅ResponseEntity 반환
- (네이밍이 좋지 않지만 HttpResponse로 리팩터링 전 임시로 사용할 VO 느낌이라고 생각해주시면 될 것 같습니다!)
HttpResponseGenerator가 ResponseEntity 받아서 실제로 Http 응답 생성 -> 응답

jjongwa

안녕하세요 허브~🌿 다이노입니다🦖

커밋 하나 하나 톺아보며 허브의 발자취를 따라가 봤는데요..
가장 먼저 드는 생각은 요구사항을 도출해나가는 과정이 정말 깔꼼하다- 였습니다.
심지어 구현도 빨라. 이거 어케 토요일에 다함?
리뷰를 하는 동안 제 코드와 비교를 안할 수가 없더라구요..
덕분에 전 싱싱미역 상태가 되어버렸습니다. 책임지세요.

리뷰 내용은 많이 없구 보면서 제가 궁금했던 부분 위주로 남겨 봤습니다..!
확인해 보시고 허브의 생각 코멘트로 남겨 주신 후 리뷰 요청 보내주시면 바로 approve 하지 않을까 싶어요 .!
궁금한거 더 생기면 직접 찾아가겠습니다 후후 그럼 이만

jjongwa · 2023-09-04T06:42:31Z

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

+    private RequestHeader readHeader(final BufferedReader bufferedReader) throws IOException {
+        final StringBuilder stringBuilder = new StringBuilder();
+        for (String line = bufferedReader.readLine(); !"".equals(line); line = bufferedReader.readLine()) {
+            stringBuilder.append(line).append(CRLF);
+        }
+        return RequestHeader.from(stringBuilder.toString());
+    }


해당 로직을 readHeader라는 메서드로 분리 후 RequestHeader의 정팩매를 호출하는 이유가 궁금합니다!
저는 RequestHeader의 from 메서드에 해당 로직을 포함 시키는 것이 괜찮다고 생각했거든요.!

혹시 추후에 HttpRequest라는 클래스를 생성해 책임을 분리하고자 하는 빌드업인가요.? 아래의 readBody도 마찬가지구요!

step3 반영하면서 HttpRequest를 사용할 예정이긴 합니다!
해당 로직이 from 메서드 내부로 들어간다면 from 메서드 내부에서 BufferedReader를 사용하기 때문에 readHeader 메서드로 분리했습니다!

결합도 측면에서 바라볼 수 있는 문제였군요..!
아까 얘기 나눠본대로 request를 읽어서 각각의 Line, Header, body로 파싱하는 역할의 클래스를 두면 좋다는 생각이 들었습니다!
덕분에 하나 더 배워갑니다ㅎㅎ

jjongwa · 2023-09-04T06:54:10Z

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

+            final Session session = sessionManager.findSession(httpCookie.get("JSESSIONID"));
+            if (session != null) {
+                return new ResponseEntity(HttpStatus.FOUND, INDEX_PAGE);
+            }
+            return new ResponseEntity(HttpStatus.OK, LOGIN_PAGE);
+        }
+        final String account = requestBody.get("account");
+        final String password = requestBody.get("password");
+        return InMemoryUserRepository.findByAccount(account)
+                .filter(user -> user.checkPassword(password))


(뭔가 이후에 클래스 분리 후에 진행하려고 놔둔 느낌이 있지만..)"JESSIONID", "account", "password" 같은 친구들도 상수화 시켜주면 좋을 것 같아요.!

현재 구조에서 해당 부분을 상수화해주면 더욱 코드가 읽기 좋을 것 같습니다 👍

jjongwa · 2023-09-04T07:01:00Z

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

+    public QueryString() {
+        this(Map.of());
+    }
+
+    public QueryString(final Map<String, String> items) {
+        this.items.putAll(items);
+    }


클래스 내부에서만 쓰이는 것 같아서 private으로 해줘도 좋을 것 같아요!

좋습니다 👍 👍 꼼꼼히 봐주신게 느껴집니다.. 🙇

jjongwa · 2023-09-04T07:02:08Z

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

+    }
+
+    public String parseUriWithOutQueryString() {
+        int queryStringIndex = uri.indexOf(QUERY_STRING_BEGIN);


final 실종사건 후후

jjongwa · 2023-09-04T07:03:26Z

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

+    private String generateResponse(final ResponseEntity responseEntity, final String responseBody) {
+        return String.join(
+                CRLF,
+                generateHttpStatusLine(responseEntity.getHttpStatus()),
+                generateContentTypeLine(responseEntity.getUri()),
+                generateContentLengthLine(responseBody),
+                "",
+                responseBody
+        );
+    }


와우 아주 깔꼼하네요..

칭찬 감사합니다 👍

jjongwa · 2023-09-04T07:10:59Z

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

+    }
+
+    private String generateContentTypeLine(final String url) {
+        if (url.endsWith(".css")) {


요런 파일 확장자 친구들도 하나의 enum 클래스로 모아놓는건 어떠신가요?

요것도 투머치라고 생각하면 그냥 무시해 주십쇼

너무 좋은 것 같아서 복사 붙여넣기 하겠습니다 👍

jjongwa · 2023-09-04T07:17:40Z

tomcat/src/main/resources/static/409.html

요친구는 허브가 새로 추가한 것 같은데,
이미 등록된 유저일때 409 에러를 반환하기 위한 역할인가요?

맞습니다! 사용자가 중복되는 경우 409 페이지를 보여주고 싶어서 추가했습니다!

아하 이렇게 또 다른 페이지를 띄워줄 생각은 못했는데
저도 참고하겠습니다ㅎㅎ

jjongwa · 2023-09-04T07:17:55Z

tomcat/src/main/resources/static/favicon.ico

헉 그냥 빈 파일 넣어도 되는거군요

에러만 안보이도록 몰래....

jjongwa · 2023-09-04T07:18:37Z

tomcat/src/test/java/org/apache/coyote/http11/Http11ProcessorTest.java

테스트 꼼꼼 레전드

호호... 감사합니다 👍

jjongwa · 2023-09-04T07:23:54Z

tomcat/src/test/java/org/apache/coyote/http11/common/SessionTest.java

+    @Test
+    void 값을_설정한다() {
+        // given
+        final Session session = new Session("UUID");
+
+        // when
+        session.setAttribute("hello", "world");
+
+        // then
+        assertThat(session.getAttribute("hello")).isEqualTo("world");
+    }
+
+    @Test
+    void 값을_가져온다() {
+        // given
+        final Session session = new Session("UUID");
+        session.setAttribute("hello", "world");
+
+        // expect
+        assertThat(session.getAttribute("hello")).isEqualTo("world");
+    }


요건 의미상 달라서 각각 테스트 메서드를 작성하신 건가요?
저같으면 그냥 하나로 퉁칠 것 같은데 뭔가 의미가 있기 때문에 작성하지 않았을까 생각이 드네요.
이렇게 같은 코드의 메서드를 작성했을 때 허브가 생각하는 장점이 무엇이 있는지 궁금합니다..!

아하 각각 테스트 하는 메서드가 달라서 분리해보았습니다!
현재는 저장하고 조회하는 구조가 간단하지만, 복잡한 경우에는 해당 방식을 이용해서 각각 검증하는 것이 조금 더 안전하게(신뢰성 있는 코드로) 느껴졌습니다.
서로 의존하는 느낌이 들지만 해당 클래스 내의 메서드가 때문에 크게 문제되지 않다고 생각했습니다!

오홍 위에는 set에 대한 테스트, 아래는 get에 대한 테스트 라는 의미를 부여해 놓고,
로직이 복잡해졌을 때 조금 더 꼼꼼하게 검증하도록 리팩토링을 진행할 수도 있곘군요.!
이해해 버렸습니다. 대박

sonarcloud · 2023-09-04T08:37:46Z

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

greeng00se · 2023-09-04T08:41:44Z

안녕하세요 🦖
커밋 하나하나 꼼꼼하게 봐주셨다니 너무 감동입니다...
칭찬이 너무 많아서 부끄럽습니다..
리뷰도 필요한 부분이 많아서 거의 다 반영했습니다!
반영 못한 부분은 리팩터링 할 때 더 열심히 반영해보도록 할게요!
🙇

jjongwa

빠른 리뷰 반영까지.. 멋집니다 허브
허브가 어떤 관점으로 코드를 작성해 나가는지 엿볼 수 있어서 아주 만족하며 리뷰 남기고 있습니다ㅎㅎ
반영해주신 코드 확인 해서 이번 스텝은 approve & merge 할께요ㅎㅎ
다음 스텝에서도 잘 부탁드립니다~

greeng00se added 23 commits September 2, 2023 00:41

test: step1을 위한 학습 테스트 추가

c56d439

feat: 정적 리소스에 대한 응답 기능 추가

46c12fd

feat: css 지원 기능 추가

1610ec2

feat: Query String 파싱 기능 구현

fe3f46d

docs: 요구사항 추가

926358e

refactor: QueryString을 위한 Util 클래스 제거 및 QueryString 일급 컬렉션 추가

1d3421d

feat: 빈 favicon 추가

1fc1627

feat: Http 요청의 첫 라인을 의미하는 RequestLine, HttpMethod enum 추가

2f33003

feat: RequestLine 클래스 queryString 파싱하는 기능 추가

df5ae6b

refactor: process 메서드 flow 개선

f4db484

refactor: 기존의 util 클래스 제거하고 HttpResponseGenerator 추가

ac2c3b0

feat: RequestHeader 클래스 추가

218040f

feat: RequestBody 클래스 추가

0f6aa13

feat: Found Redirect 기능 추가

43fdcbe

feat: 회원가입 기능 추가

8565ec1

refactor: 패키지 분리

f1397e1

feat: 쿠키값을 가지고 있는 httpCookie 추가

fbdd91e

feat: 쿠키 세팅하는 기능 추가

b20d51c

feat: Session, SessionManager 추가

92046ec

feat: 로그인시 세션에 사용자 저장 기능 추가

f73d0b7

refactor: 불필요한 static 메서드 제거

5f00860

fix: 자동 정렬된 index.html 수정

c21ccf1

fix: 코드 스멜 제거

fc0d914

greeng00se requested a review from jjongwa September 2, 2023 13:09

greeng00se self-assigned this Sep 2, 2023

jjongwa requested changes Sep 4, 2023

View reviewed changes

greeng00se added 4 commits September 4, 2023 17:17

refactor: JSESSIONID, account, passoword 상수화

7d81b97

refactor: final 빠진 부분 추가

93cf49b

refactor: 외부에서 사용하지 않는 생성자 private으로 변경

9191f51

feat: HttpExtensionType Enum 추가

d3fa943

jjongwa approved these changes Sep 4, 2023

View reviewed changes

jjongwa merged commit 187f9c2 into woowacourse:greeng00se Sep 4, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 1, 2단계] 허브(방대의) 미션 제출합니다. #302

[톰캣 구현하기 - 1, 2단계] 허브(방대의) 미션 제출합니다. #302

greeng00se commented Sep 2, 2023

jjongwa left a comment

jjongwa Sep 4, 2023

greeng00se Sep 4, 2023

jjongwa Sep 4, 2023

jjongwa Sep 4, 2023

greeng00se Sep 4, 2023

jjongwa Sep 4, 2023

greeng00se Sep 4, 2023

jjongwa Sep 4, 2023

greeng00se Sep 4, 2023

jjongwa Sep 4, 2023

greeng00se Sep 4, 2023

jjongwa Sep 4, 2023

greeng00se Sep 4, 2023

jjongwa Sep 4, 2023

greeng00se Sep 4, 2023

jjongwa Sep 4, 2023

jjongwa Sep 4, 2023

greeng00se Sep 4, 2023

jjongwa Sep 4, 2023

greeng00se Sep 4, 2023

jjongwa Sep 4, 2023

greeng00se Sep 4, 2023

jjongwa Sep 4, 2023

sonarcloud bot commented Sep 4, 2023

greeng00se commented Sep 4, 2023

jjongwa left a comment

[톰캣 구현하기 - 1, 2단계] 허브(방대의) 미션 제출합니다. #302

[톰캣 구현하기 - 1, 2단계] 허브(방대의) 미션 제출합니다. #302

Conversation

greeng00se commented Sep 2, 2023

jjongwa 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

sonarcloud bot commented Sep 4, 2023

greeng00se commented Sep 4, 2023

jjongwa left a comment

Choose a reason for hiding this comment