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단계] 헤나(박현서) 미션 제출합니다. #378

Merged
merged 50 commits into from
Sep 5, 2023

Conversation

hyena0608
Copy link

@hyena0608 hyena0608 commented Sep 4, 2023

안녕하세요 로지 ~ 리뷰 잘 부탁드립니다 👍

특정 부분 내용 남기겠습니다!

  • Http11ResponseConverter
    • 응답할 HttpResponse를 byte[] 로 변환시켜주는 부분입니다.
    • 현재는 크게 필요하지 않다고 생각되는 부분입니다.
  • RequestHandlerComposite
    • HttpMethod와 RequestPath로 매핑된 RequestHandler 입니다.
    • RequestHandler는 인터페이스로 여러 요청에 맞는 핸들러들이 구현되어 있습니다.
      • UserLoginRequestGetHandler : 로그인 페이지 핸들러
      • UserRegisterRequestGetHandler : 회원가입 페이지 핸들러
      • UserRegisterRequestPostHandler : 회원가입 요청 핸들러
      • HomeRequestHandler : 'Hello world!' 기본 페이지 핸들러
      • ResourceRequestHandler : 정적 페이지 핸들러 (html, css, js)
  • RequestLine
    • ex) GET /login?account=hyena HTTP/1.1
  • RequestPath
    • ex) /login
  • ResourceReader
    • 정적 파일(html, css, js)를 읽기 위한 유틸 클래스입니다.

큰 흐름은 아래와 같습니다~
image

@hyena0608 hyena0608 self-assigned this Sep 4, 2023
@sonarcloud
Copy link

sonarcloud bot commented Sep 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

Copy link
Member

@kyY00n kyY00n left a comment

Choose a reason for hiding this comment

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

하이 헤나!

읽는 내내 감탄하다가 2회독을 해도 코멘트를 못 남겼어요.
변수/메서드명 같은 건 개인 취향이 많이 탄다고 생각해서 따로 코멘트는 안달았어요. 중요한 건 아닌 것 같아서요. 그래서 approve 해요. (죄송..)

사실 어제 처음 PR 확인했을 땐 코드가 2500줄이라 놀랐는데,
오늘 다시 읽어보니 구조가 간결하고 로직이 깔끔해서 편하게 읽었어요!
샘플코드의 하드코딩이 흔적도 없이 사라졌다는 점도 멋졌습니다 :D

질문이 딱 하나 있었던 것 같은데 다음 미션 진행하면서 그 이야기만 조금 나눠보면 될 듯합니다! 수고했어요 헤나.


import java.util.Arrays;

import static java.util.Locale.ENGLISH;
Copy link
Member

Choose a reason for hiding this comment

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

신기하네여, ENGLISH 안쓰면 어떻게 돼요?

Copy link
Author

Choose a reason for hiding this comment

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

대소문자 변경 조건이 다른 언어들이 있어서 값이 달라지는 문제가 생길 수도 있다고해서 습관적으로 명시했어. 지금은 물론 ENGLISH는 안써도 사실 크게 상관은 없을듯!

Comment on lines +53 to +54
public static class HttpResponseBuilder {

Copy link
Member

Choose a reason for hiding this comment

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

너무너무 좋은 것 같아요 👍🏻 저도 쓸래요

return this;
}

public HttpResponseBuilder sendRedirect(final String uri) {
Copy link
Member

Choose a reason for hiding this comment

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

오 저는 setHttpStatus랑 addHeader(LOCATION, uri) 이런식으로 할거라 생각했는데
sendRedirect로 합쳤네요 추상적..👍🏻

processor.process(socket);

// then
final String httpResponse = HttpFormTestUtils.builder()
Copy link
Member

Choose a reason for hiding this comment

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

볼수록 감탄.. 진짜 나 이거 스트링으로 해서 개고생했는데..

}

@Test
void chart_bar_js를_응답한다() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

js 테스트는 코드가 거의 비슷해서 ParameterizedTest 써도 괜찮을거같기도 하거.. 아닐거같기도 하고 헤나맘

Copy link
Author

Choose a reason for hiding this comment

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

@ParameterizedTest 의존성 없어서 사용안했는데 다음 단계에서 추가해볼게~

Comment on lines +23 to +27
mapping.put(new MappingInfo(GET.name(), "/"), new HomeRequestHandler());
mapping.put(new MappingInfo(GET.name(), "/login"), new UserLoginRequestGetHandler());
mapping.put(new MappingInfo(GET.name(), "/register"), new UserRegisterRequestGetHandler());
mapping.put(new MappingInfo(POST.name(), "/register"), new UserRegisterRequestPostHandler());
}
Copy link
Member

Choose a reason for hiding this comment

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

멋져요.. 🚀👏🏻 한눈에 구조를 볼 수 있어 행복합니다

Comment on lines +12 to +13
private Http11ResponseConverter() {
}
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

@kyY00n kyY00n merged commit 6f23da7 into woowacourse:hyena0608 Sep 5, 2023
2 checks passed
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.

2 participants