Skip to content

Conversation

@mzeong
Copy link
Member

@mzeong mzeong commented Sep 6, 2024

안녕하세요 시소! 제리입니다 👋

  • 학습 테스트(파일, 입출력 스트림, HTTP 활용하기)를 모두 완료했습니다.
  • 1, 2단계 미션 구현을 완료했습니다.
  • 코드가 전혀 깔끔하지 않습니다. 빠르게 리팩토링 해볼게요. (시소 미안합니다.. 😵‍💫😵‍💫)
  • 2단계 Session 구현하기를 제대로 구현하지 못해 오작동하는 부분이 있어요. (유효하지 않은 사용자도 로그인 처리됨)

편하게 리뷰 남겨주세요! 잘 부탁드립니다

Copy link

@shin-jisong shin-jisong left a comment

Choose a reason for hiding this comment

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

안녕하세요 제리!
깔끔하지 못한 코드라고 말씀해 주셨지만 2단계까지를 깔끔하게 잘 작성하셨다고 생각합니다 👍
말씀하신 보충 사안들 무리하지 마시고 완로 후 재요청 보내 주세요 🙇‍♂️

@@ -0,0 +1,22 @@
## 1단계 - HTTP 서버 구현하기

Choose a reason for hiding this comment

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

README를 작성해 주셨네요! 추후 확인하기 좋은 방안이에요 👍

tomcat/README.md Outdated
- [x] 회원가입 페이지를 보여줄 때는 GET 사용, 회원가입 버튼을 누르면 POST 사용
- [x] 회원가입 완료 시 `/index.html`로 리다이렉트
- [x] 로그인 페이지에서도 로그인 버튼을 누르면 POST 사용
- [ ] Cookie에 JSESSIONID 값 저장하기
Copy link

@shin-jisong shin-jisong Sep 7, 2024

Choose a reason for hiding this comment

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

완료되지 않은 부분은 어떻게 진행되는 걸까요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

Cookie 클래스를 만들지 않아도 Cookie에 JSESSIONID 값을 저장할 수 있어서 진행하지 않았는데
리팩토링 과정에서 HttpCookies 클래스를 만들어줬습니다!

}

public int findContentLength() {
if (fields.containsKey("Content-Length")) {

Choose a reason for hiding this comment

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

재사용되고 있는 단어들은 변수로 빼보는 것도 좋을 듯해요!

Copy link
Member Author

Choose a reason for hiding this comment

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

HeaderKey 이넘을 생성해 하드코딩된 문자열을 개선했습니다!

boolean jsessionid = request.getHeaders().findInCookie("JSESSIONID");
String setCookie = null;
if (!jsessionid) {
UUID idValue = UUID.randomUUID();

Choose a reason for hiding this comment

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

세션의 ID를 랜덤으로 생성하는 것은 Session의 책임이라고 볼 수도 있지 않을까요?
제리는 어떻게 생각하시나요?
(리팩토링 예정인 걸 알지만 Session 클래스가 존재해서 여쭤보았어요 추후 고려해 주시면 좋을 듯해요!)

Copy link
Member Author

Choose a reason for hiding this comment

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

Session 내에 ID를 랜덤으로 생성하여 세션을 만드는 정적 팩터리 메서드를 두었습니다 👍

Copy link

@shin-jisong shin-jisong left a comment

Choose a reason for hiding this comment

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

코멘트로 남겨 다시 리뷰 남깁니다!

@mzeong
Copy link
Member Author

mzeong commented Sep 11, 2024

안녕하세요 시소! 제리입니다
지난 description에서 설명했던 오류(유효한 JSESSIONID가 아니더라도 로그인 처리)는 해결되었습니다

지난 리뷰 요청 이후 변경사항만 모아보기

@shin-jisong
Copy link

잘 반영해 주셔서 이만 머지합니다! 수고하셨어요 👍
바쁘셔서 그랬겠지만 테스트 코드가 없어서 여유가 되신다면 테스트 코드도 보강해 보면 어떨까 싶어요!

@shin-jisong shin-jisong merged commit 7ba6940 into woowacourse:mzeong Sep 11, 2024
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