[무늬] 3단계 - HTTP 웹 서버 구현 미션 제출합니다. #207
Conversation
- Exception 네이밍 수정 - 제공되는 api 활용 - Optional을 매개변수, 필드에서 제거 - 매직스트링 상수화
- Http 요청라인을 올바르게 읽어오지 못하는 경우에 대한 수정 - 여러 확장자에 대한 ContentType 처리 추가
- 로그인하지 않은 상태면 로그인 화면을 보여주도록 함.
- 클래스명 변경 - deprecated api 변경 - 단위테스트 추가
jaeyeonling
left a comment
jaeyeonling
left a comment
There was a problem hiding this comment.
회사 업무로 인해 리뷰가 많이 늦었습니다 ㅜㅜ
3단계 요구사항을 잘 구현해주셨습니다 👍
피드백 남겨드렸으니 확인하고 반영해주세요!
| import webserver.http.request.HttpSession; | ||
|
|
||
| public class SessionDataBase { | ||
| private static Map<String, HttpSession> sessions = new HashMap<>(); |
There was a problem hiding this comment.
| private static Map<String, HttpSession> sessions = new HashMap<>(); | |
| private static Map<String, HttpSession> sessions = new ConcurrentHashMap<>(); |
여러 스레드에서 접근할 수 있는 경우 동시성을 보장해주는 것이 좋습니다!
| TemplateLoader loader = new ClassPathTemplateLoader(); | ||
| loader.setPrefix("/templates"); | ||
| loader.setSuffix(".html"); | ||
| Handlebars handlebars = new Handlebars(loader); |
There was a problem hiding this comment.
Handlebars를 매번 생성하는 것은 어떠한 단점이 있을지 고민해보고 재사용할 수 있다면 어떠한 장점이 있을지 고민해보세요!
| } catch (ServerException e) { | ||
| logger.error(e.getMessage()); | ||
| return HttpResponse.internalServer(e.getMessage()).build(); | ||
| return HttpResponse.internalServerError(e).build(); |
|
|
||
| import java.util.Optional; | ||
|
|
||
| public enum ContentType { |
There was a problem hiding this comment.
ContentType의 옵션(charset, boundary, ...)들은 상황에 따라 추가될 수 있을 것 같아요.
정적인 enum으로 관리하긴 어려워보이는데 어떻게 생각하시나요?
| if (user != null && password.equals(user.getPassword())) { | ||
| return HttpResponse.ok() | ||
| .bodyByPath("./templates/index.html") | ||
| .setCookie("JSESSIONID", httpRequest.getSessionId(), "/") |
There was a problem hiding this comment.
JSESSIONID는 매우 특별한 의미를 가진만큼 상수로 분리해보면 어떨까요?
| throw new NotReadableHttpMessageException("HTTP 요청메시지의 요청라인이 비어있습니다."); | ||
| } | ||
| String source = bufferedReader.readLine(); | ||
| logger.debug(String.format("requestLine : %s", source)); |
There was a problem hiding this comment.
| logger.debug(String.format("requestLine : %s", source)); | |
| logger.debug("requestLine : {}", source); |
로거의 파라미터 매핑 기능을 활용할 수 있겠네요!
|
|
||
| assertThat(response.getStatusLine()).isEqualToComparingFieldByField( | ||
| new StatusLine(Protocol.ONE_POINT_ONE, StatusCode.OK)); | ||
| assertThat(response.getHeader()) |
There was a problem hiding this comment.
테스트 코드를 봤을 때 로그인 실패 페이지로 이동한다. 라는 것을 명확하게 알기 힘드네요~
assert문에서 해당 테스트의 목적을 명확하게 검증해보면 좋겠어요!
| JAVASCRIPT("application/javascript", Optional.empty(), Optional.empty()), | ||
| TEXT("text/plain", Optional.empty(), Optional.empty()); | ||
| HTML("text/html", "charset=utf-8", ""), | ||
| CSS("text/css", "", ""), |
There was a problem hiding this comment.
위 피드백을 반영하면 필요없을 수 있는 내용이지만 빈 값들을 추가하기 위해서 불필요한 값들이 많이 추가됐는데요.
이 또한 중복으로 본다면 생성자를 통해 해결할 수 있을 것 같습니다!
|
|
||
| public String value() { | ||
| StringBuilder stringBuilder = new StringBuilder(); | ||
| String charsetToAppend = (charset == "") ? "" : (";" + charset); |
There was a problem hiding this comment.
String이 객체라는 사실을 생각하며 객체의 비교에서 왜 equals를 사용하는지 공부해보아요!
| protected HttpResponse get(HttpRequest httpRequest) { | ||
| if ("true".equals(httpRequest.getCookieValue("logined"))) { | ||
| return HttpResponse.ok() | ||
| .body(TemplateFactory.of("user/list", UserDataBase.findAll())) |
There was a problem hiding this comment.
ViewTemplate를 매 컨트롤러에서 사용하지 않고 공통으로 사용하도록 설계해보아요!
2 participants
제출이 많이 늦었습니다 😭
3단계 리뷰도 잘부탁드립니다!