[연습] 자동차 경주 by This2sho · Pull Request #514 · woowacourse/java-racingcar-precourse

This2sho · 2022-12-01T08:40:35Z

🚗 구현 기능 목록

자동차

각 자동차에 이름을 부여할 수 있다.
- [예외처리] 자동차의 이름이 5자 초과일 경우 예외처리한다.
전진하거나 멈출 수 있다.
- 연료가 4 이상일 경우 전진한다.
- [예외처리] 연료가 0 미만 9 초과의 값이 들어오면 예외처리한다.

자동차 경주 게임

경주에 참여할 자동차들을 등록한다.
- [예외처리] 참여할 자동차는 최소 1대여야한다.
이동 횟수를 등록한다.
- [예외처리] 이동횟수가 0 이하면 예외처리한다.
자동차 경주를 시작한다.
- 각 이동횟수마다 각 자동차에게 랜덤한 연료를 주입한다.
자동차 경주 게임을 완료한 후 누가 우승했는지를 알려준다.(여러명의 우승자가 나올 수 있다.)

입력

자동차 이름은 쉼표(,)를 기준으로 구분하며 이름을 입력받는다.
- [예외처리] 쉼표를 기준으로 나눴을 때 자동차 이름이 0보다 작거나 5보다 크면 예외처리한다.
사용자는 몇 번의 이동을 할 것인지를 입력할 수 있어야 한다.
- [예외처리] 이동 횟수는 숫자여야 한다.
사용자가 잘못된 값을 입력할 경우 에러 메시지를 출력 후 그 부분부터 입력을 다시 받는다.

출력

자동차 이름과 자동차의 현재 위치를 출력한다.
최종 우승자를 출력한다.
- 우승자가 여러 명일 경우 쉼표(,)를 이용하여 구분한다.

동작 순서

경주에 등록할 자동차를 입력받는다.
경주 시도 횟수를 입력받는다.
시도 횟수 만큼 자동차가 전진하거나 멈춘다.
경주가 종료되고 우승자를 알려준다.

자동차 이름을 부여하는 부분은 자동차쪽으로 수정

0보다 작으면 -> 0 이하면

MIN_FUEL -> MIN_MOVE_FUEL 최소 연료 = MIN_FUEL

CarGameTest -> 이동횟수_등록_실패_테스트

레이스를 이동횟수 만큼 실행 -> 레이스를 1회만 실행하고 이동횟수를 차감하도록

sh111-coder · 2022-12-01T10:04:06Z

