톰캣 구현 1단계 주드 미션 제출합니다 #326

kevstevie · 2023-09-04T05:34:25Z

안녕하세요 다즐
미션 참 어렵네요
지저분한 코드 리뷰 잘 부탁드립니다.. 6시 전까지 보충 좀 해보겠습니다.

대략적인 플로우는 이렇습니다

request reader -> frontcontroller -> 각 controller -> response 생성

woo-chang

안녕하세요 주드 😀
객체 간 역할도 잘 분리되어 있고 깔끔하게 잘 구현되어 있어 리뷰하기 너무 편했습니다!
수정할 부분은 크게 없고, 몇 가지 질문과 저의 주관적인 생각을 담은 커멘트 남겨보았습니다 :)
확인해보시고 답변과 필요한 부분에 대해서는 수정해주시면 감사합니다~
추가로 예외 처리와 테스트 코드는 아직 작성하시지 않은 것 같아서 리팩토링하면서 추가하면 더 좋을 것 같아요 수고하셨습니다 🙇🏻‍♂️

woo-chang · 2023-09-04T07:59:02Z

study/src/test/java/study/FileTest.java

        final String fileName = "nextstep.txt";

        // todo
-        final Path path = null;
+        final Path path = Path.of(getClass().getClassLoader().getResource(fileName).getPath());
+        System.out.println(path);


디버깅을 위해 사용한 출력문은 삭제해도 좋을 것 같아요 :)

부끄럽네요..

woo-chang · 2023-09-04T08:07:17Z

tomcat/src/test/java/nextstep/jwp/db/InMemoryUserRepositoryTest.java

+import nextstep.jwp.model.User;
+import org.junit.jupiter.api.Test;
+
+class InMemoryUserRepositoryTest {


@SuppressWarnings("NonAsciiCharacters") @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)

소소하지만 한글 테스트 메서드명 사용을 위한 어노테이션을 추가하는건 어떨까요?

어노테이션보다 인텔리제이 설정을 선호하는데 어떤가요

좋은 방법이라고 생각합니다 😀

저는 협업하는 과정에서는 프로젝트를 클론했을 때 개인이 별도의 인텔리제이 설정을 하지 않아도 된다는 편의성을 생각했었습니다. 현재는 개인이 진행하는 작업이기에 인텔리제이 설정으로도 충분하다는 생각이 드네요 👍

협업을 하더라도 팀끼리 컨벤션을 맞추면 되는 부분이기에 적절한 방법을 선택하면 될 것 같아요!

woo-chang · 2023-09-04T08:11:22Z

tomcat/src/main/java/nextstep/jwp/db/InMemoryUserRepository.java


 public class InMemoryUserRepository {

    private static final Map<String, User> database = new ConcurrentHashMap<>();
+    private static final AtomicLong sequence = new AtomicLong(1);


동시성까지 꼼꼼하게 챙겨주셨네요 👍

woo-chang · 2023-09-04T08:13:46Z

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


-            final var responseBody = "Hello world!";
+            RequestReader requestReader = new RequestReader(new BufferedReader(new InputStreamReader(inputStream)));


BufferedReader도 자원 회수를 위해 try catch resources안에 넣어주는게 어떨까요?

woo-chang · 2023-09-04T08:15:27Z

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


-            final var responseBody = "Hello world!";
+            RequestReader requestReader = new RequestReader(new BufferedReader(new InputStreamReader(inputStream)));
+            requestReader.read();


생성자에서 바로 초기화해주지 않고 read 메서드를 별도로 호출하는 이유가 궁금합니다!

음 그냥 별 이유는 없이 생성자에 복잡한 로직을 넣고 싶지 않았어요

woo-chang · 2023-09-04T08:48:17Z

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

+            }
+            this.body = sb.toString();
+        }
+        headers.put(Header.CONTENT_LENGTH.getName(), String.valueOf(file.length()));


Suggested change

headers.put(Header.CONTENT_LENGTH.getName(), String.valueOf(file.length()));

headers.put(Header.CONTENT_LENGTH.getName(), String.valueOf(body.length()));

큰 차이는 발생하지 않을 것 같지만, createBodyByPlainText와 동일하게 body의 length를 추가하는 것으로 일관성을 유지해도 좋을 것 같아요 ㅎㅎ

body를 밖에서 받으면서 로직 수정됐습니다!

woo-chang · 2023-09-04T14:01:40Z

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

