[HTTP 서버 구현하기 - 2, 3단계] 배럴(김나은) 미션 제출합니다. #84

knae11 · 2021-09-02T09:47:02Z

안녕하세요 검프:)

2,3 단계 미션 제출합니다!
금방 될 것 같았는데 역시 아니었어욯ㅎㅎㅎㅎ
생각보다 리플렉션을 사용한 것이 디버깅하거나 사용하기 어렵더라구요;;
쿠키 세션 관련 인증을 controller 전에 끊어주고 싶었는데 쉽지 않았네요ㅠㅠ (현재 로직은 Controller 까지 헤더가 관여하게 되었습니다.)
검프의 의견을 듣고 다른 코드들도 보면서 리팩토링 해나가 볼게요!

테스트 코드 커버리지가 엄청 떨어졌네요;;
세션 값이 랜덤한 값이다 해당 부분 테스트코드 만드는 부분이 쉽지 않았는데 추가할 수 있는 방법을 찾아보겠습니다!!

바쁜 시간 내서 꼼꼼하게 리뷰해줘서 고마워요 😀

- post /login 테스트 코드 수정 - ICO ContentType 추가

- 사용하는 곳은 없고 현재는 콘솔로 파싱내용을 보여주기만 한다.

Livenow14

안녕하세요 배럴!! 다시 찾아온 리뷰어 검프입니다 ㅎㅎ
1차 데모에서 추가된 부분으로 기존 코드에 적용하느라 고생했어요 😁
먼저 질문에 대한 답을 남길게요.

Q. 생각보다 리플렉션을 사용한 것이 디버깅하거나 사용하기 어렵더라구요;;
쿠키 세션 관련 인증을 controller 전에 끊어주고 싶었는데 쉽지 않았네요ㅠㅠ (현재 로직은 Controller 까지 헤더가 관여하게 되었습니다.)

A. 저는 새로운 기능이 생겼을 때, 이를 기존 코드에 이식하는 도중, 도저히 어떻게 이식해야 할지 감이 잡히지 않으면, 현재 요청부터 응답까지 어떤 플로우로 흘러가고 있는지 간단하게 그려봐요. (이때 사용되는 객체들도 함께 적어요)
그렇게 하면 현재 서비스의 전반적인 플로우가 그려져서, 플로우 내에 기능을 추가할 수 있나 없냐가 조금더 명확해지더라고요.
이 부분에서 추가할 수 없다라는 결론이 나왔을 땐, 대다수가 서비스 흐름을 잘못 이해하고 있던 경우가 많았어요.
작은 기능들이 모여 큰 기능을 형성해야하는데, 이미 큰 기능이 자리잡고 있어 작은 기능이 들어갈 자리가 없었던 거였죠.
결론적으로, 이에 대한 답변은, 한번 전반적인 플로우를 그려보는건 어떨까요?
끊어줄 부분, 넣어줄 부분 등이 조금더 명확해지리라 믿어요!

1단계 때 Controller 부분을 깔끔하게 구현해주셔서 전반적인 플로우에서는 피드백 할 것이 없었어요!
이번에는 네이밍과, 객체의 역할 분리에 관련된 피드백을 많이 했어요!! 생각해보시고 괜찮다 싶으면 반영해주세요 ☺️

기능 부분 피드백

로그인 요청 실패

테스트 코드 피드백

RequestHandlerTest에서 공통적인 요청이나, 공통적인 응답은 메소드로 분리해보는 것은 어떨까요? 😊

app/src/main/java/nextstep/jwp/HttpServer.java

Livenow14 · 2021-09-02T15:15:47Z

app/src/main/java/nextstep/jwp/HttpServer.java

+        final RequestLine requestLine = new RequestLine(extractRequestLine());
+        this.requestHeader = new RequestHeader(extractHeaders());
+        final RequestBody requestBody = new RequestBody(extractRequestBody());
+        this.mappingHandler = new MappingHandler(requestLine, requestHeader, requestBody);


단순히 생성자의 인자로 들어가기때문에, 이전의 방식이 더 좋아보여요. 변수 인스턴스화를 하게된 이유가 궁금해요!

