Skip to content

[1단계 Tomcat 구현하기] 폰드(권규택) 미션 제출합니다. #577

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

Merged
merged 7 commits into from
Sep 7, 2024

Conversation

tackyu
Copy link

@tackyu tackyu commented Sep 6, 2024

안녕하세요 백호! BE 폰드입니다.🙇
웹 서버 구현이라기 보다는 미션 요구 사항만 최소한으로 지키게 되었네요..
따끔한 리뷰 해주십쇼..
감사합니다!

Copy link
Member

@Arachneee Arachneee left a comment

Choose a reason for hiding this comment

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

안녕하세요! 폰드
1단계라 리뷰할 내용이 별로 없네요.
빨리 머지하고 다음 단계를 진행하는 것이 좋을 것 같아요

Comment on lines +54 to +55
String queryString = requestUri.substring(index + 1);
String[] queryParameters = queryString.split("&");
Copy link
Member

Choose a reason for hiding this comment

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

QueryString 형식에 대한 예외 처리가 추가되면 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

= 확인을 통해 key와 value가 없는 상황을 검증하도록 수정하였습니다!

if (Objects.equals(requestUri, "/")) {
return "Hello world!";
}
if (requestUri.startsWith("/login?")) {
Copy link
Member

Choose a reason for hiding this comment

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

url에 /login만 입력했을 때는 로그인 페이지에 접속하지 못할 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

url에 확장자가 없는 경우, html을 추가하도록 수정하였습니다.
다른 확장자에 대해서는 3단계에서 리팩터링 하겠습니다!

@Arachneee Arachneee merged commit abf2fff into woowacourse:tackyu Sep 7, 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