[2단계 - 자동차 경주 리팩터링] 허브(방대의) 미션 제출합니다. #538

greeng00se · 2023-02-11T13:36:11Z

안녕하세요 😄

2단계는 혼자 리팩터링 하는 미션이지만, 1단계 미션에서 남겨주신 리뷰를 통해 페어와 함께 많은 이야기를 나누었습니다.
리뷰 남겨주신 내용이 정말 많은 도움이 되었던 것 같습니다.

리뷰를 기반으로 결정한 내용

제어할 수 없는 부분

나머지 부분에 대한 테스트를 더욱 꼼꼼하게 작성하는 거로 마무리했습니다.
기존에 작성했던 RepeatableTest의 경우 제 마음의 안정을 위해 남겨두었습니다!!

위임 메서드 테스트

이전의 테스트 코드가 결과를 검증하도록 해두었기 때문에 특별한 변화는 주지 않았습니다.

Count 클래스 테스트를 위한 getter

리뷰 남겨주신 대로 decrease 후 게임을 진행하지 못하게 하는 방법도 의미가 있다는 것에 동의를 하고, 따라서 getter를 제거하고 isPlayable로 테스트를 작성했습니다.

검증과 변환의 분리

말씀해 주신 레이싱카 크기의 어플리케이션에서는 이런 부분까지 나눈다면 클래스가 지나치게 많다는 생각입니다. 이 부분에 대해서 전적으로 동의합니다.
이번 미션에서 클래스를 최대한 분리해 보는 방향으로 코드를 작성하고 싶어서 Parse 하는 부분도 따로 클래스로 분리를 해보았습니다.
이유는 다음과 같습니다.
1. validator에서 변환이 이루어지는 경우 하나의 메서드에서 두 가지 동작을 하는 것처럼 느껴졌습니다.
2. 따라서 InputView로 변환을 하는 부분을 옮겨보았는데, 변환을 하는 부분 자체를 검증하고 싶었습니다. (split 하는 로직 등..)
따라서 InputParser라는 클래스로 분리하였고, 사실 조금 과하다는 생각이 들긴 하지만 테스트를 통해 마음의 안정감을 얻었습니다!

2단계 리팩터링

2단계 요구사항 + 기존의 코드를 리팩터링 하는 시간을 가졌습니다.
페어와 협업을 한다고 가정을 하고, 요구사항을 정리하고 조금 더 상세한 내용을 코드에 담도록 고민했습니다.

기존에 추가하지 못했던 기능 추가

중복 이름을 받는 경우 예외를 던지는 코드를 추가하였습니다.
시도 횟수를 100이하의 정수값을 입력받도록 제한을 추가하였습니다.

IntelliJ 테스트 실행 결과 메시지 개선

ParameterizeTest의 name을 이용하여 테스트 결과를 조금 더 한눈에 볼 수 있도록 개선하였습니다.

예외 메시지

예외 메시지가 조금 더 자세한 내용을 보여주도록 리팩터링 했습니다.

차 위치 비교 부분 변경

기존에 getPosition을 이용하여 비교를 했던 부분을 Comparable을 구현하도록 변경했습니다.

추가로 부족한 부분, 개선을 하면 좋겠다고 생각하시는 부분에 대해 리뷰 남겨주시면 바로 반영하겠습니다!
감사합니다!

- 추가로 예외 메시지를 던지는 조건을 메서드로 분리하여 읽기 쉽게 변경

- 객체보단 기능을 중심으로 작성

knae11

안녕하세요 :) 허브!

미션 하면서 해보고 싶은 내용 다 해보세요~~ㅋㅋㅋㅋㅋ 납득 가능하게 이유까지 남겨주어서 좋네요!

이번 미션에서 클래스를 최대한 분리해 보는 방향으로 코드를 작성하고 싶어서 Parse 하는 부분도 따로 클래스로 분리를 해보았습니다.
이유는 다음과 같습니다.

2단계 미션을 하면서 신경쓴 부분들을 PR 내용에 남겨주어서 알아차리가 너무 좋았답니다 :)

force push 한 기록들이 있던데요! 혼자 사용하는 브랜치는 force push 해도 상관 없지만 공유하는 브랜치에서 사용하면 커밋내용이 꼬일 수 있어서 하면 지양해야 한다는거 아시죠? (혹시나 해서 코멘트 남겨요! 사실..저도 혼자 사용하는데서는 force push...사용한답니다...ㅎㅎㅎ)

2단계 코멘트 남겨보았습니다! 이상한 부분이 있거나 애매한 부분들이 있다면 알려주세요 >_< 새롭게 시작하는 월요일도 화이팅!

knae11 · 2023-02-12T09:34:07Z

README.md

- [x] 자동차는 값을 입력받고 이동한다.
-    - 4 이상이면 전진한다.
-    - 3 이하의 값이면 멈춘다.
+## 개요


업데이트 되는 문서 좋아요! 👍

knae11 · 2023-02-12T09:35:40Z

