[톰캣 구현하기 - 3, 4단계] 디노(신종화) 미션 제출합니다. #441
Conversation
| protected HttpResponse doPost(final HttpRequest request) throws IOException { | ||
| return null; | ||
| } | ||
|
|
||
| protected HttpResponse doGet(final HttpRequest request) throws IOException { | ||
| return null; | ||
| } |
| HttpResponse service(final HttpRequest request) throws IOException; | ||
| } |
There was a problem hiding this comment.
이 부분 오늘 구구가 슬랙에 남긴 거긴 한데,
컨트롤러 인터페이스에서 파라미터를 줄이거나 더 추가하지 마시고 그대로 사용하라고 하네요...
저도 사실 HttpRequest만 받고 있어서 이유는 강의에서 들어봐야 할 것 같아요!
일단 언급만 해놓겠습니다!
There was a problem hiding this comment.
넵넵 저도 확인하고 구조 변경했습니다.!
| .filter(it -> requestLine.hasFileExtension(it.getExtension())) | ||
| .findAny() | ||
| .orElse(HTML); | ||
| .orElseThrow(NotAllowedMethodException::new); |
There was a problem hiding this comment.
여기서 Extension을 찾지 못하면 NotAllowedMethodException을 발생시키고 있는데,
왜 해당 예외를 발생시키는 걸까요??
There was a problem hiding this comment.
NotAllowedExtenstionException 만들어 반환하도록 변경했습니다!
| private static final int KEY_INDEX = 0; | ||
| private static final int VALUE_INDEX = 1; | ||
| private static final int SPLIT_LIMIT = 2; |
| @@ -1,17 +1,19 @@ | |||
| package org.apache.coyote.http11; | |||
| package org.apache.catalina; | |||
There was a problem hiding this comment.
sessionManager 어디에서 선언해야 하는가?
이 부분에 대해서 Apache의 coyote, catalina 패키지 중에 어떤 패키지에 둘지 고민하신 것 같아요!
찾아보니 톰캣의 catalina에서 Session 관리를 한다고 하더라구요!
그래서 catalina 패키지가 맞는 것 같습니다!!
실제 코드에서 구구가 만든 Manager를 구현하고 있는데 catalina 패키지에 만들어 놓으셨네요!
| public class DefaultController extends AbstractController { | ||
|
|
||
| @Override | ||
| protected HttpResponse doGet(final HttpRequest request) throws IOException { |
There was a problem hiding this comment.
왜 빈 response를 만든 뒤 Controller 거치며 요소 추가하는 식으로 만들어야 하는가
저는 컨트롤러 로직에 따라서 Response가 달라지기 때문에
컨트롤러 로직 안에서 Response의 요소가 만들어져서 반환된다고 생각했어요!
일단 저의 생각은 이런데, 이런 의미로 질문하신게 아닐까요??
| @@ -0,0 +1,19 @@ | |||
| package nextstep.jwp; | |||
There was a problem hiding this comment.
jwp -> catalina -> coyote 의 의존성 방향에서 어느 클래스를 어느 패키지에 위치시켜야 하는가
이 부분은 현재 어디가 마음에 안 드시는 걸까요??
저는 의존성 방향은 생각조차 못 했는데,, 대단하십니다!
There was a problem hiding this comment.
패키지 간 양방향 의존이 일어나지 않는 방향으로 구현하고 싶었습니다!
|
Kudos, SonarCloud Quality Gate passed!
|
|
안녕하세요 성하~ 전체적으로 reponse 생성 로직과 패키지간 의존성 방향을 중심으로 리팩터링 진행해 봤어요. 성하에게 남긴 질문에 대한 명확한 해답은 마지막까지 잘 부탁드려요!!🙇🙇 |
There was a problem hiding this comment.
안녕하세요! 디노!
미션 요구 사항 충분히 만족하신 것 같아서 이만 머지하도록 하겠습니다!
이전 피드백에서 제안 드린 부분 잘해주셨는데,
제 의도와는 조금 다른 부분이 있었어서 제가 의도했던 부분을 코멘트에 남겼습니다!! 🙇🏻♂️
이번 톰캣 미션 디노 코드 리뷰하면서 저보다 꼼꼼하게 생각하고 작업하신게 느껴져서
많이 배웠습니다!! 👍🏻👍🏻
특히 저는 생각 안 했던 의존성 부분을 고려하시는 부분이 인상적이었어요!!!
디노 코드 리뷰해서 영광이었습니다 ! 🙇🏻♂️
| doPost(request, response); | ||
| return; | ||
| } | ||
| throw new UnsupportedOperationException(); |
There was a problem hiding this comment.
제가 제안드린 방향을 고려해서 해주신 것 같아요!! 👍🏻
다만 실제 동작은 Mapping된 컨트롤러에서 지원하지 않는 메소드면
405 method not allowed가 응답되기 때문에 405가 뜨도록 리팩토링해보는 것을 제안했던 것이었습니다!!
그래도 잘 고려해주셔서 감사합니다 ㅎㅎ
미션이니 이 부분은 언급만 하고 머지하도록 하겠습니다!! 👍🏻👍🏻👍🏻
| protected void setResponse( | ||
| final HttpResponse response, | ||
| final String resource, | ||
| final HttpStatusCode statusCode | ||
| ) throws IOException { | ||
| final ResponseBody responseBody = FileExtractor.extractFile(resource); | ||
| final ResponseHeader responseHeader = ResponseHeader.from(responseBody); | ||
|
|
||
| response.setStatusLine(new StatusLine(statusCode)); | ||
| response.setResponseHeader(responseHeader); | ||
| response.setResponseBody(responseBody); | ||
| } |
There was a problem hiding this comment.
setResponse로 이렇게 Response를 설정해주고 Processor에서 Response를 사용하는 군요!
배워갑니다,, 👍🏻👍🏻👍🏻
안녕하세요 성하~ 디노입니다!🦖
개인적으로도 맘에 들지 않아 조금 더 고민하고 있는 부분이 꽤 있는데,
혼자서 고민하는 것 보다 성하의 리뷰를 받아 보고 리팩터링을 진행하는 쪽이 조금 더 효과적일 것 같아
우선 리뷰 요청 보냅니다.!
고민되는 부분을 간단히 정리하자면
이번 리뷰도 다시 한 번 잘 부탁드립니다!!🙇🙇