[톰캣 구현하기 - 1,2단계] 베로(김은솔) 미션 제출합니다. #325
Conversation
There was a problem hiding this comment.
안녕하세요 베로~~ 오랜만입니다!
이미 3단계 리팩터링이 되신 것 같은 느낌이 듭니다,,!! 코드를 깔끔하게 작성 + PR 그림을 통해서 잘 이해할 수 있었어요.
아래 내용들은 제가 베로의 코드를 보면서 전체적인 리뷰입니다!
커밋들을 뒤에서부터 봤는데 단위 테스트도 있으면 어땠을까? 라는 조심스러운 생각이 듭니다. 물론 Http11ProcessorTest에서 통합 테스트로 커버할 수 있지만 단위 테스트를 통해 상대방이 더 쉽게 이해할 수 있지 않을까라는 생각입니다~~
정적 팩터리 메서드를 사용할 때, 생성자에 대해 private 해주신게 있고, 아닌게 있어서 한번 쭉 살펴보시는걸 추천합니다!
"도메인에서 I/O에 관한 의존이 꼭 필요한걸까?"에 대해 같이 의견 나눠봐요~
베로의 의견을 듣고 싶어서 Request Changes 남겨볼게요!
|
|
||
| @Override | ||
| public boolean canHandle(final HttpRequest httpRequest) { | ||
| final boolean isPostMethod = httpRequest.getMethod() == HttpMethod.POST; |
There was a problem hiding this comment.
HttpRequest에게 POST 메서드인지 물어볼 수 있을 것 같아요~
There was a problem hiding this comment.
HttpRequest 가 HttpMethod 를 입력 받아 같은 값인지 판별하는 getMethod 를 추가했습니다 👍🏻
| private final String httpVersion; | ||
| private final HttpStatusCode httpStatus; | ||
|
|
||
| public HttpResponseStatusLine(final String httpVersion, final HttpStatusCode httpStatus) { |
There was a problem hiding this comment.
해당 생성자는 사용되지 않는 것 같아요.
혹시 그대로 놔두신 이유가 있으실까요??
There was a problem hiding this comment.
현재는 HttpVersion 이 1.1로 고정되어 있기 때문에 기본값을 넣어두어도 되지만, 이후에 HttpVersion 이 변경될 때는 필요할 생성자라고 생각했습니다!
그렇지만 현재는 1.1 만 사용되고 있으니 없애도 무방할 것 같아 삭제했습니다 😄
| this.httpStatus = httpStatus; | ||
| } | ||
|
|
||
| public HttpResponseStatusLine(final HttpStatusCode httpStatus) { |
There was a problem hiding this comment.
정적 팩터리 메서드로 해당 객체를 생성하는 것 같은데 private 로 막으시는건 어떠신가요?
| @Override | ||
| public boolean preHandle(final HttpRequest httpRequest, final HttpResponse httpResponse) { | ||
| if (httpRequest.containsCookieAndJSessionID()) { | ||
| final Session session = SessionManager.getInstance().findSession(httpRequest.getCookie().getJSessionID()) |
There was a problem hiding this comment.
session 이 사용되지 않고 있는데 혹시 Exception 처리 때문에 사용하신걸까요??
현재에는 별다른 로직이 없어서 사용되지 않는 것 같긴합니다만,,
현재 요구사항에 맞는 메서드를 하나 추가하시는건 어떠실까요?
There was a problem hiding this comment.
Exception 처리 때문에 사용한 게 맞습니다 예리하시군요
그리고 sonar cloud 에서 orElseThrow 를 할 때는 변수를 만들어야 한다고 경고해서 변수를 만들어주도록 했습니다.
우르 말대로 요구사항에 맞는 메서드가 있는 게 좋을 것 같아서 validateSession 이라는 메서드를 추가해서 사용했습니다 !
| this.body = body; | ||
| } | ||
|
|
||
| public HttpResponse(final HttpResponseStatusLine responseLine, final String body) { |
There was a problem hiding this comment.
생성자들이 모두 해당 클래스에서만 사용되는 것 같은데, private 로 막는거 어떻게 생각하시나요?
| return controllers.stream() | ||
| .filter(controller -> controller.canHandle(httpRequest)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new IllegalArgumentException("요청을 처리할 수 없습니다.")); |
There was a problem hiding this comment.
/index.html, / 에 접근했을 때 위와 같은 exception 이 나오는데 혹시 확인 한 번 해주실 수 있으실까요?
There was a problem hiding this comment.
/favicon.ico 관련 exception 인 것 같은데 그렇게 큰 이슈는 아니어서 선택해서 적용해주시면 좋을 것 같습니다~
| httpResponse.addAttributes(responseBody); | ||
| } | ||
|
|
||
| private boolean prehandleInterceptors(final HttpRequest httpRequest, final HttpResponse httpResponse) { |
There was a problem hiding this comment.
저 기능을 하는 메서드 이름 짓기 굉장히 힘들더라고요..ㅎㅎ...
Interceptor 를 통과했는지를 판단하는 메서드라고 생각해서 isPassedThroughInterceptors 로 변경해보았습니다!
| return; | ||
| } | ||
| final Controller controller = findController(httpRequest); | ||
| final ResponseEntity responseBody = controller.service(httpRequest); |
There was a problem hiding this comment.
정말 별거 아니긴한데,, 다른 변수명을 사용해보시는게 어떠실까요?
addAttributes 메서드 내부에서도 responseBody에서 Header를 꺼내는게 조금 어색해보여서요~~
There was a problem hiding this comment.
다시 보니 확실히 어색하긴 하네요
단순하게 responseEntity 로 변경했습니다 ㅎㅎ
| values.put(name, value); | ||
| } | ||
|
|
||
| public void remoteAttribute(final String name) { |
| import org.apache.coyote.http11.HttpRequest; | ||
| import org.apache.coyote.http11.HttpRequestURI; | ||
|
|
||
| public abstract class RestController implements Controller { |
There was a problem hiding this comment.
왜 추상 클래스로 하셨는지 베로의 의견을 여쭤봐도 될까요??
RestController에서 path 가 아닌 특정 인스턴스 필드가 추가되거나,
canHandle 의 메서드가 변경될 수 있다면, 사이드 이펙트들이 모든 Controller에 퍼지기 때문에 나중에 유지보수성이 괜찮을까? 라는 개인적인 생각이 들어요.
이건 수정하기보다는 베로의 의견을 듣고 싶습니다~~~
There was a problem hiding this comment.
RestController 를 추상 클래스로 만든 이유는 path 라는 변수가 모든 Restful 한 컨트롤러에 중복되게 등장할 상황이 예상되었기 때문입니다. (현재 코드에서도 모든 RestController 를 상속한 컨트롤러들은 모두 path 가 필요합니다)
이후에 특정 인스턴스 필드가 추가되는 경우 사이드 이펙트를 최소화하기 위한 조치를 취하겠지만, 추상 클래스의 특성상 사이드 이펙트들이 퍼지는 것은 중복 코드를 줄이기 위한 트레이드 오프라고 생각합니다.
그런데 우르가 이야기해주신 것처럼 canHandle 의 메서드가 변경되었을 때가 치명적일 것 같긴 하네요. 현재 super.canHandle() 을 사용하는 상황이라 사이드 이펙트가 꽤 크리티컬하게 작용할 듯 합니다.
이에 대한 대안으로 RestController 가 canHandle 을 재정의하지 않고, path 가 같음을 확인해주는 기능을 protected method 로 따로 분리하는 방향은 어떻게 생각하시나요?
There was a problem hiding this comment.
베로 말씀이 맞습니다.
중복 코드 줄이기 vs 사이드 이펙트 범위 늘어남 둘 중 하나를 선택하는 트레이드 오프라고 생각해요.
제 생각에는 canHandle 메서드가 변경될 여지가 없어보입니다 ㅋㅋㅋ path 만 같은지 확인하는거니까요?
베로가 제안해주신 방법도 좋아보입니다!! 다만 "굳이 해야할까?"라는 생각도 듭니다.
베로가 생각하시기에는 이 canHandle은 절대 변하지 않을거야 라고 생각이 드시면 안하셔도 무방하다고 생각해요
|
안녕하세요 우르 ! |
|
Kudos, SonarCloud Quality Gate passed!
|
|
저도 미션해보니까 단위 테스트,, 쉽지 않을 것 같아요 리뷰 잘 반영주신 것 같아서 이만 머지하고 다음 단계에서 뵙겠습니다 고생하셨습니다! |
안녕하세요 우르! 오랜만이네요! 다시 만나 반갑습니다 😄
제가 구현한 구조는 아래를 참고해주시면 감사하겠습니다 😆
대략적인 플로우는 다음과 같습니다.
Tomcat내부의Http11Processor가 클라이언트의 요청을 파싱하여HttpRequest를 생성하고, 응답을 위한 기본HttpResponse를 생성합니다.HttpRequest와HttpResponse를DispatcherServlet의service()로 전달합니다.DispatcherServlet내부의Interceptor중, 요청을 처리할 수 있는Interceptor의preHandle()을 실행합니다.preHandle()의 결과값이 false 라면Controller요청을 처리하지 않고 리턴합니다.Controller를 찾고,Controller의service를 호출하여 각 로직을 처리한 후ResponseEntity를 리턴 값으로 받습니다.ResponseEntity내부의 body, header 값들을 전달받은HttpResponse에 추가합니다.HttpResponse를 전달합니다.SessionManager를 호출하는 주체와 요청, 응답을 생성하는 책임에 대해 중점적으로 리뷰해주시면 감사하겠습니다!리뷰 감사합니다 🙇🏻♀️