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

[엘리] WAS 1단계 미션 제출합니다. #96

Merged
merged 19 commits into from
Sep 11, 2020
Merged

Conversation

YebinK
Copy link

@YebinK YebinK commented Sep 9, 2020

안녕하세요 홍시!
다시 만나게 되어 너무 기뻐요 🙂

Spring의 Filter와 DispatcherServlet을 참고하여 만들어 보았습니다.
잘 부탁드립니다!

Copy link

@hongsii hongsii left a comment

Choose a reason for hiding this comment

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

안녕하세요~ 엘리!
1단계 요구사항 구현을 잘 해주셨네요! 💯
몇 가지 피드백을 남겼으니, 다음 단계를 진행하시면서 같이 반영해주세요~!

}

public String getRequestPath() {
return requestUri.getPath().getRequestPath();
Copy link

Choose a reason for hiding this comment

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

규칙 4. 한 줄에 한 점만 사용 을 지키도록 수정해보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

디미터 법칙을 위반하고 있었네요!
반영했습니다~

Path path = Paths.get(FileIoUtils.class.getClassLoader().getResource(filePath).toURI());
return Files.readAllBytes(path);
} catch (URISyntaxException e) {
throw new RuntimeException(String.format("%s: URI로 변환할 수 없습니다.", filePath));
Copy link

Choose a reason for hiding this comment

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

RuntimeException 보다 IllegalArgumentException 과 같은 구체적인 예외 클래스를 활용해보면 어떨까요?
예외 메시지와 함께 구체적인 예외 클래스를 통해 발생한 원인을 좀 더 빨리 파악할 수 있게 해줄 수 있을 것 같습니다!

Map<String, String> bodies = new HashMap<>();
for (String body : splitBody) {
String[] keyValue = body.split("=");
bodies.put(keyValue[0], keyValue[1]);
Copy link

Choose a reason for hiding this comment

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

userId= 와 같이 값을 비워 요청하면 문제가 발생하지 않을까요?
예외 케이스에 대해서도 적절히 고민해보면 좋을 것 같아요~

Copy link
Author

Choose a reason for hiding this comment

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

클래스로 분리해 적용해보았습니다 🙂

}

public Map<String, String> parse() {
String[] splitBody = body.split("&");
Copy link

Choose a reason for hiding this comment

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

"&" 를 상수로 선언해 의도가 명확히 드러나도록 바꿔보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

👍
아래 로직도 클래스 분리를 해봤습니다!

public class RequestPath {
private final String fullPath;
private final String requestPath;
private final Map<String, String> requestParameters = new HashMap<>();
Copy link

Choose a reason for hiding this comment

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

파라미터와 관련된 일급 컬렉션을 만들어 관리해보면 어떨까요?

class HttpRequestTest {

@Test
void name() throws IOException {
Copy link

Choose a reason for hiding this comment

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

테스트 메서드명을 바꾸시는 것을 깜빡하신 것 같아요~ :)

Comment on lines +23 to +24
assertThat(httpRequest.getRequestBody().getBody())
.isEqualTo("userId=javajigi&password=password&name=pobi&[email protected]");
Copy link

Choose a reason for hiding this comment

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

body 뿐만 아니라 다른 필드도 정상적으로 생성되는지 검증해보면 어떨까요?

Comment on lines +12 to +16
this.requestUri = new RequestUri(bufferedReader);
this.requestHeader = new RequestHeader(bufferedReader);
if (HttpMethod.POST == getMethod()) {
this.requestBody = new RequestBody(bufferedReader, requestHeader.getContentLength());
}
Copy link

Choose a reason for hiding this comment

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

현재 각 클래스로 BufferedReader 를 전달해서 처리하고 있는데요~
이러한 방식도 괜찮다고 생각하지만, HttpRequest 클래스 입장에선 각 클래스가 BufferedReader 를 얼마나 읽냐에 따라 순서가 보장되지 않는 단점이 있습니다.

HttpRequest 클래스 한 곳에서 요청을 읽어 순서를 보장하면 응집도를 높일 수 있고, 다른 클래스 변경에도 취약하지 않은 구조로 변경할 수 있을 것 같습니다.
뿐만 아니라, RequestUri, RequestHeader 에서도 BufferedReader 와의 의존성을 끊을 수 있어 각 클래스를 테스트하기가 쉬워질 듯 합니다~!

Copy link
Author

Choose a reason for hiding this comment

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

그렇군요! 테스트할 때마다 매번 BufferedReader를 만드는 과정이 번거로웠는데, 알려주셔서 감사합니다!
좀 더 유연한 구조로 바꿔보았어요! 🙂

import java.io.BufferedReader;
import java.io.IOException;

public class HttpRequest {
Copy link

Choose a reason for hiding this comment

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

책임 분리 💯

import java.util.HashMap;
import java.util.Map;

public class HandlerMapping {
Copy link

Choose a reason for hiding this comment

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

클래스 설계 👍

@hongsii hongsii merged commit be76360 into woowacourse:yebink Sep 11, 2020
Copy link
Author

@YebinK YebinK left a comment

Choose a reason for hiding this comment

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

꼼꼼한 리뷰 감사합니다 홍시 🙂
말씀하신 피드백들 반영해서 전체적으로 수정했습니다. 훨씬 깔끔해졌어요!
2단계에서 뵈어요~

}

public Map<String, String> parse() {
String[] splitBody = body.split("&");
Copy link
Author

Choose a reason for hiding this comment

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

👍
아래 로직도 클래스 분리를 해봤습니다!

Map<String, String> bodies = new HashMap<>();
for (String body : splitBody) {
String[] keyValue = body.split("=");
bodies.put(keyValue[0], keyValue[1]);
Copy link
Author

Choose a reason for hiding this comment

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

클래스로 분리해 적용해보았습니다 🙂

}

public static Servlet find(HttpRequest httpRequest) {
RequestMapping key = handlerMapping.keySet().stream()
Copy link
Author

Choose a reason for hiding this comment

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

그런 방법도 있군요!
equals, hashCodegetter를 사용하도록 변경했습니다!

Comment on lines +12 to +16
this.requestUri = new RequestUri(bufferedReader);
this.requestHeader = new RequestHeader(bufferedReader);
if (HttpMethod.POST == getMethod()) {
this.requestBody = new RequestBody(bufferedReader, requestHeader.getContentLength());
}
Copy link
Author

Choose a reason for hiding this comment

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

그렇군요! 테스트할 때마다 매번 BufferedReader를 만드는 과정이 번거로웠는데, 알려주셔서 감사합니다!
좀 더 유연한 구조로 바꿔보았어요! 🙂

public RequestUri(BufferedReader bufferedReader) throws IOException {
String uri = bufferedReader.readLine();
if (uri.isEmpty()) {
return;
Copy link
Author

Choose a reason for hiding this comment

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

exception을 던지도록 했습니다!

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