+        url = Resolver.resolve(url);
+        String path = getClass().getClassLoader().getResource(STATIC + url).getPath();
+        File file = new File(path);
+        try (BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
+            StringBuilder sb = new StringBuilder();
+            String line;
+            while ((line = bufferedReader.readLine()) != null) {
+                sb.append(line).append(System.lineSeparator());
+            }
+            this.body = sb.toString();
+        }


Response 내부에서 body를 생성하기보다는 외부에서 body를 만들어서 넣어주는건 어떨까요?

그렇게 된다면 text body를 설정하는 메서드와 file body를 설정하는 메서드가 1개로 통합될 수 있을 것 같아서 제안드립니다!

좋습니다 생각해보니 실제로 responseBody는 밖에서 받고 있군요

woo-chang · 2023-09-04T14:03:47Z

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

+
+    private static final String STATIC = "static";
+
+    private final RequestReader requestReader;


RequestReader는 요청 관련 데이터를 가지고 있는 것으로 이해했는데, 응답에서 요청에 대한 의존성을 직접적으로 가지기 보다는 필요한 값만 가지도록 하는 것은 어떨까요?!

woo-chang · 2023-09-04T14:07:23Z

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

+    public String getSession() {
+        return params.get(JSESSION);
+    }


Suggested change

public String getSession() {

return params.get(JSESSION);

}

public String getCookie(String name) {

return params.get(name);

}

Cookier가 JSESSIONID명을 직접적으로 가지기보다는, name을 통해 필요한 key를 전달받아서 사용하는 건 어떠신지 궁금합니다 🤓

좋습니다 쿠키, 리퀘스트에서 모두 jsession에 대한 정보를 지웠습니다

woo-chang · 2023-09-04T14:08:59Z

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

+    public Response createBodyByPlainText(String text) {
+        this.body = text;
+        headers.put(Header.CONTENT_LENGTH.getName(), String.valueOf(body.length()));
+        return this;


스스로를 반환하게 함으로 빌더 패턴을 가져갈 수 있네요 배워갑니다 👍

kevstevie

안녕하세요 즐선생님 리뷰 잘 받았습니다
이전보단 코드가 나아진 느낌이 드네요

kevstevie · 2023-09-05T06:17:17Z

study/src/test/java/study/FileTest.java

        final String fileName = "nextstep.txt";

        // todo
-        final Path path = null;
+        final Path path = Path.of(getClass().getClassLoader().getResource(fileName).getPath());
+        System.out.println(path);


부끄럽네요..

kevstevie · 2023-09-05T06:21:37Z

tomcat/src/main/java/nextstep/jwp/presentation/FrontController.java

+    static {
+        controllers.put("/", frontController.mainPageController);
+        controllers.put("/login", frontController.loginController);
+        controllers.put("/register", frontController.loginController);
+        controllers.put("/index", frontController.indexController);
+        controllers.put("/index.html", frontController.indexController);
+    }


좋은 것 같습니다👍

kevstevie · 2023-09-05T06:28:51Z

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

+    public String getSession() {
+        return params.get(JSESSION);
+    }


좋습니다 쿠키, 리퀘스트에서 모두 jsession에 대한 정보를 지웠습니다

kevstevie · 2023-09-05T06:30:12Z

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

+        String[] split = line.split(" ");
+        String name = split[0].substring(0, split[0].length() - 1);


그렇네요 천재인가요?

kevstevie · 2023-09-05T10:45:56Z

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


-            final var responseBody = "Hello world!";
+            RequestReader requestReader = new RequestReader(new BufferedReader(new InputStreamReader(inputStream)));
+            requestReader.read();


음 그냥 별 이유는 없이 생성자에 복잡한 로직을 넣고 싶지 않았어요

kevstevie · 2023-09-05T10:46:31Z

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

+        url = Resolver.resolve(url);
+        String path = getClass().getClassLoader().getResource(STATIC + url).getPath();
+        File file = new File(path);
+        try (BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
+            StringBuilder sb = new StringBuilder();
+            String line;
+            while ((line = bufferedReader.readLine()) != null) {
+                sb.append(line).append(System.lineSeparator());
+            }
+            this.body = sb.toString();
+        }


좋습니다 생각해보니 실제로 responseBody는 밖에서 받고 있군요

kevstevie · 2023-09-05T10:48:14Z

tomcat/src/test/java/nextstep/jwp/db/InMemoryUserRepositoryTest.java

+import nextstep.jwp.model.User;
+import org.junit.jupiter.api.Test;
+
+class InMemoryUserRepositoryTest {


어노테이션보다 인텔리제이 설정을 선호하는데 어떤가요

kevstevie · 2023-09-05T10:49:08Z

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

+            }
+            this.body = sb.toString();
+        }
+        headers.put(Header.CONTENT_LENGTH.getName(), String.valueOf(file.length()));


