[톰캣 구현하기 1, 2단계] 히이로(문제웅) 미션 제출합니다.

[MoonJeWoong]

안녕하세요 도기, 히이로입니다!

이번 레벨4에서 개인적인 목표를 미션을 진행하면서 빠르게 구현을 마치고 리팩토링을 하는 방향으로 잡고 있습니다.
하지만 코드를 보시면 아시겠지만 마냥 쉽지만은 않았습니다... ㅎㅎ 때문에 도기가 리뷰하게될 코드가 많이 난잡해진 것 같네요... 😢

Tomcat 클래스의 책임이 점점 비대해짐에 따라 이를 분리하고 추상화하고 싶은 욕구가 컸습니다. 그러나 이 다음 미션에서 해당 부분들을 진행하는 방향으로 설계되었다는 생각이 들어 이를 최대한 배제하고 진행했던 것 같습니다.

리뷰 마감 전까지 최대한 보기 편하시도록 리팩토링해보도록 하겠습니다.
혹시 리뷰 진행하시면서 이상하거나 궁금하신 점 있으시면 언제든 DM 부탁드립니다.

감사합니다! 🙇

[kdkdhoho]

우선 먼저 말씀드렸던 시간보다 다소 늦어진 점 사과드립니다 🙇‍♂️

코드 지저분해서 보기 어렵다고 하셨는데, 충분히 읽기 쉬운 코드여서 리뷰하는 데에 불편함은 없었어요.
그리고 리뷰하면서 히이로와 페어를 진행한다는 마인드로 리뷰를 남겼어요. 그래서 주로 제안하는 코멘트들이 많습니다. 확인하시고 히이로의 생각을 남겨주시면 감사할 것 같아요.

해당 미션에 대한 이해도나 지식이 높은 수준은 아니라, 구조적인 측면을 리뷰드리기 쉽지 않네요..! 더군다나 히이로도 전반적으로 리팩터링을 진행하신다고 하셨으니, 그때까지 저도 열심히 공부한 다음 구조와 관련된 리뷰를 남기도록 노력해볼게요.
앞으로도 같이 화이팅입니다 !! 💪

[MoonJeWoong]

안녕하세요 도기!
리뷰해주신 사항들 반영하고 재요청드립니다.

제 예상보다 너무 꼼꼼하게 봐주셔서 정말 감동받았습니다. 한편으로는 중간에 완료하지 못한 책임 분리 작업이나 리팩토링을 너무 하지 못해서 도기가 필요 이상으로 고민하지는 않았을까 미안한 마음도 들었습니다.

일단은 3단계에서 본격적으로 httpRequest, response 객체분리가 이뤄질 것 같아서 해당 부분은 분리하지 않고 그대로 두고 리뷰해주신 사항들을 최우선적으로 반영하였습니다. 커밋 단위를 수정 요청해주신 단위로 해둬서 확인하실 때 참고해주시면 좋을 것 같습니다.

바쁜 와중에 정성스러운 리뷰 감사드려요 도기! 🙇

[MoonJeWoong]

아 404페이지 반환 로직에서 NPE 처리해야 된다고 bug 감지되었네요. 해당 부분은 Optional로 풀어내봤는데 여기는 어떤지 도기의 의견이 궁금하네요!

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
11 Code Smells

0.0% Coverage
0.0% Duplication

The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

[MoonJeWoong]

여기가 반환 타입을 optional로 수정해본 부분입니다!

[kdkdhoho]

사실 요즘 Optional에 대해 좋지 않은 생각과, Optional로 하기 전 코드와 비교해도 충분히 괜찮다고 생각이 들긴 하는데요.
그렇다고 sonarcloud를 떼어내기도 불가능하니 일단 이대로 가는 것도 좋다고 봅니다!

[kdkdhoho]

안녕하세요 히이로! 1, 2단계 리팩터링 수고하셨습니다 👏
DM에서 나눴듯이 구조에 대한 내용은 3, 4단계에 기대하며 이번 단계는 Approve 하겠습니다.

다만, 일부 코멘트를 남겼는데요. 이는 다음 단계에서 함께 진행해주시면 될 것 같습니다.
추가로 리뷰로는 안남겼지만 리뷰 내용과 겹치는 코드들이 좀 있을 거에요. 해당 부분들도 다같이 수정 제안드립니다.

수고하셨어요 👍

[kdkdhoho]

현재 process 메서드가 굉장히 절차지향 느낌이 강하고 코드가 굉장히 기네요!

아래처럼 코드를 간결하게 줄일 수 있을 것 같아요.

Suggested change

	Map<String, String> parsedBody = new HashMap<>();
	if (requestHeaders.containsKey("Content-Length")) {
	String requestBody = extractRequestBody(bufferedReader, requestHeaders);
	parseMultipleValues(parsedBody,
	requestBody, FORM_VALUES_DELIMITER, FORM_KEY_VALUE_DELIMITER);
	}
	Map<String, String> parsedBody = parseRequestBodyIfContainsHeader(requestHeaders, bufferedReader);

private Map<String, String> parseRequestBodyIfContainsHeader(
            Map<String, String> requestHeaders,
            BufferedReader bufferedReader
    ) throws IOException {
        if (requestHeaders.containsKey("Content-Length")) {
            String requestBody = extractRequestBody(bufferedReader, requestHeaders);
            return parseMultipleValues(requestBody, FORM_VALUES_DELIMITER, FORM_KEY_VALUE_DELIMITER);
        }
        return new HashMap<>();
    }

public static Map<String, String> parseMultipleValues(
            String multipleValues,
            String valuesDelimiter, String keyValueDelimiter
    ) {
        Map<String, String> parsedValues = new HashMap<>();
        Arrays.asList(multipleValues.split(valuesDelimiter)).forEach(header -> {
            String[] splited = header.split(keyValueDelimiter);
            parsedValues.put(splited[KEY_INDEX], splited[VALUE_INDEX]);
        });
        return parsedValues;
    }

[kdkdhoho]

그리고 Content-Length가 현재는 여기밖에 안쓰이지만, HttpHeader와 관련된 문자열은 다양하게 쓰일 수 있을 것 같아요 🙊