사용되지 않아서 불필요하다고 생각했던 것 같아요!ㅎㅎㅎ 지금은 HttpRequest 라는 객체에 모아두었는데, 프로퍼티로 설정하는 것과 변수 인스턴스로 만드는 검프만의 기준은 어떤 것인지 궁금하네요!ㅎㅎㅎ

저는 인스턴스화 된 변수가 다른곳에서 쓰이고, 할당된다면 인스턴스화를 진행하는 거 같아요!
저기서는 생성자의 인자로만 들어가고 있어 남긴 피드백이었습니다. 😊

Livenow14 · 2021-09-02T15:19:39Z

app/src/main/java/nextstep/jwp/constants/Header.java

 public enum Header {
    CONTENT_LENGTH("Content-Length"),
    LOCATION("Location"),
-    CONTENT_TYPE("Content-Type");
+    CONTENT_TYPE("Content-Type"),
+    SET_COOKIE("Set-Cookie"),
+    COOKIE("Cookie");

    private final String key;


Suggested change

public enum Header {

CONTENT_LENGTH("Content-Length"),

LOCATION("Location"),

CONTENT_TYPE("Content-Type");

CONTENT_TYPE("Content-Type"),

SET_COOKIE("Set-Cookie"),

COOKIE("Cookie");

private final String key;

public enum HeaderType {

CONTENT_LENGTH("Content-Length"),

LOCATION("Location"),

CONTENT_TYPE("Content-Type");

CONTENT_TYPE("Content-Type"),

SET_COOKIE("Set-Cookie"),

COOKIE("Cookie");

private final String value;

위와 같은 네이밍은 어떤가요?!

HeaderType, value 가 더 명확해 보이네요:)

Livenow14 · 2021-09-02T15:24:28Z

app/src/main/java/nextstep/jwp/constants/Http.java

    private Http() {
    }


Suggested change

private Http() {

}

private HttpOperation() {

}

Http라는 네이밍은 조금 Http 자체를 뜻하는 거 같아요. 위와같은 네이밍은 어떨까요?!

HttpTerms 라고 하였어요 :)

Livenow14 · 2021-09-02T15:26:38Z

app/src/main/java/nextstep/jwp/controller/Controller.java

        return ResponseEntity
                .responseResource("/index.html")
                .build();
    }

    @PostMapping(path = "/register")
-    public String register(RequestBody body) throws IOException {
+    public String register(RequestHeader header, RequestBody body) throws IOException {


HttpRequest를 만들어 RequestHeader, RequestBody를 넣어주면, 전체 컨트롤러에 공통 적용 가능하고, 역할을 분리 할 수 있을 거 같아요!

좋은 생각입니다! 해당 내용으로 변경할게요 :)

Livenow14 · 2021-09-02T16:03:45Z

app/src/main/java/nextstep/jwp/request/HttpCookie.java

+public class HttpCookie {
+    private final Map<String, String> cookies;
+
+    public HttpCookie(String cookie) {
+        this.cookies = parseCookie(cookie);
+    }


Map 자료구조에 값들이 생성될 때만 저장되고 있어요.
Cookie에 값을 넣어주고, Response시에 HttpResponse(코드에서 ResponseEntity.class)에 넣어주는 방식은 어떨까요?

해당부분은 Request 에서 사용되는 것 같은데.. 혹시 어떤 부분을 의도한 건지 좀 더 자세히 설명해주실 수 있을까요? 🙏

HttpCookie가 HttpRequest에서만 있어야 할까? 라는 의문점에서 나온 피드백이었어요!
HttpResponse에도 Cookie를 가지고 있으면 더 자연스러워 보일 거 같았어요 😊

app/src/main/java/nextstep/jwp/request/HttpSessions.java

Livenow14 · 2021-09-02T16:07:04Z

app/src/main/java/nextstep/jwp/request/HttpSessions.java

+    public static void addSession(String id) {
+        SESSIONS.put(id, new HttpSession(id));
+    }


여러 상황을 고려해서 인자로 HttpSession을 넘겨 주는 건 어떨까요?
HttpSession.setAttribute 이후 HttpSessions에 넣어줄 상황도 생길 수 있다고 생각이 들어요

오! 좋은 생각입니다! :)

Livenow14 · 2021-09-02T16:09:01Z

app/src/main/java/nextstep/jwp/request/RequestHeader.java

+            return new HttpCookie(headers.get(Header.COOKIE.getKey()));
+        }
+        throw new HttpException("쿠키헤더가 없습니다.");
+    }
 }


Cookie도 RequestHeader와 생명 주기를 함꼐 하게 인스턴스화를 진행해보는 것은 어떨까요?
배럴의 의견도 말씀해주세요 :)

같이 진행해 보는 방향으로 변경하였습니다! :)

app/src/test/java/nextstep/jwp/request/HttpCookieTest.java

knae11 · 2021-09-04T13:52:55Z

안녕하세요 검프 :)

피드백 준 내용 적용해 보았습니다! 검프가 얘기해 준 대로 따라가다보니 코드들이 더 정리되는 느낌을 받았어요.
또한, RequestHandlerTest 에서 쿠키에 따라 분기처리 하기 어려웠던 부분들에 대해 ControllerTest를 추가해 주었습니다.

쿠키에 있는 sessionId의 유효성 검증 로직은 여러군데 빼보려고 노력했는데, 여기서는 Service 로직과 섞여 있어서 Controller 단에서 해주는게 제일 좋다고 판단하였습니다.

더 피드백 줄 내용이 있다면 DM 주거나 편하게 말씀해 주세요 :)

아! 그리고 login 페이지에 접속이 401이 뜬 것은 제가 확인했을 때 정상동작 하여서... 혹시 검프의 다른 JSESSIONID 가 남아있기 때문이 아닐까 생각해봅니다..!ㅎㅎㅎ)

Livenow14 · 2021-09-05T07:54:09Z

app/src/main/java/nextstep/jwp/response/HttpResponse.java

+            headers.put(HeaderType.CONTENT_TYPE.getValue(), "text/html;charset=utf-8");
+            headers.put(HeaderType.CONTENT_LENGTH.getValue(), "0");


"text/html;charset=utf-8", "0"를 HeaderType의 프로퍼티로 옮기는 것은 어떨까요???

반영했어요 :) 검프 의견대로 반영해 보긴 했는데, 저는 HeaderType은 헤더의 키가 되는 type 값들이 모인 곳이라 왜 defaultValue 까지 HeaderType 에 들어가는 의견을 주신건지 조금 궁금하네요?!ㅎㅎㅎ

Livenow14 · 2021-09-05T07:54:55Z

app/src/test/java/nextstep/jwp/controller/ControllerTest.java

+    }
+}


자동으로 개행 잡도록 셋팅해둔줄 알았는데.. 안되나보네욬ㅋㅋㅋㅋ 제 셋팅을 다시 확인해 볼게욬ㅋㅋㅋ 감사합니다!!

app/src/test/java/nextstep/jwp/request/HttpTermsCookieTest.java

app/src/main/java/nextstep/jwp/request/HttpSessions.java

Livenow14

안녕하세요 배럴 😊
네이밍과 코드 수정으로 가독성이 정말 좋아졌어요!

(로그인 요청 실패부분은 남아있는 Session 값들 때문이었어요! 문제 없었습니다!)
간단한 피드백남겨요.

마지막으로, 이번 피드백에선 소나 큐브의 code smell을 줄여보는 것은 어떨까요?

knae11 · 2021-09-05T13:27:23Z

검프 안녕하세요 :)
검프의 피드백 및 소나큐브의 몇가지 code smell 부분들을 고쳐보았습니다.
code smell 중 나머지 부분은 지금 형태에서 더 좋게 바꾸는 방법들이 잘 떠오르지 않네요ㅠㅠ 현 상황을 유지해도 나쁘지 않을 것 같다는 의견이 조금 들어서요!ㅎㅎㅎ 혹시 보고 개선했으면 좋겠다면 다시 알려주세요 :)

Livenow14

안녕하세요 배럴!! 코드 스멜이 많이 줄었네요 ㅎㅎㅎ
간단한 피드백과 개선사항에 대한 의견 남겨요!

DEFAULT_REDIRECT_URL로 상수를 만들어 보는 것은 어떨까요 ?

