[톰캣 구현하기 3, 4단계] 키아라(김도희) 미션 제출합니다#487
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
swonny
left a comment
swonny
left a comment
There was a problem hiding this comment.
안녕하세요 키아라!
프로젝트 관련 이슈가 생겨,,, 리뷰가 늦어진 점 죄송합니다ㅠㅠ
3, 4단계 구현하느라 고생 많으셨습니다👍🏻
1, 2단계에서도 충분히 잘 구현하셨는데 3, 4단계를 진행하면서 구조가 더 깔끔해진 게 느껴집니다
궁금한 부분들만 몇 가지 코멘트 남겨두었는데 확인해주세요!
추가로 3단계 요구사항에 해당하는 Controller 인터페이스 포맷은 맞춰보시는 것을 추천드립니다
고생 많으셨습니다! 다음 미션도 화이팅!!!
| if (httpRequest.hasSessionId()) { | ||
| return httpResponse | ||
| .addBaseHeader(httpRequest.getContentType()) | ||
| .redirect("/index.html"); |
There was a problem hiding this comment.
저는 다 setXX() 해줬는데 ㅎ.. 객체를 반환해서 메서드 체이닝으로 하는거 좋네요👏🏻
| } | ||
|
|
||
| private Response register(RequestReader requestReader) throws IOException { | ||
| private static void join(HttpRequest httpRequest) { |
| public interface Controller { | ||
|
|
||
| Response service(RequestReader requestReader) throws IOException; | ||
| HttpResponse service(HttpRequest httpRequest, HttpResponse httpResponse); |
There was a problem hiding this comment.
3차 요구사항에 맞춰 반환 타입을 void로 변경하면 좋을 것 같습니다
Http11Processor.process()에서 HttpResponse 객체를 생성하고, 생성한 response에 setter로 response 필드를 추가해주는 형식으로 변경할 수 있을 것 같아요
| private StatusCode statusCode; | ||
| private String body; | ||
|
|
||
| public HttpResponse() { |
| return new String(chars); | ||
| } | ||
|
|
||
| private int getContentLength() { |
There was a problem hiding this comment.
해당 클래스에 getXX()와 같은 네이밍의 메서드가 많이 보입니다
그런데 실제로 getter의 역할 이상의 로직을 수행하고 있어요
단순 getter의 역할보다 더 많은 역할을 하기 때문에 코드를 읽을 때 메서드의 내부를 한 번 더 확인해야 정확히 어떤 일을 수행하는지 알 수 있다는 점이 아쉬웠습니다!
이건 리팩토링 요구사항은 아니고, 나중에 한번 고려해보시면 좋을 것 같아 코멘트 남깁니다
| } | ||
|
|
||
| public String getContentType() { | ||
| String accept = headers.getOrDefault(ACCEPT.getName(), HTML.getType()); |
There was a problem hiding this comment.
content type같은 고정적인 값들은 enum으로 관리하면 좋을 것 같다는 의견입니다!
enum에서 확장자에 매핑되는 content type을 갖고, getContentType()과 같은 메서드로 확장자와 매핑되는 content type을 불러올 수 있으면 분기문을 줄일 수 있을 것 같아요
그리고 getOrDefault()를 활용해 HTML을 가져오는 부분이 좋다고 생각했는데, 왜 HTML을 default로 가져오게 하셨는지 이유가 궁금합니다!
추가로 RequestHeaders 객체를 만들면 HttpRequest 객체가 조금 더 가벼워지고 책임이 분리될 수 있을 것 같다고 생각하는데 어떻게 생각하시나요?
| } | ||
|
|
||
| return SessionManager.getSessionId(user); | ||
| String sessionId = SessionManager.getSessionId(user); |
There was a problem hiding this comment.
getSessionId()에서 session을 저장한다는 것은 메서드 내부를 확인해야 알 수 있을 것 같아요!
단순히 저장되어 있는 session을 가져오는 getter처럼 느껴집니다
저는 이럴 때 정팩메를 사용해 컬렉션에 저장 + 생성 로직을 추가해줬었는데 키아라 의견도 궁금합니다!
| private static final IndexController INSTANCE = new IndexController(); | ||
|
|
||
| public class IndexController implements Controller { | ||
| private IndexController() { |
| .addBaseHeader(requestReader.getContentType()) | ||
| .createBodyByText("Hello world!"); | ||
| public HttpResponse doPost(HttpRequest httpRequest, HttpResponse httpResponse) { | ||
| return null; |
2 participants
안녕하세요 메리🎄
말씀해주신 request body의 포맷 처리 부분은 RequestBody 객체를 하나 만들어서 Content-Type에 따라 분기처리가 가능할 수 있도록 확장성을 열어두었습니다.
다만 지금 미션에선 application/x-www-form-urlencoded 타입만 다루고 있는 것 같아 다른 타입에 대한 처리는 해주지 않았습니다.
감사합니다.