[톰캣 구현하기 - 1,2단계] 이오(이지우) 미션 제출합니다. #343

LJW25 · 2023-09-04T07:54:20Z

안녕하세요 지토 🙂
이번 리뷰 잘부탁드립니다.
기능 구현은 완료하였지만 전체적으로 부족한 부분이 많네요.🥲
대부분의 메소드가 private라 통합 테스트만 작성하였는데, 부족한 테스트는 리팩토링 과정에서 구현하도록 하겠습니다.

고려한 부분

쿠키
미션에서 Cookie를 별도의 클래스를 분리해 구현하였지만, 저의 경우 Header와 QueryString을 Map<String, String>으로 두고 있었기 때문에 이와 비슷한 구조&역할인 Cookie 또한 Map으로 구현하였습니다. 분리에서 오는 장점보다 관리해야할 클래스를 늘린다는 단점이 더 크게 느껴졌고, 통일성을 유지하는 것이 더 낫다고 생각해서요.
Processor
현재 Processor가 하는 일이 과도하게 많다는 생각이 들어 고민중에 있습니다. 여러 Handler 메소드를 사용하여 리팩토링 하였지만 아직 부족한 것 같아서요. (다음 단계의 미션으로 Controller를 구현하면 좀 나아질 것으로 예상되긴 합니다.) 지토는 현재 단계의 Processor가 충분하다고 느끼시나요? 혹은 더 간결해 질 수 있도록 역할을 분리해야한다고 생각하시나요?

sonarcloud · 2023-09-04T07:57:14Z

Kudos, SonarCloud Quality Gate passed!

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

sonarcloud · 2023-09-04T08:01:20Z

Kudos, SonarCloud Quality Gate passed!

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

apptie

안녕하세요 이오!
미션 고생하셨습니다 👍

(테스트가 없다는 것만 빼면) 훌륭하게 코드를 작성하신 것 같습니다

일단 이오의 의견이 궁금한 부분이 있어서 Request changes로 설정하도록 하겠습니다
리뷰는 뭐 이것저것 제안 드리기는 했는데 별 내용 없어서...적용하고 싶으신 부분만 적용해주시면 좋을 것 같습니다

고려한 부분의 경우 제 생각이 궁금하신?것? 같아서, 그에 대한 제 생각은 밑에다가 추가적으로 적어두도록 하겠습니다!

쿠키
저도 헤더나 쿠키나 쿼리 스트링이나 아무튼 Map 구조라는 것에 동의합니다!
통일성을 유지하는 것도 중요한 관점이라고 생각하고요 ㅎㅎ
다만 저는 헤더나 쿠키나 쿼리 스트링을 모두 별도의 래핑 클래스로 감싸주기는 했습니다.
감쌀 경우 가독성은 좋아지더라고요! 시간이 없어서 테스트 코드 작성할 때 후회하기는 했지만요...
예를 들어 헤더같은 경우는 적어도 하나 이상의 데이터가 필요한데 쿠키는 아예 값이 없는 빈 쿠키도 허용하기는 해서 이런 차이점을 좀 명확하게 표현하기 위해서는 감싸는게 더 잘 보이겠다 싶더라고요 ㅎㅎ
꺼내오는 메서드도 이름을 조금 더 명확하게 할 수 있었고...
근데 이건 개인 차이라 아무튼 이오가 선택하신 근거가 충분히 납득이 되어서 좋았습니다? 네
Processor
확실히 담당하고 있는 일이 많기는 한 것 같습니다
근데 말씀해주신 대로 Step3에서 개선될 내용이라 굳이 신경쓸 필요가 있을까? 싶기는 하네요...
결론만 말씀드리면 Step2에서는 이오가 해주신 것처럼 메서드 분리만으로 충분하다고 생각합니다!
여기서 클래스로 분리하게 되면 Step3의 내용을 침범하는 것이라고 생각합니다
하면 좋지만 굳이? 느낌...

apptie · 2023-09-04T08:42:28Z

