-
Notifications
You must be signed in to change notification settings - Fork 388
[Tomcat 구현하기 - 3단계] 커비(이현지) 미션 제출합니다. #677
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
Conversation
unifolio0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기능 구현과 테스트에는 문제가 없어서 merge합니다! 하지만 4단계에서 꼭 전부 반영해주세요~
| RequestBody requestBody = request.getBody(); | ||
| String account = requestBody.get("account"); | ||
| String password = requestBody.get("password"); | ||
| String email = requestBody.get("email"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getBody()를 통해 RequestBody를 불러와서 물어보는 것보다 request.getBodyValue()와 같은 메소드를 만드는 건 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
물어보는 편이 낫겠네요! 반영완료!
| } | ||
| } | ||
|
|
||
| private static void login(HttpRequest request, HttpResponse response, SessionManager sessionManager) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10줄이 넘어요😭😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드 분리해보았습니다!
| String[] cookiesElements = rawCookies.split("; "); | ||
| for (int i = 0; i < cookiesElements.length; i++) { | ||
| String[] cookiePair = cookiesElements[i].split("="); | ||
| cookieGroup.put(cookiePair[0], cookiePair[1]); | ||
| if (cookiePair.length > 1) { | ||
| cookieGroup.put(cookiePair[0], cookiePair[1]); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depth가 너무 깊은 것 같아요😭😭
상수처리도 부탁드려요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반영 완료!
| httpResponse.addHeader(HeaderName.CONTENT_TYPE, httpRequest.getContentType()); | ||
| httpResponse.addHeader(HeaderName.SET_COOKIE, httpRequest.getHttpCookie()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Response에 Content-Type과 Set-Cookie가 꼭 필요한 요소였나요...?🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content-Type은 보통 있는 걸로 알고있어요!
Set-cookie는 이전에 보내진 쿠키가 있으면 유지하기 위해서인데 해당 맥락을 위해 조건문을 추가하였습니다!
| if (httpRequest.isStaticRequest()) { | ||
| staticController.service(httpRequest, httpResponse); | ||
| } | ||
| if (!httpRequest.isStaticRequest()) { | ||
| Controller controller = controllers.get(httpRequest.getPath()); | ||
| controller.service(httpRequest, httpResponse); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (httpRequest.isStaticRequest()) { | |
| staticController.service(httpRequest, httpResponse); | |
| } | |
| if (!httpRequest.isStaticRequest()) { | |
| Controller controller = controllers.get(httpRequest.getPath()); | |
| controller.service(httpRequest, httpResponse); | |
| } | |
| if (httpRequest.isStaticRequest()) { | |
| staticController.service(httpRequest, httpResponse); | |
| return; | |
| } | |
| Controller controller = controllers.get(httpRequest.getPath()); | |
| controller.service(httpRequest, httpResponse); |
이렇게 처리하는 건 어떤가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
더 깔끔하네요~ 반영 완료!
| } | ||
|
|
||
| @Override | ||
| protected void doGet(HttpRequest request, HttpResponse response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if문 안의 조건을 메서드로 추출하면 더 보기 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반영 완료!
| import org.apache.coyote.request.HttpRequest; | ||
| import org.apache.coyote.response.HttpResponse; | ||
|
|
||
| public class StaticController extends AbstractController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정적 파일을 처리하는 Controller이랑 동적으로 처리하는 Controller이랑 같은 패키지에 있는게 어색해보이네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정적 컨텐츠 처리 책임은 웹서버, 즉 서블릿보다 앞 단에 있어야할 것 같아서 해당 책임을 processor로 옮겼습니다!
| private String content; | ||
|
|
||
| public ResponseBody() { | ||
| this.content = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
질문입니다!""를 넣어주는 지가 궁금해요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
초기화를 위해 넣어주었습니다!
| StringBuilder response = new StringBuilder(); | ||
| response.append(versionOfProtocol) | ||
| response.append(versionOfProtocol.getValue()) | ||
| .append(" ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수화 가능할 것 같아요!
귀찮으면 formatting도 괜찮을 지도...
public String createStatusLineResponse() {
return "%s %s %s ".formatted(version, httpStatusCode.getCode(), httpStatusCode.getMessage());
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
훨씬 간략하네요👍
| // then | ||
| assertThat(response).isEqualTo("HTTP/1.1 200 OK"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Controller 테스트 무조건 작성부탁드려요
안녕하세요 비토!
이번 단계에서 진행한 리팩토링은 다음과 같아요!
Processor에서 input, outputStream을 받고
Dispatcher에서 컨트롤러로 연결, 기본적인 헤더, 응답 설정
Controller에서는 비즈니스 로직으로 연결되게 분리해보았습니다!
ps. 학습 테스트에서 컨텍스트 로딩 오류가 계속 발생하여 새파일로 덮어쓴다음에 다시 테스트를 작성하는 이슈가 있었습니다😂
ps. 부족한 테스트는 더 추가하겠습니다😂