[톰캣 구현하기 3, 4단계] 애쉬(정설희) 미션 제출합니다. #492
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
안녕하세요 애쉬 ❄️
이번에도 좋은 코드 잘 봤습니다 ㅎ.ㅎ !! 리뷰이께 많이 배워가는 것 같아요.
기존의 좋은 설계에 이어서, 테스트 코드 또한 깔끔하고 꼼꼼하셔서 인상적이었습니다.
저도 도움이 되는 리뷰어가 되어드리고자...! (그랬으면 좋겠네요)
이번에는 코드 레벨에서의 제안을 많이 드리는 데에 초점을 맞춰 리뷰를 진행했습니다.
하지만 어디까지나 개인적인 의견이니 가볍게 보고 넘어가주셔도 좋습니다 ㅎㅎ
(참고로 코멘트 중 질문을 남긴 부분은 🙋♀️ 이모지를 붙여 놓았어용)
요구사항 충분히 만족하고 잘 진행하셨다고 생각하기에 approve merge하도록 하겠습니다.
다음 미션도 화이팅하세요 💪
| public boolean isCommitted() { | ||
| return statusLine != null; | ||
| } |
There was a problem hiding this comment.
처음에는 필드에 null을 넣는 생성자를 두는 게 의문이었는데,
이 메서드를 통해, 완성된 상태인지 여부를 관리하는군요! 👍👍
| public static ResponseHeader create() { | ||
| return new ResponseHeader(); | ||
| } | ||
|
|
There was a problem hiding this comment.
RequestHeader의 empty() 메서드와 같은 의도로, 빈 생성자를 정적 팩터리 메서드로 정의했다고 이해했는데, 맞을까요? create이라는 단어만 봤을 때에는 그 의도가 조금 헷갈렸어요!
commit과 연관된 단어로 init과 같이 좀 더 의도가 드러나는 네이밍은 어떨까요? 😀
|
|
||
| import org.apache.coyote.Cookie; | ||
|
|
||
| public class RequestHeader { | ||
|
|
||
| private final Map<String, String> headers = new HashMap<>(); | ||
| private final List<Cookie> cookies; | ||
| private final List<Cookie> cookies = new ArrayList<>(); |
There was a problem hiding this comment.
🙋♀️
지난 단계 리뷰에서는 주요 사항이 아니라고 생각해 다루지 않았는데요,
Cookie도 헤더 중 하나인데 따로 관리하신 이유가 있을까요?
현재 미션 요구사항에 따르면 Request의 헤더에는 값을 새로 저장할 일이 없으니,
굳이 한 Depth를 더 두기보다는(header->cookies->cookie)
코드 레벨의 조회 로직을 간단히 처리하는 쪽에 더 집중하신 것으로 이해했는데 맞을까요?!
혹시 다른 이유도 있을까 궁금해서 질문드립니다!!
| public Optional<String> findCookie(final String key) { | ||
| return cookies.stream() | ||
| .filter(cookie -> Objects.equals(key, cookie.getKey())) | ||
| .findAny(); | ||
| if (optionalCookie.isEmpty()) { | ||
| return null; | ||
| } | ||
| return optionalCookie.get().getValue(); | ||
| .findAny() | ||
| .map(Cookie::getValue); | ||
| } |
|
|
||
| public class Http11RequestReader { | ||
|
|
||
| private static final String END_OF_LINE = ""; |
| class LoginRequestHandlerTest { | ||
|
|
||
| @Nested | ||
| class get요청 { |
There was a problem hiding this comment.
@Nested 로 테스트 케이스를 적절하게 나눠주신 것 같아요 ~~ 가독성 굿
| import org.apache.coyote.Cookie; | ||
| import org.apache.coyote.MimeType; | ||
| import org.apache.coyote.request.Request; | ||
| import org.apache.coyote.response.Response; | ||
| import org.apache.coyote.response.StatusCode; |
There was a problem hiding this comment.
지금도 너무 너무 좋지만, jwp <-> coyote 간에 이런 작은(?) 의존성들도 없애고 싶다면
catalina 패키지에 서블릿 전용 Request, Response를 따로 정의하는 건 어떨까요 !
| private Optional<String> findAccountInSession(final Request request) { | ||
| final var sessionId = request.findSessionId(); | ||
| if (sessionId.isEmpty()) { | ||
| return Optional.empty(); | ||
| } | ||
| return SessionManager.findById(sessionId) != null; | ||
|
|
||
| final var session = SessionManager.findById(sessionId.get()); | ||
| if (session == null) { | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| final var account = session.getAttribute("account"); | ||
| if (account == null) { | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| return Optional.of(account); |
There was a problem hiding this comment.
private Optional<String> findAccountInSession(final Request request) {
return request.findSessionId()
.map(SessionManager::findById)
.map(found -> found.getAttribute("account"));
}이렇게 줄여보는 건 어떨까요?!
| class Http11RequestHeaderParserTest { | ||
|
|
||
| @Test | ||
| void RequestHeader_파싱() { |
| class Http11ResponseWriterTest { | ||
|
|
||
| @Test | ||
| void statusLine이_null일_때_응답으로_바꾸려고하면_예외_발생() { |
안녕하세요 도이!
미션 요구사항을 지키는데 집중을 했던 미션이었습니다!
이번에도 좋은 리뷰 부탁드립니다