[Tomcat 구현하기 1 - 2단계] 리건(이한길) 미션 제출합니다.#554
[Tomcat 구현하기 1 - 2단계] 리건(이한길) 미션 제출합니다.#554lilychoibb merged 12 commits intowoowacourse:hangilleefrom
Conversation
| threads: | ||
| min-spare: 2 | ||
| max: 2 | ||
| min-spare: 2 |
There was a problem hiding this comment.
min-spare 이 두 줄이라 컴파일 에러가 나서 GreetingControllerTest 가 안 돌아가네요
확인하고 수정해주세요~
There was a problem hiding this comment.
Confilct 해결하다가 놓쳤네요! 수정하겠습니다~
| final var outputStream = connection.getOutputStream()) { | ||
|
|
||
| final var responseBody = "Hello world!"; | ||
| final BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream, "UTF-8")); |
There was a problem hiding this comment.
인코딩 명시를 해주셨군요 👍
StandardCharsets.UTF_8 상수로 적어줘도 좋을 것 같네요!
|
|
||
| public String getRequestPath() { | ||
| return requestLine.split(" ")[1]; | ||
| } |
There was a problem hiding this comment.
RequestLine 을 객체로 분리해줘도 좋을 것 같아요!
| return Arrays.stream(params) | ||
| .map(param -> param.split("=")) | ||
| .collect(Collectors.toMap(token -> token[0], token -> token[1])); | ||
| } |
There was a problem hiding this comment.
HttpCookie 의 코드와 중복되는 감이 있는데, 해당 작업을 HttpRequest 에서 하는 이유가 있을까요?
There was a problem hiding this comment.
두 객체에서 모두 body로 전달 된 query string을 파싱하다보니 중복된 것 같습니다.
BodyParser 같은 객체로 분리해보겠습니다!
| login(outputStream, request); | ||
| } | ||
| if ("/register".equals(url) && "GET".equals(method)) { | ||
| buildHtmlResponse(outputStream, url + ".html"); |
There was a problem hiding this comment.
요청한 리소스 확장자가 없을 경우에 확장자를 붙여주는 로직을 분리하는 건 어떻게 생각하시나요?
(해당 if 문들은 3단계에서 매핑 객체로 분리하면 사라질 것 같아서 따로 리뷰 남기지 않겠습니다 🤗)
There was a problem hiding this comment.
저도 매번 확장자를 붙여주는게 상당히 중복된다고 생각했습니다. 분리해보겠습니다.
| return; | ||
| } | ||
|
|
||
| final User user = InMemoryUserRepository.findByAccount(params.get("account")).get(); |
There was a problem hiding this comment.
findByAccount 가 2번 되는 것 같은데, 변수를 위로 빼서 쿼리가 한번만 되게 해도 될 것 같아요! (지금은 실제로 쿼리는 안 날라가지만)
There was a problem hiding this comment.
너무 가볍게 생각했던 것 같습니다. ifPresent() 같은 메소드를 활용해 개선하겠습니다.
|
|
||
| public String getCookieValue(final String key) { | ||
| return cookies.get(key); | ||
| } |
There was a problem hiding this comment.
HttpCookie 에서 JSessionId 를 직접 꺼내주는 건 어떻게 생각하시나요?
There was a problem hiding this comment.
외부에서 JSESSIONID가 아닌 쿠키를 필요로 할 때 재활용 할 수 있도록 이렇게 구현했습니다. 릴리는 어떻게 생각하시나요? 어느 쪽이 더 좋을까요?
There was a problem hiding this comment.
그게 더 나을 것 같아요ㅎㅎ JSessionId를 다른 곳에서 꺼내쓰는 듯한 느낌이 들었던 것 같은데 착각했나봅니다😅
| "Location: " + location + " ", | ||
| "" | ||
| ).getBytes(); | ||
| } |
There was a problem hiding this comment.
response 도 필드를 두면 중복 코드가 많이 사라질 것 같아요!
| } | ||
|
|
||
| private SessionManager() { | ||
| } |
There was a problem hiding this comment.
혹시 SessionManager 를 싱글톤으로 두지 않고 static 메서드로 사용하시는 이유가 있을까요?
There was a problem hiding this comment.
현재 세션 매니저의 역할을 생각해봤을 때, 싱글톤으로 관리할 필요를 느끼지 못했습니다. 어차피 둘 모두 생명 주기를 제어하긴 힘들고, 멀티 스레드 환경(동시성)에서 발생하는 문제를 해결해야 하는 부분도 있습니다. 전역으로 활용된다라는 점을 봤을 땐 싱글톤과 static 모두 큰 차이는 없어 보이는데 릴리는 어떻게 생각하시나요?
There was a problem hiding this comment.
리건 의견에도 동의합니다👍
저는 현재 상황에선 차이점이 크게 느껴지지 않을 수 있으나 싱글톤에 비해 static 은 확장성과 테스트가 불리하다고 생각했어요.
무엇보다 객체가 아닌 static 메서드로 관리한다는 점에서 객체지향적인가? 하는 생각이 들기도 하고요!
하지만 지금은 사실 SessionManager 가 역할이 단순해서, 리건 방식도 좋습니다🤗
lilychoibb
left a comment
lilychoibb
left a comment
There was a problem hiding this comment.
리건 고생많으셨어요!
질문 주신 것에 대한 답변 남겨놨어요. 확인하고 다음 단계 진행해주세요🤗
3단계 파이팅입니다 💪🏻
2 participants
안녕하세요, 릴리! 👋
레벨 2 페어 미션 이후로 오랜만입니다!
이번 미션은 1, 2단계 완료 후, 3단계 진행 중 제출하게 되었습니다.
리뷰 잘 부탁드립니다!