[톰캣 구현하기 3,4단계] 리오(오영택) 미션 제출합니다. #496
Conversation
|
|
||
| @Configuration | ||
| public class CacheWebConfig implements WebMvcConfigurer { | ||
|
|
||
| @Override | ||
| public void addInterceptors(final InterceptorRegistry registry) { | ||
| WebContentInterceptor webContentInterceptor = new WebContentInterceptor(); |
There was a problem hiding this comment.
WebContentInterceptor는 어떤 역할을 하나요?
| @@ -0,0 +1,9 @@ | |||
| package common; | |||
There was a problem hiding this comment.
Constants의 값들 모두 apache package 에서만 사용되는데 apache 내부가 아닌 동일 계층으로 두신 이유가 있을까요?
There was a problem hiding this comment.
jwp 패키지에서도 사용될 것 같아서 넣어놨는데 안 쓰이게 됐군요..! 변경하겠습니다! 감사합니다~!
| public static final String SPACE = " "; | ||
| public static final String CRLF = "\r\n"; | ||
|
|
||
| private Constants() {} |
There was a problem hiding this comment.
static 클래스에 인스턴스화 방지👍
Enum을 사용하면 추후 reflection을 통한 생성 대해서도 방지 할 수 있습니다!
|
|
||
| protected void doPost(Request request, Response response) { /* NOOP */ } | ||
|
|
||
| protected void doPut(Request request, Response response) { /* NOOP */ } |
There was a problem hiding this comment.
해당 메스드는 구현하지 않은 핸들러에게 이 메서드의 요청이 오면 어떻게 되나요?
ex)
DELETE /login HTTP/1.1
There was a problem hiding this comment.
지금은 abstractController의 메서드가 호출되어 아무일도 일어나지 않는군요..! 해당 예외도 처리하도록 하겠습니다!
| } | ||
|
|
||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
전반적으로 출력과 관련한 로직을 toString()을 Override하셨는데요.
java doc도 그렇고 effective java에서도 toString()의 재정의를 권장하고 있습니다.
toString()은 디버깅의 목적 등으로 해당 객체에 대한 정보를 사람이 읽기 쉬운 간결하지만 유익한 표현을 반환하도록 사용된다고 이해했습니다.
전반적으로 toString()이 단순 객체의 정보 전달이 아닌 출력(뷰) 형식에 의존적인 로직이 포함되어 있는 것 같아서요. (관례적으로 쓰는 getXxx(), setXxx() 네이밍의 객체를 보면 기대하는 로직이 있지 않으신가요?!)
저는 toString()메서드를 보았을 때, 뷰 로직이 담겨있을 거라곤 기대하지 않았었습니다!
먼저, 다른 메서드이름을 사용해보는건 어떨까요?
출력에 관련한 로직의 분리가 필요한지는 좀 더 고민해보고 결정해보죠!
There was a problem hiding this comment.
확실히 toString 보다는 다른 메서드 명을 사용하는 것이 나을 것 같네요!
|
|
||
| private void serviceIfHasStaticResourcePath(HttpRequest httpRequest, HttpResponse httpResponse) { | ||
| if (httpRequest.hasStaticResourcePath() || httpResponse.hasStaticResourcePath()) { | ||
| controllerManager = new StaticControllerManager(); |
There was a problem hiding this comment.
여러 쓰레드를 통해 process가 수행 될 때 마다 new StaticControllerManager()가 실행 되는데, 이 부분도 개선 하면 좋겠습니다!
StaticControllerManager의 인스턴스 변수가 없으니 싱글톤으로 제공해도 괜찮아 보여요
There was a problem hiding this comment.
저도 이 부분을 참 많이 고민했었는데요,
sonar lint에서 자꾸 냄새난다고 하는 게 짜증나기도 하고 그래서 일단 매번 생성하도록 했습니다.
싱글턴으로 다시 바꿔볼게요! (5번째 바꾸는...)
| this.httpRequestBody = httpRequestBody; | ||
| } | ||
|
|
||
| public HttpMethod getHttpMethod() { |
|
|
||
| void validateLength(long length) { | ||
| if (length != body.getBytes().length) { | ||
| throw new IllegalArgumentException("Response Body의 길이가 유효하지 않습니다."); |
There was a problem hiding this comment.
발생할 수 있는 상황에 대해 꼼꼼하게 처리하셨군요!👍👍
| public static final String LOCATION = "Location"; | ||
| public static final String RESOURCE_PATH = "Resource-Path"; | ||
|
|
||
| private final Map<String, List<String>> headers; |
There was a problem hiding this comment.
value를 List 관리한 이유가 있나요?
단순 String으로 사용해도 문제가 없어보입니다!
There was a problem hiding this comment.
같은 필드에 대해서 여러 밸류가 존재할 수 있습니다!
예를 들어 쿠키 같은 경우도 여러 개를 실어줄 수 있겠죠..?! ^^
HTTP/1.1 200 OK
Content-Type: text/html
Set-Cookie: cookie1=value1; Expires=Wed, 21 Oct 2023 07:28:00 GMT
Set-Cookie: cookie2=value2; Path=/; Secure
Set-Cookie: cookie3=value3; HttpOnly
There was a problem hiding this comment.
엇 그렇네요!
리오 말씀대로 구현하는게 범용적이고 좋네요!! 감사합니다👍
| @Test | ||
| void POST요청시_이미_가입된_회원이면_예외를_반환한다() { | ||
| // given | ||
| InMemoryUserRepository.save(new User("로이스", "잘생김", "내마음속")); |
|
SonarCloud Quality Gate failed.
|
안녕하세요 로이스!
시간이 없어서 테스트를 다 못 짰네요ㅠㅠ
우선 프로덕션 코드 쪽만 리뷰해주실 수 있으실까요?
급하게 PR 날리느라 코드 설명도 첨부못해서 죄송합니다ㅠㅠㅠㅠ
common 패키지 내부에 인터페이스들로 추상화를 해서 의존성을 분리하려고 해봤어요
리뷰 잘 부탁드립니다 감사합니다요