exception이 던져지는 최하단 로직에서 예외를 감쌀 수는 없을까요?

인스턴스 변수와 로컬 변수가 같은 이름을 공유하고 있네요,

로컬 변수 네이밍을 변경하거나, 인스턴스 변수인 headers를 조작하게 하면 어떨까요 :)

Livenow14 · 2021-09-05T13:37:06Z

app/src/main/java/nextstep/jwp/constants/HeaderType.java

    private final String value;
+    private String defaultValue;


defaultValue라는 네이밍 보다.

private final String type;
private final String value;

로 정하는 것은 어떨까요~~?

제 생각에는 클래스 이름 자체를 Header로 하고
private final String type;
private final String value;
이렇게 해야 더 좋을 것 같아요 :)

- HeaderType -> Header 클래스 이름 수정, 내부 메소드 명 수정 - Handler 예외 Exception -> 구체적으로 작성

- 프로퍼티명과 내부 로컬변수명 겹치지 않도록 수정

knae11 · 2021-09-05T14:26:44Z

저는 redirectTo("/index.html") 은 중복되는 코드이긴 하지만, 각각이 어떤 곳으로 리다이렉트 시킬 것인지 명시적으로 보여주는 것도 좋다고 생각이 들어요. 그래서 중복이 되지만 지금 수준에서는 중복을 줄여 코드로 빼는 것보단 현 상태를 유지하고 싶다는 생각이 드는데 어떨까요?
내부에서 잡기보단 구체적인 예외를 던지도록 작성했어요. 내부에서 예외처리를 해준다면 이 또한 중복되는 코드의 증가라고 생각이 들었습니다.
해당 내용 변수명 다르게 수정하였습니다 :)

sonarqubecloud · 2021-09-05T14:27:38Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

93.0% Coverage
0.0% Duplication

knae11 · 2021-09-05T14:30:53Z

추가적으로 sonar Qube code smell에 대한 의견

GetHandler, PostHandler 의 인자 없는 생성자이지만 그대로 둔 것은.. 내부 매소드를 static 으로 수정하고 싶지 않지 않아서 입니다!
지금 정도의 프로젝트에서는 Exception에 메세지 정도는 자유롭게 작성해도 좋다고 판단하였습니다.

knae11 · 2021-09-05T14:31:56Z

빠른 피드백과 의견 감사합니당~~ㅎㅎㅎ 혹시 code smell 을 반영하지 않은 의도에 대해서 다른 의견 있거나 그래도 고쳤으면 좋겠다고 판단하신다면 의견 남겨주세요 :)

Livenow14 · 2021-09-05T15:51:09Z

코드스멜에서 따로 남기지 않았던 부분들도 의도를 남겨주셨네요 :)
수정 후 전반적으로 코드가 깔끔해 졌어요!! 1, 23 단계 함께 하시느라 고생하셨어요 ㅎㅎ
남은 미션도 화이팅이에요 😊

knae11 added 6 commits September 2, 2021 13:11

refactor: mappingHandler를 HttpServer와 생애주기를 맞춤

2f7ae19

feat: login 하는 경우, Set-Cookie 구현

b5e58df

- post /login 테스트 코드 수정 - ICO ContentType 추가

feat: request 로 들어온 쿠키 파싱 기능 구현

82bbf52

- 사용하는 곳은 없고 현재는 콘솔로 파싱내용을 보여주기만 한다.

feat: 로그인하여 login 페이지에 접속한 경우 index 페이지로 리다이렉트 기능 구현

f632ba3

refactor: 사용하지 않는 메소드 정리 및 코드형식 맞춤

557c4da

docs: check done todos

abe590b

Livenow14 suggested changes Sep 2, 2021

View reviewed changes

knae11 added 7 commits September 4, 2021 20:23

refactor: HttpRequest를 받도록 변경, redirectTo 인자로 uri 추가

5a5b175

refactor: Handler 메소드 정리

9fd55e4

refactor: 상수패키지 클래스 이름 및 메소드 이름 수정

80d9537

fixed: register 깨지는 테스트 수정

3aa7310

refactor: cookie, session 부분 피드백 반영

a2b9540

