[톰캣 구현하기 - 3, 4단계] 민트(유재민) 미션 제출합니다. #462

yujamint · 2023-09-10T17:23:53Z

키아라 안녕하세요. 민트입니다.
요구사항에 따라 리팩토링 진행하면서 1,2단계 보다는 훨씬 나은 코드가 된 것 같네요.
그럼에도 보기 힘들 수는 있을 것 같습니다.. 리뷰 잘 부탁드리겠습니다.

";" 뒤에 옵션이 붙는 것을 고려하여 이를 처리하는 역할을 담당한다.

Response Header의 순서로 인해 실패하는 테스트를 위해 조정

kiarakim

수고하셨어요 민트🌱
전체적으로 클래스를 잘 분리해주셨지만 아쉬운 점은 각 클래스의 책임에 맞게 충분히 역할을 분배하지 못한 것 같아요.
고쳤으면 하는 부분들을 몇개 짚었는데 다 변경하실 필욘 없구 상황에 맞게 그리고 민트의 기준에 맞게 변경하시면 될 것 같아요. 그리구... LMS 요구사항을 한 번 더 읽어보는 것을 추천드립니덩😉

kiarakim · 2023-09-11T08:55:39Z

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

+    private HttpRequest readHttpRequest(BufferedReader bufferedReader) throws IOException {
+        List<String> lines = readRequesteHeaders(bufferedReader);
+        RequestLine requestLine = RequestLine.from(lines.get(0));
+        HttpHeaders httpHeaders = HttpHeaders.from(lines.subList(1, lines.size()));
+        String requestBody = readRequestBody(bufferedReader, httpHeaders.contentLength());
+        return new HttpRequest(requestLine, httpHeaders, requestBody);
    }


이 메서드부터 시작해서 요청을 읽고 요청라인, 헤더, 바디를 읽는 부분은 HttpRequest가 갖고있는게 어떨까요?
Http11Processor의 책임일지 HttpRequest의 책임에 더 가까울지 생각해보면 좋을 것 같아요.

리뷰를 보고 생각하니 확실히 책임이 분리되는 것도 괜찮을 거 같더라고요.
대신 BufferedReader를 통해 직접 읽어들이는 것보다 '읽기'만의 책임을 갖는 객체가 따로 있는 게 좋을 것 같다고 생각했습니다.
HttpRequestReader를 구현하는 방향으로 리팩토링 했으니 혹시나 별로라면 말씀해 주세요~

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

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

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

yujamint · 2023-09-12T11:48:55Z

안녕하세요 키아라 리뷰 반영 완료했습니다.
요구사항을 제대로 안 읽어서 놓친 부분이 꽤 많았더라고요.. 특히 Cookie 클래스는 최종 리팩토링에서도 추가하지 않았습니다. 당장은 Cookie 클래스를 구현함으로써 어떤 부분의 코드를 개선할 수 있을지 감이 잘 안 오더라고요. 시간에 쫓기는 삶을 살고 있는지라 일단은 리팩토링 가능한 부분 최대한 해봤습니다.
의도하신대로 수정되지 않은 부분이 있다면, 편하게 말씀해 주셔도 좋습니다~
이번 미션 고생 많으셨어요 MVC 미션도 파이팅입니다

kiarakim

수고하셨어요 민트ㅎㅎ🌱
오늘부터 MVC미션이라 톰캣 리뷰 반영하면서 마음이 조급했을텐데 잘 반영해주셨어요!
이번엔 상수화나 메서드 분리같은 것들 위주로 짚어드렸어요.
그치만 저희는 이제 MVC 미션에 집중해야하니 만약 반영한다면 호로록 간단히만 반영하고 새 미션 시작합시당~

kiarakim · 2023-09-12T13:48:34Z

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

            bufferedOutputStream.flush();
