[톰캣 구현하기 - 3, 4단계] 아코(안석환) 미션 제출합니다. #466
Conversation
세부사항 - controller 도입하기 - 파일 읽어오는 기능 util 클래스로 분리하기
세부사항 - sessionManager에 동시성 컬렉션 적용 - Executors로 Thread Pool 적용
| if (REQUEST_MAPPER.containsKey(path)) { | ||
| return REQUEST_MAPPER.get(path); | ||
| } | ||
| return new PageController(); |
There was a problem hiding this comment.
| if (REQUEST_MAPPER.containsKey(path)) { | |
| return REQUEST_MAPPER.get(path); | |
| } | |
| return new PageController(); | |
| return REQUEST_MAPPER.getOrDefault(path, new PageController()); |
이 부분은 취향 차이라고 생각하는데
getOrDefault를 사용해서 라인을 줄일 수 있어요!
There was a problem hiding this comment.
엄청 깔끔하네요! 배워갑니다 저도 if문이 없는 부분이 더 가독성이 좋은 것 같습니다👍👍
| private static final String CONTENT_TYPE = "Content-Type"; | ||
| private static final String CHARSET_UTF_8 = ";charset=utf-8"; | ||
| private static final String CONTENT_LENGTH = "Content-Length"; |
There was a problem hiding this comment.
AbstractController를 상속하는 Controller 클래스에서 중복해서 사용하는 상수들(CONTENT_TYPE, CONTENT_LENGTH, LOCATION 등)은 상위 추상 클래스에 protected로 선언한 후 사용하면서 중복을 없애는 건 어떻게 생각하시나요??
There was a problem hiding this comment.
저도 중복되는 것은 protected로 처리되는 것이 반복되는 요소를 줄일 수 있어서 좋을 것 같습니다!
| private HttpRequest( | ||
| public HttpRequest( |
There was a problem hiding this comment.
HttpRequest, QueryString, RequestBody 클래스에서 기본 생성자를 private에서 public으로 다시 열어준 이유가 있을까요??
위 클래스들에서 전부 정적 팩터리 메서드를 사용하고 있는 걸로 보이는데, 이 때 정적 팩터리 메서드와 생성자에 대해 아코만의 기준이 있다면 들어보고 싶어요!
There was a problem hiding this comment.
저도 기본적으로 정적팩토리 메소드를 이용할 때에는 생성자를 private로 합니다. 제가 이번에 생성자를 public으로 둔 이유는 각 controller 테스트의 given의 편의성을 높이기 위해서 였습니다. 그래서 생성자를 통해 HttpRequest를 생성해서 테스트를 진행했습니다. 이처럼 저는 테스트의 편의성을 위해 정적펙토리 메소드가 있음에도 생성자를 Private에서 public으로 접근제어자를 변경했는데 이 부분에 대해서 망고는 어떻게 생각하시나요?
There was a problem hiding this comment.
저는 개인적으로 테스트를 위한 프로덕션 코드 수정을 안좋아하는 편이어서 private으로 두긴 했어요ㅎ
이것도 편의성이냐 안정성이냐 어느 부분을 중요하게 여기느냐에 따라 사람마다 다를 것 같네요 😎
tomcat/src/main/java/org/apache/coyote/http11/httpmessage/response/ContentType.java
Show resolved
Hide resolved
| StatusCode statusCode; | ||
| HttpHeader httpHeader; | ||
| String body; |
There was a problem hiding this comment.
HttpResponse 클래스에서는 default 접근 제어자를 사용한 이유가 있을까요??
There was a problem hiding this comment.
아 부끄럽네요.. 이 부분 빠르게 수정하겠습니다.
| if (request.hasBlankRegisterUserBody()) { | ||
| response.setStatusCode(StatusCode.REDIRECT); | ||
| response.addHeader(LOCATION, REGISTER_PAGE); | ||
| return; | ||
| } |
There was a problem hiding this comment.
실제 서비스를 생각한다면 이 경우에는 단순히 회원가입 페이지로 리다이렉트 되고 사용자는 리다이렉트 된 이유를 알지 못하고 계속 회원가입을 시도할 것 같아요.
사용자 편의성을 위해 예외 처리를 하고 클라이언트로 넘겨주는 건 어떻게 생각하나요??
There was a problem hiding this comment.
지금 당장 여기서 예외를 발생시키면 화면이 나타나지 않아 이 부분을 클라이언트가 알 수 있도록 잘못된 값을 입력했다는 것을 알 수 있게 bad request로 요청을 보내도록 수정했습니다. 즉, 공백을 넣어서 회원가입을 하면 "입력한 아이디, 비밀번호, 이메일에는 공백이 들어오면 안됩니다."의 문구가 나타나도록 수정했습니다!
| if (request.getHttpMethod() == HttpMethod.POST) { | ||
| doPost(request, response); | ||
| } | ||
| if (request.getHttpMethod() == HttpMethod.GET) { | ||
| doGet(request, response); | ||
| } |
There was a problem hiding this comment.
POST나 GET이 아닌 HttpMethod가 요청으로 들어오면 어떻게 될까요??
There was a problem hiding this comment.
현재로서는 아무런 동작을 하지 않네요.. POST와 GET외에는 지원하지 메소드라는 예외처리를 진행하겠습니다.
| private static final Map<String, Controller> REQUEST_MAPPER = new HashMap<>(); | ||
|
|
There was a problem hiding this comment.
| private static final Map<String, Controller> REQUEST_MAPPER = new HashMap<>(); | |
| private static final Map<String, Controller> REQUEST_MAPPER = new HashMap<>(); | |
| private RequestMapping() { | |
| } |
인스턴스를 만들지 않는 유틸 클래스에는 private 기본 생성자를 추가해주세요..!
Utility classes, which are collections of static members, are not meant to be instantiated. Even abstract utility classes, which can be extended, should not have public constructors.
Java adds an implicit public constructor to every class which does not define at least one explicitly. Hence, at least one non-public constructor should be defined.
| public class FileFinder { | ||
|
|
||
| private static final String FILE_PATH = "static/"; | ||
| private static final String NOT_FOUND_PAGE = "404.html"; | ||
|
|
There was a problem hiding this comment.
여기도 private 기본 생성자를 추가해주세요!
SonarCloud 참고
There was a problem hiding this comment.
쓰레드와 동시성 관련한 테스트는 저도 아직 제대로 감을 잡지 못해서 찾아보는 중입니다 😥
같이 고민해보아요..
There was a problem hiding this comment.
저도 계속 찾아보았는데 방식이 떠오르지 않아서 이 부분 추가하지 못했습니다 ㅜㅜ
|
Kudos, SonarCloud Quality Gate passed!
|
|
망고 이번 리뷰받은 것 모두 반영했습니다. 추가적으로 가독성 향상을 위해 response 객체를 chainning 형식으로 heade와 body 등 값을 설정할 수 있게 수정했습니다. 또한 패키지 구조에 대해서 고민을 하고 controller(servlet)을 coyote에서 catalina 패키지로 이동했습니다! |
안녕하세요 망고!
1, 2단계가 마무리 되고 3, 4단계를 진행하면서 리팩토링과 동시성 처리 및 스레드 확장을 적용했습니다.
리팩토링 과정에서 Controller가 생긴 만큼 Controller에 대한 테스트를 진행했습니다.
이번 미션을 진행하면서 고민이 되었던 점이 있었는데요!
동시성 관련 기능들을 추가했지만 이를 테스트를 해야할 방법에 대해서 알지 못해서 부득이 하게 이 부분은 테스트를 작성하지 못했습니다.(아예 감이 잡히지 않습니다 ㅜㅜ) 망고는 이 부분을 어떻게 해결하셨나요? 이 부분에 대한 망고의 조언이 듣고 싶습니다.
이번 리뷰도 잘 부탁드립니다!🙇♂️