feat: login 페이지와 관련된 Controller test 구현

93970f6

style: polish codes

aba6397

Livenow14 reviewed Sep 5, 2021

View reviewed changes

Livenow14 self-requested a review September 5, 2021 08:00

Livenow14 suggested changes Sep 5, 2021

View reviewed changes

knae11 added 3 commits September 5, 2021 22:09

style: 파일 끝 개행처리

34c667c

refactor: defaultValue HeaderType 으로 이동

d656a2b

refactor(HttpResponse): sonarQube code smell 내용 개선

fd3a119

Livenow14 suggested changes Sep 5, 2021

View reviewed changes

knae11 added 3 commits September 5, 2021 23:24

refactor: 검프 피드백 반영

a804af5

- HeaderType -> Header 클래스 이름 수정, 내부 메소드 명 수정 - Handler 예외 Exception -> 구체적으로 작성

refactor(RequestHeader): sonarQube code smell 개선

4366da5

- 프로퍼티명과 내부 로컬변수명 겹치지 않도록 수정

style: polish codes

9946153

Livenow14 merged commit 3629461 into woowacourse:knae11 Sep 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HTTP 서버 구현하기 - 2, 3단계] 배럴(김나은) 미션 제출합니다. #84

[HTTP 서버 구현하기 - 2, 3단계] 배럴(김나은) 미션 제출합니다. #84

knae11 commented Sep 2, 2021 •

edited

Loading

Livenow14 left a comment

Livenow14 Sep 2, 2021

knae11 Sep 4, 2021

Livenow14 Sep 5, 2021 •

edited

Loading

Livenow14 Sep 2, 2021

knae11 Sep 4, 2021 •

edited

Loading

Livenow14 Sep 2, 2021

knae11 Sep 4, 2021

Livenow14 Sep 2, 2021

knae11 Sep 4, 2021

Livenow14 Sep 2, 2021

knae11 Sep 4, 2021

Livenow14 Sep 5, 2021

Livenow14 Sep 2, 2021

knae11 Sep 4, 2021

Livenow14 Sep 2, 2021

knae11 Sep 4, 2021

knae11 commented Sep 4, 2021 •

edited

Loading

Livenow14 Sep 5, 2021

knae11 Sep 5, 2021

Livenow14 Sep 5, 2021

knae11 Sep 5, 2021

Livenow14 left a comment

knae11 commented Sep 5, 2021

Livenow14 left a comment

Livenow14 Sep 5, 2021

knae11 Sep 5, 2021

knae11 commented Sep 5, 2021

sonarqubecloud bot commented Sep 5, 2021

knae11 commented Sep 5, 2021 •

edited

Loading

knae11 commented Sep 5, 2021

Livenow14 commented Sep 5, 2021

		headers.put(HeaderType.CONTENT_TYPE.getValue(), "text/html;charset=utf-8");
		headers.put(HeaderType.CONTENT_LENGTH.getValue(), "0");

[HTTP 서버 구현하기 - 2, 3단계] 배럴(김나은) 미션 제출합니다. #84

[HTTP 서버 구현하기 - 2, 3단계] 배럴(김나은) 미션 제출합니다. #84

Conversation

knae11 commented Sep 2, 2021 • edited Loading

Livenow14 left a comment

Choose a reason for hiding this comment

기능 부분 피드백

로그인 요청 실패

테스트 코드 피드백

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Livenow14 Sep 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knae11 Sep 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knae11 commented Sep 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Livenow14 left a comment

Choose a reason for hiding this comment

knae11 commented Sep 5, 2021

Livenow14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knae11 commented Sep 5, 2021

sonarqubecloud bot commented Sep 5, 2021

knae11 commented Sep 5, 2021 • edited Loading

추가적으로 sonar Qube code smell에 대한 의견

knae11 commented Sep 5, 2021

Livenow14 commented Sep 5, 2021

knae11 commented Sep 2, 2021 •

edited

Loading

Livenow14 Sep 5, 2021 •

edited

Loading

knae11 Sep 4, 2021 •

edited

Loading

knae11 commented Sep 4, 2021 •

edited

Loading

knae11 commented Sep 5, 2021 •

edited

Loading