-        } catch (IOException | UncheckedServletException e) {
+        } catch (Exception e) {


왜 Exception으로 변경하셨을까용?

Controller.service()에서 발생할 수 있는 에러로 여러 종류가 있을 거라고 생각했습니다.
발생할 수 있는 에러를 커스텀 예외로 추상화해서 catch 했다면 좋았을 것 같지만, 당장은 모든 예외를 잡을 수 있는 Exception으로 변경해서 개발을 빠르게 하고자 했네요 ㅋㅋㅋ..

kiarakim · 2023-09-12T13:50:06Z

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

+            HttpRequest httpRequest = requestReader.readHttpRequest(bufferedReader);
+            HttpResponse httpResponse = new HttpResponse(httpRequest.httpVersion());

-            String responseBody = readFile(requestHandler.getResponseFilePath());
+            Controller controller = requestMapping.getController(httpRequest);
+            controller.service(httpRequest, httpResponse);

-            List<String> responseHeaders = new ArrayList<>();
-            responseHeaders.add("HTTP/1.1 " + requestHandler.getHttpStatus() + " ");
-            responseHeaders.add(contentTypeHeader);
-            responseHeaders.add("Content-Length: " + responseBody.getBytes().length + " ");
-            for (Entry<String, String> headerEntry : requestHandler.getHeaders().entrySet()) {
-                responseHeaders.add(headerEntry.getKey() + ": " + headerEntry.getValue());
-            }
-            String responseHeader = String.join("\r\n", responseHeaders);
+            httpResponse.setBody(FileReader.readFile(httpResponse.getResponseFileName()));
+            httpResponse.addHeader(CONTENT_LENGTH.getValue(), String.valueOf(httpResponse.getBody().getBytes().length));
+            httpResponse.addHeader(CONTENT_TYPE.getValue(), httpRequest.getContentTypeByAcceptHeader());


확실히 훨씬 간결해졌네요!👍

kiarakim · 2023-09-12T13:51:28Z

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

+import java.util.ArrayList;
+import java.util.List;
+
+public class HttpRequestReader {


클래스 분리 굿~🍰🌭🍕🍩

kiarakim · 2023-09-12T13:53:46Z

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

+    public static String readFile(String fileName) {
+        String filePath = FileReader.class.getClassLoader().getResource(STATIC_DIRECTORY_PATH + fileName).getPath();
+        try (Stream<String> lines = Files.lines(Paths.get(filePath))) {
+            return lines.collect(Collectors.joining("\n", "", "\n"));


이게 어떤 의미인지 설명해주실 수 있나요?

결론부터 말씀드리자면 join한 문자열의 끝에 개행을 추가해주고 싶어서 joining 메서드를 활용했습니다.

List의 원소들마다 한 줄씩 띄어주기 위해 "\n"으로 join하고 있으며, join이 끝난 문자열에 suffix로 "\n"을 한 번 더 붙여주고 있습니다.
파라미터의 중간에 들어가는 빈 문자열("")은 prefix 자리입니다. 현재 상황에서 prefix는 필요 없지만 suffix를 넣어주기 위해 의미 없는 값을 넣어줬습니다.

kiarakim · 2023-09-12T14:03:25Z

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

+
+    @Override
+    protected void doPost(HttpRequest request, HttpResponse response) throws Exception {
+        String[] splitBody = request.getBody().split("&");


RegisterController에선 상수화 해줬으면서..!

kiarakim · 2023-09-12T14:06:03Z

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

+        Optional<String> account = getValueOf("account", splitRequestBody);
+        Optional<String> email = getValueOf("email", splitRequestBody);
+        Optional<String> password = getValueOf("password", splitRequestBody);
+
+        if (account.isEmpty() || email.isEmpty() || password.isEmpty()) {


이 부분을 따로 메소드로 빼서 isInvalidateRegister(String[])으로 확인하고자 하는 걸 명시하면 좋을 것 같아요

좋은 생각인 것 같습니다. 근데 account, email, password 변수를 if문 분기 이후에도 사용하고 있습니다. 그래서 저는 그냥 이대로 남겨 놓는 것도 괜찮을 것 같네요..!
splitRequestBody에서 한 번 더 값을 꺼내오는 게 좀 지저분할 것 같아서요..

kiarakim · 2023-09-12T14:08:12Z

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

+        int queryStringIndex = path.indexOf("?");
+        if (queryStringIndex == -1) {


"?"??
-1??
상수화 부탁해용

sonarcloud · 2023-09-12T16:24:04Z

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

yujamint added 16 commits September 9, 2023 01:29

refactor: HttpHeaders 클래스 구현

3f0294c

refactor: RequestLine 클래스 구현

8305190

refactor: HttpRequest 클래스 구현

0d43834

refactor: HttpRequest.requestUri 변수명 path로 변경

cb4b214

refactor: StatusLine 클래스 구현

e0a48ae

refactor: HttpStatus 열거형 구현

3ac7fc4

refactor: HeaderValue 클래스 구현

49c7bc1

";" 뒤에 옵션이 붙는 것을 고려하여 이를 처리하는 역할을 담당한다.

refactor: HttpResponse 클래스 구현

f7f4b30

test: 테스트에서의 expected 값 일부 조정

68e6b8b

Response Header의 순서로 인해 실패하는 테스트를 위해 조정

refactor: StatusLine 클래스 제거와 그에 따른 HttpResponse 코드 수정

f93afb6

refactor: HttpRequest 정적 팩토리 메서드 삭제

a4e4859

refactor: Controller 구현

75cc474

refactor: 쓰이지 않는 클래스 제거

0c52760

test: 스레드 학습 테스트 작성

33143c2

feat: 스레드 풀 적용

14f4173

feat: 동시성 컬렉션 사용

f10daee

yujamint requested a review from kiarakim September 10, 2023 17:23

yujamint self-assigned this Sep 10, 2023

kiarakim requested changes Sep 11, 2023

View reviewed changes

yujamint added 11 commits September 11, 2023 22:01

refactor: 오타 수정

e9eb960

refactor: Http Request를 읽어들이는 클래스(HttpRequestReader) 분리

47be420

refactor: HttpResponse의 ContentType을 구하는 역할 HttpRequest 객체에 위임

b9c41d4

refactor: ContentType 관련 변수 enum 구현

ce51f43

refactor: FileReader 구현

a734474

fix: HttpStatus message 필드 추가

5fe3478

refactor: RequestLine 상수명 변경

632d976

refactor: RequestLine 상수,변수명 변경

eb08f63

refactor: HeaderValue 팩토리 메서드 분기 처리 개선 및 상수 사용

7da6384

refactor: 헤더 조회 메서드명 개선

c09577b

refactor: HttpHeadersType enum 구현에 따른 플레인 텍스트 제거

d0be660

yujamint added 6 commits September 12, 2023 02:00

refactor: 존재하지 않는 계정에 대한 예외 처리 추가

4ce0385

refactor: 세션 저장 로직 개선

718dc00

refactor: existsByAccount 메서드 제거

281c806

refactor: 리터럴 변수 상수화

f27518e

refactor: RequestMapping 객체 구현

405a942

refactor: QueryString과 Path 구분하는 Path 객체 구현

f252d96

kiarakim approved these changes Sep 12, 2023

View reviewed changes

refactor: 매직 넘버 상수화

f8b86c9

kiarakim merged commit d0b0986 into woowacourse:yujamint Sep 12, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 3, 4단계] 민트(유재민) 미션 제출합니다. #462

[톰캣 구현하기 - 3, 4단계] 민트(유재민) 미션 제출합니다. #462

yujamint commented Sep 10, 2023

kiarakim left a comment

kiarakim Sep 11, 2023

yujamint Sep 12, 2023

yujamint commented Sep 12, 2023 •

edited

Loading

kiarakim left a comment

kiarakim Sep 12, 2023

yujamint Sep 12, 2023

kiarakim Sep 12, 2023

kiarakim Sep 12, 2023

kiarakim Sep 12, 2023

yujamint Sep 12, 2023

kiarakim Sep 12, 2023

kiarakim Sep 12, 2023

yujamint Sep 12, 2023

kiarakim Sep 12, 2023

sonarcloud bot commented Sep 12, 2023

		int queryStringIndex = path.indexOf("?");
		if (queryStringIndex == -1) {

[톰캣 구현하기 - 3, 4단계] 민트(유재민) 미션 제출합니다. #462

[톰캣 구현하기 - 3, 4단계] 민트(유재민) 미션 제출합니다. #462

Conversation

yujamint commented Sep 10, 2023

kiarakim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujamint commented Sep 12, 2023 • edited Loading

kiarakim 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

sonarcloud bot commented Sep 12, 2023

yujamint commented Sep 12, 2023 •

edited

Loading