[2단계 - 자동차 경주 리팩터링] 오잉(이하늘) 미션 제출합니다. by hanueleee · Pull Request #588 · woowacourse/java-racingcar

hanueleee · 2023-02-12T15:27:34Z

안녕하세요 카프카!

1단계와 2단계를 혼동하여 1단계를 제대로 진행하지 못한 것 같아 굉장히 아쉬움이 큽니다 🥲
1단계에서 말씀해주신 피드백들을 반영하여 리팩토링을 진행했습니다!
잘 부탁드립니다 🙇‍♀️

주요 수정 사항

validate 로직

기존에 CarNamesValidator와 TryCountValidator로 나뉘어져있던 검증 로직들을 Validator 클래스 하나로 통합하였습니다.
말씀해주신 carNames에서의 null체크에 대해 고민하다보니, 또다른 피드백이었던 Parser의 parsing 전 검증과 엮어야겠다는 생각이 들었습니다. carNames에 대한 검증에 들어가기 전에 1차적으로 Parser에서 null과 빈문자열에 대한 예외처리를 해줌으로서 해당 문제를 해결하고자 했습니다.

RacingCarGameController

carNames와 tryCount에 대한 입력을 받고 검증 실패시 재입력을 진행하는 로직을 기존 반복문에서 재귀함수 형태로 리팩토링했습니다.
RacingCar의 기본 위치 값을 0으로 세팅하고, RoundManager의 getStartStatus메소드를 사용해 본격적인 경주 시작 전에 기본 위치 값을 1씩 증가시켰습니다.

궁금한 점

getter 사용은 최대한 지양해야한다라는 객체지향 생활 체조 원칙을 최대한 지키기 위해
RacingCar클래스에서

getDesc메소드 : (라운드 실행 결과를 출력하기 위해) RacingCar의 현상태를 문자열로 재구성해 반환
compareTo메소드 오버라이드: (가장 큰 position을 가진 RacingCar를 구하기 위해) 인자로 전달된 객체의 position과 현 객체의 position의 차이값 반환
toDto메소드 : (position값 순서대로 정렬된 객체들을 그대로 전달하지 않고 복사본을 전달하기 위해) dto로 복사

등의 방법을 사용했습니다.

하지만 어떠한 글에서는 view에서 값을 출력하기 위해 getter를 사용하는 것은 괜찮다라는 내용이 있었습니다.
카프카는 해당 문제에 대해 어떻게 생각하시는지 궁금합니다. 감사합니다!

(기존) RacingCar객체의 디폴트 위치가 1 -> getCurrentRound 메소드에서 RacingCar들의 현재 상태 반환 (수정) RacingCar객체의 디폴트 위치가 0 -> getStartStatus 메소드에서 RacingCar들을 한칸씩 전진시킨 후 RacingCar들의 현재 상태 반환

1. validator를 인스턴수 변수로 수정 2. validate 검사 후 재입력 로직을 반복문에서 재귀로 수정

@DisplayName

1. AdvanceJudgementTest 전진 불가능 경우 테스트에 @DisplayName 추가 2. CarNamesValidatorTest와 TryCountValidatorTest 통합

include42 · 2023-02-13T01:59:43Z

안녕하세요 오잉, 리뷰어 카프카입니다.
남겨주신 질문에 대한 답변이 길어질 듯 해서, 먼저 코멘트로 남겨둡니다.

객체지향 생활 체조를 보면 getter/setter 사용을 지양하라는 원칙이 있지요?
해당 원칙은 도메인 클래스에 적용되는데, 아래와 같은 상황을 방지하고자 하는 목적이 있습니다.

RacingCar가 움직이게 하고 싶다. 어떻게 하면 될까요?
- car.setPosition(car.getPosition() + 1) : getter와 setter를 이용해 도메인에서 처리해야 할 로직을 외부가 수행하고 있습니다.
- car.move() : 움직이는 동작의 책임을 car에 위임하고, 외부에서는 메소드 호출을 통해 동작을 수행합니다.

위의 두 가지 예시를 보면 느낌이 오시죠?
특히 setter의 경우, 도메인의 상태를 외부에서 조작해 예기치 않은 오류를 일으킬 수 있습니다. 그리고 도메인의 유효성을 깨뜨릴 수 있고요. 그래서 정말 왠만하면 앞으로 사용하지 않는 것이 좋습니다.
다만 getter의 경우 아예 사용을 금지할 수는 없어요. 상황에 따라서는 상태를 전달받아야 할수도 있으니까요. 다만 도메인에서 getter를 통해 값을 바로 전달해주는 상황이 최대한 적어지게 해보자, 라는게 이번 미션의 목표인 것이죠.

제가 생각하는 기준은 다음과 같습니다.

setter: 절대 사용 금지. 도메인은 정해진 메소드들을 통해 내부에서 유효하게 값이 변경되고, DTO/VO는 생성시 값을 final로 유지하며 값 변경이 필요한 경우 아예 새로 생성해야 한다.
getter: 도메인에서는 가급적 사용하지 않는다. DTO/VO의 경우 getter로 값을 전달하는 목적을 가지므로, 뷰나 컨트롤러에서 사용할 수 있다.

추가로, 리뷰에 적기 길어서 RacingCar 개선에 대한 코멘트도 여기 남겨두겠습니다.

RacingCar 클래스를 개선한 걸 봤는데, 좋은 부분은 다음과 같습니다.

compareTo 메소드 오버라이드: 매우 훌륭한 개선입니다. 💯 💯
toDTO 메소드: 이 부분도 정말 잘 하셨습니다. 이 메소드를 사용해서 DTO를 뷰에 전달해 주고, 뷰에서 해당 DTO의 값을 바탕으로 출력을 해주면 되겠죠?
실제 현업에서도 이런 패턴을 많이 사용하게 됩니다. 다만 한 가지 조언을 드리자면, 이렇게 하면 도메인이 DTO에 대해 의존을 가지게 되겠죠?
대신 DTO의 생성자 인자로 도메인을 받게 한 뒤, 외부에서 도메인을 인자로 전달해서 DTO를 생성해주면 어떨까요? DTO는 해당 값의 근원이 되는 도메인에 대해 알아야 하니까요.
이 코멘트에 대해 고민해보고, 적용하는게 좋을 것 같다면 적용해보세요. 필수는 아닙니다(지금의 toDTO도 충분히 잘하신 개선작업입니다)

다만 아쉬운 부분도 있었습니다.
getDesc 메소드를 보면 RacingCar 클래스가 출력이 어떻게 될지까지 책임을 지고 있네요. 그런데 생각해보면, 각각의 차의 이름과 position 정보만 output에 전달해주면 그걸 출력하는건 어렵지 않을텐데요. (getDesc 메소드와 같은 역할을 하는 메소드를 OutputView에 만들어주면 되니까요)
그러나 OutputView에 도메인 객체를 바로 전달하는건 좋지 않아 보입니다. 대신, 만들어둔 DTO를 OutputView에 전달하면 어떨까요? (현재는 List을 전달해서 각 라운드별 결과를 출력해주고 있던데, 이 부분을 List 전달하도록 수정해보라는 코멘트입니다)

include42

안녕하세요, 오잉! 리뷰어 카프카입니다.

리팩토링한 내용 잘 확인하였습니다. 코드가 이전보다 많이 개선된 걸 느낄 수 있었습니다. 💯
아직 수정이 필요한 부분에 코멘트 달아두었으니, 확인을 부탁드립니다.
질문주신 내용 + 도메인 코드 리팩토링에 대한 조언은 코멘트를 별도로 달아두었으니 확인해주세요.

조금만 더 힘내봅시다! 고생 많으셨습니다 💪

include42 · 2023-02-13T01:21:48Z

