[톰캣 구현하기 - 1,2단계] 도이(유도영) 미션 제출합니다. #328
Conversation
- 로그인 시 응답객체는 수정 필요함
- 모든 요청마다 세션 값이 없으면 생성하는 대신 - 로그인 요청 시에만 Session을 생성하도록 수정함
- 인터페이스 적용으로 인해 사용하지 않는 메서드가 너무 많아짐
|
Kudos, SonarCloud Quality Gate passed!
|
| this.body = body; | ||
| } | ||
|
|
||
| public static Request from( |
There was a problem hiding this comment.
앗 여기는 파라미터가 여러갠데, from을 사용해주셨군요.
혹시 of로 바꿔보는 건 어떨까요?
|
|
||
| @Override | ||
| public String toString() { | ||
| return "HTTP/1.1 " + status.getCode() + " " + status.name() + LINE_SEPARATOR |
There was a problem hiding this comment.
Http 응답값을 빌드하는 것을 toString으로 해주셨군요.
HTTP의 응답 같은 경우에는 항상 고정되어 있어서, toString을 사용해도 괜찮은 것 같네요.
처음에, toString으로 응답을 구성한 걸보고 코멘트를 남길라 했는데, 생각해보니 괜찮은 것 같아요.
디버깅용으로 볼 때도, HTTP 응답형식이 더 읽기 편할 것 같기도 하고요.
응답값을 구성할 때 개행을 +LINE_SEPARTOR 로 문자열을 이어줬는데 String.join을 사용해보는 건 어떨까요?
There was a problem hiding this comment.
Content-Type도 headers에서 관리해보면 어떨까요?(혹시 테스트 때문에 이렇게 하신건가요?)
There was a problem hiding this comment.
Content-Type도 headers에서 관리해보면 어떨까요?(혹시 테스트 때문에 이렇게 하신건가요?)
Content-Type, Content-Length는 대체로 필수적인 요소이기 때문에 다른 헤더와는 구분해두어도 좋다고 생각했습니다.
하지만 그렇게 하니 헤더가 아닌 것처럼 보여서 추후 헤더에 대해 일괄적인 관리가 필요할 때 문제가 생길 수 있을 것 같아요.
또 필수적이라면 오히려 더 Headers에 넣고서 검증 로직을 추가하는 게 나을 것 같네용
반영하겠습니다!!
There was a problem hiding this comment.
toString의 사용 고민스럽기는 합니다.. ㅎㅎ
맞아요 디버깅할 때 응답 형식인 건 편했어요. 하지만 body를 전부 출력하니 html에 대한 응답을 볼 때는 반대로 불편하더라구요. (콘솔에 찍히는 내용이 너무 많아서)
좀 더 고민해볼게요~
String.join 반영하도록 하겠습니다 !
| } | ||
|
|
||
| public boolean isNew() { | ||
| return !SESSION_MANGER.contains(this.id); |
There was a problem hiding this comment.
저장하는 대상인 Session에서 SessionManager에 대해 접근할 수 있는 부분은 조금 위험할 수도 있는 것 같아요.
Session은 도메인 객체인데, SessionManager라는 영속화 계층을 참조하고 영속화 계층이 맡는 역할을 일부 담당하고 있네요.
이 Session이 실제 저장되어 있는 Session인지는 SessionManager의 책임으로만 두는건 어떨까요?
There was a problem hiding this comment.
헉 너무 좋습니다 !! 구현에 급급해 미처 생각하지 못한 부분을 찾아주셔서 감사합니다ㅎㅎ 반영하겠습니다
| } | ||
|
|
||
| public void add(String name, String value) { | ||
| values.put(name, value); |
There was a problem hiding this comment.
밑에 별도의 key를 세팅해주는 메서드를 분리했으니, 이 메서드는 없애보는 건 어떨까요?
일급컬렉션으로서의 장점을 더 가져갈 수도 있을 것 같아요.
There was a problem hiding this comment.
홍실의 말씀에 동감합니다. 커스텀 헤더로도 사용할 수 있으려면 key를 직접 입력할 수 있어야 한다고 생각해 이 메서드를 정의했지만, 지금으로서는 일급 컬렉션의 장점이 더 중요한 것 같네요! 반영하겠습니다 !!
tomcat/src/test/java/nextstep/org/apache/coyote/http11/RequestHandlerTest.java
Show resolved
Hide resolved
안녕하세요 홍실~!
리팩터링 및 테스트 코드 보완은 리뷰 받으며, 또 과정 진행하며(3단계가 리팩터링이네요!) 추가로 더 진행해야 할 것 같아요.
현재 주요 클래스에 대해 설명드리자면 아래와 같습니다.
Tomcat의 DispatcherServlet 구조를 참고하며 구현해보려고 했는데 이에 대해 아직 정확히 이해를 하고 있지 않은지 네이밍이 모호한 것 같네요.
네이밍도 리팩터링하며 개선해보도록 하겠습니다!
RequestHandler:Http11Processer에서 받은 요청에 대해HandlerAdaptor를 통해 적절한 응답을 반환해줍니다.HandlerAdaptor: 매핑된HandlerMethod목록을 가지고 있으며, 요청 객체를 받아 적절한HandlerMethod를 호출합니다.HandlerMapping: 요청 메시지의HTTP 메서드와 경로를 가지고 있는 객체로 요청 정보와HandlerMethod를 매핑할 때 key값으로 사용합니다.HandlerMethod: Request 객체를 받아 Response 객체를 반환하는 함수형 인터페이스입니다.요청에 대해 핸들링해주는 부분이 복잡하게 되어있고, 적절히 추상화하지 못해서, 그 부분을 위주로 수정할 것 같아요.
부족함이 많아서 읽기 불편하실 것 같아 걱정되지만 그래도 잘 부탁드립니다 !! 😅