[톰캣 구현하기 1 & 2단계] 오션(김동해) 미션 제출합니다 #347
Conversation
| this.requestBody = requestBody; | ||
| } | ||
|
|
||
| public static HttpRequest from(final InputStream inputStream) throws IOException { |
There was a problem hiding this comment.
개인적으로 InputStream 을 전달해서 사용한다면 InputStream 에서 데이터를 읽은 상태에 따라 의도치 않게 작동할 확률이 굉장히 높을 것 같아요!
InputStream 을 생성한 주체에서만 의도한 순서대로 작동하도록 하는것이 좋을것 같습니다!!😊
There was a problem hiding this comment.
음 전 InputStream을 받은 bufferedReader를 메소드에 전달하였는데요.
같은 클래스 내부에서 사용된다하더라도 의도치 않게 작동할 확률이 있다고 생각합니다! 결국 개발자의 휴먼에러이니까요..
제 의견은 개발자가 유지보수가 어려울 수 있지만 '호출이 보장되어야 하는 로직' 이라는 이유로는 굳이 전달해서 사용하지 않을 이유가 없다고 생각해서 현재방식 그대로 이어갔습니다.
리팩토링 단계 때 어려움이 발생하면 다시한번 고려해보겠습니다!
| } | ||
|
|
||
| private static void validate(final String line) { | ||
| if (line.isBlank() && line.isEmpty()) { |
There was a problem hiding this comment.
| if (line.isBlank() && line.isEmpty()) { | |
| if (line.isBlank()) { |
isBlank() 가 isEmpty() 까지 같이 검증해주는 걸로 알고 있어요~
| try { | ||
| Objects.requireNonNull(resource); | ||
| } catch (NullPointerException e) { | ||
| throw new ResourceNotFoundException(resourcePath); |
There was a problem hiding this comment.
GET 메서드 처리에서 요청 대상에 확장자가 없을 경우 html 을 확장자로 처리함으로 발생하는 리소스를 찾지 못하는 경우들이 발생할텐데요 ( ex. GET /hello HTTP/1.1 )
이 부분을 404가 아닌 500 으로 처리한 이유가 궁금해요~~
There was a problem hiding this comment.
따로 500으로 처리한 이유는 없었습니다! 404로 처리하려고 했는데 커스텀 예외를 제대로 잡아주지 못해 500으로 빠져버렸네요 .. 404로 제대로 매핑되도록 처리하였습니다!
| final Map<String, String> requestParam = Arrays.stream(httpRequest.getRequestBody().split("&")).takeWhile(it -> !it.isEmpty()) | ||
| .map(it -> it.split("=")).collect(Collectors.toMap(it -> it[0], it -> it[1])); |
There was a problem hiding this comment.
특정 값에 대해 상수 분리가 덜 되었던 이유는 리팩토링 후 클래스 분리가 제대로 되면 상수분리를 진행하려고 하다보니까 그렇게 된 것 같습니다!
추후 3단계에서 리팩토링과 함께 깔끔한 상수 분리 진행하도록 하겠습니다 ..
| if(isLoggedIn(httpRequest)){ | ||
| return foundResponse(httpRequest, INDEX); | ||
| } |
| @@ -29,19 +55,143 @@ public void process(final Socket connection) { | |||
| try (final var inputStream = connection.getInputStream(); | |||
| final var outputStream = connection.getOutputStream()) { | |||
|
|
|||
| final var responseBody = "Hello world!"; | |||
| final HttpRequest httpRequest = HttpRequest.from(inputStream); | |||
There was a problem hiding this comment.
개인적으로 InputStream 을 전달해서 사용한다면 InputStream 을 사용하는 메서드의 호출 순서가 확실하게 보장되지 않을 경우 의도치 않게 작동할 확률이 굉장히 높을 것 같아요!
호출 순서가 보장되어야 하는 로직이라서 InputStream 을 사용하는 한곳에서만 의도한 순서대로 절차적인 메서드를 작성하여 작동하도록 하는것이 좋을것 같다는 의견입니다!!😊
There was a problem hiding this comment.
음 전 InputStream을 받은 bufferedReader를 메소드에 전달하였는데요.
같은 클래스 내부에서 사용된다하더라도 의도치 않게 작동할 확률이 있다고 생각합니다! 결국 개발자의 휴먼에러이니까요..
제 의견은 개발자가 유지보수가 어려울 수 있지만 '호출이 보장되어야 하는 로직' 이라는 이유로는 굳이 전달해서 사용하지 않을 이유가 없다고 생각해서 현재방식 그대로 이어갔습니다.
리팩토링 단계 때 다시한번 고려해보겠습니다!
| String headersString = headers.entrySet() | ||
| .stream() | ||
| .map(entry -> entry.getKey() + ": " + entry.getValue()) | ||
| .collect(Collectors.joining("\r\n")); |
There was a problem hiding this comment.
| .collect(Collectors.joining("\r\n")); | |
| .collect(Collectors.joining(System.lineSeperator())); |
줄바꿈 문자가 어플리케이션이 실행되는 환경에 다를 수 있기에 환경에 의존하지 않게 위에 코드처럼 작성해보면 어떨까요오??
| if (SESSIONS.containsKey(id)) { | ||
| return SESSIONS.get(id); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Optional 로 처리하는 방식도 좋을것 같아요!!😊
( 호출하는 쪽에서 세션이 없는 경우에 대해 어떻게 처리했는지에 대해 신경쓰지 않아도 되는 장점이 있을 것 같아요!! )
There was a problem hiding this comment.
Optional 로 처리하니 훨씬 깔끔하게 진행할 수 있게 된 것 같습니다! 수정하였습니다~
| absolutePath = absolutePath.split(NON_RESERVED_QUERY_PARAM_DELIMITER)[0]; | ||
| } | ||
| if (!absolutePath.contains(EXTENSION_DELIMITER)) { | ||
| absolutePath += DEFAULT_EXTENSION; |
There was a problem hiding this comment.
| absolutePath += DEFAULT_EXTENSION; | |
| absolutePath += EXTENSION_DELIMITER + HttpContentType.HTML.getContentType() |
HttpContentType 에 있는 HTML 을 재사용해보는 건 어떨까요오?? 😁
| public static HttpCookie parseCookie(String cookie) { | ||
| Map<String,String> parsedCookie = new HashMap<>(); | ||
| if(cookie!=null) { | ||
| parsedCookie = Arrays.stream(cookie.split(";= ")) |
There was a problem hiding this comment.
| parsedCookie = Arrays.stream(cookie.split(";= ")) | |
| parsedCookie = Arrays.stream(cookie.split("; ")) |
오션은 ";= " 를 기준으로 나누었는데 어떤 동작을 의도하신건지 궁금해요오~ 😁
There was a problem hiding this comment.
스플릿이 수정해주신대로 계획했었는데 밑에서도 '='을 사용하다보니 오타가 발생했던 것 같습니다 ㅠㅠ 수정했습니다!
|
Kudos, SonarCloud Quality Gate passed!
|
안녕하세요 "스플릿".split(" ")[0] 잘부탁드립니다.
코드를 handler로 분리하여 진행을 하려다가 3단계 리팩토링 과정을 보고 그때 진행하려고 합니다 ㅎㅎ. . 기능구현에 충실해봤어요
리팩토링은 피드백 이후에 진행하겠습니다 !
감사합니다