src/main/java/racingcar/controller/RacingGameController.java

-            outputView.printPosition(cars);
+            racingGame.play();
+            List<Car> cars = racingGame.findCurrentCarPositions();
+            outputView.printCurrentCarPositions(cars);


좀 더 명확한 메서드 네이밍으로 변경해주셨군요! :)

knae11 · 2023-02-12T09:36:11Z

src/main/java/racingcar/domain/Car.java

+    private final Name name;
+    private final Position position;


오~ 불변객체로~!

knae11 · 2023-02-12T09:41:45Z

src/main/java/racingcar/domain/Name.java

@@ -4,7 +4,8 @@ public class Name {

    private static final int NAME_LOWER_BOUND = 1;
    private static final int NAME_UPPER_BOUND = 5;
-    private static final String INVALID_NAME_LENGTH_MESSAGE = "차의 이름은 1자 이상 ~ 5자 이하여야 합니다.";
+    private static final String INVALID_NAME_LENGTH_MESSAGE =
+            "차의 이름은 " + NAME_LOWER_BOUND + "자 이상, " + NAME_UPPER_BOUND + "자 이하여야 합니다. 입력된 차 이름 : ";


에러메서지가 좀 더 친절해졌군요 :)

knae11 · 2023-02-12T09:43:35Z

src/main/java/racingcar/domain/Name.java

