Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1-2단계 톰캣 구현하기] 감자(김주하) 미션 제출합니다. #509

Merged
merged 18 commits into from
Sep 6, 2024

Conversation

khabh
Copy link

@khabh khabh commented Sep 5, 2024

안녕하세요 페드로🤖 리뷰이 감자입니다 🥔🥔
레벨1, 2 때 대화를 많이 나눠보지 못했는데, 이번 미션으로 소통할 기회가 생겨서 좋네요 🥰
리뷰 잘 부탁드립니다~

@hw0603 hw0603 self-requested a review September 6, 2024 13:43
Copy link
Member

@hw0603 hw0603 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 감자! 작성해 주신 코드 잘 읽었습니다.

제 코드와 다르게 전반적으로 코드가 깔끔하네요👍
Optional API를 잘 활용하시는 것 같아요.

아직 리팩토링을 진행하는 단계가 아니기도 하고, 수정할 부분이 많지 않아 이번 단계에서는 간단한 질문과 고민해 볼 만한 점들을 코멘트로 남겨두고 승인하겠습니다!

다음 단계 진행 시 확인해 보시고 선택적으로 반영해주세요.

Comment on lines +11 to +13
validateAccount(account);
validatePassword(password);
validateEmail(email);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

도메인 객체에 검증 책임을 잘 부여해 주셨네요👍

@@ -29,7 +27,7 @@ public interface Manager {
*
* @param session Session to be added
*/
void add(HttpSession session);
void add(Session session);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[질문] Session은 어떻게 보면 저희가 만든 커스텀 객체인데, 인터페이스 시그니처를 커스텀 객체에 맞게 수정하기보다는 인터페이스에서 정의하는 시그니처를 따를 수 있도록 Session 객체를 정의해 주는 것이 맞지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확실히 기존 Manager 시그니처를 따르도록 Session을 정의하는 게 맞는 방향이겠네요, 수정했습니다!

"/register",
"/index"
);
private final static SessionManager SESSION_MANAGER = new SessionManager();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static final 순서가 바뀌어 있네요!


final var responseBody = "Hello world!";
BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(inputStream));
HttpRequest request = HttpRequest.of(bufferedReader);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HttpResponseWriter를 모르는데, HttpRequestReader를 알아야 하는 구현이 보는 사람에 따라 다소 어색하게 느껴질 수 있을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HttpRequestReader 클래스를 추가해서 HttpRequestBufferedReader를 받지 않는 구조로 수정해 봤습니다!

return String.join(LINE_SEPARATOR, formattedHeaders);
}

// public void addSession(Session session) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용하지 않는 주석은 지워주세요!

public static ResponseFile of(URL resource) throws IOException {
File resourceFile = new File(resource.getFile());
Path resourceFilePath = resourceFile.toPath();
String contentType = Files.probeContentType(resourceFilePath) + ";charset=utf-8";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

와 이런 메서드가 있었네요😲

}
ResponseFile responseFile = ResponseFile.of(resource);
return HttpResponse.createFileResponse(responseFile);
} catch (Exception e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좀 더 명확한 예외를 잡는 것이 좋을 것 같아요

return HttpResponse.createRedirectResponse(HttpStatus.FOUND, "/index.html");
}

private HttpResponse login(HttpRequest request) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

한 사용자가 로그인 API를 반복적으로 호출하면 호출되는 수 만큼 메모리에 세션이 생길 것 같은데 이 부분은 어떻게 생각하시나요?

현재의 구현도 괜찮을까요? 세션에 이미 존재하는 사용자라면 반복적인 로그인 요청 시 새로운 세션을 만들지 않도록 처리해야 할까요?

Copy link
Author

@khabh khabh Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

페드로의 말대로 기존에 로그인 시 생성한 세션이 계속 누적되는 문제가 발생할 수 있겠네요 🥲 세션에 이미 존재하는 사용자일 경우에는 새로운 세션을 생성하지 않고, 기존 세션을 업데이트 하는 방식으로 수정했습니다

refactor: 세션이 있는 사용자의 로그인 요청 시 기존 세션의 User를 업데이트하도록 수정

@hw0603 hw0603 merged commit 728632e into woowacourse:khabh Sep 6, 2024
@khabh khabh deleted the khabh branch September 7, 2024 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants