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

[nathan & jerry] 웹서버 4단계 - 쿠키를 이용한 로그인 구현 #63

Open
wants to merge 13 commits into
base: nathan-jerry
Choose a base branch
from

Conversation

jeremy0405
Copy link
Member

안녕하세요 nathan과 jerry입니다.

4단계 미션 PR을 보냅니다. 😄

항상 감사드립니다.


기능요구사항

  • 로그인 메뉴 클릭 시 페이지 이동
  • 로그인 기능
  • 로그아웃 기능

한 일

refactor: Controller 역할을 하던 Response 클래스를 추출해서 Controller Class 작성
feat: 로그인, 로그아웃 기능 구현 (SessionId=uuid 형태로 쿠키 세팅)
docs: 미션 4단계에서 학습한 내용 README 정리

@jeremy0405 jeremy0405 added the review-BE Improvements or additions to documentation label Mar 30, 2022
@wheejuni wheejuni self-requested a review March 31, 2022 21:58
@wheejuni wheejuni self-assigned this Mar 31, 2022
Copy link

@wheejuni wheejuni left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다. 💯
그런데 테스트가 너무 없습니다.

특히 Pair 클래스에 대한 테스트는 모든 경우의 수에 대해 진행해 주시면 좋겠습니다.

Comment on lines +21 to +23
map.put(new ControllerMapper(HttpMethod.POST, "/user/create"), UserJoinController.getInstance());
map.put(new ControllerMapper(HttpMethod.GET, "/user/logout"), UserLogoutController.getInstance());
map.put(new ControllerMapper(HttpMethod.POST, "/user/login"), UserLoginController.getInstance());

Choose a reason for hiding this comment

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

이미 여기서 하나의 객체만 보관해서 로직을 처리할때 꺼내는 구조로 되기 때문에, 굳이 하부 컨트롤러들이 싱글턴일 이유가 없어 보이기도 합니다.


@Override
public void process(Request request, Response response) throws IOException {
byte[] body = Files.readAllBytes(new File("./webapp" + request.getPath()).toPath());

Choose a reason for hiding this comment

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

FileReader 같은 클래스가 따로 있으면 좋을 것 같은 느낌이네요~

}
log.debug("Save Fail: {}", user);
pairs.add(new Pair("Location", "http://localhost:8080/user/form.html"));
response.write(HttpStatus.FOUND, pairs);

Choose a reason for hiding this comment

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

실패해도 FOUND 군요?

@@ -0,0 +1,61 @@
package util;

public class Pair {

Choose a reason for hiding this comment

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

이런 종류의 클래스를 설계하실 때는 정말 정말 신중해야 합니다.

  1. 꼭 필요한가요?
  2. 이미 있지 않을까요? 다른 라이브러리에서 찾아 보았습니까?
  3. 테스트가 충분히 되었나요?

이 세가지 질문에 당당하게 대답하실 수 있으셔야 합니다.

Comment on lines +4 to +5
private final String key;
private final String value;

Choose a reason for hiding this comment

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

문자열만 받을 수 있게 설계하신 것은 아쉽습니다!

private final String value;

public Pair(String key, String value) {
this.key = key.trim();

Choose a reason for hiding this comment

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

정말 .trim() 해도 괜찮을까요?

Comment on lines +20 to +22
public boolean isContentLength() {
return key.equals("Content-Length");
}

Choose a reason for hiding this comment

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

범용적으로 사용될 수 있는 클래스인데... 프레젠테이션 계층에 강하게 결합하고 있군요.
별로 좋은 메소드가 아닌 듯 합니다 ^^;;

Comment on lines +7 to +15
public static HttpMethod create(String httpMethod) {
if (httpMethod.equals("GET")) {
return GET;
}
if (httpMethod.equals("POST")) {
return POST;
}
return null;
}

Choose a reason for hiding this comment

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

앞으로 메서드가 추가될때마다 (PUT, PATCH 등...) 로직도 계속 새로 작성하실 거죠?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-BE Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants