[1-2단계 - Tomcat 구현하기] 배키(백은희) 미션 제출합니다 #550

ehBeak · 2024-09-06T08:22:26Z

안녕하세요. 알파카 🦙🦙
리뷰이 배키입니다 :)
많이 부족한 코드이지만, 잘 부탁드립니다~~

편하게 리뷰 부탁드립니다. 그리고 이해가 안 가는 부분이 있다면 언제든지 dm 주세요 😊

slimsha2dy

안녕하세요 배킈! 이번 미션의 리뷰를 맡게 된 알파카입니다 ㅎㅎ
금요일부터 갑자기 허리 디스크 통증이 악화되고 몸살도 겹쳐서 계속 병상에 누워 있던지라 리뷰가 많이 늦어진 점 사죄의 말씀 드립니다 🙇
전체적으로 구조가 너무 깔끔하다는 생각이 드는 코드였습니다.
제가 리뷰하는 입장이었지만 저는 고려하지 못한 점들이 눈에 많이 띄어서 아주 많이 배워갈 수 있던 시간이었습니다 👍
추상화와 상속을 적극 활용해서 코드의 반복을 줄이고 재사용성을 높인 부분이 아주 인상 깊었습니다.
그래서 리뷰는 대부분 좀 미시적이고 사소한 부분 위주로 달게 된 것 같네요..ㅎㅎ
어디까지나 의견일 뿐이니 참고해보시고 배킈의 의견도 적극 남겨주시면 감사하겠습니다!

slimsha2dy · 2024-09-08T16:25:58Z

tomcat/src/main/java/org/apache/catalina/mapper/Mapper.java

+    }
+
+    public Servlet getServlet(String url) {
+        Pattern key = values.keySet().stream()


key의 패턴들을 순회하며 서블릿에 매핑해 주는 방식 꽤나 괜찮네요!
몰래 제 코드에도 적용하고 싶네요 ㅎㅎ

사소한 부분이긴한데 keySet 대신 entrySet을 사용하면 굳이 한번 더 map에서 get을 호출할 필요 없이 바로 찾은 서블릿을 반환할 수 있을 것 같은데, 스트림 내부에서 키를 다시 호출해야 해서 이렇게 구현하신 걸까요?

딱히 이유는 없었고, value를 가져오려면 key 값을 알아야 한다. -> key 값을 필터링 해야한다. -> get을 통해서 값을 반환한다. 이런 사고흐름으로 코드를 작성했던 것 같아요😅 알파카 의견대로 다시 get을 호출하는 것보다 stream내에서 바로 반환하는 것이 좋겠네요! 반영했습니다 :)

slimsha2dy · 2024-09-08T16:34:48Z

tomcat/src/main/java/org/apache/catalina/servlets/DefaultServlet.java

+    @Override
+    public void service(HttpRequest request, HttpResponse response) throws IOException {
+
+        URL resource = getClass().getClassLoader().getResource("static" + request.getRequestURI());


레벨 1, 2 미션들과 저희 팀 프로젝트에선 가독성을 위한 컨벤션으로 여러 메서드의 연속 호출에 개행을 넣었는데, 평소 배키는 이 경우엔 따로 개행을 넣어주지 않는 편인가요?

stream이 아니고 120자를 넘지 않는 한, 개행을 따로 하지 않았던 것 같아요!
(대부분 메서드 체이닝에 개행을 하는 경우는 stream을 사용하는 경우라, 위와 같은 경우에는 어떻게 할지는 모르겠네요..하하)
개인적으로, 개행을 하면 오히려 길어지는 느낌이라.. 개행을 하지 않았습니다😊

slimsha2dy · 2024-09-08T16:40:27Z

tomcat/src/main/java/org/apache/catalina/servlets/DefaultServlet.java

+                return "Hello world!";
+            }
+        }
+        return "";


resourceURL이 null일 경우 빈 문자열을 반환하는 것으로 보이는 코드네요
처음 if에서 null일 경우 early return으로 빈 문자열을 반환하고 if 블럭 안의 코드를 밑으로 빼서 실행하도록 하면 들여쓰기 깊이도 줄고 가독성도 더 좋아질 것 같은데 어떠신가요?

오 그렇네요.. 코드가 훨씬 깔끔해 지겠네요 👍

slimsha2dy · 2024-09-08T16:44:05Z

tomcat/src/main/java/org/apache/catalina/servlets/HttpServlet.java

+public abstract class HttpServlet implements Servlet {
+
+    @Override
+    public void service(HttpRequest request, HttpResponse response) throws IOException {


get과 post 요청 분리를 추상 클래스에서 하셨군요 깔끔하네용 👍

slimsha2dy · 2024-09-08T16:47:55Z

tomcat/src/main/java/org/apache/catalina/servlets/http/response/HttpResponse.java

+    private static final String START_LiNE_FORMAT = "%s %s %s ";
+    private static final String HEADER_FORMAT_TEMPLATE = "%s: %s \r\n";
+
+    private HttpStatus httpStatus = HttpStatus.OK;


생성자가 아닌 선언부에서 변수 초기화를 하신 이유가 있으실까요?

HttpResponse의 START_LINE_FORMAT과 HEADER_FORMAT_TEMPLATE는 어떤 객체에서든 같은 값을 가지기 때문입니다!

slimsha2dy · 2024-09-08T17:02:55Z

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

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


loginServlet의 log는 static final임에도 변수명이 소문자였는데 따로 기준이 있으신 걸까요?

앗 그 부분을 신경쓰지 못했네요😅 log와 session 부분은 lms의 예시를 그대로 가져오다 보니, 그렇게 된 것 같습니다.
통일이 필요할 것 같아서, 소문자로 통일했습니다! 소문자로 통일한 이유는 상수값이지만 객체의 인스턴스를 참조하기 때문입니다.

slimsha2dy · 2024-09-08T17:06:25Z

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

+
+    private static SessionManager instance;
+
+    public static SessionManager getInstance() {


세션매니저만 따로 싱글톤으로 관리하는 이유가 궁금합니다!

세션 매니저는 세션을 관리하는 객체로, session 맵을 관리합니다. 그래서 딱 하나만 있어도 된다고 생각했습니다~

Servlet들도 지금 한 객체만 사용 중인 것으로 아는데 싱글톤이 아닌 것으로 보이네용
따로 기준이 있으신 걸까요?

앗 기존의 servlet 들도 싱글톤으로 만들고자 했는데, 아직 만들지 못했네요😅
다음 단계에 적용해 보도록 하겠습니다~

slimsha2dy · 2024-09-08T17:11:49Z

tomcat/src/main/java/org/apache/catalina/servlets/http/Cookies.java

@@ -0,0 +1,8 @@
+package org.apache.catalina.servlets.http;
+
+public class Cookies {


이 클래스의 분리 이유가 궁금하네요. 왜 아래의 메서드만 Cookie에서 분리하셨을까요?
그리고 제가 느끼기엔 정적 팩토리 메서드 분리용 클래스로 보이는데 클래스 이름이 Cookies인 이유도 궁금합니닷!

앗.. 이것 또한 lms에서 가져온 예시라.. 가볍게 생각했던 것 같네요.

알파카가 말씀하신 대로 저도 정적 팩토리 메서드 분리용 클래스 정도로 이해했습니다. 현재는 사실 분리할 필요가 없지만, 이후에 다른 sessionId나 다른 쿠키의 옵션등이 필요할 때, 이곳에서 팩토리 메서드를 정의해야겠다고 생각했습니다.

Cookies라고 적으니, 일급컬렉션처럼 느껴질 수도 있겠네요! 그 부분은 따로 생각을 못했어요😅

일단 클래스의 역할이 아직 모호해서 지금은 없애는 것이 좋다는 판단이 들었습니다~

저도 저런 네이밍을 많이 봤던 것 같아서 궁금해 찾아봤는데요.
뒤에 s를 붙이는 건 유틸리티 또는 팩토리 메서드를 보유하는 네임스페이스의 뜻으로 사용된다고 하고 실제로 Java 7 api 디자이너 사이에서 유행이었다고 하네요..ㅎㅎ
근데 저는 배키 말씀처럼 일급컬렉션처럼 느껴지기도 해서 그렇게 와닿지 않는 네이밍이라는 생각이 들긴 합니다 😆

오.. 그렇군요..! 덕분에 새로운 걸 알아가네요~!👍

slimsha2dy · 2024-09-08T17:13:52Z

tomcat/src/main/java/org/apache/catalina/servlets/RegisterServlet.java

+    }
+
+    private String getFileContent(URL resourceURL) {
+        if (resourceURL != null) {


slimsha2dy · 2024-09-08T17:19:32Z

tomcat/src/main/java/org/apache/catalina/servlets/Servlet.java

+import org.apache.catalina.servlets.http.response.HttpResponse;
+
+public interface Servlet {
+    void service(HttpRequest request, HttpResponse response) throws IOException;


service 메서드가 HttpResponse를 반환하는 대신 인자로 받은 HttpResponse의 필드를 setter로 변경하는 방식이네요. 익숙하지 않은 구조인데 혹시 따로 이유가 있으신가요?

미션을 하면서 톰캣의 구조를 많이 참고했어요. 그런데 저도 그 부분에 대해서 의문이더라고요..!

그래서 그 이유에 대해서 찾아보고 고민해 봤습니다.
요청과 응답은 하나의 객체로 관리되는 것이 좋습니다. 만약 servlet에서 httpResponse을 생성한다면, 이후에 filter와 같이 응답을 가로채는 경우, 응답을 관리하기 어려울 것 같습니다. 응답을 servlet에서 관리하니까요...!

가장 큰 이유는, 클라이언트와 서버가 요청과 응답을 주고받는 동안, 요청과 응답은 하나로 관리 되어야 한다고 생각합니다. 그러면 주고받는 것보다 하나의 객체를 processor에서 생성하고 이를 사용하는 것이 관리가 편할 것이라 생각합니다.

위의 내용이 가장 와닿는 내용이었던 것 같아요.

그 외에 딱 와닿는 내용은 없어서.. 혹시 알파카가 생각하신 부분이 있다면 공유 부탁드려요 🙇‍♀️

저도 배키가 말씀하신 게 큰 이유라는 생각이 드네요.
제 의견이 있어서 말씀드린 건 아니고 기존 톰캣의 구조를 몰랐어서 질문드린 거였는데 잘 설명해주셔서 이해가 잘 된 것 같습니다 🙇

ehBeak added 27 commits September 6, 2024 16:41

docs(README): 1단계 기능 구현 목록 작성

39098a4

feat(Http11Processor): 인덱스 페이지에 접근한다.

ee386cb

refactor(HttpResponse): 헤더 필더 추가

833f01d

refactor(HttpResponse): MIME 타입을 지정하여 css 지원

4ff2543

refactor(HttpResponse): MIME 타입을 지정하여 css 지원

4b05d65

feat(HttpRequest): query param을 반환하도록 구현

c85cc29

refactor(DefaultServlet): 정적 파일의 관리하는 DefaultServlet 구현

0a0064f

feat(LoginServlet): 로그인을 관리하는 LoginServlet 구현

ff522bc

feat(HttpRequest): 스트링 쿼리 파싱로직 구현

2162c76

docs(README): 2단계 1번, 2번 기능 목록 추가

c95fc17

fix(LoginServlet): /login 로 접속할 수 있도록 수정

2015cd6

feat(LoginServlet): 로그인 성공시 302 반환

a34a3d5

feat(LoginServlet): 로그인 성공시 '/index.html'로 리다이렉트

d0372bb

feat(LoginServlet): 로그인 실패시 401.html로 리다이렉트

0f4f060

feat(RegisterServlet): 회원가입시 register.html로 이동

deb85b7

feat(RegisterServlet): 회원가입 기능 구현

768f3b1

fix(RegisterServlet): 회원가입 기능 저장 오류 수정

4900299

refactor(Request): 로그인 방식 post로 변경

5c524d6

docs(README): 2단계 기능 구현 목록 추가

8e51552

refactor(Servlet): javax.servlet 관련 관련 코드 제거

411ebca

feat(LoginServlet): 쿠키 구현

4c731fe

refactor: core 패키지 삭제 및 클래스 이동

25d5bff

feat(Session): 쿠키를 통해 로그인 여부를 확인하고 로그인 했다면 index.html로 리다이렉트

d49b846

refactor(Mapper): Mapper 클래스 싱글톤으로 변경 및 예외 처리

0291080

remove: 주석 삭제

34d907e

test(study): 학습 테스트

d461a3c

test(cache): 학습 테스트

e3a70d0

ehBeak self-assigned this Sep 6, 2024

ehBeak requested a review from slimsha2dy September 6, 2024 09:25

slimsha2dy requested changes Sep 8, 2024

View reviewed changes

ehBeak added 9 commits September 9, 2024 14:26

refactor: keySet대신 entrySet 사용

148c506

refactor: early return 을 사용해서 depth 줄이기

51f1206

refactor(HttpResponse): HttpStatus 사용

ed97a88

refactor: 매개변수명 구체적으로 수정

e520643

refactor: getOrDefault 사용

0d4df8b

refactor: 변수 대소문자 통일

f8ade7c

refactor: early return 을 사용하여 depth 줄이기

ffebc47

refactor: Cookie 클래스 삭제

b8da4a1

fix: Cookie 클래스 삭제

365f490

slimsha2dy merged commit de17267 into woowacourse:ehbeak Sep 10, 2024


		public class SessionManager implements Manager {

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


		private static SessionManager instance;

		public static SessionManager getInstance() {

		@@ -0,0 +1,8 @@
		package org.apache.catalina.servlets.http;

		public class Cookies {

[1-2단계 - Tomcat 구현하기] 배키(백은희) 미션 제출합니다 #550

[1-2단계 - Tomcat 구현하기] 배키(백은희) 미션 제출합니다 #550

Uh oh!

Conversation

ehBeak commented Sep 6, 2024

Uh oh!

slimsha2dy 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!

slimsha2dy Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

slimsha2dy Sep 10, 2024 •

edited

Loading