[톰캣 구현하기 3, 4단계] 썬샷(오진택) 미션 제출합니다. #479

Ohjintaek · 2023-09-11T06:44:25Z

이번 리뷰도 잘 부탁드려요 연어!!
1, 2단계 때부터 3단계의 내용을 일부 고려하면서 해서 (다들 그렇겠지만) 1, 2단계보다는 좀 더 쉬웠던 거 같네요
Request를 이용해서 적절한 Handler를 매핑하도록 하는 부분이 재밌었던 거 같아요. 원래 예시로 주어졌던 AbstractController에는 doGet()이나 doPost() 메서드가 있어서 Http 메서드에 따라 호출하는 거 같은데 처음에 '이 메서드들은 무슨 역할이지?' 생각만 하고 찾아보지는 않아서 현재 구조가 되었습니다.
스레드나 동시성 같은 부분은 학습 테스트를 안 하고 힌트만 보고 구현해서 아직 이해가 부족한 채로 구현한 상태입니다.

nuyh99

안녕하세요 썬샷!
바쁜 일상 속에서 매우 빠르게 구현하셨군요...대단합니다

간단하게 코멘트 남겨봤습니다!
반영해주시면 좋을 것 같은 부분은 아래와 같아요.

Tomcat과 비슷한 Controller, AbstractController로 구조를 바꿔보면 좋을 것 같습니다.
jwp, apache 패키지가 있는데 비즈니스와 WAS의 기능적인 기준으로 패키지 위치를 바꿔보면 좋을 것 같습니다.
전반적으로 상수화를 하거나 열거형으로 휴먼에러를 줄이면 더 가독성이 좋아질 것 같아요.
시간적 여유가 되신다면 테스트 코드도 작성해보는 것은 어떨까요...?

원하시는 부분만 반영해서 리뷰 요청 주셔도 좋습니다!
화이팅...!

tomcat/src/main/java/org/apache/catalina/connector/Connector.java

tomcat/src/main/java/org/apache/catalina/SessionManager.java

tomcat/src/main/java/nextstep/jwp/commandcontroller/AbstractController.java

nuyh99 · 2023-09-12T05:02:13Z

tomcat/src/main/java/nextstep/jwp/commandcontroller/AbstractController.java