-        if (name.length() < NAME_LOWER_BOUND || NAME_UPPER_BOUND < name.length()) {
-            throw new IllegalArgumentException(INVALID_NAME_LENGTH_MESSAGE);
+    private void validate(final String name) {
+        if (isInvalidNameLength(name)) {


따로 조건문을 빼주니 validate에서 어떤 내용을 확인해주는지 보기 좋네요~

knae11 · 2023-02-12T09:54:33Z

src/test/java/racingcar/domain/CarsTest.java

-        List<String> carNames = List.of("car1", "car2", "car3");
-
-        Cars cars = Cars.from(carNames);
+    @DisplayName("findWinners 메서드는 차량이 존재하지 않는 경우 예외를 던진다.")


findWinners의 역할은 우승자를 찾는것 아닌가요?

앗! 말씀하신대로 우승자가 존재하지 않는 경우 예외를 던진다. 라는 DisplayName이 더 적절해보입니다! 또한 그에 따른 예외 메시지를 변경하여 적용해보겠습니다!

knae11 · 2023-02-12T09:55:11Z

src/test/java/racingcar/domain/CarTest.java

@@ -7,11 +7,11 @@
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.CsvSource;

+@DisplayName("Car 클래스")


클래스나 메서드 이런 내용은 DisplayName에 들어가지 않아도 좋을 듯 싶어요 :)

knae11 · 2023-02-12T09:55:49Z

src/test/java/racingcar/domain/CarTest.java

 public class CarTest {

-    @ParameterizedTest
+    @ParameterizedTest(name = "move 메서드는 값을 입력받고 4이상인 경우 전진한다. 초기 위치: 0 입력값: {0} 동작 후 위치: {1}")


knae11 · 2023-02-12T09:57:09Z

src/test/java/racingcar/domain/CarsTest.java


+@DisplayName("Cars 클래스")


Car와는 다른 Cars 만의 역할에 대해 테스트를 작성해봐도 좋을 듯 하네요

앗! 말씀하신대로 모든 차들을 반환하는 테스트를 작성하지 않은 것 같습니다!!
또한 등록된 차들의 경주를 진행한다고 생각하여 move 대신 race로 메서드명을 변경했습니다.

knae11 · 2023-02-12T10:06:04Z

src/main/java/racingcar/domain/Position.java

@@ -2,7 +2,7 @@

 import java.util.Objects;

-public class Position {
+public class Position implements Comparable<Position> {


Position 클래스의 경우 기본적으로 컴파일러가 생성해주는 생성자를 사용하고 있는데요. 접근 제어자를 활용하여 생성의 범위를 좁히는 건 어떨까요~?

이 부분에 대해서 package-private 생성자를 사용하는게 맞을까요? 일단 도전해보겠습니다!

greeng00se · 2023-02-12T11:24:14Z

주말에도 리뷰 남겨주셔서 감사합니다.
리팩터링을 진행하면서 놓친 부분을 세심하게 리뷰 남겨주셔서 정말 감사합니다!!

리뷰 해주신 부분 개인적으로 생각하여 코드에 반영을 해보았습니다.
혹시나 제가 생각한 부분이 리뷰 남겨주신 의도와 다른 경우 알려주시면 정말 감사하겠습니다!!

force push한 걸 들켜버렸군요.. 😢
처음에는 커밋을 추가하여 이전에 잘못된 커밋 메시지를 변경해보려 했지만, 기록이 남아 force push를 해버렸습니다..ㅜㅜ
이 부분에 대해서 더 좋은 방법이 있을까요?

knae11

허브, 리뷰 내용 반영한 부분 확인했습니다. 👍
레이싱카 미션 너무 잘 수행해주셨네요! 앞으로도 화이팅입니다~~~~~~
Merge 할게요!

force push 이유가 커밋메세지 때문이었군요. 이미 remote에 push 했다면 force push 말고는 커밋메세지를 변경할 수 있는 방법은 저도 모르겠네요ㅠㅠ

greeng00se added 15 commits February 10, 2023 21:34

refactor: RacingGame을 초기화하는 부분을 동사형 메서드명으로 변경

cf2c296

refactor: final 키워드를 명시하지 못한 부분 final 추가

1fdc17c

refactor: 올바른 이름 길이가 아닌 경우 자세한 예외 메시지를 던지도록 수정

96a84fb

- 추가로 예외 메시지를 던지는 조건을 메서드로 분리하여 읽기 쉽게 변경

refactor: 도메인 로직에서 사용하지 않는 getter 제거

d896987

refactor: 입력값을 파싱하는 로직을 InputValidator에서 InputView로 이동

ee19c72

docs: 요구사항 문서가 지속 가능하도록 개선

9dd485c

- 객체보단 기능을 중심으로 작성

refactor: Comparable을 구현하여 Car 내에서 직접 위치값을 비교하도록 수정

a3dc0dc

refactor: 비교연산자를 이용하여 값을 비교하는 부분을 읽기 자연스러운 방향으로 수정

8aac5f9

test: IntelliJ Test Results 가독성 개선

503907a

test: TestNumberGenerator 패키지 변경

2fc54a4

test: extracting이 변수명에 의존하지 않고, 메서드를 의존하도록 변경(변수명 변경시 런타임 오류가 발생하도록 수정)

c78f646

refactor: 게임을 진행하고 현재 결과를 반환하는 play 메서드 분리

9be525d

refactor: 차의 이름에 대한 예외 메시지 수정 (1자 이상 ~ 5자 이하 -> 1자 이상, 5자 이하)

a2566b4

refactor: 시도 횟수를 최대 100까지 입력 받을 수 있도록 수정

a883094

test: RandomNumberGenerator 테스트 출력 메시지 가독성 개선

f61601f

greeng00se force-pushed the step2 branch from 694ecaa to e59f652 Compare February 11, 2023 23:43

greeng00se added 3 commits February 12, 2023 08:50

feat: 값 파싱하는 책임을 가진 InputParser 클래스 추가

d702f7c

feat: 비지니스 로직에서 중복된 차 이름 검증하는 부분 추가

b1ec968

refactor: 변수명 수정 및 사용하지 않는 테스트 제거

a1e9313

greeng00se force-pushed the step2 branch from e59f652 to a1e9313 Compare February 11, 2023 23:52

knae11 requested changes Feb 12, 2023

View reviewed changes

greeng00se added 6 commits February 12, 2023 19:21

refactor: scanner 사용 후 자원정리 하도록 수정

83e0e9d

refactor: cars가 하는 역할과 책임에 대해 설명하도록 수정

ed81369

refactor: 접근 제한자 package-private으로 변경

d6001f3

test: 클래스의 DisplayName 제거

d2908b1

refactor: 경주를 진행하는 메서드명을 race로 변경

a9b86b8

test: 경주에 참가한 모든 차의 이름과 현재 위치를 반환하는 getCars 메서드 테스트 추가

e326ca5

knae11 approved these changes Feb 13, 2023

View reviewed changes

knae11 merged commit 30a1797 into woowacourse:greeng00se Feb 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 자동차 경주 리팩터링] 허브(방대의) 미션 제출합니다. #538

[2단계 - 자동차 경주 리팩터링] 허브(방대의) 미션 제출합니다. #538

greeng00se commented Feb 11, 2023

knae11 left a comment •

edited

Loading

knae11 Feb 12, 2023

knae11 Feb 12, 2023

knae11 Feb 12, 2023

knae11 Feb 12, 2023

knae11 Feb 12, 2023

knae11 Feb 12, 2023

greeng00se Feb 12, 2023

knae11 Feb 12, 2023

knae11 Feb 12, 2023

knae11 Feb 12, 2023

greeng00se Feb 12, 2023 •

edited

Loading

knae11 Feb 12, 2023

greeng00se Feb 12, 2023

greeng00se commented Feb 12, 2023 •

edited

Loading

knae11 left a comment


		@DisplayName("Cars 클래스")

[2단계 - 자동차 경주 리팩터링] 허브(방대의) 미션 제출합니다. #538

[2단계 - 자동차 경주 리팩터링] 허브(방대의) 미션 제출합니다. #538

Conversation

greeng00se commented Feb 11, 2023

리뷰를 기반으로 결정한 내용

2단계 리팩터링

knae11 left a comment • 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

greeng00se Feb 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greeng00se commented Feb 12, 2023 • edited Loading

knae11 left a comment

Choose a reason for hiding this comment

knae11 left a comment •

edited

Loading

greeng00se Feb 12, 2023 •

edited

Loading

greeng00se commented Feb 12, 2023 •

edited

Loading