body를 밖에서 받으면서 로직 수정됐습니다!

sonarcloud · 2023-09-06T06:38:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
8 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

woo-chang

리뷰 반영하신거 모두 확인했습니다! 수고하셨습니다~!
구조도 개선되고 각각의 클래스가 가져야할 역할을 잘 나누신 것 같아서 읽기 편했습니다 ㅎㅎ
테스트 코드는 다음 미션을 진행하면서 채워보셔도 좋을 것 같아요 😃
다음 미션도 기대하겠습니다 :)

kevstevie added 9 commits September 2, 2023 00:40

feat: index.html 출력 구현

e5ccf98

feat: css 적용

4772fa8

feat: 쿼리 파싱 구현

350b66b

feat: 로그인 리다이렉트 구현

d597dc8

refactor: 헤더 순서 수정

937b2a2

feat: body 읽어서 로그인 구현

296eb41

feat: 로그인 세션 쿠키 구현

77d5eff

test: 통합 테스트 추가

2339099

refactor: map으로 url - controller 매칭

ee9370d

woo-chang assigned kevstevie Sep 4, 2023

woo-chang self-requested a review September 4, 2023 07:58

kevstevie added 2 commits September 4, 2023 20:24

refactor: 세션 다시 구현

09921ea

refactor: throws 구문 제거

96340a1

woo-chang requested changes Sep 4, 2023

View reviewed changes

kevstevie added 4 commits September 5, 2023 14:14

test: 학습테스트 캐싱

a0f8f41

Merge branch 'temp' into step1

d913b0f

refactor: 리뷰 반영 및 리팩터링

6567d96

refactor: 테스트 추가 및 로직, 컨벤션 수정

5f5c43d

kevstevie commented Sep 5, 2023

View reviewed changes

kevstevie added 4 commits September 5, 2023 21:44

refactor: 띄어쓰기 수정

7e17d6d

chore: 코드 커버리지 왜 안되는걸까요

42c100f

refactor: 세션 구현 방식 변경

a7f459c

refactor: 리스폰스 생성 구조 및 세션 초기화 수정

4f3e737

woo-chang approved these changes Sep 6, 2023

View reviewed changes

woo-chang merged commit 196bb64 into woowacourse:kevstevie Sep 6, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

톰캣 구현 1단계 주드 미션 제출합니다 #326

톰캣 구현 1단계 주드 미션 제출합니다 #326

kevstevie commented Sep 4, 2023 •

edited

Loading

woo-chang left a comment

woo-chang Sep 4, 2023

kevstevie Sep 5, 2023

woo-chang Sep 4, 2023

kevstevie Sep 5, 2023

woo-chang Sep 6, 2023

woo-chang Sep 4, 2023

woo-chang Sep 4, 2023

woo-chang Sep 4, 2023

kevstevie Sep 5, 2023

woo-chang Sep 4, 2023

kevstevie Sep 5, 2023

woo-chang Sep 4, 2023

kevstevie Sep 5, 2023

woo-chang Sep 4, 2023

woo-chang Sep 4, 2023

kevstevie Sep 5, 2023

woo-chang Sep 4, 2023

kevstevie left a comment

kevstevie Sep 5, 2023

kevstevie Sep 5, 2023

kevstevie Sep 5, 2023

kevstevie Sep 5, 2023

kevstevie Sep 5, 2023

kevstevie Sep 5, 2023

kevstevie Sep 5, 2023

kevstevie Sep 5, 2023

sonarcloud bot commented Sep 6, 2023

woo-chang left a comment


		final var responseBody = "Hello world!";
		RequestReader requestReader = new RequestReader(new BufferedReader(new InputStreamReader(inputStream)));

	headers.put(Header.CONTENT_LENGTH.getName(), String.valueOf(file.length()));
	headers.put(Header.CONTENT_LENGTH.getName(), String.valueOf(body.length()));


		private static final String STATIC = "static";

		private final RequestReader requestReader;

		String[] split = line.split(" ");
		String name = split[0].substring(0, split[0].length() - 1);

톰캣 구현 1단계 주드 미션 제출합니다 #326

톰캣 구현 1단계 주드 미션 제출합니다 #326

Conversation

kevstevie commented Sep 4, 2023 • edited Loading

woo-chang 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

kevstevie 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

sonarcloud bot commented Sep 6, 2023

woo-chang left a comment

Choose a reason for hiding this comment

kevstevie commented Sep 4, 2023 •

edited

Loading