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

[디디] 1단계 - HTTP 웹 서버 구현 미션 제출합니다. #129

Merged
merged 15 commits into from
Sep 19, 2020

Conversation

fucct
Copy link

@fucct fucct commented Sep 16, 2020

안녕하세요 리뷰어님! 제출이 많이 늦어졌네요😭

어느 순간 정신을 차려보니 너무 복잡해져 버려서 급하게 마무리지은감이 있네요..
많이 부족하지만 피드백 주신다면 최대한 노력해보겠습니다. 이상하다 싶은 부분 전부 말씀해주세요!
생각해볼만한 내용도 피드백 주신다면 정말 감사하겠습니다.
질문 몇가지 작성해보았으니 답변 주시면 감사하겠습니다.

  • Map<String, String> 형태로 들어온 Body나 Params를 컨트롤러의 파라미터로 바인딩 하는 과정에 Object로 만든 걸 해당 타입으로 바꾸는 과정에서 여러개의 파라미터 바인딩을 하는게 어렵더라구요. 그런 역할은 Converter가 아니라 Argument Resolver에서 해주는 게 맞나요?😅
  • 테스트 과정에서 불가피하게 bufferedReader같은 리소스를 사용했는데, 이런 리소스들은 수동으로 닫아주는 게 맞을까요?

Copy link

@phs1116 phs1116 left a comment

Choose a reason for hiding this comment

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

안녕하세요, 디디!
리뷰어 화투입니다.
추상화에 대해서 많이 노력하신게 보이네요 👍
피드백 몇가지 추가 했는데 다음 리팩토링 진행하시면서 참고 부탁드릴게요. : )

Comment on lines +3 to +6
public class InvalidRequestBodyException extends RuntimeException {
public InvalidRequestBodyException() {
super("지원하지 않는 request body 형식입니다.");
}
Copy link

Choose a reason for hiding this comment

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

예외에 대해서 꼼꼼하게 정의를 해주셨네요 👍
몇가지는 http 메시지에 대해서 정의된 예외로 보이는데 이를 같이 조금 더 추상화해봐도 좋을 것 같아요!

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 byte[] loadFileFromClasspath(String filePath) throws IOException {
if (filePath.equals("/")) {
return loadFileFromClasspath("/index.html");
Copy link

Choose a reason for hiding this comment

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

상수로 뺴셨는데 그대로 사용하셨네요. :)

public static final String INDEX_PAGE = "/index.html";

public static byte[] loadFileFromClasspath(String filePath) throws IOException {
if (filePath.equals("/")) {
Copy link

Choose a reason for hiding this comment

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

이런 값들도 상수로 뺴는 걸 고민해봐도 좋겠어요.
static/notFound.html도 같이요 :)

Comment on lines +42 to +57
if (path.contains(".") && !path.endsWith(".html")) {
byte[] bytes = FileIoUtils.loadFileFromClasspath(path);

try {
dos.writeBytes("HTTP/1.1 200 OK \r\n");
dos.writeBytes("Content-Length: " + bytes.length + "\r\n");
dos.writeBytes(
"Content-Type: " + servletRequest.getHeader("Accept").split(",")[0] + ";charset=utf-8\r\n");
dos.writeBytes("\r\n");
dos.write(bytes, 0, bytes.length);
dos.flush();
} catch (IOException e) {
logger.error(e.getMessage());
}
return;
}
Copy link

Choose a reason for hiding this comment

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

정적 자원에 대한 부분도 별도 핸들러로 분리하고, 응답도 SerlvetResposne로 함께 추상화 할 수 있지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다! 나름대로 주신 피드백에 맞도록 반영했습니다!

field.setAccessible(true);
field.set(instance, value);
} catch (NoSuchFieldException | IllegalAccessException e) {
e.printStackTrace();
Copy link

Choose a reason for hiding this comment

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

e.printStackTrace(); 는 안티패턴이라 별도 로거를 통해서 로그를 찍는게 좋을 것 같네요. :)

Comment on lines +14 to +17
private final MethodType method;
private final String path;
private final Map<String, String> queryParams;
private final String protocolVersion;
Copy link

Choose a reason for hiding this comment

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

이 값들은 HTTP 메시지에 따르면 헤더는 아닐텐데요. :)

Copy link
Author

Choose a reason for hiding this comment

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

모르고 있었네요.. 알려주셔서 감사합니다..😅

Comment on lines +45 to +46
String view = attributes.getOrDefault("View", null);
byte[] body = FileIoUtils.loadFileFromClasspath("/" + view + ".html");
Copy link

Choose a reason for hiding this comment

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

View라는 값이 필요하다면 ModelAndView 등으로 추상화해서 받는것도 괜찮을 것 같아요.


public class ServletResponse {
private final StatusCode statusCode;
private final Map<String, String> attributes;
Copy link

Choose a reason for hiding this comment

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

지금 attributes보다는 헤더의 역할을 더 하고 있는거 같은데 맞나요? :)

@RequestMapping(type = RequestHeader.MethodType.GET, value = {"/", "/index"})
public ServletResponse index() {
LinkedHashMap<String, String> response = new LinkedHashMap<>();
response.put("View", "index");
Copy link

Choose a reason for hiding this comment

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

Servlet 인터페이스를 한번 따라서 구현해보면 어떨까요? :)
뷰에 대한 정보도 조금 더 추상화해서 도전해봐도 좋겠어요.

Copy link
Author

@fucct fucct Sep 20, 2020

Choose a reason for hiding this comment

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

정확하게 Servlet 인터페이스가 어떤 역할을 해주는 녀석인지 잘 모르겠어요. Spring을 보면서 주워 모은 지식들만 있어서 그런가보네요.. 😭
말씀하신 Servlet이 Controller 클래스마다 생성되는 녀석이 맞나요?

userService.save(user);

Map<String, String> response = new LinkedHashMap<>();
response.put("Location", "http://localhost:8080/index.html");
Copy link

Choose a reason for hiding this comment

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

putd이 아닌 헤더에 대한 정보라고 저금 더 명확히 해도 좋을 것 같아요.
더 나아가면 View에 대한 정보를 추상화해서 여기에 redirect를 표현해도 좋을 것 같구요.

(서블릿의 리턴값을 모델과 뷰에 대한 정보로 :) )

Copy link
Author

Choose a reason for hiding this comment

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

말씀주신 ModelAndView방식으로 수정했습니다!

@phs1116 phs1116 merged commit e113d1e into woowacourse:fucct Sep 19, 2020
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