-
Notifications
You must be signed in to change notification settings - Fork 90
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
[소니] 레벨4 1단계 - HTTP 웹서버 구현 #116
Conversation
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.
안녕하세요, 소니!
구현을 대부분 잘 해주셨지만, 몇 가지 개선점이 필요해 보입니다.
코멘트 남겼으니 확인 부탁드려요~!
src/main/java/web/RequestHeader.java
Outdated
private boolean isEmpty(String line) { | ||
return Objects.isNull(line) || Objects.equals(line, ""); | ||
} |
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 check에 대한 코드라 StringUtils와 같이 공용으로 빼도록 리펙토링하는게 좋아보입니다.
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.
찾아보니까 StringUtils라는 라이브러리가 있더라구요! 현재 필요한 로직은 StringUtils에서 제공하는 메서드로 대체 가능하여 변경했습니다.😊
} catch (IOException e) { | ||
String requestPath = requestLine.getPath(); | ||
byte[] body; | ||
if (requestPath.endsWith("/user/create") && HttpMethod.POST == requestLine.getMethod()) { |
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.
url에 대한 핸들링을 별도의 클래스로 분리시키면 훨씬 깔끔해 질 것 같습니다.
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.
HttpRequestController를 만들어 적용해보았습니다.
@DisplayName("RequestBody 조회") | ||
@Test | ||
void check_RequestBody_Content() throws IOException { | ||
String request = "POST /user/create HTTP/1.1\n" |
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.
동일한 request값을 계속 재사용하는 형태인데 공통으로 사용하도록 추상화 or 분리 시키면 어떨까요?
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.
테스트 데이터를 test/resource 폴더에 따로 분리하였습니다.
추상 클래스를 만들어 테스트 케이스를 불러 HttpRequest를 만드는 부분을 구현해보았습니다.
import org.junit.jupiter.api.DisplayName; | ||
import org.junit.jupiter.api.Test; | ||
|
||
class RequestBodyTest { |
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.
RequestHandler에 대한 리다이렉트 테스트도 추가 부탁드립니다.
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.junit.jupiter.api.DisplayName; | ||
import org.junit.jupiter.api.Test; | ||
|
||
class RequestHeaderTest { |
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.
대부분의 테스트가 되는지의 관점에만 작동하고 있습니다.
예외 케이스 예를 들어, 문자열이 대소문자일 경우, 예상치 못한 값 or 띄어쓰기 or null 등 추가할 수 있는 부분에 대해 추가 부탁드립니다.
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.
예외 사항에 대해서도 테스트케이스를 추가했습니다
src/main/java/web/RequestHeader.java
Outdated
import java.util.Map; | ||
import java.util.Objects; | ||
|
||
public class RequestHeader { |
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.
logger를 이용해 Header를 출력하도록 변경했습니다.
하비 안녕하세요. 구조를 많이 바꾸다 보니 피드백 반영이 늦어졌네요.. 다시 한 번 리뷰 부탁드립니다. 감사합니다! |
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.
안녕하세요, 소니!
패턴의 사용이나 각 객체에 역할 부여가 잘 되어 있네요. 💯
테스트도 잘 작성해 주셔서 추가로 드릴 코멘트가 없네요.
다음 단계 진행 부탁드립니다!
안녕하세요 우테코2기 소니입니다😊
이번 미션은 1단계 요구사항 대로 동작할 만큼만 구현했습니다.
HttpRequest나 HttpResponse 클래스로 분리하는 리팩토링은 2단계 요구사항에 있어 일단 이렇게 PR을 보냅니다!
구현한 부분에서도 부족함이 많을텐데, 피드백 주시면 열심히 고쳐보겠습니다.
감사합니다!