[톰캣 구현하기 1, 2단계] 쥬니(전정준) 미션 제출합니다. #349
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
안녕하세요! 쥬니. 리뷰어 에단입니다! 이렇게 만나뵙게 되어 영광입니다🙇♂️🙇♀️🙇
짧은 시간임에도 불구하고 HttpRequest와 HttpResponse 객체를 만들다니 대단하신것 같습니다! 걱정하신거와는 달리 코드가 읽기 쉬워서 좋았습니다! 쥬니 코드를 보고 많이 배웠습니다. 제 코드가 너무 부끄러워지네요. 저도 열심히 리팩토링 해봐야겠습니다.
3단계가 리팩토링이라 최대한 설계에 관한 질문은 자제했습니다. 설계 부분은 의도를 묻는 질문 위주로 남겨보았습니다. 코멘트는 다음 단계에서 반영해주시면 됩니다. 이번 단계는 approve 하겠습니다. 더 멋진 코드 기대할게요.
| private static final int PATH_INDEX = 1; | ||
| private static final String ACCOUNT = "account"; | ||
| private static final String PASSWORD = "password"; | ||
|
|
| try (InputStream inputStream = connection.getInputStream(); | ||
| OutputStream outputStream = connection.getOutputStream(); | ||
| BufferedReader br = new BufferedReader(new InputStreamReader(inputStream))) { |
| public static HttpUri from(String fullPath) { | ||
| int index = fullPath.indexOf("?"); | ||
|
|
||
| if (index == -1) { |
There was a problem hiding this comment.
index가 -1은 무엇을 의미할까요? 한 눈에 확인할 수 있게 변경하면 좋을것 같아요!
There was a problem hiding this comment.
아래와 같이 수정하였는데요 ㅎㅎ
조금 더 적절한 변수 명을 사용하고 싶은데 생각나는게 없네요 ㅠㅠ
if (index == QUERY_STRING_NOT_EXIST_INDEX) {
return new HttpUri(fullPath, QueryString.from(""));
}| public String get(String key) { | ||
| if (params.containsKey(key)) { | ||
| return params.get(key); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
간단한 메서드여서 해당사항은 없지만 클린 코드에 Guard Clause이 있습니다.
There was a problem hiding this comment.
전반적으로 코드에 NPE 방지를 위한 검증 로직 추가했습니다 !
| @@ -0,0 +1,35 @@ | |||
| package nextstep.jwp.http; | |||
|
|
|||
| public class HttpResponse { | |||
|
|
||
| public class HttpHeaders { | ||
|
|
||
| private final Map<String, String> httpHeaders; |
There was a problem hiding this comment.
HttpHeaders를 이용해서 header를 관리하는건 좋습니다! 추가적으로 HttpHeader를 Enum을 만들어서 key 값으로 사용해보면 어떨까요?
There was a problem hiding this comment.
상수 처리 하면서, 해당 사항 반영 했는데 코멘트가 남겨져 있었군요 ㅎㅎ !
좋은 의견 감사합니다 !
| if (HttpMethod.GET.equals(request.getHttpMethod())) { | ||
| return handleGetMethod(request); | ||
| } | ||
|
|
||
| if (HttpMethod.POST.equals(request.getHttpMethod())) { | ||
| return handlePostMethod(request); | ||
| } | ||
|
|
There was a problem hiding this comment.
Login과 Register는 POST와 GET이 동시에 존재해서 이렇게 구현하신 것 같습니다. 저는 POST와 GET 역할을 같이하게 되면 srp원칙에 위배가 된다고 생각하는데요. 이에 대해서 어떻게 생각하시나요?
There was a problem hiding this comment.
위 상황에서의 SRP 위배는 바라보는 관점에 따라 달라진다고 생각해요.
적절한 예시는 아닐 수 있지만, 움직일 수 있는 자동차 클래스가 있다고 가정해볼게요 !
자동차는 입력에 따라 전진, 혹은 후진이 가능한데, 전진 / 후진 여부를 판단하는 것이 자동차 클래스에서 수행되는 것이 SRP 위배일까요 ?
저는 아니라고 생각해요 ㅎㅎ
위 예시를 현재 미션에 대입해보면, 자동차라는 클래스가 Handler를 의미하고, 전진 / 후진 명령 판단을 할 수 있는데 필요한 입력이 Method라고 생각해요. (적절하지 않은 예시일수도 있습니다 ㅎㅎ)
추가적으로, 외부에서 URL과 Method에 따라 다른 Handler를 호출하도록 한다면, NotFound / Method Not Allowed Status를 어떻게 판단할 수 있을지 잘 모르겠네요.
에단은 SRP가 위반된다고 생각하셔서, 분리하신 것 같은데 위 상황에서 어떤 방식으로 분기처리를 하실 것인지 여쭤봐도 될까요 !?
There was a problem hiding this comment.
저는 LoginHandler가 get과 Post에 따라서 해주는 역할이 다르다고 생각했습니다. 그래서 내부에서 분기로 처리하는게 아닌 클래스를 LonginHandler와 LoginPageHandler 로 분리했습니다!
| } | ||
|
|
||
| public boolean hasValues() { | ||
| return params.size() != 0; |
There was a problem hiding this comment.
| return params.size() != 0; | |
| return !params.isEmpty(); |
이 방법은 어떨까요?
There was a problem hiding this comment.
상수를 제거할수도 있고, 좋은 방법이라고 생각합니다 !
다만, 저는 최대한 부정 조건문을 지양하려고 하는데요.
이는 위와 같은 메서드의 반환 값으로 사용할 때에는 해당되지 않으려나요 ?
There was a problem hiding this comment.
저도 그 부분에 대해서는 자세히 모르겠습니다. 다만 부정조건을 지양하라는 말은 if 문 조건에서 사용하는 걸로 알고 있었습니다!
저는 최대한 자바 컬렉션 메서드를 잘 사용하자라는 입장이라서 코멘트를 남겼었습니다.
| private HttpResponse createRedirectResponse(HttpRequest request, String location) { | ||
| HttpStatus httpStatus = HttpStatus.FOUND; | ||
| HttpVersion httpVersion = request.getHttpVersion(); | ||
| HttpBody httpBody = HttpBody.from(""); |
There was a problem hiding this comment.
redirect 응답에서는 바디가 없어서 자주 사용하게 될 것 같은데요.
자주 쓰이는 만큼 상수화해서 관리해보면 어떨까요?
There was a problem hiding this comment.
어떤 부분을 상수화해서 관리하라는 의미인지 이해가 안 되네요 ㅠ_ㅠ
아마, 위 부분도 리팩토링 진행하면서 바뀔 것 같은데, 다음 PR에서 확인해주세요 !!
There was a problem hiding this comment.
제가 범위를 이상하게 표시해놨네요ㅜㅜ
private static final HttpBody EMPTY = HttpBody.from("")을 의미었습니다!
안녕하세요 에단 !
처음에는 그냥.. 하나의 클래스에서 다 구현하다가, 도저히 안 되겠다 싶어서 클래스 분리를 했더니 3단계가 리팩토링이더라구요..
되돌리기에는 너무 늦어버렸고, 시간은 촉박하고 ㅠㅠ 정말 부족함이 많아 보이는 코드입니다.
솔직하고, 공격적인 리뷰 부탁드립니다 ! 많이 배워가고 싶습니다 😄
로직의 흐름이 간단한데, 코드가 지저분해서 리뷰하시는데에 도움이 될까 싶어 간단하게 남깁니다 !
HttpRequest객체로 파싱한다.HandlerAdaptor에서 요청을 처리할 Handler를 결정한다.HttpResponse객체를 반환한다.