[톰캣 구현하기 3, 4단계] 블랙캣(송우석) 미션 제출합니다. #477
Conversation
There was a problem hiding this comment.
블랙캣 안녕하세요!
이번에도 깔끔한 코드로 개선해주셨네요 ㅎㅎ
1,2 단계 때 워낙 잘해주셔서 구조적으로는 오히려 제가 배우면서 리뷰 많이 했습니다.
이번 단계에서 패키지에 대한 고민도 하셨던 것 같습니다!
요청을 처리하면서, 예외를 발생하는 구조도 인상깊게 잘 봤습니다.
몇 가지 컨벤션이 맞지 않아서 이 부분만 수정해주세요 ㅎㅎ
수정하시고 머지하셔도 될 것 같습니다.
그리고 추가적으로 질문에 블랙캣의 생각을 듣고 싶습니다!
이번 미션도 수고 많으셨습니다 :)
| private final AuthService authService = new AuthService(); | ||
|
|
||
| @Override | ||
| protected void doGet(final HttpRequest request, final HttpResponse response) { |
| @Override | ||
| protected void doPost(final HttpRequest request, final HttpResponse response) { | ||
| try { | ||
| final String account = request.getBodyValueOf(ACCOUNT_QUERY_KEY); |
There was a problem hiding this comment.
현재 아래와 같이 HttpResponse 객체에서 없는 값에 대해선 예외를 터트리고
해당 컨트롤러에서 잡은 후 400 Bad Request 를 던지고 있는 구조입니다!
public String getBodyValueOf(final String body) {
return Optional.ofNullable(this.body.getValue(body))
.orElseThrow(NoSuchBodyValueException::new);
}try {
final String account = request.getBodyValueOf(ACCOUNT_QUERY_KEY);
final String password = request.getBodyValueOf(PASSWORD_QUERY_KEY);
...나머지 로직들...
} catch (NoSuchBodyValueException e) {
setResponse(response, HttpStatus.BAD_REQUEST, "/400.html");
}| this.serverSocket = createServerSocket(port, acceptCount); | ||
| this.executor = Executors.newFixedThreadPool(maxThreads); |
There was a problem hiding this comment.
블랙캣은 newFixedThreadPool를 사용하신 이유는 무엇인가요?!
| } | ||
| } | ||
|
|
||
| protected void doGet(HttpRequest request, HttpResponse response) { |
There was a problem hiding this comment.
doGet, doPost 매개변수에 final 키워드가 누락되어있습니다!
|
|
||
| public class SessionManager { | ||
|
|
||
| private static final Map<String, Session> SESSIONS = new HashMap<>(); | ||
| private static final Map<String, Session> SESSIONS = new ConcurrentHashMap<>(); |
| @@ -0,0 +1,54 @@ | |||
| <!DOCTYPE html> | |||
| public static Controller getSupportedServlet(final HttpRequest request) { | ||
| return servlets.stream().filter(it -> it.isSupported(request)) | ||
| .findFirst() | ||
| .orElseThrow(NotSupportedRequestException::new); |
There was a problem hiding this comment.
| public static Controller getSupportedServlet(final HttpRequest request) { | |
| return servlets.stream().filter(it -> it.isSupported(request)) | |
| .findFirst() | |
| .orElseThrow(NotSupportedRequestException::new); | |
| public static Controller getSupportedServlet(final HttpRequest request) { | |
| return servlets.stream() | |
| .filter(it -> it.isSupported(request)) | |
| .findFirst() | |
| .orElseThrow(NotSupportedRequestException::new); |
ㅎㅎ 컨벤션 다 확인해보는 건 항상 힘든 것 같아요 저도 그렇고 ㅠㅠ
블랙캣의 더 완성도 있는 코드가 되길 바라며 제안해봅니다 👍
| setResponse(response, HttpStatus.OK, "/login.html"); | ||
| } | ||
|
|
||
| private boolean AuthenticationUser(final HttpRequest request) { |
There was a problem hiding this comment.
메서드 명이 대문자와 명사로 시작해서 조금 어색한 것 같아요~
authenticateUser는 어떨까요?
|
Kudos, SonarCloud Quality Gate passed!
|
안녕하세요 제이!
이번에 리팩터링하면서 기능적 변경사항이 몇가지 생겼습니다!
1. 각 요청 처리 시 예외 발생 시 리다이렉트가 아닌 에러 페이지를 에러 코드와 함께 같이 반환
아무래도 클라이언트 측면에서 한번 더 요청을 안보내도 되며, 에러 코드를 통해서 바로 알 수 있도록 하였습니다!
2.
로그인 요청 - 로그인 페이지 요청,회원가입 요청 - 회원가입 페이지 요청컨트롤러를 통합미션에서 요구하는 Controller, AbstractController 구조를 적용하면서 하나의 클래스로 합쳤습니다.
3.
isSupported()를 통한 논리적 null 체크를 제거.미션 1,2 에서는 회원가입 POST 요청 시
isSupported()메서드를 통해서account,password를 가진request만 들어오기 때문에실제
service()단에서 해당body값들을 가져올 때 별도의 null 체킹을 하지 않았는데요,이번에 리팩터링하면서 요청 url 로만 검사를 하게 하였고, 실제로
body값을 가져올 때 해당 값이 없다면 예외를 터트리는 구조로 변경하였습니다.이번 리뷰도 감사합니다! 🙇♂️