[톰캣 구현하기 1,2단계] 이레(이다형) 미션 제출합니다. #329
Conversation
|
|
||
| public class RequestLine { | ||
|
|
||
| private final String requestMethod; |
There was a problem hiding this comment.
requestMethod 를 ENUM 타입으로 관리하는 것은 어떨까요 ?
관리도 쉽고 사용도 편할 것 같아요 !
|
|
||
| outputStream.write(response.getBytes()); | ||
| outputStream.flush(); | ||
| RequestLine requestLine = readRequestLine(reader); |
There was a problem hiding this comment.
지금도 좋지만 Request라는 큰 객체로 관리하면 좀 더 깔끔해 질 것 같습니다!
| } | ||
| lines.add(line); | ||
| } | ||
| return RequestHeader.from(lines); |
There was a problem hiding this comment.
readHeader 에서도 parse를 해주고 RequestHeader.from 내부에서도 parse를 해주는 것으로 보이는데요 !
stream의 takeWhile을 이용하여 split 하고 collect 해주면 이 두가지 로직을 한번에 처리할 수 있을 것 같아요 !
There was a problem hiding this comment.
Optional의 스트림 기능을 이용해서
User user = InMemoryUserRepository.findByAccount(account)
.filter(u -> u.checkPassword(password))
.orElseThrow(() -> new IllegalArgumentException("해당하는 계정이 존재하지 않거나 비밀번호가 일치하지 않습니다"));
와 같이 바꾸고 싶었지만, 이렇게 되면 두개인 예외가 하나로 합쳐져야 해서 불가피하게 위와 같이 결정하게 되었습니다.
후에 서비스를 만드는 것을 가정할 때 해당 내용을 분리한 것이 좋을 것 같아서요
감사합니다!
There was a problem hiding this comment.
제 의도는 RequestGenerator의 readHeader , Request.from 의 반복문을 한번에 줄일 수 있을 것 같아서 말씀드렸어요!
final Map<String, String> headers = br.lines().takeWhile(it -> !it.isEmpty())
.distinct()
.map(it -> it.split(": "))
.collect(Collectors.toMap(it -> it[0], it -> it[1]));
이런식으로 헤더를 쉽게 만들 수 있을 것 같아서요! 굳이 적용하실 필요는 없습니다 🙂
|
|
||
| private static String findResponseBody(final String requestURI) throws IOException { | ||
| URL requestedFile = ClassLoader.getSystemClassLoader().getResource("static" + requestURI); | ||
| try (BufferedReader br = new BufferedReader(new FileReader(requestedFile.getFile(), Charset.forName("UTF-8")))) { |
There was a problem hiding this comment.
URL 파일을 스트링으로 변환하는 로직인 것 같은데 Files.readAllBytes를 참고해도 좋을 것 같아요 !
There was a problem hiding this comment.
URL requestedFile = ClassLoader.getSystemClassLoader().getResource("static" + requestURI);
return new String(Files.readAllBytes(new File(requestedFile.getFile()).toPath()));
return String.valueOf(Files.readAllBytes(path));
와 같이 수정했습니다.
| if (requestURI.endsWith("html")) { | ||
| return new HttpResponseBuilder().init() | ||
| .httpStatus(OK) | ||
| .header("Content-Type: text/html;charset=utf-8 ") |
There was a problem hiding this comment.
contentType을 enum으로 관리하면 어떨까요 ?!
확장자 or accept에 맞는 타입만 넣어주면 되기 때문에 밑에 로직하고 분리 할 필요가 없어질 것 같아요!
There was a problem hiding this comment.
contentType를 enum으로 한다면,
HTML("text/html;charset=utf-8 ")
CSS("ccs/~")
JS
..
와 같을텐데, 클라이언트에서 요청하는
- 리소스의 content타입을 제가 모르기도하고,
- 리소스 타입이 추가될때마다 enum에 코드 변경이 일어나야 하는데, 좋은 방법인지 잘 모르겠습니다!
의견 부탁드립니다~
There was a problem hiding this comment.
enum,statifFileHandler든 잘 모르는 타입을 컨텐츠 타입을 하드하게 삽입해줘야 되는 것으로 보여요!
그럴 경우에는 리소스 타입이 계속 추가된다고 했을때 enum 타입에서만 변경이 일어나면 더 좋을 것 같아서 enum 쪽으로 관리하면 어떨까 라는 코멘트 달아봤습니다 🙂
| import java.util.Map; | ||
|
|
||
| public class RequestHeader { | ||
| final private Map<String, String> headers; |
|
|
||
| private static HttpResponse login(final RequestBody requestBody) { | ||
| try { | ||
| String account = requestBody.getContentValue("account"); |
There was a problem hiding this comment.
account, password 같은 부분들은 상수화 하는게 좋을 것 같아요!
There was a problem hiding this comment.
상수화에 대한 생각이 갈리기는 하는데요
여러번이 아닌, 딱 한번 사용하는 문자열에 대해서 상수화를 하는 이유가 있을까요?
개인적으로는 상수화 하는 것이 가독성측면에서 나쁘다고 생각해서, 꼭 필요한 상수 JSSESSIONID라는 (변경 가능성이 있는 문자열)만 다른 패키지에서 상수화시켜 사용했습니다.
혹시 의견 있으시면 바로 적용 하도록 하겠습니다.
감사합니다!
There was a problem hiding this comment.
한번 사용하는 문자열까지 상수화를 하면 지나치게 복잡해질 수 있을 수 있어서 이레 의견에는 어느정도 동의합니다!
저는 재사용가능성, 상수 자체의 가독성 등의 이유로 왠만하면 상수화 하려고 하는 편인데 사람마다 의견이 다를 수 있는 것 같스빈다.
이레가 그렇게 생각하신다면 그렇게 진행하는 것이 좋을 것 같아요 !!
|
|
||
| public static RequestBody from(final String input) { | ||
| Map<String, String> contents = new HashMap<>(); | ||
| String[] body = input.split("&"); |
There was a problem hiding this comment.
stream 을 활용하면 더 깔끔하게 작성할 수 있을 것 같아요 ~
|
|
||
| private static final Logger log = LoggerFactory.getLogger(LoginHandler.class); | ||
|
|
||
| private static final String EXTENSION = ".html"; |
| import java.io.IOException; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import java.util.UUID; |
There was a problem hiding this comment.
사용하지 않는 import문은 정리하면 좋을 것 같아요 !
|
Kudos, SonarCloud Quality Gate passed!
|
| URL requestedFile = ClassLoader.getSystemClassLoader().getResource("static" + requestURI); | ||
| return new String(Files.readAllBytes(new File(requestedFile.getFile()).toPath())); | ||
| } catch (NullPointerException e) { | ||
| throw new NotExistFileException(); |
There was a problem hiding this comment.
이 예외가 발생했을 경우에는 404 html 로 리다이렉트 되도록 하는 것이 좋을 것 같아요 !
이 과정은 3단계에서 진행해도 좋을 것 같아요 !
|
|
||
| public static HttpCookie from(String cookieHeader) { | ||
| Map<String, String> cookies = new HashMap<>(); | ||
| Arrays.stream(cookieHeader.split(COOKIES_DELIMITER)) |
There was a problem hiding this comment.
cookieHeader가 null 일 경우도 고려해봐야 할 것 같아요 !
비어있다면 기능적으로 문제가 발생합니다..!
|
|
||
| public boolean isSameRequestMethod(String value) { | ||
| return Arrays.stream(RequestMethod.values()) | ||
| .anyMatch(requestMethod -> requestMethod.name().equals(value)); |
There was a problem hiding this comment.
requestLine.getRequestMethod().isSameRequestMethod("GET") 이걸 통해서 GET 메소드인지 확인하려고 하는것 같아요!
다만 지금과 같은 로직으로 실행되면 무조건 true가 나오는 상황이 발생하여 리다이렉트가 불가합니다!
위의 로직 대신 이건 어떨까요 ?
| .anyMatch(requestMethod -> requestMethod.name().equals(value)); | |
| return this.name().equals(value); |
| private RegisterHandler() { | ||
| } | ||
|
|
||
| public static HttpResponse handle(final HttpRequest request) { |
There was a problem hiding this comment.
가입도 로그인처럼 세션추가 후 쿠키추가하면 좋을 것 같아요 !
현재는 쿠키탑재가 안되는 상황이네요 😭
안녕하세요 오션! 리뷰 잘 부탁드립니다!
예외 처리는 모두 IllegalArgumentException으로 처리하였는데 후에 리팩토링 과정에서 커스텀 예외로 변경하도록 하겠습니다.
제 코드의 전체 흐름은 정리해서 곧 올리도록 하겠습니다.