src/main/java/racingcar/domain/RacingCar.java


    private void plusPoint() {
-        point++;
+        position++;


이부분에서는 두 가지 개선이 필요해 보입니다.

아직 plusPoint라는 메소드명이 변경되지 않았습니다.

++ 연산 대신, 위에 상수를 정의해두고 해당 값을 사용하면 어떨까요?

include42 · 2023-02-13T01:29:08Z

src/main/java/racingcar/domain/Range.java

    private final int max;

    public Range(int min, int max) {
+        if (max < min) {


이 부분은 저번 리뷰때 언급된 부분이네요.
물론 swap을 하는 것도 좋지만, 일단 이 상황이 발생했다는건 코드에 뭔가 심각한 실수가 있다는 뜻이죠. 그래서 그걸 명확히 알 수 있다면 좋을 것 같습니다.
그래서 제 생각엔, 예외를 발생시키는게 좋지 않을까 싶습니다.

말씀하신대로 예외를 던지고 프로그램이 종료되도록 수정했습니다.
근데 어떠한 예외를 발생시키는 것이 좋을지 고민이 됩니다.
저는 IllegalArgumentException을 발생시켰는데, 좀 더 적절한 예외가 있을까요..?!

IllegalArgumentException이면 적절하다고 생각합니다! 😄

include42 · 2023-02-13T01:33:17Z

src/main/java/racingcar/controller/RacingCarGameController.java


        outputView.printResultHeader();
-        outputView.printRoundResult(roundManager.getCurrentRound());
+        outputView.printRoundResult(roundManager.getStartStatus());


혹시 RoundManager.getStartStatus 메소드의 역할이 무엇일까요?
해당 메소드가 필요한 이유에 대해 고민해보면 좋을 것 같네요.

제시된 실행결과를 보면 시도할 횟수+1회가 출력이 되는데, 그 중 첫번째 출력값은 기본세팅(모든 차들이 1에 위치)이더라구요! 그래서 본격적인 게임 시작전에 기본세팅+기본세팅출력을 위해 해당 메소드를 구현했습니다.

include42 · 2023-02-13T01:34:08Z

src/main/java/racingcar/utils/Parser.java

+    private static final String PARSING_EXCEPTION_MESSAGE = "빈 입력입니다.";
+
    public static List<String> parsing(String text, String delimiter) {
+        if (text == null || text.equals("")) {


isBlank에 대해 검색해보시면 더 간단하게 리팩토링이 가능해 보입니다.

그리고 delimiter를 외부에서 전달받고 있는데, parser 가 가져도 괜찮지 않을까요?
책임을 어느 쪽에서 가진다고 보면 좋을지 고민해봅시다. 😄

자동차 경주 프로그램 이외에도 Parser가 사용될 수 있다고 생각해서 delimiter를 외부에서 전달받은 방식으로 구현했습니다!
하지만 이번 미션에서는 이름은 쉼표(,)를 기준으로 구분한다는 요구사항이 정확히 있으므로, 말씀하신대로 Parser가 delimiter를 클래스 변수로 가지고 있어도 좋을 것 같습니다 😄

생각해보신 기준도 적절하다고 생각합니다. 다만 말씀해 주신대로 이번 미션에서는 delimiter가 가지고 있는 편이 좋을 것 같네요 👍
다음 미션에서 유사하게 Parser를 구현한다면 입력값에 따라 여러 delimiter가 쓰일 수도 있을듯한데, 이때엔 어떻게 하면 좋을지 페어와 논의를 해보기를 권해요 😄

include42 · 2023-02-13T01:35:44Z

src/main/java/racingcar/validator/Validator.java

+    private static final String NAME_DUPLICATE_EXCEPTION_MESSAGE = "자동차 이름은 중복될 수 없습니다.";
+    private static final String TRYCOUNT_NUMERIC_EXCEPTION_MESSAGE = "시도할 횟수는 자연수만 가능합니다.";
+
+    public void validateNames(List<String> names) {


이름에 대해 종합적으로 검증해주도록 구조를 잘 짜 주셨습니다.
이렇게 작성해두면 이후 검증 항목이 추가되거나 삭제되어도 유지보수하기 어렵지 않겠네요. 💯

include42 · 2023-02-13T01:38:45Z

src/test/java/racingcar/domain/AdvanceJudgementTest.java

@@ -22,6 +22,7 @@ class AdvanceJudgementTest {



혹시 커밋하기 전에 all test를 돌려보셨나요?
제가 돌려봤을때에는 RoundManagerTest에서 2건 실패하는 테스트가 있네요.
해당 실패한 테스트는 기존 코드가 수정된 것을 테스트에 반영하지 않아서 실패한 것인데,
이러한 경우 (물론 TDD로 개발하면 제일 좋지만) 바로 테스트를 코드와 동기화해줘야 이후 생길 버그를 방지할 수 있습니다.

실패하는 테스트가 프로젝트에 있어서는 안됩니다. 이 부분은 앞으로 미션 진행하시면서, 꼼꼼하게 확인하는 습관을 권합니다 😄

include42 · 2023-02-13T01:39:28Z

src/test/java/racingcar/validator/ValidatorTest.java

+
+    @ParameterizedTest()
+    @DisplayName("횟수 유효성 검사 테스트")
+    @ValueSource(strings = {"1", "3", "9", "100", "300", "500"})


성공하는 케이스에 대해 적절하게 테스트 잘 작성해 주셨습니다. 👍

…임을 RacingCar에게서 분리

include42

안녕하세요, 오잉! 리뷰어 카프카입니다.
이번 리뷰에 대해 수정을 잘 해주셔서, 다시 확인해봤을때 크게 걸리는 부분이 없었습니다. 👍
단위 테스트도 잘 작성해 주셨고, DTO의 적용도 바로 잘 해주셨네요.

이번 미션에서 DTO에 대해, 그리고 getter, setter의 사용에 대해 고민을 많이 하셨다고 생각합니다.
새로 고민하고 학습한 부분에 대해 정리를 잘 해두시고, 다음 미션에서도 잘 활용하시면 좋겠습니다.

이번 미션은 여기서 approve 하겠습니다. 고생 많으셨습니다 😄

include42 · 2023-02-13T15:25:06Z

src/main/java/racingcar/domain/RoundManager.java


-    public List<String> getCurrentRound() {
-        List<String> roundResult = new ArrayList<>();
+    public List<RacingCarDto> getStartStatus() {


반환값을 RacingCarDto 로 변경해 주신 부분 좋습니다.
OutputView에서도 해당 DTO를 통해 값 출력이 잘 되는것을 확인했습니다 😄

hanueleee added 9 commits February 12, 2023 16:23

refactor: InputView에서 next를 nextLine으로 수정

87048f8

refactor: CarNamesValidator와 TryCountValidator를 Validator로 통합

07c166f

refactor: Range의 min, max 값 검증 추가

f589023

refactor: Car의 인스턴수 변수명 수정 (point->position), 초기 위치 수정 (1->0)

d7976fd

refactor: Parser에서 파싱 전 유효성 검증 로직 추가

ed62826

refactor: RacingCarGameController 수정

93eb250

1. validator를 인스턴수 변수로 수정 2. validate 검사 후 재입력 로직을 반복문에서 재귀로 수정

refactor: Test 수정

9142f5d

1. AdvanceJudgementTest 전진 불가능 경우 테스트에 @DisplayName 추가 2. CarNamesValidatorTest와 TryCountValidatorTest 통합

refactor: ParsingTest 테스트케이스 추가

f21241f

include42 requested changes Feb 13, 2023

View reviewed changes

hanueleee added 4 commits February 13, 2023 14:47

refactor: RacingCar의 advance메소드에서 ++사용 대신 상수로 값 증가

5629be9

refactor: 라운드 진행 결과 출력에 RacingCar의 getDesc메소드 대신 toDto 사용 -> 출력에 대한 책…

376f1e7

…임을 RacingCar에게서 분리

refactor: Range객체 생성자의 인자 검증 로직 추가

730fd77

refactor: 전진테스트 수정

9c2dfbe

include42 approved these changes Feb 13, 2023

View reviewed changes

include42 merged commit 765db96 into woowacourse:hanueleee Feb 13, 2023

Conversation

hanueleee commented Feb 12, 2023

주요 수정 사항

궁금한 점

Uh oh!

include42 commented Feb 13, 2023

Uh oh!

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

hanueleee Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

include42 left a comment

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

hanueleee Feb 13, 2023 •

edited

Loading