[ky.kiske] step2 리뷰 요청 드립니다. by SimJung · Pull Request #35 · NewKrew2021/jwp-was-kakao

SimJung · 2021-02-03T06:20:54Z

안녕하세요 카이입니다!!
이번 미션 리뷰 잘 부탁드립니다. 감사합니다!!

* Controller 개선 * 다양한 Handler 추가

* 보안 컨트롤러 추가

* HttpRequest 테스트 코드 수정 * Controller 헬퍼 함수 추가

* 쿠키 HttpOnly 추가, 일원화

* 커스텀 에러 추가 * 에러 핸들링 메서드 추가 * 로깅 추가

findstar

안녕하세요. ky 웹서버 리뷰를 맡은 제임스입니다. 😁
전체적으로 요구사항을 충족하는 기능 구현을 잘 해주셨네요!
크게 수정할 부분은 없지만, 조금 더 보완하면 좋을것 같아 코멘트 달아두었습니다.
확인하시고 한번 더 고민해주세요 😄
그럼 화이팅입니다!

findstar · 2021-02-03T14:18:22Z

+import java.util.List;
+import java.util.Map;
+
+public class HttpRequest {


HttpRequest 객체가 한꺼번에 너무 많은 일을 하고 있지는 않은가요?
HttpRequestLine, HttpRequestHeader 와 같이 좀 더 세분화 해보면 어떨까요?

findstar · 2021-02-03T14:24:28Z

+
+public abstract class Controller {
+    private static final Logger log = LoggerFactory.getLogger( Controller.class );
+    protected final Map<PathInfo, Handler> handlers = new LinkedHashMap<>();


handlers 는 굳이 LinkedHashMap 을 사용한 이유가 있을까요?

Handlers 가 순서를 기반으로 등록되어야 할 필요가 있다면 차후에 실수하기 쉬운 구조 입니다.
path 를 기반으로 동작할 handler 가 다른 방식으로 매칭될 수는 없을지 고민해봐주세요 😄

findstar · 2021-02-03T14:36:58Z

+    private static final String PROTOCOL = "HTTP/1.1";
+
+    private final DataOutputStream dos;
+    private final Map<String, String> headers = new HashMap<>();


HttpHeader 를 별도의 객체로 분리해보면 좋겠습니다.
마참가지로 ContentType , HttpStatus 로 분리할 수 있습니다.

findstar · 2021-02-03T14:38:19Z

+        log.info("{}", startLine);
+    }
+
+    public void sendView(byte[] body) throws IOException {


IOException 을 그대로 던지지 말고 try / catch 구문으로 처리한 뒤에
좀 더 의미있는 exception 을 던지는 방향으로 개선해보면 어떨까요?

findstar · 2021-02-03T14:38:39Z

+
+import java.util.Objects;
+
+public class PathInfo {


PathInfo 객체를 별도로 선언하셨군요 💯

findstar · 2021-02-03T15:04:23Z

@@ -0,0 +1,10 @@
+<!DOCTYPE html>


custom error page 도 작성해 주셨네요 💯

findstar · 2021-02-03T15:07:50Z

+package exception.utils;
+
+public class NoFileException extends Exception {
+    public NoFileException() {


NoFIleException 과 같은 경우에는 어떤 파일을 찾을 수 없는지 표시될 수 있게 개선되면 더 좋겠습니다.

findstar · 2021-02-03T15:10:55Z

+        setBasePath("");
+        putHandler("/js/.*", "GET", this::handleFile);
+        putHandler("/css/.*", "GET", this::handleFile);
+        putHandler("/fonts/.*", "GET", this::handleFile);


Fonts 파일이 로딩되지 않는 현상이 있습니다. 확인 부탁드려요 :smile

findstar · 2021-02-03T15:13:18Z

-        System.out.println("Running Test3");
-    }
-}
+//package study.reflection;


이 클래스를 사용하지 않는다면 정리해 주세요 :smile

findstar · 2021-02-03T15:16:43Z

+        String postMsg = "POST /api HTTP/1.1\n" +
+                "Host: localhost:8080\n" +
+                "Connection: keep-alive\n" +
+                "Content-Length: 33\n" +
+                "Accept: */*\n" +
+                "\r\n" +
+                "userId=javajigi&password=password";
+        postRequest = makeRequest(postMsg);
+
+        String getMsg = "GET /index.html HTTP/1.1\n" +
+                "Host: localhost:8080\n" +
+                "Connection: keep-alive\n" +
+                "Accept: */*\n" +
+                "";
+        getRequest = makeRequest(getMsg);
+
+        String queryMsg = "GET /user/create?" +
+                "userId=jack&password=password&name=jackwon&email=jackwon%40kakaocorp.com HTTP/1.1\n" +
+                "Host: localhost:8080\n" +
+                "Connection: keep-alive\n" +
+                "Accept: */*\n" +
+                "";
+        queryRequest = makeRequest(queryMsg);


Http 요청에 대한 raw String 을 이렇게 관리하지 말고
test/resources/ 디렉터리에 파일 형태로 관리하면 어떨까요?
다음처럼 FileInputStream 을 사용하면 Socket connection 에서 획득하는 InputStream 과 같이 테스트 할 수 있습니다.

InputStream in = new FileInputStream(new File(http_request_example_test_case));

- 이후 스텝에서 쓰일 테스트 코드 주석 제거 및 패키지 변경

- test 내 resources에서 Http 요청 코드를 관리하도록 변경

- 기존의 단순 throws Exception을 try / catch를 사용하도록 개선

- HttpRequestLine, HttpRequestHeader로 역할을 분리

- DB에 직접 접근하는 로직을 Service로 이관

- 기존의 OutputStream 에서 HttpResponse를 받도록 변경

SimJung · 2021-02-04T18:57:10Z

리뷰 감사합니다!! 코멘트를 참고하다보니 조금 더 섬세하게 작업했어야함을 느꼈습니다 ㅎㅎ..
말씀해주신 LinkedHashMap을 사용한 이유는 각 컨트롤러의 우선순위를 지정해주기 위하여 순서가 보장되는 구조를 사용할 필요가 있었기 때문입니다. 현재의 우선순위는 UserController가 우선이고 ViewController가 후순위입니다.
또한 말씀해주신 fonts 파일이 로딩되지 않는 상황을 캐치하려 했으나 문제 상황을 아직 포착하지 못했습니다😭
리뷰 감사합니다. 잘 부탁드립니다!!

findstar

안녕하세요 제임스입니다.
피드백 드린 사항 잘 반영해주셨네요 👏
질문에 답변드리면
Handlers 가 순서를 기반으로 등록되어야 할 필요가 있다면 차후에 실수하기 쉬운 구조 입니다.
path 를 기반으로 동작할 handler 가 다른 방식으로 매칭될 수는 없을지 고민해봐주세요 😄
그럼 코멘트 확인하시고 계속 화이팅입니다.

findstar · 2021-02-05T09:35:51Z

+
+public abstract class Controller {
+    private static final Logger log = LoggerFactory.getLogger( Controller.class );
+    protected final Map<PathInfo, Handler> handlers = new LinkedHashMap<>();


Handlers 가 순서를 기반으로 등록되어야 할 필요가 있다면 차후에 실수하기 쉬운 구조 입니다.
path 를 기반으로 동작할 handler 가 다른 방식으로 매칭될 수는 없을지 고민해봐주세요 😄

findstar · 2021-02-05T09:37:18Z

+    public HttpRequest(InputStream in) throws IOException {
+        this(new BufferedReader(new InputStreamReader(in)));
+    }
+
+    public HttpRequest(BufferedReader br) throws IOException {
+        String line = br.readLine();
+        httpRequestLine = new HttpRequestLine(line);
+        httpRequestHeader = new HttpRequestHeader(br);
+        setBody(br);
+    }


InputStream 을 받아서 HttpRequest 인스턴스를 생성하는 static method 로 변경해보는건 어떨까요?

findstar · 2021-02-05T09:37:50Z

+        ControllerManager.registerController(new UserController());
+        ControllerManager.registerController(new ViewController());


Webserver 가 ControllerManager 를 알고 있어야할 까요?
RequestHandler 의 static 으로 처리할 수도 있습니다.

- HttpStatus ENUM 클래스로 변환 - ContentType ENUM 클래스로 변환

- defaultHandle 메소드를 통해 순서에 의존하지 않도록 변경

kakao-jack-won added 12 commits January 29, 2021 16:53

feat(HttpRequest): HTTP Request 메시지 파싱후 객체 생성

1df9275

feat(HttpRequest): HTTP 요청 쿼리스트링 파싱기능 추가

08da7e9

feat(Controller): 컨트롤러 뼈대 추가

8ca6d09

feat(Controller): 웹서버 기능 구현

21fc581

* Controller 개선 * 다양한 Handler 추가

refactor(Controller): 추상 클래스로 변경, 컨트롤러 개편

ff80e87

* 보안 컨트롤러 추가

refactor(HttpResponse): HTTP 응답 객체 개편, View 도입

b781fa6

refactor(HttpResponse): HTTPResponse 테스트 코드 추가

7e2a6cc

* HttpRequest 테스트 코드 수정 * Controller 헬퍼 함수 추가

refactor(html): 주소 일원화

057a01f

refactor(AcceptanceTest): 요청 테스트

d30f3a7

* 쿠키 HttpOnly 추가, 일원화

feat(User): User 입력값 검증

5e92911

fix(Log, HttpResponse): 로그 수정, 폰트 콘텐츠 타입 수정

da0fcee

feat(Exception): 에러처리 강화

858c75e

* 커스텀 에러 추가 * 에러 핸들링 메서드 추가 * 로깅 추가

findstar suggested changes Feb 3, 2021

View reviewed changes

SimJung added 9 commits February 5, 2021 01:45

test : 테스트 주석 제거

e691a52

- 이후 스텝에서 쓰일 테스트 코드 주석 제거 및 패키지 변경

test : 요청 코드 파일로 분리

dd6c557

- test 내 resources에서 Http 요청 코드를 관리하도록 변경

feat : NoFileException 시 해당 파일의 경로 표시

70e8068

refactor : Method ENUM 클래스로 분리

6e58a92

refactor : Exception handling 리팩토링

7eff946

- 기존의 단순 throws Exception을 try / catch를 사용하도록 개선

refactor : HttpRequest 분리

4e48b4e

- HttpRequestLine, HttpRequestHeader로 역할을 분리

refactor : HttpResponse 내부 Map을 클래스로 분리

b21098f

feat : UserService 추가

33cb6ef

- DB에 직접 접근하는 로직을 Service로 이관

refactor : Controller 인자 변경

a179df8

- 기존의 OutputStream 에서 HttpResponse를 받도록 변경

findstar suggested changes Feb 5, 2021

View reviewed changes

SimJung added 5 commits February 7, 2021 15:13

refactor : WebServer의 컨트롤러 레지스터 과정을 컨트롤러 매니저 내부의 초기화 과정으로 변경

6831142

refactor : HttpRequest에서 inputStream을 인자로 받는 static factory method 생성

fa2be8f

refactor : Map을 갖는 클래스를 ENUM으로 변환

9e83435

- HttpStatus ENUM 클래스로 변환 - ContentType ENUM 클래스로 변환

refactor : IllegalHttpResponseException 추가

866f206

refactor : 컨트롤러 순서 비의존적으로 로직 변경

2a4213c

- defaultHandle 메소드를 통해 순서에 의존하지 않도록 변경

		ControllerManager.registerController(new UserController());
		ControllerManager.registerController(new ViewController());

Uh oh!

Conversation

SimJung commented Feb 3, 2021

Uh oh!

findstar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimJung commented Feb 4, 2021

Uh oh!

findstar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants