[Review] 웹서버 3단계 - POST로 회원 가입 (2022/03/29)

 

[Shine & 케이] 웹서버 3단계 - POST로 회원 가입 by zbqmgldjfh · Pull Request #43 · codesquad-members-2022/java-was

이번 과제는 팀원과 함께 pair 프로그래밍을 통해 HTTP 서버를 만드는 과제였습니다.

1, 2 단계는 간단하여 reivew 가 적었기에 본격적인 부분인 3단계 부터 리뷰정리를 하게 되었습니다.

 

1. 코드 리뷰

1. 알맞은 Exception 던지기

돌이켜 생각해보니 습관처럼 IllegalArgumentException을 던지도록 한 것 같다.

따라서 RuntimeException을 상속하는 DuplicatedUserException 을 만들어 사용하게 되었다.

public class DuplicatedUserException extends RuntimeException {
    public DuplicatedUserException(String message) {
        super(message);
    }
}

 

2. 의미 없는 Return 문 제거하기

원래는 if문 안에서 return을 하고, if문 밖에서 예외를 던지도록 하였는데, 두 코드의 위치를 바꿔
if문 에 걸리면 바로 예외가 터지도록 하였고, 이후 if 문 밖에서는 자연스럽게 return 이 필요없도록 수정하게 되었습니다!

 

추가적으로 user를 찾아오는 메서드가 optional을 반환하도록 변경하였습니다.

 

3. 적합한 이름 다시 고민해보기

사실 이름 관련 피드백은 pr을 보낼때 마다 겪는 것 같다...

음 이게 내가 구현할 당시에는 적합하다 생각하여 이름을 만들었는데, 또 시간이 지나 다시보면 어색한 이름들이 많다.

이번 "createRequest" 또한 그러하다.

 

이 메서드를 호출하면 결과적으로 Request 객체를 반환하기 때문에 위와 같이 메서드 이름을 정하게 되었다.

public static HttpRequest parse(InputStream in) throws IOException {
    BufferedReader reader = new BufferedReader(new InputStreamReader(in));
    RequestLineTokens token = tokenMaker(reader.readLine());
    Map<String, String> header = parseHttpHeader(reader);
    return buildRequest(reader, header, token);
}

하지만 해당 매서드에서 Request 객체를 만드는 일은 끝에서 일어나는 사소한 작업이다.

진정한 "생성" 에 해당되는 부분은 buildRequest 에 국한 된다. 부분의 기능이 전체 메서드의 이름이 된 상황이였던것 이다.

 

오히려 중간에 Request 객체를 만들기 위해 진행되는 parsing 작업이 더 중요하다 생각되었다.

따라서 이름을 parse 로 변경하게 되었다.

 

4. Token 들을 하나의 객체로!

Http 메서드의 requestLine 을 공백 기준으로 split 하여 덩어리를 나눈 후, 이를 HttpRequest를 만들때 빌더 방식으로 인자를 전달하게 된다.

 

하지만 인자가 requestLineTokens[1] 과 같이 명확하게 보이지가 않는다.

물론 호출하는 메서드가 .path 니까 경로는 전달한다는 것을 알기는 하지만 불편하다.

또 각 token들이 각자 분리 되어있어 관리또한 힘들다.

 

따라서 requestLine 자체를 하나의 객체로 만들어 사용하였다.

참고로 inner Class 로 생성하게 되었다.

당장은 requestLine을 객체로 만들어 사용하는곳이 이곳 parser 안에서만 사용한다 생각했기 때문이다.

public class RequestParser {
    // 생략...

    private static class RequestLineTokens {
        private String method;
        private String path;
        private String protocol;

        public RequestLineTokens(String method, String path, String protocol) {
            this.method = method;
            this.path = path;
            this.protocol = protocol;
        }
    }
}

 

5. 의미 없는 메서드의 사용

2개의 메서드 모두 response, request 객체를 각각 생성하는 일을 하고 있다.

메서드의 이름이 의미를 전달하기 때문에 생성하는 부분을 함수로 감싸주게 되었다. 메서드 한번정도의 호출은 cost 매우 작기 때문이다.

따라서 다음과 같이 대칭성 있고, 가독성 높은 코드가 나오게 된다 생각하였다.

HttpRequest request = buildRequest(in);
HttpResponse response = buildResponse(out);
controlServlet(request, response);

하지만 리뷰어 께서는 오히려 "네 가독성이 크게 의미없고, 오히려 가독성이 떨어질 가능성이 높은 것 같습니다." 라고 달아주셨다.

 

사실 여기에 동의하지 않는다. 리뷰어의 생각이 무조건 정확한 방향이라 생각하지 않기 때문에, 좀더 생각하게 되었다.

왜 오히려 가독성이 떨어지는 것 일까? .... 리뷰어님께 따로 DM을 보내봐야 겠다

 

어찌됬든 변경된 모습은 다음과 같다.

HttpRequest request = RequestParser.parse(in);
HttpResponse response = new HttpResponse(out);
controlServlet(request, response);

그냥 메서드를 제거 하고, 바로 생성하도록 하였다.

 

6. 반복되는 객체 생성

잘 생각해보면 매번 parser를 생성할 필요가 없다.

리뷰어님의 원래 의도는 아마 처음에 한번 RequestParser를 생성한 후, 이후 부터는 메서드만 호출하여 계속 parsing 하길 권하신것이라 생각되지만,

 

나는 그냥 전체를 static 메서드만 가지고 있는 Utill Class 로 만들었다.

애당초 객체를 만들 필요가 없는것 같다. 그냥 static 메서드만 호출하여 처리만 하면 된다.