[톰캣 구현하기 - 3, 4단계] 아마란스(송세연) 미션 제출합니다. #440
Conversation
- 요청에 대한 처리를 Controller가 수행하도록 설계
- 자잘한 문법 경고 해결
|
|
||
| private SessionManager() { | ||
| } | ||
| private static final SessionManager INSTANCE = new SessionManager(); |
There was a problem hiding this comment.
INSTANCE를 대문자로 써야 할까요? INSTANCE가 상수가 맞나요?
https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names
There was a problem hiding this comment.
습관적으로 싱글톤 패턴을 적용할 때마다 인스턴스를 상수 취급했었는데, 제가 상수에 대해 간과하고 있었네요...!
알려주셔서 감사합니다
| private static final SessionManager INSTANCE = new SessionManager(); | ||
| private static final ConcurrentMap<String, Session> SESSIONS = new ConcurrentHashMap<>(); | ||
|
|
||
| public static SessionManager InstanceOf() { |
There was a problem hiding this comment.
메서드명이 대문자로 시작하는게 코딩 스타일 가이드에 맞나요?
| if (id == null) { | ||
| return null; | ||
| } | ||
| return SESSIONS.get(id); |
There was a problem hiding this comment.
getOrDefault(...) 메서드를 활용해보시기 바랍니다.
|
|
||
| private final ServerSocket serverSocket; | ||
| private boolean stopped; | ||
| private final ThreadPoolExecutor threadPool; |
There was a problem hiding this comment.
ExecutorService를 사용하시고, 왜 바꾸라고 하는지 고민해보시고 코멘트에 의견 남겨주세요.
There was a problem hiding this comment.
이제보니 ExecutorService는 ThreadPoolExecutor가 구현하는 인터페이스군요.
필드 타입을 인터페이스로 변경하는 건 더 높은 확장성을 갖기 때문에 권장되는 것으로 알고 있는데, 이것이 구구가 의도하신 바가 맞을까요?
| this.serverSocket = createServerSocket(port, acceptCount); | ||
| this.stopped = false; | ||
| this.threadPool = new ThreadPoolExecutor(acceptCount, maxThreads, 0L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<>(100)); |
There was a problem hiding this comment.
https://www.baeldung.com/java-executor-service-tutorial
- 위 링크를 참조하여 Executors를 사용해서 스레드풀을 만들어보세요.
- ThreadPoolExecutor의 생성자 파라미터에서 keepAliveTime을 0으로 설정한 이유가 뭔가요?
There was a problem hiding this comment.
혹시 Executors에서 제공하는 정적 팩토리 메서드에서도 ThreadPoolExecutor 객체를 생성하고 반환하고 있는데, 혹시 왜 정적 팩토리 메서드를 호출해서 생성하기를 권장하시는 것인지 여쭤봐도 될까요?
저는 ThreadPoolExecutor 생성자를 직접 호출하는 편이 파라미터 값을 세밀하게 설정해줄 수 있어서 요구사항을 만족시키기 적절하다고 생각했었습니다.
- 사실 어느 값이 적절할지 마땅히 떠오르지 않아서 큰 이유 없이 설정해준 값입니다. 이제보니 너무 짧은 것 같아 0.3초로 수정해주었습니다.
There was a problem hiding this comment.
- 상황에 따라 세부설정을 하는 것도 좋지만 자바가 제공하는 기능을 활용하면 의도를 더 쉽게 전달할 수 있겠네요.
- keepAliveTime는 어떤 값이 적절할지 좀 더 찾아보시면 좋을 것 같아요!
| if (userOptional.isPresent() | ||
| && userOptional.get().checkPassword(parsedRequestBody.get("password"))) { | ||
| String setCookie = null; | ||
| if (!cookie.isExist(JSESSIONID)) { |
| Map<String, String> parsedRequestBody = parseRequestBody(request); | ||
| Optional<User> userOptional = InMemoryUserRepository.findByAccount( | ||
| parsedRequestBody.get("account")); | ||
| if (userOptional.isPresent() |
There was a problem hiding this comment.
if절 조건문은 복잡하게 작성하지 마세요.
적절한 메서드로 추출해서 사람이 의미를 파악하기 쉽게 만드세요.
| SessionManager.InstanceOf().addLoginSession(jSessionId, userOptional.get()); | ||
| } | ||
| HttpResponseHeader responseHeader = new HttpResponseHeader( | ||
| getContentType(request.getAccept(), request.getPath()), |
There was a problem hiding this comment.
매번 클라이언트에서 어떤 파라미터를 전달해야할지 신경써야겠네요.
내가 만든 톰캣을 쓰는 사람이 편하려면 어떻게 하는게 좋을까요?
There was a problem hiding this comment.
빌더 패턴을 적용해서 원하는 파라미터만 넣을 수 있게 하면 될까요....?🤔
| String[] queryTokens = request.getBody().split("&"); | ||
| for (String queryToken : queryTokens) { | ||
| int equalSeparatorIndex = queryToken.indexOf("="); | ||
| if (equalSeparatorIndex != -1) { |
There was a problem hiding this comment.
전체 코드에서 모든 2뎁스를 1뎁스로 만드세요.
| return handle401(request); | ||
| } | ||
|
|
||
| private Map<String, String> parseRequestBody(HttpRequest request) { |
There was a problem hiding this comment.
http와 관련된 처리는 각 객체에서 처리하도록 만들어야 비즈니스 로직에 집중할 수 있습니다.
지금 코드는 http 처리와 비즈니스 로직이 섞여 있어요.
- HTTP 요청에 Cache-Control 헤더 추가
- Config 파일 수정
- instance를 상수 표기에서 변수 표기로 변경 - 메서드 첫글자가 대문자에서 소문자로 수정
- HttpRequestHeader, HttpRequestBody가 입력 데이터를 적절히 파싱하도록 함
|
Kudos, SonarCloud Quality Gate passed!
|
|
+) sonarLint 테스트가 통과하도록 수정을 해봤는데, 하나가 도저히 사라지지 않네요... |
| final HttpResponse response = new HttpResponse(); | ||
| controller.service(httpRequest, response); | ||
| outputStream.write( | ||
| response.toString() |
There was a problem hiding this comment.
요런때는 response에 메서드를 새로 추가하는게 낫겠네요
outputStream.write(response.getBytes());
아참 요건 싱글톤 패턴을 제안한 걸로 보이는데 굳이 적용하지 않아도 괜찮겠네요 |
안녕하세요, 1,2 레벨 때 리뷰받은 내용을 토대로 리팩토링해보았습니다.
잘 부탁드려요!