[3단계 - Tomcat 구현하기] 비토(오상훈) 미션 제출합니다. #594

unifolio0 · 2024-09-10T01:37:57Z

안녕하세요 이상! 4단계 코드는 되돌리고 다시 리뷰 요청 보냅니다!

바뀐 점

Controller 세분화
HttpCookie 클래스 생성
HttpResponse에 Builder 패턴 적용

리뷰 잘 부탁드립니다!

…to unifolio0

kang-hyungu

비토 3단계 다시 구현하시기 바랍니다. 코드 퀄리티에 신경 쓰세요.
테스트 코드도 제가 제시한 것 말고는 하나도 안 보이네요.
테스트 코드가 없는데 다른 개발자는 어떻게 정상 동작한다고 확신할 수 있죠?
꼼꼼하게 테스트도 추가하시기 바랍니다.

kang-hyungu · 2024-09-10T04:09:37Z

study/src/test/java/thread/stage0/SynchronizationTest.java

@@ -41,7 +40,7 @@ private static final class SynchronizedMethods {

        private int sum = 0;

-        public void calculate() {
+        public synchronized void calculate() {


왜 synchronized를 붙이라고 했을까요?

만약 synchronized가 없으면 테스트 코드에서 실행되고 있는 1000개의 스레드 중 일부가 sum이 정확히 변경되어 저장하기 전에 접근하게 되면서 정확하게 1000번 증가한 값이 나오지 않게 됩니다.
그래서 synchronized를 사용해 한번에 하나의 스레드만 접근 가능하게 하고 다른 스레드는 그동안 대기하게 만들어 스레드 안전성을 보장해줍니다.

kang-hyungu · 2024-09-10T04:10:25Z

study/src/test/java/thread/stage0/ThreadPoolsTest.java

@@ -31,8 +30,8 @@ void testNewFixedThreadPool() {
        executor.submit(logWithSleep("hello fixed thread pools"));

        // 올바른 값으로 바꿔서 테스트를 통과시키자.
-        final int expectedPoolSize = 0;
-        final int expectedQueueSize = 0;
+        final int expectedPoolSize = 2;


Executors.newFixedThreadPool(...)와 Executors.newCachedThreadPool()는 어떤 차이가 있나요?

public static ExecutorService newFixedThreadPool(int nThreads) { return new ThreadPoolExecutor(nThreads, nThreads, 0L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<Runnable>()); }

public static ExecutorService newCachedThreadPool() { return new ThreadPoolExecutor(0, Integer.MAX_VALUE, 60L, TimeUnit.SECONDS, new SynchronousQueue<Runnable>()); }

public ThreadPoolExecutor(int corePoolSize, int maximumPoolSize, long keepAliveTime, TimeUnit unit, BlockingQueue<Runnable> workQueue)

newFixedThreadPool은 매개변수로 주어진 수만큼 스레드를 생성하고 해당 개수만큼의 스레드만 사용합니다. 그래서 해당 테스트에서 3개의 스레드를 실행시켰지만 2개의 스레드만 실행이 되고 남은 1개는 큐에서 대기하게 됩니다.
newCachedThreadPool은 처음에는 스레드를 생성하지 않고 요청이 들어올때마다(코드상으론 Integer.MAX_VALUE만큼) 스레드를 생성하게 됩니다. 따라서 테스트에서 3번의 요청이 들어와서 3개의 스레드가 실행되고 있고 큐에서 대기하고 있는 스레드는 없습니다.

kang-hyungu · 2024-09-10T04:12:20Z

tomcat/src/main/java/org/apache/coyote/http11/config/UnauthorizedInterceptor.java

+    private final Set<String> unauthorizedPaths = new HashSet<>();
+
+    public UnauthorizedInterceptor() {
+        unauthorizedPaths.add("/401");


좀 더 간결하게 작성할 방법 없을까요

Set.of를 통해 생성하는 방식으로 추가했는데 이게 더 좋은 건지는 모르겠습니다...

kang-hyungu · 2024-09-10T04:12:45Z

tomcat/src/main/java/org/apache/coyote/http11/config/UnauthorizedInterceptor.java

+    }
+
+    public boolean checkPath(HttpRequest httpRequest) {
+        return !httpRequest.containsKey("Location") && unauthorizedPaths.contains(httpRequest.getPath());


Localtion 문자열 대신 HttpHeaderName을 활용해보시기 바랍니다.

HttpHeaderName을 쓰도록 수정했고 관련된 다른 부분의 코드들도 전부 수정했습니다!

kang-hyungu · 2024-09-10T04:13:41Z

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


    @Override
    protected HttpResponse doPost(HttpRequest httpRequest) {
-        return null;
+        throw new RuntimeException();


RuntimeException 예외를 사용하지 마시고 적절한 예외를 사용하세요.

예외 처리한 것 전부 적절한 예외로 수정했습니다!

kang-hyungu · 2024-09-10T04:21:32Z

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

 import org.apache.coyote.http11.httpresponse.HttpStatusLine;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

 public class LoginController extends AbstractController {

    private static final Logger log = LoggerFactory.getLogger(LoginController.class);
+    private static final String LOGIN_PATH = "static/login.html";

    private final Session session = Session.getInstance();

    @Override
    protected HttpResponse doPost(HttpRequest httpRequest) {


메서드 이렇게 길게 쓰실건가요?

메서드 분리와 역할 분담으로 간소화했습니다!

kang-hyungu · 2024-09-10T04:22:25Z

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


-        return new HttpResponse(httpStatusLine, httpResponseHeader);
+    private boolean checkToken(String[] token) {
+        return Arrays.stream(token).anyMatch(t -> t.split("=").length < 2);
    }

    @Override
    protected HttpResponse doGet(HttpRequest httpRequest) {


메서드 10줄이 넘지 않게 정리하시고

문자열 제거 하시고

http에서 처리할 일을 컨트롤러에게 맡기지 마세요.

문자열 전부 상수화 했고 역할을 이동하여 코드를 간소화했습니다!

kang-hyungu · 2024-09-10T04:22:38Z

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

+            log.info(user.toString());
+            HttpStatusLine httpStatusLine = new HttpStatusLine(httpRequest.getVersion(), HttpStatusCode.FOUND);
+            return new HttpResponse.Builder(httpStatusLine)
+                    .setCookie("JSESSIONID=" + jsessionid)


JSESSIONID 계속 중복해서 보이네요

상수 처리했습니다!

kang-hyungu · 2024-09-10T04:23:12Z

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

+    private HttpResponse redirectLoginPage(HttpRequest httpRequest) throws URISyntaxException, IOException {
+        HttpStatusLine httpStatusLine = new HttpStatusLine(httpRequest.getVersion(), HttpStatusCode.OK);
+
+        var resourceUrl = getClass().getClassLoader().getResource(LOGIN_PATH);


파일 입출력 처리를 왜 컨트롤러에서 하는거에요...

파일 입출력 처리를 전부 Response로 이동시켰습니다!

kang-hyungu · 2024-09-10T04:23:48Z

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

+    protected HttpResponse doGet(HttpRequest httpRequest) {
+        try {
+            String fileName = "static/404.html";
+            var resourceUrl = getClass().getClassLoader().getResource(fileName);


getClass().getClassLoader().getResource(...)

게속 반복해서 등장하네요. 중복을 제거하세요

해당 부분을 staticResource메소드에서만 사용하도록 하여 중복을 제거했습니다!

kang-hyungu

비토 코멘트 남겼구요
같은 피드백이 반복되지 않도록 꼼꼼하게 챙겨보시기 바랍니다.
그리고 http request 관련 로직도 복잡해보이는데 테스트 코드를 더 보충해보세요.

kang-hyungu · 2024-09-12T06:29:28Z

tomcat/src/main/java/com/techcourse/config/UnauthorizedInterceptor.java

+import org.apache.coyote.http11.HttpHeaderName;
+import org.apache.coyote.http11.httprequest.HttpRequest;
+
+public class UnauthorizedInterceptor {


이게 왜 인터셉터죠?

인터셉터가 필요한 상황인가요?

인터셉터가 was와 무슨 관련이 있죠?

처음에 에러 페이지들인 401.html, 404.html, 500.html과 같은 페이지를 직접 호출해서 요청하는 건 막아야 된다고 생각해서 관련된 로직을 만드려고 하다가 스프링에서 사용했던 Interceptor와 비슷하다고 생각해서 만들었습니다.

지금보니 너무 과하게 생각했던 것 같습니다.

해당 클래스는 삭제했습니다!

kang-hyungu · 2024-09-12T06:29:56Z

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

@@ -1,4 +1,4 @@
-package org.apache.coyote.http11.controller;
+package com.techcourse.controller;



public interface Controller { void service(HttpRequest request, HttpResponse response) throws Exception; }

해당 형식으로 수정했습니다!

kang-hyungu · 2024-09-12T06:30:26Z

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

+    private static final String ACCOUNT = "account";
+    private static final String PASSWORD = "password";
+    private static final String INDEX_PATH = "/index.html";
+    private static final String JSESSIONID = "JSESSIONID";


세션 아이디를 컨트롤러에서 알 필요가 있을까요?
누구의 관심사일까요?
COOKIE_DELIMITER, SESSION_USER_NAME도 마찬가지입니다.

cookie와 session과 관련된 정보는 각각 HttpResponse와 Session으로 옮겼습니다!

kang-hyungu · 2024-09-12T06:33:54Z

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

            log.error(e.getMessage(), e);
        }
    }
+
+    private HttpResponse getHttpResponse(HttpRequest httpRequest) throws IOException, URISyntaxException {


메서드를 10줄로 줄여보세요.

StringBuilder를 활용하여 간소화했습니다!

kang-hyungu · 2024-09-12T06:34:51Z

tomcat/src/main/java/org/apache/coyote/http11/httprequest/HttpRequestConvertor.java

+        if (requestLine == null) {
+            throw new IllegalArgumentException("요청이 비어 있습니다");
+        }
+        if (requestLine.split(REQUEST_LINE_DELIMITER).length < 3) {


3이 뭔가요

다시 한번 전체적으로 검사하여 상수화 처리 했습니다!

kang-hyungu · 2024-09-12T06:36:34Z

tomcat/src/main/java/org/apache/coyote/http11/httprequest/HttpRequestConvertor.java

+    private static final int TUPLE_KEY_INDEX = 0;
+    private static final int TUPLE_VALUE_INDEX = 1;
+    private static final int HEADER_KEY_INDEX = 0;
+    private static final SessionManager SESSION_MANAGER = new SessionManager();


세션 매니저 객체를 다른데서 생성할 수도 있겠군요?

SessionManager를 싱글톤으로 만들어 동일한 세션 매니저 객체를 사용하도록 수정했습니다!

kang-hyungu · 2024-09-12T06:37:17Z

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

+            return this;
+        }
+
+        public HttpResponseBuilder staticResource(String path) {


10줄 넘기지 말 것.

간소화했습니다!

kang-hyungu · 2024-09-12T06:39:04Z

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

    }

-    public void addHeaders(String key, String value) {
-        headers.put(key, value);
+    public String getString() {


getString이라 붙이면 무슨 동작을 하는지 다른 개발자가 메서드명만 보고 판단하기 어렵습니다.
그냥 문자열 반환이 아니라 HTTP 헤더를 스펙에 맞춰 생성하고 있으니 적절한 이름을 붙이세요.
안 그러면 어떤 의도인지 다른 개발자가 매번 코드를 파악해야 해요.

그리고 10줄 넘기지 마세요.

createHeadersResponse로 변경하고 StringBuilder로 간소화했습니다!

kang-hyungu · 2024-09-12T06:39:10Z

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

+        int size = headers.keySet().size();
+        int i = 1;
+        for (HttpHeaderName key : headers.keySet()) {
+            if (i < size) {


depth를 1로 줄였습니다!

kang-hyungu · 2024-09-12T06:39:27Z

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

+
+public class SessionManager implements Manager {
+
+    private static final Map<String, Session> SESSIONS = new HashMap<>();


동시성 문제는 없습니까?

해당 부분은 4단계라고 판단해서 HashMap으로 했었습니다. Concurrent Collections로 수정했습니다!

kang-hyungu

코드가 훨씬 나아졌네요. pr 머지할테니 다음 단계 진행하시죠

geoje and others added 24 commits September 5, 2024 11:11

fix: remove implementation logback-classic on gradle (woowacourse#501)

fed02f6

fix: add threads min-spare configuration on properties (woowacourse#502)

7e91356

Merge branch 'unifolio0' of https://github.com/unifolio0/java-http in…

44fc288

…to unifolio0

refactor: favicon.ico 파일 추가

7ed4d23

refactor: 변수명 수정

078af63

refactor: 불필요한 로직 제거

25e67eb

fix: 잘못된 url 입력에 대한 null 처리

8e570a7

fix: input값이 덜 들어왔을 시 에러 처리

3b1799d

refactor: HttpResponse 클래스에 builder 패턴 적용

f986e25

refactor: 불필요한 파일 삭제

dda9c17

refactor: 없는 url에 대한 상태코드 변경

c53b87a

refactor: Cookie 클래스로 추출

b24f95f

refactor: depth 감소

8672b31

refactor: 쓰레드 안전성 보장

61ff56c

refactor: 쓰레드풀 생성

74fce63

refactor: 존재하지 않는 url에 대한 처리 책임 이동

75f9655

refactor: 패키지 이동

4ae9b63

refactor: 사용하지 않는 import 삭제

9bff59a

test: 학습 테스트

b3ae80d

refactor: depth 감소

a522403

refactor: 특정 페이지에 대한 권한 처리 적용

3273872

refactor: 권한 처리 시 redirect는 제외

a4cc800

refactor: 4단계 제거

5f3957e

refactor: 4단계 제거

7277aa2

unifolio0 requested a review from kunsanglee September 10, 2024 01:38

kang-hyungu requested changes Sep 10, 2024

View reviewed changes

unifolio0 force-pushed the step2 branch from ae065ff to 7277aa2 Compare September 10, 2024 05:54

unifolio0 added 3 commits September 10, 2024 19:57

refactor: 상수화

79da605

refactor: 예외 클래스 변경

2740606

refactor: HttpResponse의 Builder 패턴 간소화

5de4b86

unifolio0 added 9 commits September 12, 2024 13:05

test: RequestMappingTest, RegisterControllerTest 추가

5b53b77

test: HttpRequestConvertorTest 작성

ff86414

test: HttpCookieConvertorTest 작성

5a28c68

test: LoginControllerTest 작성

bd236dc

test: HttpResponseTest 작성

0039a00

test: HttpRequestMaker 적용

3e6b67d

refactor: 디버깅용 출력문 삭제

cd9c986

test: 오류가 나는 테스트 수정

dbf14ba

test: DisplayName 추가

afa9586

kang-hyungu requested changes Sep 12, 2024

View reviewed changes

unifolio0 added 18 commits September 12, 2024 20:00

refactor: Controller의 service 형식 변경

7b0131d

refactor: 상수화

ef947a1

refactor: 로직 이동

98f9e5b

refactor: 역할 이동

222b4ad

refactor: 상수 이동

c4a0070

fix: 세션이 삽입이 안 되는 오류 수정

4ec21d2

fix: SessionManager 싱글톤화

4fc70b6

refactor: 메소드명 수정

1b628be

refactor: 코드 간소화

f5a4916

refactor: depth 간소화

84a3721

refactor: 사용하지 않는 메소드 및 변수 삭제

245384b

refactor: 메소드 간소화

99a54b9

refactor: 메소드명 수정

05d3356

refactor: 클래스 삭제

dae1388

refactor: Concurrent Collections로 수정

99c5f8e

refactor: 불필요한 테스트 삭제

459958d

test: 불필요한 변수 삭제

3037360

test: RequestBody 생성 테스트 작성

9e8ea4d

kang-hyungu approved these changes Sep 13, 2024

View reviewed changes

kang-hyungu merged commit a7cde5e into woowacourse:unifolio0 Sep 13, 2024

		@@ -1,4 +1,4 @@
		package org.apache.coyote.http11.controller;
		package com.techcourse.controller;


		public class SessionManager implements Manager {

		private static final Map<String, Session> SESSIONS = new HashMap<>();

[3단계 - Tomcat 구현하기] 비토(오상훈) 미션 제출합니다. #594

[3단계 - Tomcat 구현하기] 비토(오상훈) 미션 제출합니다. #594

Uh oh!

Conversation

unifolio0 commented Sep 10, 2024

바뀐 점

Uh oh!

kang-hyungu 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!

kang-hyungu 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