[톰캣 구현하기 1, 2단계] 코코닥(이승용) 미션 제출합니다. #373
Conversation
feat: login 홈페이지에 대한 요청, 응답 구현
refactor: Http11Processor 의 중복 코드 제거
|
SonarCloud Quality Gate failed.
|
| private static void validateStartLine(final String startLine) { | ||
| if (startLine.split(" ").length != 3) { | ||
| throw new IllegalArgumentException("Invalid HTTP Request Message"); | ||
| } | ||
| } |
There was a problem hiding this comment.
매직넘버 상수화에 관해서는 어떻게 생각하시나요?
필요성에 공감하신다면 코드 전반에 거쳐 매직넘버 & delimiter를 상수화 하는 것도 좋을 듯 합니다!
There was a problem hiding this comment.
공감합니다. 매직넘버 상수화를 통해 가독성을 챙겨보도록 하겠습니다!
| private static String getBody(final BufferedReader bufferedReader, final Map<String, String> header) throws IOException { | ||
| if (header.containsKey("Content-Length")) { | ||
| int contentLength = Integer.parseInt(header.get("Content-Length")); | ||
| char[] buffer = new char[contentLength]; | ||
| bufferedReader.read(buffer, 0, contentLength); | ||
| return new String(buffer); | ||
| } | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
"Content-Length"와 같은 키값은 반복해도 사용되는 만큼 상수화로 표현해도 좋을 듯합니다.
이것 이외에도 문자열 파싱 과정에서 발생하는 매직 넘버를 상수화 하는 방안도 한번 고민해보시면 좋을 듯 해요!
There was a problem hiding this comment.
정확하게 봐주셨습니다 로이햄. 상수화를 조금 더 신경써보겠습니다.
| private static Map<String, String> getHeader(final List<String> primitiveRequest) { | ||
| final Map<String, String> header = new HashMap<>(); | ||
| final int size = primitiveRequest.size(); | ||
| for (int i = 1; i < size; i++) { | ||
| final String request = primitiveRequest.get(i); | ||
| if (request.equals(CRLF.getValue())) { | ||
| break; | ||
| } | ||
| final String[] headerKeyValue = request.split(COLON.getValue() + BLANK.getValue()); | ||
| header.put(headerKeyValue[0], headerKeyValue[1]); | ||
| } | ||
| return header; | ||
| } |
There was a problem hiding this comment.
-
":"를 기준으로 헤더를 파싱할 때, 헤더의 값에 ":"가 포함되지 않을 경우 예외처리가 필요할 듯합니다. 또 indexOf()와 subString()을 활용한다면 안전하게 파싱할 수 있을 듯 합니다.
-
사소한 내용이지만 강화 for문을 적용해서 가독성을 높일 수 있을 것 같아요.
-
가독성을 높이기 위해 getHeader와 getBody를 파싱하는 로직을 별도의 클래스로 분리하는 것도 고려해봄직 합니다!
There was a problem hiding this comment.
예외 처리에 대해 크게 공감하여 헤더에 대한 예외 처리 기능을 추가했습니다!
다만, 현재 반복문은 반드시 (size - 1)번 돌아야하기 때문에 강화 for문을 적용하기가 어려웠습니다 ㅠㅠ
클래스를 분리하는 것과 관련해서는 조금 더 고민해보겠습니다!!
| public String cookie(final String key) { | ||
| if (cookie.containsKey(key)) { | ||
| return cookie.get(key); | ||
| } | ||
| return ""; |
There was a problem hiding this comment.
보다 간결한 코드를 위해 getOrDefault() 메서드를 활용해보는 것도 좋을 듯 합니다!
| private String id; | ||
| private Map<String, Object> values; | ||
|
|
||
| public Session(final String id) { |
There was a problem hiding this comment.
Session Manager(필드 선언 초기화)와 달리 Session class에서는 values를 생성자에서 초기화한 이유가 있으실까요?
There was a problem hiding this comment.
차이점이라고 한다면, static 변수에 대한 초기화인지 인스턴스 변수에 대한 초기화인지로 나뉜 것 같습니다. 인스턴스 변수는 최대한 생성자에서 초기화하려했고, static 변수는 static 블럭을 활용하여 초기화에 섬세한 조정이 필요하지 않은 이상, 선언과 동시에 초기화를 해주려고 한 것 같습니다!
| return new HttpResponseMessageBuilder(); | ||
| } | ||
|
|
||
| public static class HttpResponseMessageBuilder { |
There was a problem hiding this comment.
빌더 패턴 굿입니다👍
Response message 생성 로직이 해당 클래스에 모두 포함되어 있는데,
추후 여력이 된다면 Response 메시지 생성 관련 복잡성을 별도의 클래스에 위임해보는 방식도 고민해볼 수 있을듯 해요!
| public Object resolve(final HttpRequest httpRequest) { | ||
| final HttpCookie httpCookie = httpRequest.getHttpCookie(); | ||
| final String jSessionId = httpCookie.cookie("JSESSIONID"); | ||
| if ("".equals(jSessionId)) { |
There was a problem hiding this comment.
isEmpty() 메서드를 활용하면 명확히 표현할 수 있다는 장점이 있을 듯 해요!
There was a problem hiding this comment.
맞습니다! 다만 jSessionId 가 null 인 경우에는 isEmpty() 를 호출할 때 NPE 에러가 발생할 여지가 있어서, 이런 식의 처리를 하게 되었습니다.
| final String responseBody = "Hello world!"; | ||
| return HttpResponse.builder() | ||
| .header("Content-Type", "text/html;charset=utf-8") | ||
| .header("Content-Length", String.valueOf(responseBody.getBytes().length)) |
There was a problem hiding this comment.
.autoSetContentLength()를 활용하면 HttpReponse 빌더가 본문의 길이를 자동으로 설정한다고 합니다!
There was a problem hiding this comment.
엇 그런 기능이 있나요?! 어떤 객체의 메서드인지 궁금합니다.
|
|
||
| private List<Argument> arguments = new ArrayList<>(); | ||
|
|
||
| @Override |
There was a problem hiding this comment.
메서드가 길어서 가독성이 좀 떨어지는 측면이 있는 듯 해요!
POST와 GET 메서드 처리 로직 정도만 분리한다고 해도 훨씬 가독성이 좋아질 듯 합니다!
(코드 라인을 잘못 지정했네요 handle() 메서드에 관한 내용입니다)
There was a problem hiding this comment.
맞습니다.. 가독성이 많이 떨어져서 이번에 POST와 GET의 내용을 분리해보았습니다.
| @Override | ||
| public HttpResponse handle(final HttpRequest httpRequest) throws IOException { | ||
| if (httpRequest.getHttpMethod() == HttpMethod.POST) { | ||
| final Map<String, String> formData = FormDataParser.parse(httpRequest.getBody()); | ||
| final User user = new User(formData.get("account"), formData.get("password"), formData.get("email")); | ||
| InMemoryUserRepository.save(user); | ||
| return HttpResponse.builder() | ||
| .redirect("http://localhost:8080/index.html") | ||
| .build(); | ||
| } else { | ||
| final Session argument = arguments.stream() | ||
| .filter(ag -> ag.getImlClass() == Session.class) | ||
| .map(ag -> (Session) ag) | ||
| .findFirst() | ||
| .orElseThrow(IllegalArgumentException::new); | ||
| if (argument.getId() != null) { | ||
| return HttpResponse.builder() | ||
| .redirect("http://localhost:8080/index.html") | ||
| .build(); | ||
| } | ||
| final String fileName = "static/register.html"; | ||
| final URL resourceUrl = getClass().getClassLoader().getResource(fileName); | ||
| final Path path = new File(resourceUrl.getPath()).toPath(); | ||
| final String responseBody = new String(Files.readAllBytes(path)); | ||
| return HttpResponse.builder() | ||
| .header("Content-Type", "text/html;charset=utf-8") | ||
| .header("Content-Length", String.valueOf(responseBody.getBytes().length)) | ||
| .body(responseBody) | ||
| .build(); | ||
| } | ||
| } |
There was a problem hiding this comment.
'파일 읽기 - 응답 생성' 로직을 메서드 단위로 각기 분리하여 가독성을 높이는 것도 좋을 듯 합니다.
존경하는 로이햄 안녕하신지요..
테스트 따위 없는 못난 코드 보여드려서 죄송할 따름입니다.
제가 내일이면 포천 어딘가의 부대에서 열심히 훈련을 하게 됩니다. 그래서 코드리뷰 반영이 느릴 것 같아요.
하지만 열심히 해보겠습니다!!!!!!!!!