tomcat/src/main/java/nextstep/jwp/service/UserService.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class UserService {


UserService 👍

전 이 생각을 못하고 그냥 InMemoryUserRepository를 가져다 썼는데 좋은 접근 방법인 것 같습니다

apptie · 2023-09-04T08:44:13Z

tomcat/src/main/java/nextstep/jwp/service/UserService.java

+        final Optional<User> user = InMemoryUserRepository.findByAccount(account);
+
+        if (user.isEmpty()) {
+            throw new UnauthorizedException(account);
+        }
+        if (!user.get().checkPassword(password)) {
+            throw new UnauthorizedException(account);
+        }


해당 코드의 경우
Optional<User>가 아니라 User로 타입을 변경하고, null을 허용한다고 가정한다면
지금 로직과 유사하거나, Optional보다 비용이 덜 들게 될 것 같습니다(Optional을 생성하는 비용이 없음)

조금 더 Optional을 활용할 수 있는 코드로 리펙토링 해 보시는건 어떨까요?

refactor(UserService): Optional 사용 방식 변경

Optional을 잘 활용하지 못하고 있었던것 같네요, 감사합니다! 😀

apptie · 2023-09-04T08:48:24Z

tomcat/src/main/java/nextstep/jwp/service/UserService.java

+        final String password = requestBody.get("password");
+        final String email = requestBody.get("email");
+
+        if (InMemoryUserRepository.findByAccount(account).isPresent()) {


Optional.ifPresent()를 활용해보시는건 어떠실까요?

refactor(UserService): Optional 사용 방식 변경

동일하게 반영하였습니다~

apptie · 2023-09-04T09:28:06Z

tomcat/src/main/java/nextstep/jwp/service/UserService.java

+        if (!user.get().checkPassword(password)) {
+            throw new UnauthorizedException(account);
+        }
+        log.info(user.toString());


Suggested change

log.info(user.toString());

log.info("{}", user);

취향 차이기는 하지만 이런 방법도 있습니다

refactor(all): 코드 리팩토링
code smell 없는 코드 좋네요..👍

apptie · 2023-09-04T09:42:53Z

tomcat/src/main/java/nextstep/jwp/service/UserService.java

+        return setSessionWithUUID(user.get());
+    }
+
+    private String setSessionWithUUID(final User user) {


해당 메서드를 호출하는 클라이언트인 register()나 logIn() 입장에서 UUID를 기반으로 저장한다는 사실을 알 필요가 있을까요?
이오는 어떻게 생각하시나요?

refactor(UserService): 메소드 네이밍 변경

그렇네요! 미처 놓쳤던 부분이었습니다. 수정했습니다 :)

apptie · 2023-09-04T10:18:15Z

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

+    private String formatHeader(final String h) {
+        return h + ": " + headers.get(h);
+    }


getResponse()에서만 호출되는 private method로 보이는데, 정렬을 통일해주시면 좋을 것 같습니다!

다른 코드를 보니 호출되는 메서드 밑에 위치해주시는 식으로 정렬해주신 것 같더라고요 ㅎㅎ

추가적으로 h가 아마도 header의 key인 것 같은데, 조금 더 명확한 네이밍을 사용해주시는 건 어떨까 건의드려봅니다!

apptie · 2023-09-04T10:22:16Z

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

+                input.append(s);
+                input.append(LINE_SEPARATOR);


Suggested change

input.append(s);

input.append(LINE_SEPARATOR);

input.append(s)

.append(LINE_SEPARATOR);

요런 식으로 체이닝도 가능합니다!

apptie · 2023-09-04T10:22:57Z

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

+        try {
+            bufferedReader.read(buffer, 0, contentLength);
+        } catch (final IOException e) {
+            throw new RuntimeException(e);


Unchekced Exception을 던져주는 것을 의도하신걸까요?

refactor(Http11Processor): 예외 처리 방식 변경

이 부분도 위의 메소드와 동일하게 로그를 남기도록 변경하였습니다!

확인했습니다!

apptie · 2023-09-04T10:24:15Z

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

+            final Http11Response response = new Http11Response(OK, resource);
+            checkContentType(request, response);
+            return response;
+


개행..? 제거..?

apptie · 2023-09-04T10:26:15Z

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

+    private void checkContentType(final Http11Request request, final Http11Response response) {
+        final String accept = request.getHeader("Accept");
+        if (accept != null && accept.contains("css")) {
+            response.addHeader("Content-Type", "text/css;charset=utf-8 ");


Content-Type도 Status나 Method처럼 enum으로 관리하는 방법은 어떠실까요?

Content-Type을 찾는 로직까지 하나로 캡슐화할 수 있을 것 같습니다!

feat(*Header): Header Enum 구현

Header를 enum으로 구현하였습니다. 구현 과정에서 ResponseHeader와 RequestHeader를 합쳐야하나 고민을 많이 했지만 각각 사용되는 헤더의 종류가 달라 구분을 주는 것이 명확하다고 생각하여 따로 두었습니다. 두 enum이 공통된 부분이 많아 상속으로 구현하고 싶었는데 enum은 상속을 적용할 수 없어 아쉽네요..🫠

저도 헤더의 종류도 다르고, 그로 인해 동작도 변경되니(대표적으로 ResponseHeader의 Location) 이오의 판단에 동의합니다!

상수 클래스가 되었든 enum이 되었든 상속과 같은 공통 처리는 저도 딱히 떠오르지는 않네요...그나마 상수 클래스 하나 만들어서 모든 헤더를 명시하고 공통 헤더가 아닌 헤더는 접두사로 REQUEST나 RESPONSE로 구분하는 것 정도?
지저분해서 별로일 것 같기는 하네요..

sonarcloud · 2023-09-06T08:56:38Z

Kudos, SonarCloud Quality Gate passed!

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

apptie

고생하셨습니다!

테스트 코드도 추가되었고, 리펙토링한 내용도 충분히 좋게 작성해주신 것 같아서 소소한 리뷰 몇 가지 남기고 approve를 하도록 하겠습니다

이오가 진행하실 3단계가 기대되네요 ㅎㅎ
화이팅입니다!

apptie · 2023-09-06T09:36:41Z

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

+import java.util.HashMap;
+import java.util.Map;
+
+public class SessionManager {


답변 감사합니다! stateless라고 판단하셔서 이렇게 선택하셨군요
다른 분들도 동일하게 작업하신 것 같더라고요 ㅎㅎ

apptie · 2023-09-06T09:42:49Z

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

+            assertThat(socket.output()).contains("HTTP/1.1 302 Found");
+            assertThat(socket.output()).contains("JSESSIONID");
+            assertThat(socket.output()).contains("Location: /index.html");


SoftAssertions를 사용하신다면, softly와 체이닝을 하지 않으면 위 사진처럼 틀린 내용을 처음 하나밖에 알려주지 않습니다!

또한 assertThat()contains()는 체이닝이 가능해서, 다음과 같이 작성하실 수도 있을 것 같습니다!

apptie · 2023-09-06T09:43:02Z

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

+import org.apache.coyote.http11.Http11Processor;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+import support.StubSocket;

 class Http11ProcessorTest {


테스트 추가 👍

apptie · 2023-09-06T09:47:03Z

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

+    private void checkContentType(final Http11Request request, final Http11Response response) {
+        final String accept = request.getHeader("Accept");
+        if (accept != null && accept.contains("css")) {
+            response.addHeader("Content-Type", "text/css;charset=utf-8 ");


저도 헤더의 종류도 다르고, 그로 인해 동작도 변경되니(대표적으로 ResponseHeader의 Location) 이오의 판단에 동의합니다!

상수 클래스가 되었든 enum이 되었든 상속과 같은 공통 처리는 저도 딱히 떠오르지는 않네요...그나마 상수 클래스 하나 만들어서 모든 헤더를 명시하고 공통 헤더가 아닌 헤더는 접두사로 REQUEST나 RESPONSE로 구분하는 것 정도?
지저분해서 별로일 것 같기는 하네요..

apptie · 2023-09-06T09:47:15Z

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

+        try {
+            bufferedReader.read(buffer, 0, contentLength);
+        } catch (final IOException e) {
+            throw new RuntimeException(e);


확인했습니다!

apptie · 2023-09-06T09:51:04Z

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

+import java.util.List;
+import java.util.Map;
+
+public class Http11Request {


아 그래서 http11 패키지에 두셨군요 제가 의도를 파악하지 못한 것 같습니다 ㅎㅎ;

그런 이유라면 근거로 충분할 것 같습니다! 다만 HTTP 버전도 여러개고, 톰캣에서는 Http11NioProcessor, Http11Nio2Processor등등과 같이 다양하게 제공하는 만큼 Request나 Response도 그만큼 더 늘어날 것 같기는 하지만...거기까지는 필요 없을 것 같아서 이전의 방식도 좋은 것 같습니다!

물론 3단계에서 GET / HTTP/1.1을 처리할 수 있는 객체인 RequestLine을 만들라는 내용이 있어서 변경하신 내용도...괜찮은 것 같기도 하고 그렇습니다

LJW25 added 14 commits September 3, 2023 16:55

test(IOStreamTest): IOStream 학습

00797bc

feat(Http11Request): HttpRequest 객체 생성

5d15f31

feat(Http11Response): HttpResponse 객체 생성

722eaad

feat(Http11Processor): 인덱스 페이지 접근 구현 (미션1-1)

8986f2d

refactor(Http11Processor): NPE 방지 추가

c647bc9

feat(Http11*): css 설정 구현

e038285

feat(Http11*): QueryString 파싱 구현

fe1459f

feat(Http11Response): Response 객체 상태코드 추가

a83ca3e

feat(Http11Processor): 로그인 여부에 따른 페이지 이동 구현

9f0af06

chore(login.html): 로그인 요청 방식 변경

d95d07c

feat(Http11*): POST 구현

a4f5414

feat(Http11*): Cookie 구현

54dbe20

feat(Http11*): Session 구현

63ce5fe

refactor(all): 구조 변경 및 컨벤션 적용

c840b3e

LJW25 requested a review from apptie September 4, 2023 07:54

LJW25 self-assigned this Sep 4, 2023

test(Http11Processor): 테스트코드 추가

6fdde09

apptie requested changes Sep 4, 2023

View reviewed changes

LJW25 added 9 commits September 5, 2023 18:23

refactor(Http11*): 컨벤션 리팩토링

40c75a8

refactor(UserService): Optional 사용 방식 변경

e3c9ece

refactor(UserService): 메소드 네이밍 변경

a35bb6f

refactor(Http*): 클래스 네이밍 변경

5e1f9dd

refactor(Http11Processor): 예외 처리 방식 변경

de99202

refactor(Constant): 상수처리

702c0b7

feat(*Header): Header Enum 구현

a0b819c

refactor(Status): status 메소드 위치 변경

d62ae11

refactor(all): 코드 리팩토링

90b384e

apptie approved these changes Sep 6, 2023

View reviewed changes

apptie merged commit 88bef21 into woowacourse:ljw25 Sep 6, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 1,2단계] 이오(이지우) 미션 제출합니다. #343

[톰캣 구현하기 - 1,2단계] 이오(이지우) 미션 제출합니다. #343

LJW25 commented Sep 4, 2023 •

edited

Loading

sonarcloud bot commented Sep 4, 2023

sonarcloud bot commented Sep 4, 2023

apptie left a comment

apptie Sep 4, 2023

apptie Sep 4, 2023

LJW25 Sep 6, 2023

apptie Sep 4, 2023

LJW25 Sep 6, 2023

apptie Sep 4, 2023

LJW25 Sep 6, 2023

apptie Sep 4, 2023

LJW25 Sep 6, 2023

apptie Sep 4, 2023

apptie Sep 4, 2023

apptie Sep 4, 2023

apptie Sep 4, 2023

LJW25 Sep 6, 2023

apptie Sep 6, 2023

apptie Sep 4, 2023

apptie Sep 4, 2023

LJW25 Sep 6, 2023

apptie Sep 6, 2023

sonarcloud bot commented Sep 6, 2023

apptie left a comment •

edited

Loading

apptie Sep 6, 2023

apptie Sep 6, 2023

apptie Sep 6, 2023

apptie Sep 6, 2023

apptie Sep 6, 2023

apptie Sep 6, 2023

[톰캣 구현하기 - 1,2단계] 이오(이지우) 미션 제출합니다. #343

[톰캣 구현하기 - 1,2단계] 이오(이지우) 미션 제출합니다. #343

Conversation

LJW25 commented Sep 4, 2023 • edited Loading

고려한 부분

sonarcloud bot commented Sep 4, 2023

sonarcloud bot commented Sep 4, 2023

apptie 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

sonarcloud bot commented Sep 6, 2023

apptie left a comment • edited Loading

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

LJW25 commented Sep 4, 2023 •

edited

Loading

apptie left a comment •

edited

Loading