[톰캣 구현하기 - 3, 4단계] 도치(김동흠) 미션 제출합니다. #478
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
| private RequestMapping() { | ||
| final AbstractController loginHandler = new LoginHandler("/login"); | ||
| final AbstractController memberRegisterHandler = new MemberRegisterHandler("/register"); | ||
| controllers.add(loginHandler); | ||
| controllers.add(memberRegisterHandler); | ||
| } |
There was a problem hiding this comment.
RequestMapping이 싱글톤 패턴을 따랐다면, 핸들러 개발을 추가될 수 있는 거라고 생각합니다. 그럼 코드를 바꾸는 것보다, 톰캣을 실행할때, 객체를 넣어주는 것이 좋지 않을까요?
그게 아니더라도, 의존성 주입을 활용하면 좋을 것 같습니다
There was a problem hiding this comment.
이 부분은 reflection미션에서 반영할 수 있을 것 같아요!
| public Controller getController(HttpRequest request) { | ||
| for (AbstractController controller : controllers) { | ||
| if (controller.isMatch(request)) { | ||
| return controller; | ||
| } | ||
| } | ||
| return new EmptyController(); |
There was a problem hiding this comment.
Controller를 반환하는데 for문에서 다형성을 AbstractController로 적용한 이유가 뭘까요? 업캐스팅이 일어날 것 같습니다.
| this(DEFAULT_PORT, DEFAULT_ACCEPT_COUNT, 50); | ||
| } | ||
|
|
||
| public Connector(final int port, final int acceptCount) { | ||
| public Connector(final int port, final int acceptCount, final int maxThreads) { | ||
| this.serverSocket = createServerSocket(port, acceptCount); | ||
| this.stopped = false; | ||
| this.maxThreads = maxThreads; | ||
| this.executorService = new ThreadPoolExecutor( | ||
| Math.max(this.maxThreads, DEFAULT_MAX_THREAD), | ||
| Math.max(this.maxThreads, DEFAULT_MAX_THREAD), |
There was a problem hiding this comment.
질문
- 매개변수로 들어온 최대 스레드 수와 기본 스레드 수를 200으로 둔 이유가 뭔가요?
- 스레드 풀의 최소 수와 스레드 풀이 만들 수 있는 최대 스레드 수를 동일하게 둔 이유가 뭔가용?
There was a problem hiding this comment.
-
에 대해서는 사실 최적의 성능을 내려면 테스트를 통해 정하는 게 좋겠지만, 그 정도의 최적화가 필요한 상황은 아니라 많은 고려를 하지 는 않았습니다.. max thread를 정할 때 기준이 될만한 점이라면
서비스가 cpu사용이 많은 서비스인지-io로 인한 대기가 많은 서비스 인지에 따라 많은 스레드가 있어도 그 switching을 cpu가 충분히 감당할 수 있는지 가 하나의 기준이 될 것 같고, 많은 스레드를 감당할만큼 서버의 메모리 등의 자원이 충분한가 도 기준이 될 것 같아요! -
는 실제 사용하는 서비스라 한다면 가장 적은 사용자가 있는 시간대에도 이 정도 스레드는 유지되어야 한다는 기준으로 설정할 것 같아요. 이 프로젝트만으로는 최소 스레드로 마땅히 기준 삼아야될 게 생각나지 않네요.. 그래도 최대 스레드 수보다는 작게 하는게 좋았을 것 같습니다
|
|
||
| public class SessionManager { | ||
|
|
||
| private static SessionManager sessionManager; | ||
| private static final Map<String, Session> SESSIONS = new HashMap<>(); | ||
| private static final Map<String, Session> SESSIONS = new ConcurrentHashMap<>(); |
| public class RequestMapping { | ||
|
|
||
| private static RequestMapping instance; | ||
| private final List<AbstractController> controllers = new ArrayList<>(); |
There was a problem hiding this comment.
후술하겠지만, Controllers를 map형태로 가지고 있으면 어떨까요?
<url(String)과 Controller>로요, 그런다면, 다형성도 지킬 수 있지 않을까요? 현재 코드는 다형성이 조금 깨진 것으로 보입니다.
There was a problem hiding this comment.
isMatch()로 검사하는 게 마음에 걸렸는데.. 맞는 말입니다. 수정하겠습니다!
| } else if (httpMethod.equals("GET")) { | ||
| doGet(httpRequest, httpResponse); | ||
| } else { | ||
| redirectNotFoundPage(httpResponse); |
There was a problem hiding this comment.
이 부분에 대해서는 로그인 핸들러와 같은 질문입니다. 일치하는 메서드가 없으면 아무것도 응답하지 않아도 될 것 같습니다.
그리고 로그인과 회원가입 컨트롤러의 경우 왜 이름이 핸들러인가요? 클래스명에 통일성이 있으면 좋을 것 같아요!
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class MemberRegisterHandler extends AbstractController { |
There was a problem hiding this comment.
패키지로 따지자면, AbstractController의 구현체들은 jwp패키지에 있는 것이 더 적합하지 않을까요? 카탈리나의 리퀘스트 매핑에서 요청에 해당하는 컨트롤러들을 실행시키는데
해당 컨트롤러는 개발자가 서블릿의 구조를 모르고 사용해야 하기 때문입니다.
| final RequestHandler memberRegisterHandler = new MemberRegisterHandler("/register"); | ||
| handlers.add(loginHandler); | ||
| handlers.add(memberRegisterHandler); | ||
| this.requestMapping = RequestMapping.getInstance(); |
There was a problem hiding this comment.
패키지구조를 나눠놨는데, RequestMapping을 직접적인 의존하기 보다는 중간에 인터페이스를 두면 어떨까요?
There was a problem hiding this comment.
RequestMapping은 인스턴스가 하나뿐인 객체이고, 다형성도 현 상태로는 필요치 않아서 패키지 의존성 역전의 의미만으로 인터페이스를 두는 건 과한 것 같다는 생각이 들었어요!
There was a problem hiding this comment.
넵!
개인적으로는 패키지 의존성 분리를 위해서 인터페이스를 두는 게 좋다고 생각했슴니다!
| @@ -92,4 +72,40 @@ private String contentType(final String resourcePath) { | |||
| } | |||
| return "text/html"; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
private String contentType(final String resourcePath) {
if (resourcePath.endsWith(".css")) {
return "text/css";
}
return "text/html";
}
부분의 경우 Enum으로 관리하면 어떨까요?
그리고 js나 svg ico같은 파일의 경우 어떻게 파일을 넘겨주시나요?
There was a problem hiding this comment.
html 이나 css파일도 정적 리소스 컨트롤러가 처리하는 것 아닌가용?
제가 코드에서 발견을 못한 듯 싶네요
| public String getResponse() throws IOException { | ||
| return buildResponse(); | ||
| public void addCookie(final String key, final String value) { | ||
| cookieValues.put(key, value); | ||
| } | ||
|
|
||
| private String buildResponse() throws IOException { |
There was a problem hiding this comment.
final File requestedResource = new File(getResource().getFile());
final String responseBody = buildResponseBody(requestedResource);
final StringBuilder sb = new StringBuilder();
sb.append(HTTP_VERSION + " " + httpStatusCode + " " + statusMessage + " ").append(CRLF)
.append("Content-Type: " + contentType(resource.getPath()) + ";charset=utf-8 ").append(CRLF)
.append("Content-Length: " + responseBody.getBytes().length + " ").append(CRLF)
.append(cookieResponse(cookieValues)).append("\r\n")
.append(responseBody);
return sb.toString();
}
이전과 같은 코드여서 리뷰를 이렇게 밖에 못남기네요.
이어지는 위의 코드에서, 마지막부분만 .append(cookieResponse(cookieValues)).append("\r\n")로 되어있네요 이부분도 상수화하면 좋을 것 같습니다.
잘 부탁드려요 이레~~
이전 pr의 질문에도 답변 남겼어요!