[Shion, Jane] HTTP 웹 서버 2단계 - 리팩토링 #12

ghost · 2021-03-31T15:24:27Z

안녕하세요 시온, 제인입니다🙂

아직 리팩토링 진행 중이지만, HTTP 웹 서버 2단계(리팩토링) 요구사항 만족 후 중간 PR드립니다.

변경된 부분

RequestHandler의 책임을 HttpRequest와 HttpResponse로 분리
Controller 패키지를 추가하여 중복코드 및 요청 URL에 대한 분기 처리를 제거
session id를 UUID로 생성하여 Map의 key로 사용하고 value는 User로 하여 db에 저장
테스트 코드 레거시 제거 및 softAssertions 적용

@ExtendWith(SoftAssertionsExtension.class)
class HttpRequestUtilsTest {

    @InjectSoftAssertions
    SoftAssertions softly;

assertJ 3.18.0부터 SoftAssertionsExtension을 사용할 수 있어서 버전업 후 적용해보았습니다. 😀

소중한 시간을 내어 리뷰해주셔서 감사합니다! 😊

2단계 리팩토링 가이드 추가

- Controller 인터페이스 추가 - AbstractController 추상 클래스 추가 - AbstractController를 상속하는 CreateUserController, ListUserController, LoginController 클래스 추가

- URL과 Controller를 Map에 저장해서 if문 제거 - response200Header 메서드에 contentType 파라미터를 추가하여 리팩토링

HTTP 웹 서버 2단계 - 리팩토링 요구사항 1,2번 구현

HttpRequest, HttpResponse 리팩토링

- 단일책임원칙을 적용하는 방향으로 리팩토링

3단계 리팩토링 리뷰 부탁드립니다.

- assertj v.3.18.0으로 변경 - HttpRequestTest, HttpRequestUtilsTest에 SoftAssertions 적용

refactor: SoftAssertionsExtension을 활용하여 테스트 코드 리팩토링

refactor: 쓰레드풀을 사용하도록 리팩토링

Message

ksundong

안녕하세요! Shion & Jane! 리뷰어 Dion입니다!
리뷰어를 등록하고 보니 저는 작년에 이 단계를 진행하지 않았더라고요!!
그래서 제가 만약에 리팩토링 한다면 어떻게 했을까? 더 개선할 포인트는 없을까? 하는 관점으로 리뷰를 해보았습니다.

테스트 코드 작성해주시는 부분 인상깊었고, AssertJ의 새로운 Feature를 시도해보는 점도 좋았습니다.
다만, 우리가 스프링에서 배웠던 내용들을 조금은 놓치고 계신것 같았어요.
그리고 자바 프로그래밍에 있어서 놓치기 쉬운 부분들 (예외 처리, Null 처리 등)이 눈에 조금 띄었습니다.

작년의 저보다 다들 잘하시는것 같아서 정말 대단하시다고 생각됩니다.
저는 작년에 User 목록을 출력해줄 때, hardBars라는 ㅋㅋㅋㅋ handleBar 짝퉁을 시도했었는데요.
한 번 도전해보시는 것도 나쁘지 않을 것 같다는 생각이 듭니다.

그리고 설계는 정답이 없기 때문에 최선을 찾아나가는 과정에서 분명히 배우시는 점이 많았을거라는 생각이 듭니다.
2단계 까지 수행해주시느라 정말 고생많으셨고, 앞으로의 미션에서 더욱더 성장하셨으면 좋겠습니다.

이번 PR은 아직 개선 포인트가 많이 보여서, Request Changes로 두겠습니다.
팀프로젝트 하시다가 심심하실 때, 수정해보세요!

ksundong · 2021-04-03T11:14:13Z

src/main/java/controller/BaseController.java

+    public void doPost(HttpRequest request, HttpResponse response) {
+
+    }
+
+    public void doGet(HttpRequest request, HttpResponse response) {
+
+    }


아직 리팩토링 중이어서 구현체가 없는걸까요?

만약에 오버라이딩 하기 위함이라면
abstract가 더 적절하지 않았을까 싶네요~

ksundong · 2021-04-03T11:15:15Z

src/main/java/controller/Controller.java

+import http.HttpRequest;
+import http.HttpResponse;
+
+public interface Controller {


인터페이스를 사용해주셨네요 👍

ksundong · 2021-04-03T11:15:34Z

src/main/java/controller/BaseController.java

+        if (method.isPost()) {
+            doPost(request, response);
+        } else {
+            doGet(request, response);
+        }


if else를 제거할 수 있는 방법은 없을까요?

ksundong · 2021-04-03T11:18:00Z

src/main/java/controller/CreateUserController.java

+    public void doPost(HttpRequest request, HttpResponse response) {
+        User user = getUserFrom(request);
+        DataBase.addUser(user);
+        response.sendRedirect("/index.html");


상수로 적절히 추출해봅시다!

ksundong · 2021-04-03T11:18:29Z

src/main/java/controller/CreateUserController.java

+    private User getUserFrom(HttpRequest request) {
+        String userId = request.getParameter("userId");
+        String password = request.getParameter("password");
+        String name = request.getParameter("name");
+        String email = request.getParameter("email");
+        return new User(userId, password, name, email);
+    }


스프링은 어떻게 이걸 처리해줄까요? 한 번 공부해봅시다.

ksundong · 2021-04-03T12:21:31Z

src/test/java/util/HttpRequestUtilsTest.java

+
    @Test
-    public void parseQueryString() {
+    void parseQueryString() {


저는 Test에 DisplayName을 사용해 명시적인 이름을 주는걸 선호하는 편이에요~

ksundong · 2021-04-03T12:25:10Z

src/test/java/webserver/HttpRequestTest.java

+    SoftAssertions softly;
+
+    @Test
+    public void request_GET() throws Exception {


여기 public은 빼줄 수 있겠죠?
테스트 코드의 작성은 좋습니다.

ksundong · 2021-04-03T12:26:20Z

src/test/java/webserver/HttpResponseTest.java

+
+    @Test
+    public void responseCookies() throws Exception {
+        // Http_Cookie.txt 결과는 응답 header에 Set-Cookie 값으로 logined=true 값이 포함되어 있어야 한다.


DisplayName으로 대신할 수 있겠죠?

ksundong · 2021-04-03T12:26:31Z

src/test/java/webserver/HttpResponseTest.java

+import java.io.FileOutputStream;
+import java.io.OutputStream;
+
+public class HttpResponseTest {


public은 뺼 수 있습니다~

ksundong · 2021-04-03T12:27:07Z

src/test/resources/Http_GET.txt

@@ -0,0 +1,4 @@
+GET /user/create?userId=javajigi&password=password&name=JaeSung HTTP/1.1


궁금한게 왜 확장자는 txt인가요?

Shion and others added 20 commits March 27, 2021 15:16

2단계 리팩토링 가이드 추가

e6e490d

Merge pull request #19 from janeljs/readme.md

c1bcc8d

2단계 리팩토링 가이드 추가

refactor: 테스트 코드 레거시 수정

074a52e

test: Request 메시지 테스트 작성

c5a9085

fix: GET일 경우 쿼리 스트링에서, POST일 경우 리퀘스트 바디에서 파라미터 추출

732de5b

test: HttpResponse 클래스 테스트 코드 작성

65f8d64

Refactor: Controller 패키지를 추가하여 중복 코드 제거

47bcfce

- Controller 인터페이스 추가 - AbstractController 추상 클래스 추가 - AbstractController를 상속하는 CreateUserController, ListUserController, LoginController 클래스 추가

Refactor: URL에 대한 분기 처리 제거 및 response200Header 메서드 리팩토링

2ef2e76

- URL과 Controller를 Map에 저장해서 if문 제거 - response200Header 메서드에 contentType 파라미터를 추가하여 리팩토링

Merge pull request #21 from janeljs/refactor2

b7eb9c3

HTTP 웹 서버 2단계 - 리팩토링 요구사항 1,2번 구현

fix : POST에서도 queryString을 활용한 데이터 전달 지원

2dfd22b

refactor : HttpRequest, HttpResponse 리팩토링

7f1068b

Merge pull request #23 from janeljs/refactor2

5a8b133

HttpRequest, HttpResponse 리팩토링

refactor: 기능 단위로 메서드 분리

950d599

- 단일책임원칙을 적용하는 방향으로 리팩토링

fix: 쿠키 보안의 취약점을 개선하기 위해 세션 DB 추가

21d952a

Merge pull request #24 from janeljs/refactor3

c75f752

3단계 리팩토링 리뷰 부탁드립니다.

refactor : 전체 리팩토링

82b8cda

refactor: SoftAssertionsExtension을 활용하여 테스트 코드 리팩토링

56be465

- assertj v.3.18.0으로 변경 - HttpRequestTest, HttpRequestUtilsTest에 SoftAssertions 적용

Merge pull request #25 from janeljs/refactor4

cec3f61

refactor: SoftAssertionsExtension을 활용하여 테스트 코드 리팩토링

refactor: 쓰레드풀을 사용하도록 리팩토링

9084520

Merge pull request #27 from janeljs/refactor4

10b36ff

refactor: 쓰레드풀을 사용하도록 리팩토링

wheejuni pushed a commit that referenced this pull request Apr 3, 2021

Merge pull request #12 from sanhee/message

827c711

Message

ksundong self-requested a review April 3, 2021 10:57

ksundong self-assigned this Apr 3, 2021

ksundong requested changes Apr 3, 2021

View reviewed changes

ghost mentioned this pull request Apr 3, 2021

HTTP 웹서버 리팩토링 PR 피드백하기 janeljs/java-was#28

Open

		@@ -0,0 +1,4 @@
		GET /user/create?userId=javajigi&password=password&name=JaeSung HTTP/1.1

[Shion, Jane] HTTP 웹 서버 2단계 - 리팩토링 #12

Are you sure you want to change the base?

[Shion, Jane] HTTP 웹 서버 2단계 - 리팩토링 #12

Uh oh!

Conversation

ghost commented Mar 31, 2021

변경된 부분

Uh oh!

ksundong 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants