[톰캣 구현하기 3, 4단계] 토리(천은정) 미션 제출합니다. #491
Conversation
- HttpRequest 클래스 구조 분리 및 역할 세분화
- HttpRequestLine
- HttpMethod
- HttpPath
- HttpProtocol
- HttpRequestHeaders
- HttpRequestBody
- HttpProtocol enum 클래스 추가
- HttpResponse 구조 변경 - HttpRequest 객체의 요청을 처리할 FrontController 추가 - Http11Processor 내부 HttpRequest path로 인한 분기 로직 책임 분리
|
Kudos, SonarCloud Quality Gate passed!
|
tjdtls690
left a comment
tjdtls690
left a comment
There was a problem hiding this comment.
토리! 리뷰가 많이 늦어져서 정말 죄송합니다.
각 클래스의 역할에 맞춰서 코드를 분리하고 아키텍처를 설계한 것을 보고 정말 많이 배웠습니다. 제가 보기에 피드백 할 부분이 별로 없었던 것 같아요.
몇 가지 질문과 리뷰 남겼으니 확인해주세요! 정말 수고 많으셨습니다.
| public static HttpRequest from(final BufferedReader bufferedReader) throws IOException { | ||
| final List<String> requestHeader = extractRequestHeader(bufferedReader); | ||
| final HttpRequestStartLine startLine = HttpRequestStartLine.from(requestHeader.get(0)); | ||
| final HttpRequestLine startLine = HttpRequestLine.from(requestHeader.get(0)); |
There was a problem hiding this comment.
첫번째 라인은 보통 Request Line 이라 하더라구요. 그래서 Header에서 Request Line을 꺼내는 모양이 조금 어색해 보였어요.
final String requestLine = bufferedReader.readLine();
final HttpRequestLine startLine = HttpRequestLine.from(requestLine);이런 식으로 첫 줄을 먼저 꺼내서 RequestLine을 만들어 주는 것은 어떨까요? 그 다음에 RequestHeaders를 만드는 식이 될것 같아요.
There was a problem hiding this comment.
저는 extractRequestHeader에서 bufferrReader를 통해 한 번에 리스트를 통해 읽어서 requestLine과 header로 잘라서 보내주었습니다 bufferReader를 여기저기 퍼트려두기 싫었어요 .. ..
일단 startLine이라고 적혀있던 네이밍을 requestLine으로 변경하였습니다 ! (놓치고 변경하지 못한 부분이네요 ..)
리뷰를 보면서 코드를 보니 아벨이 추천해주신 방식이 더 가독성이 좋아보여서 변경해버렸습니다 ˙ᵕ˙ 👍
|
|
||
| private final ServerSocket serverSocket; | ||
| private final ExecutorService executorService; | ||
| private final FrontController frontController; |
There was a problem hiding this comment.
FrontController를 싱글톤 형식으로 관리하기 위한 방법으론 Http11Processor에서 private static final로 가지는 방법도 있었을 것 같은데, Connector의 인스턴스 변수로 가지는 방법을 택한 이유가 무엇인지 궁금합니다 :)
| protected HttpCookie makeHttpCookie(final HttpRequest httpRequest) { | ||
| if (httpRequest.hasCookie()) { | ||
| return HttpCookie.from(httpRequest.getCookie()); | ||
| } | ||
| return HttpCookie.empty(); | ||
| } |
There was a problem hiding this comment.
makeHttpCookie메서드는 현재도 그렇고 앞으로도 로그인과 관련된 로직이 존재하는 LoginController에서만 사용이 될 것 같은데, 이 메서드를 LoginController로 옮기는 것은 어떨까요? 코드를 파악하기에도 좀 더 수월해질 것 같아요!
There was a problem hiding this comment.
여러곳에서 사용될 수 있을 것 같아 AbstractController에 두었는데 로그인이 성공했을 때만 사용되고 있네요 !
LoignController로 옮긴 뒤 접근제어자도 private로 변경하였습니다 ˙ᵕ˙
| import org.apache.coyote.http11.request.HttpRequest; | ||
| import org.apache.coyote.http11.response.HttpResponse; | ||
|
|
||
| public class ResourceController extends AbstractController { |
There was a problem hiding this comment.
ResourceController 컨트롤러를 LoginController, HomeController, RegisterController 이 컨트롤러들과 따로 위치시킨 이유가 궁금합니다 :)
There was a problem hiding this comment.
LoginController, HomeController, RegisterController 는 서비스 로직이라고 생각했고, ResourceController는 톰캣에서 맵핑할 컨트롤러가 존재하지 않을 때 리소스를 출력해주는 역할을 한다고 생각해서 따로 위치시켜두었습니다 !!
아벨의 의견도 궁금합니다 !
2 participants
안녕하세요 아벨 !! 오랜만이에요 ˙ᵕ˙
겨우겨우 제한 시간 전에 리뷰 요청을 드리네요.. 🥲
이번 미션을 진행하며 구조를 갈아엎는 과정에서 오류가 계속 발생해서 커밋을 잘 분리하지 못했어요..
열심히 구조를 나누고 리팩터링을 진행했는데 여러번 다시 하느라 시간이 오래걸렸네요 🥲
리뷰 잘 부탁드립니다 ! 좋은 하루 되세요 🌈