+import nextstep.jwp.request.HttpRequest;
+import nextstep.jwp.response.HttpResponse;
+
+public class AbstractController implements Controller {


미션에서 준 AbstractController는 Java EE의 HttpServlet을 투영한 것 같아요.
따라서 이번 미션에서는 doPost, doGet 등도 추가한 구조로 구현해보면 어떨까 싶습니다!

doPost()와 doGet()을 사용하는 방식으로 바꾸어주었습니다!!

nuyh99 · 2023-09-12T05:10:29Z

tomcat/src/main/java/nextstep/jwp/commandcontroller/ErrorPageController.java

+    @Override
+    public void service(HttpRequest request, HttpResponse response) throws Exception {
+        response.setStatusLine(StatusLine.of(request.getHttpVersion(), HttpStatus.NOT_FOUND));
+        response.addHeader("Content-Type", ContentType.HTML.getType());


헤더 값을 각 컨트롤러에서 하드코딩해주고 있는데 휴먼 에러의 위험이 있지 않을까요??!

Http Header도 메서드나 상태 코드와 같이 enum으로 관리해주도록 변경해주었습니다

비즈니스적인 커스텀 헤더를 추가하려고 하면 ENUM 타입의 KEY를 사용할 때 문제가 생길 수 있을 것 같아요.
Enum 제공 측은 톰캣 같은 범용적인 모듈이기 때문에 개발자가 그 Enum에 추가할 수도 없구요!

이러한 경우는 어떻게 하는 것이 좋을까요??!

nuyh99 · 2023-09-12T05:13:57Z

tomcat/src/main/java/nextstep/jwp/commandcontroller/LoginController.java

+    public void service(HttpRequest request, HttpResponse response) {
+        final Map<String, String> logInfo = Arrays.stream(request.getRequestBody().split("&"))
+                .map(input -> input.split("="))
+                .collect(Collectors.toMap(info -> info[0], info -> info[1]));
+
+        response.setStatusLine(StatusLine.of(request.getHttpVersion(), HttpStatus.FOUND));
+        final Optional<User> savedUser = InMemoryUserRepository.findByAccount(logInfo.get("account"));
+        if (savedUser.isPresent()) {
+            final User user = savedUser.get();
+            if (user.checkPassword(logInfo.get("password"))) {
+                response.addHeader("Location", "index.html");
+                final Session session = new Session(UUID.randomUUID().toString());
+                session.setAttribute("user", user);
+                SessionManager.add(session);
+                response.addCookie("JSESSIONID", session.getId());
+                return;
+            }
+        }
+        response.addHeader("Location", "401.html");
+    }
+}


이 부분도 휴먼 에러를 줄이고, 분기를 줄이는 식으로 리팩토링할 수 있을 것 같아요!

메서드 분리하고 상수화 좀 해주긴 했는데 만족스럽게 된 것 같진 않네요

nuyh99 · 2023-09-12T05:14:59Z

tomcat/src/main/java/nextstep/jwp/model/HttpCookies.java

+    private static final String DELIMITER = "\r\n";
+    private final Map<String, String> cookies = new LinkedHashMap<>();
+


상수와 객체는 개행해줘도 좋을 것 같아요!

감사합니다~

nuyh99 · 2023-09-12T05:16:52Z

tomcat/src/main/java/nextstep/jwp/response/HttpResponse.java

+    }
+
+    public void addCookie(final String name, final String value) {
+        cookies.save(name, value);
    }

    public String toResponse() {


httpResponse.toResponse()라고 호출할 것 같은데 조금 어색하다고 생각이 됩니다!

HttpResponse를 String으로 바꾸는 거라 toString()을 오버라이드할까도 생각해봤지만, toString()의 의도와 다르게 쓰이는 것 같기도 해서 convertToString()으로 바꾸어주었는데 어떤가요?

Ohjintaek · 2023-09-14T12:51:12Z

시간이 좀 더 있었으면 테스트코드를 짰을 텐데 너무 늦게 보내면 안 되니 나머지들만 반영했어요...

sonarcloud · 2023-09-14T12:51:25Z

Kudos, SonarCloud Quality Gate passed!

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

nuyh99

쉽지 않은 리팩토링 과정이였을 것 같은데 썬샷 고생하셨습니다!!
개발자의 관심과 HTTP 처리용 컴포넌트들을 잘 분리하신 것 같아요.

최대한 도움되도록 리뷰를 남기려고 했는데 도움이 되셨을 지는 모르겠네요ㅠㅠ

추가로 생각해봤으면 하는 내용들을 코멘트로 남겨봤습니다.
원하시면 머지한 이후에도 코멘트나 오프라인으로 같이 고민해볼 수 있을 것 같아요!!

nuyh99 · 2023-09-15T01:22:22Z

tomcat/src/main/java/nextstep/jwp/controller/AbstractController.java

+    protected void doPost(HttpRequest request, HttpResponse response) throws Exception { /* NOOP */ }
+    protected void doGet(HttpRequest request, HttpResponse response) throws Exception { /* NOOP */ }
+
+    protected String readResponseBody(final String requestUri) throws IOException, URISyntaxException {


공통적으로 사용할 수 있게 만들기 위해서 추상 클래스에 만들어뒀군요!
IS-A 관계를 잘 사용하신 것 같아요!

nuyh99 · 2023-09-15T01:27:31Z

tomcat/src/main/java/nextstep/jwp/controller/LoginController.java

+        final Optional<User> savedUser = findUserByLogIn(authInfo);
+        if (savedUser.isPresent()) {
+            final User user = savedUser.get();
+            response.addHeader(HttpHeader.LOCATION.getName(), "index.html");


index.html등의 정적 자원 이름이 바뀌면 여기저기 다 수정해줘야 할 것 같아요.
수정하지 않아도 컴파일 타임에서 오류가 나는 것도 아니구요!

어떻게 관리하면 좋을 지 궁금해요.

nuyh99 · 2023-09-15T01:28:01Z

tomcat/src/main/java/nextstep/jwp/controller/LoginController.java

+        response.addHeader(HttpHeader.LOCATION.getName(), "401.html");
+    }
+
+    private Optional<User> findUserByLogIn(final Map<String, String> logInfo) {


비즈니스 로직을 서비스 레이어로 분리하면 재사용성이 훨씬 높아지겠군요!

nuyh99 · 2023-09-15T01:30:56Z

tomcat/src/main/java/nextstep/jwp/controller/RequestMapping.java

+    private AbstractController errorPageController;
+
+    public RequestMapping(final List<AbstractController> controllers, final AbstractController errorPageController) {
+        this.controllers.addAll(controllers);


의존성을 생성자 주입하고 있는데 할당하는 방식이 아닌 기존 상태를 변경하는 이유가 있을까요?

nuyh99 · 2023-09-15T01:36:01Z

tomcat/src/main/java/nextstep/jwp/controller/RequestMapping.java

+public class RequestMapping implements Controller {
+
+    private final List<AbstractController> controllers = new ArrayList<>();
+    private AbstractController errorPageController;


재할당을 허용하는 이유가 궁금합니다!

nuyh99 · 2023-09-15T01:38:23Z

tomcat/src/main/java/nextstep/jwp/controller/AbstractController.java

+import org.apache.coyote.request.HttpRequest;
+import org.apache.coyote.response.HttpResponse;
+
+abstract class AbstractController implements Controller {


이 추상 클래스는 HttpServlet과 비슷한 것 같아요.
명세에 가깝게 느껴져서 패키지 위치가 웹앱 쪽이 맞을 지 의문입니다!

nuyh99 · 2023-09-15T02:01:14Z

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

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


타이트한 처리를 최상위 예외로 변경하셨군요!
아마 시간이 부족해서 그렇지 않았을까 생각해봅니다.

정의하신 커스텀 예외들을 어떤 용도로 사용하면 좋을 지

어떤 구조로 처리할 수 있을 지

위의 두 가지 모두 아니라면 커스텀 예외를 쓰는 의미는 무엇일 지

생각해보면 좋을 것 같아요!

Ohjintaek added 7 commits September 11, 2023 00:03

refactor: ResponseHeader가 출력 순서를 보장하도록 수정

2ceb3b4

refactor: HttpResponse 구조 리팩토링

97b8a82

feat: 웹 요청 분기 처리를 Controller 인터페이스를 상속하도록 구현

730c324

feat: 올바른 컨트롤러 매핑

8859566

refactor: 웹 응답 포매팅 리팩터링

c70952c

feat: ThreadPool 생성

24e135e

feat: SessionManager에서 스레드 안정성을 위해 ConcurrentHashMap 사용

62644dd

Ohjintaek requested a review from nuyh99 September 11, 2023 06:44

Ohjintaek self-assigned this Sep 11, 2023

nuyh99 requested changes Sep 12, 2023

View reviewed changes

Ohjintaek added 6 commits September 13, 2023 14:15

teat: 캐시 학습 테스트

ec51e20

teat: 스레드 학습 테스트

6114aa7

refactor: jwp와 tomcat 간 패키지 분리 리팩토링

e5e8416

refactor: Controller를 자바 Servlet과 비슷한 구조로 수정

628ccf8

feat: HTTP 헤더 enum으로 관리

b7d7d29

refactor: 상수화 및 메서드 분리

0bb3470

nuyh99 self-requested a review September 15, 2023 02:04

nuyh99 approved these changes Sep 15, 2023

View reviewed changes

nuyh99 merged commit 61dd890 into woowacourse:ohjintaek Sep 15, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 3, 4단계] 썬샷(오진택) 미션 제출합니다. #479

[톰캣 구현하기 3, 4단계] 썬샷(오진택) 미션 제출합니다. #479

Ohjintaek commented Sep 11, 2023

nuyh99 left a comment

nuyh99 Sep 12, 2023

Ohjintaek Sep 14, 2023

nuyh99 Sep 12, 2023

Ohjintaek Sep 14, 2023

nuyh99 Sep 15, 2023

nuyh99 Sep 12, 2023

Ohjintaek Sep 14, 2023

nuyh99 Sep 12, 2023

Ohjintaek Sep 14, 2023

nuyh99 Sep 12, 2023

Ohjintaek Sep 14, 2023

Ohjintaek commented Sep 14, 2023

sonarcloud bot commented Sep 14, 2023

nuyh99 left a comment

nuyh99 Sep 15, 2023

nuyh99 Sep 15, 2023

nuyh99 Sep 15, 2023

nuyh99 Sep 15, 2023

nuyh99 Sep 15, 2023

nuyh99 Sep 15, 2023

nuyh99 Sep 15, 2023

		private static final String DELIMITER = "\r\n";
		private final Map<String, String> cookies = new LinkedHashMap<>();

[톰캣 구현하기 3, 4단계] 썬샷(오진택) 미션 제출합니다. #479

[톰캣 구현하기 3, 4단계] 썬샷(오진택) 미션 제출합니다. #479

Conversation

Ohjintaek commented Sep 11, 2023

nuyh99 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

Ohjintaek commented Sep 14, 2023

sonarcloud bot commented Sep 14, 2023

nuyh99 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