+
+
+    public void moveOrStop(int fuel) {
+        validateFuel(fuel);


Fuel 부분은 Randoms 라이브러리를 사용하는데,
검증 책임은 라이브러리를 만든 쪽에 있다고 생각해서
라이브러리를 사용하는 부분은 검증하지 않아도 될 것 같습니다!

저도 같은 의견입니다 !

sh111-coder · 2022-12-01T10:06:12Z

    // 추가 기능 구현
+    private void validateName(String name) {
+        if (name.length() > MAX_NAME_LENGTH) {
+            throw new IllegalArgumentException(Message.CAR_NAME_VALIDATION);


저는 예외 메세지는 추후에도 수정할 일이 없고,
여러 곳이 아닌 해당하는 하나의 예외에만 사용되기 때문에
가독성을 위해 상수화하지 않고 있습니다!
다른 분들의 의견은 어떨지 모르겠네요!

저도 성훈님이랑 비슷한 의견입니다

sh111-coder · 2022-12-01T10:13:05Z

+import java.util.List;
+import java.util.stream.Collectors;
+
+public class CarGame {


'CarGame'이라고 네이밍을 하면 어떤 역할을 가지는 객체인지
처음에 알기 힘들 것 같아요!
어떤 책임을 가지는 객체인지 알기 쉽게 네이밍 해주면 좋을 것 같습니다!

추가적으로 CarGame이 너무 많은 역할을 하고 있는 것 같아요.
MVC 패턴을 참고해서 클래스 분리를 하는 것이 좋을 것 같아요!

sh111-coder · 2022-12-01T10:23:24Z

+        }
+    }
+
+    public List<Car> getCars() {


Collection에 getter를 사용할 때 그냥 Collection 자체를 return하게 되면
외부에서 Collection을 받아서 Collection 내부의 값을 변경할 수도 있으므로
Collections.unmodifiableList(cars)로 외부에서 내부의 값을 변경 못하도록 막아서 반환하는 게 좋을 것 같습니다!

저도 같은 생각입니다

감사합니다 이 부분은 처음 알았네요!
적용해보겠습니다~!

sh111-coder · 2022-12-01T10:26:27Z

+        for (Car car : cars) {
+            car.moveOrStop(getRandomFuel());
+        }
+        moveCounts--;


시도 횟수를 하나씩 줄여가면서 하는 것보다
for문으로 하나씩 늘려가면서 하는 게 가독성에 더 좋을 것 같습니다!

sh111-coder · 2022-12-01T10:30:00Z

+
+    public List<String> announceWinners() {
+        return cars.stream()
+            .filter(car -> car.getCurrentPosition() == cars.stream().map(Car::getCurrentPosition)


저도 Car에 Position을 얻기 위해 getter를 사용해서 받아왔었는데,
getter를 사용하는 대신 객체에 메시지를 보내자
이후에 생각나서 찾아보니
이 링크를 참고해서 Car 객체 내부에서 비교를 하도록 하는 것이 좋을 것 같습니다!

좋은 자료 감사합니다!!

sh111-coder · 2022-12-01T10:31:43Z

+    private final InputView inputView;
+    private final OutputView outputView;
+
+    public MainController() {


클래스 변수로 받을 때 new로 생성 안하고,
생성자로 MainController가 생성될 때
new로 생성하는 이유가 따로 있나요?!

딱히 이유는 없었던거 같아요.
스프링 프로젝트를 할 때 생성자 주입을 사용하다보니 습관적으로 한거 같아요!

sh111-coder · 2022-12-01T10:40:34Z

+            int moveCounts = inputView.inputMoveCounts();
+            carGame.registerMoveCounts(moveCounts);
+        } catch (IllegalArgumentException e) {
+            outputView.printError(e.getMessage());


catch에서 Error 메세지를 출력할 때,
저는 그냥 System.out.println으로 출력했는데
OutputView에 에러를 출력하는 기능을 만들어서
OutputView에게 에러 출력 책임을 맡기게 하는 것 잘 배워갑니다!

sh111-coder · 2022-12-01T10:48:33Z

+    private static final String ONLY_NUMBER_REGEX = "^[0-9]+$";
+
+    public List<String> inputCars() {
+        System.out.println(Message.INPUT_CAR_MESSAGE);


입력 문구를 출력하는 것도 출력하는 책임이라고 생각해서
출력 처리를 하는 책임을 가진 OutputView에서 처리하는 것이 좋을 것 같습니다!

흠..저도 프리코스 진행할 때 고민했었는데
저는 View라는 자체가 사용자에게 보여지는 계층이다 보니
입력에 필요한 출력은 어쩔수 없이 InputView에서 담당하는게 맞다고 생각했습니다.

InputView 클래스 자체가 입력에 관한 책임을 가지기 때문에 저도 입력에 필요한 출력은 InputView에서 담당하는것이 좀 더 낫다고 생각합니다 !

sh111-coder · 2022-12-01T10:50:56Z

+        System.out.println(Message.OUTPUT_RACE_RESULT);
+    }
+
+    private void printCar(Car car) {


포맷으로 출력하는 System.put.printf를 몰랐었는데
건호님 코드 보고 알게 되었습니다! 감사합니다

sh111-coder · 2022-12-01T10:52:28Z

+
+    private String getMove(int currentPosition) {
+        StringBuilder stringBuilder = new StringBuilder();
+        for (int i = 0; i < currentPosition; i++) {


반복문 변수도 i 말고 네이밍 해주는 것이 좋을 것 같습니다!

sh111-coder · 2022-12-01T10:55:31Z

+        @DisplayName("성공 테스트")
+        void 자동차_등록_성공_테스트() {
+            //when
+            List<String> cars = Arrays.asList("pobi", "woni", "jun");


자바 8이라 List.of를 못 써서 직접 List에 add 했었는데,
Arrays.asList가 있었군요! 배워갑니당

저도 처음에 List.of를 사용하려고 했는데 자바 8이라 안되서 당황했는데
Arrays.asList는 사용 가능하더라구요..!

sh111-coder · 2022-12-01T10:58:56Z

+
+        @Test
+        @DisplayName("실패시 에러발생")
+        void 자동차_등록_실패_테스트() {


정확하게 어떤 이유 때문에 실패하는 테스트인지 네이밍해주는 게 좋을 것 같아요!
'자동차_이름이_비었을_경우_테스트_실패()' 이런 식으로
구체적으로 적어주는 게 좋을 것 같습니다!

sh111-coder · 2022-12-01T11:01:17Z

+
+    @Test
+    @DisplayName("이동횟수 등록 실패 테스트")
+    void 이동횟수_등록_실패_테스트() {


마찬가지로 '이동_횟수가_0이면_등록_실패' 느낌으로
구체적으로 네이밍 해주시면 좋을 것 같습니다.
또, 테스트할 것이 이동 횟수가 0일때, 음수일 때, 숫자가 아닐 때 등등 여러 조건이
있으므로 @ParameterizedTest와 @ValueSource를 사용해서
하나의 메소드에 여러 조건을 추가해서 테스트하는 것이 좋을 것 같습니다!

sh111-coder

코드 잘 봤습니다!!
기능 목록도 가독성 좋게 잘 읽히는 것 같습니다!
전체적으로 하나의 클래스가 여러 책임을 가지는 것 같아서
MVC 패턴으로 클래스가 하나의 책임만을 가지게 클래스를 분리하면 좋을 것 같아요!

drunkenhw

코드 잘봤습니다. 제 실력이 부족해 리뷰에 대해 의문점이나 아닌 것 같은 점이 있으면 말씀해주시면 감사하겠습니다.

drunkenhw · 2022-12-01T13:58:00Z

    // 추가 기능 구현
+    private void validateName(String name) {
+        if (name.length() > MAX_NAME_LENGTH) {
+            throw new IllegalArgumentException(Message.CAR_NAME_VALIDATION);


저도 성훈님이랑 비슷한 의견입니다

drunkenhw · 2022-12-01T14:04:00Z

+        for (String carName : cars) {
+            this.cars.add(new Car(carName));
+        }


Suggested change

for (String carName : cars) {

this.cars.add(new Car(carName));

}

this.cars = cars.stream()

.map(Car::new)

.collect(Collectors.toList());

이런식으로 map을 사용하는 코드는 어떠신가요?

이 부분에 대해서는 stream으로 처리할 생각을 못했는데
사용하는 것도 괜찮은거 같습니다.

drunkenhw · 2022-12-01T14:04:32Z

+        }
+    }
+
+    public List<Car> getCars() {


저도 같은 생각입니다

drunkenhw · 2022-12-01T14:08:18Z

+    }
+
+    public boolean checkIsOver() {
+        return moveCounts <= 0;


0보다 작은 경우라고 표현하는 것보다 확실이 0일 경우 끝난다고 표현하는 것이 좋아보입니다

확실히 우석님이 말씀하신대로 작성하는게 side effect 가 없을 거 같네요!
감사합니댜!

drunkenhw · 2022-12-01T14:13:48Z

+        if (checkIsOver()) {
+            throw new IllegalStateException(Message.GAME_OVER);
+        }


MainController에서 while (!carGame.checkIsOver()) 하면서 반복하고 있는데 if문으로 한번 더 검증하는 로직은 불필요할 것 같아 보입니다!

drunkenhw · 2022-12-01T14:16:13Z

+        String[] inputCars = input.split(CAR_REGEX);
+        return Arrays.stream(inputCars).collect(Collectors.toList());


Suggested change

String[] inputCars = input.split(CAR_REGEX);

return Arrays.stream(inputCars).collect(Collectors.toList());

return Arrays.stream(input.split(CAR_REGEX)).collect(Collectors.toList());

한번 밖에 쓰이지 않는 변수를 할당하는 것보다 합치는 것이 어떨까요?

놓친거 같아요! 확실히 깔끔해보이네요~

drunkenhw · 2022-12-01T14:18:39Z

+        for (String inputCar : inputCars) {
+            if (inputCar.length() > Car.MAX_NAME_LENGTH) {
+                throw new IllegalArgumentException(Message.CAR_NAME_VALIDATION);
+            }
+        }


자동차의 이름에 대한 유효성 검증은 Car 객체에서 하는 것이 좋아보이고 실제로 Car 클래스에서 하시고 계셔서 없어져도 좋을 코드 같습니다!

다시보니 검증에 대한 중복이 많네요!
신경써야할 거 같습니다 감사합니다~

drunkenhw · 2022-12-01T14:20:12Z

+        for (Car car : cars) {
+            printCar(car);
+        }
+        printNewLine();


이런식으로 메서드를 만드는 것이 좋아보이네요 배워갑니다!

drunkenhw · 2022-12-01T14:21:37Z

+
+        @Test
+        @DisplayName("성공 테스트")
+        void 자동차_등록_성공_테스트() {


@DisplayName이 있는데 메서드명을 한글로 하신 이유가 궁금합니다

처음에 테스트 코드를 작성할 때 한글로 작성하고 계층형 구조로 리팩토링할 때 기존 메서드 명으로 가져갔습니다 !!

Kim0914 · 2022-12-02T14:13:00Z

+
+
+    public void moveOrStop(int fuel) {
+        validateFuel(fuel);


저도 같은 의견입니다 !

Kim0914 · 2022-12-02T14:14:25Z

+        if (fuel >= MIN_MOVE_FUEL) {
+            move();
+        }
+    }


도메인 함수 내부에서 다른 도메인 함수를 호출하는 것 보다
boolean type을 반환하도록 moveOrStop 함수를 변경해 접근해보면 어떤지 제안드립니다 !

Kim0914 · 2022-12-02T14:17:22Z

+    public CarGame() {
+        cars = new ArrayList<>();
+    }
+
+    public void registerCar(List<String> cars) {
+        validateCars(cars);
+        for (String carName : cars) {
+            this.cars.add(new Car(carName));
+        }


Car 객체들을 registerCar 함수가 아닌, 컨트롤러에서 생성해 생성자 주입 방식을 사용하는 방법을 제안드립니다 !

Kim0914 · 2022-12-02T14:20:00Z

+
+    private void startRace() {
+        outputView.printRaceStart();
+        while (!carGame.checkIsOver()) {


"게임을 종료할 수 없는 동안" 이라는 조건보다, "게임이 진행되는 동안"으로 표현한다면 좀 더 직관적일 것 같습니다.

Kim0914

생각을 많이 하고 설계를 했다는 느낌을 많이 받은 것 같습니다 !

예외 검증하는 로직에 고민이 많았는데, @This2sho님 코드 참고하면 도움이 많이 될 것 같습니다 !!

잘 보고 갑니다 :)

Kim0914 · 2022-12-02T14:31:18Z

@@ -0,0 +1,39 @@
+# 🚗 구현 기능 목록


기능 목록을 자세히 작성하신 것이 보기 좋은 것 같습니다 !

Kim0914 · 2022-12-02T14:33:34Z

+    public void registerMoveCounts(int moveCounts) {
+        validateMoveCounts(moveCounts);
+        this.moveCounts = moveCounts;
+    }


도메인에서 한번 더 검증을 해줄 필요가 있을까? 라는 생각이 있었는데
결국 계층간의 의존성 없이 개발을 하기 위해서는 확실히 한번 더 검증을 해주는 것이 좋다는 느낌이 드는 것 같습니다 :D
잘 배우고 갑니다 !

This2sho added 30 commits December 1, 2022 13:40

docs: 구현 기능 목록 정의

3732b70

docs: 구현 기능 목록 정의

64f0da6

test(Car): 자동차 이름 유효성 검사 테스트

c74ffb9

feat(Car): 자동차 이름 유효성 검사 로직 구현

d3b17d4

test(Car): 자동차 전진 기능 테스트

5402740

feat(Car): 자동차 연료에 따른 전진 기능 구현

5c37b2f

docs: 구현 기능 목록 수정

a50d563

자동차 이름을 부여하는 부분은 자동차쪽으로 수정

docs: 구현 기능 목록 기능 분리

a38d016

docs: 구현 기능 목록 기능 분리

06d72a5

test(CarGame): 자동차 등록 성공, 실패 테스트

61b8daf

feat(Car): 자동차 이름 사용할 수 있게 getter 적용

16af14f

feat(CarGame): 자동차 등록하는 기능 구현

970064c

refactor(CarGameTest): 초기화 init 메소드로 추출

b9be6d0

docs: 이동횟수 예외처리 부분 수정

d35210d

0보다 작으면 -> 0 이하면

test(CarGame): 이동횟수 등록시 예외 로직 검증

a0fd5c5

feat(CarGame): 이동횟수 등록 기능 구현

389d30f

refactor(Car): 상수명 변경

15427cd

MIN_FUEL -> MIN_MOVE_FUEL 최소 연료 = MIN_FUEL

refactor(CarGameTest): 테스트명 변경

475188d

CarGameTest -> 이동횟수_등록_실패_테스트

docs: 자동차 경주 게임 기능 분리

7f14435

test(CarGame): 자동차 경주 우승자 확인 테스트

780dc00

feat(CarGame): 자동차 경주 시작 기능 구현

2eb4395

feat(CarGame): 자동차 경주 우승자 알려주는 기능 구현

6854d10

feat(InputView): 자동차 입력 기능 구현

391b802

feat(InputView): 이동 횟수 입력 기능 구현

a36a3f5

feat(OutputView): 자동차 출력 기능 구현

7548e0f

feat(OutputView): 우승자 출력 기능 구현

40e9197

feat(InputView): 입력 메시지 추가

0d8b21e

refactor(CarGame): 자동차 경주 시작 로직 변경

8eb4a81

레이스를 이동횟수 만큼 실행 -> 레이스를 1회만 실행하고 이동횟수를 차감하도록

feat(OutputView): 경기 시작 메시지 출력 기능 추가

9cb01d1

feat(InputView): 자동차 이름 입력 유효성 검증 추가

b7c46fa

This2sho added 11 commits December 1, 2022 16:59

feat(MainController): 자동차 경주 순서에 맞게 서비스 기능과 입/출력 구현

9ae3034

feat(OutputView): 에러 출력 기능 구현

694ec26

feat(MainController): 사용자가 잘못된 값을 입력할 경우 그 부분부터 다시 입력받는 기능 추가

82bdc23

feat(Application): 프로그램 시작 기능 구현

03a3227

refactor(All): 출력관련 메시지 분리

4696e37

perf(OutputView): 개행 출력 개행 문자로 변경

a61b1a5

refactor(InputView, OutputView): view 패키지로 이동

dfd9714

docs: 구현 기능 목록에 동작 순서 추가

b8fb51b

feat(MainController): 에러 발생시 다시 입력받는 기능 수정

a0eda97

refactor(CarTest): 계층형 구조로 테스트 변경

5e3d7d3

refactor(CarGameTest): 계층형 구조로 테스트 변경

bc56861

sh111-coder reviewed Dec 1, 2022

View reviewed changes

drunkenhw reviewed Dec 1, 2022

View reviewed changes

Kim0914 reviewed Dec 2, 2022

View reviewed changes

refactor(MainController): 반복 입력받는 try-catch 부분 중복 제거

2cff2fa

		String[] inputCars = input.split(CAR_REGEX);
		return Arrays.stream(inputCars).collect(Collectors.toList());

	String[] inputCars = input.split(CAR_REGEX);
	return Arrays.stream(inputCars).collect(Collectors.toList());
	return Arrays.stream(input.split(CAR_REGEX)).collect(Collectors.toList());

Conversation

This2sho commented Dec 1, 2022

🚗 구현 기능 목록

자동차

자동차 경주 게임

입력

출력

동작 순서

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!

sh111-coder Dec 1, 2022 • 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!

sh111-coder Dec 1, 2022 • 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!

sh111-coder Dec 1, 2022 • 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!

sh111-coder left a comment

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sh111-coder Dec 1, 2022 •

edited

Loading

sh111-coder Dec 1, 2022 •

edited

Loading

sh111-coder Dec 1, 2022 •

edited

Loading