[빙봉] 자동차 경주 미션 제출합니다. #109

aegis1920 · 2020-02-13T08:37:32Z

많은 피드백 부탁드립니다! 감사합니다!!

- 커스텀 구분자 방식 변경("//#\n1#2#3" -> "//#//woowacourse#1#2#3")

- Car생성자에 이름(String) 매개변수 추가

- refactor: 메서드명 변경 printResult -> printNotice

- 입력 round 값 RacingCar 인스턴스 변수화 - 메서드 구조, 네이밍 수정 - 변경에 따른 테스트 코드 수정

jeonghoon1107

안녕하세요 빙봉!
구현 잘 하셨네요 💯
몇가지 피드백을 남겼으니 확인해주세요 :)

jeonghoon1107 · 2020-02-14T15:50:24Z

src/main/java/racingcar/domian/Car.java

+    }
+
+    private static String validateName(String name) {
+        name = name.trim();


name = null;
name.trim()을 하는 경우에는 어떻게 되나요?

validateNullOrEmpty(name); 을 name.trim()하기 전에 하도록 수정했습니다!

jeonghoon1107 · 2020-02-14T15:56:49Z

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

+
+    @DisplayName("랜덤 생성된 값이 4이상일 때 이동 테스트")
+    @ParameterizedTest
+    @CsvSource(value = {"1,false", "2,false", "3,false", "4,true", "5,true", "6,true", "7,true", "8,true", "9,true"})


테스트 코드 역시 리팩토링의 대상입니다. 너무 많은 케이스를 테스트할 필요는 없어보여요.
경계값을 기반으로 테스트하도록 변경해보면 좋을 것 같아요.

처음 리펙토링을 했을 때 0, 3, 4, 9 순으로 리펙토링을 했습니다. 그러나 다시 리펙토링하면서 RandomNumber 객체에게 메세지를 보내도록 하여 아래와 같은 테스트 코드로 수정했습니다!

if(randomNumber.getRandomNumber() >= 4) { assertThat(position.getDistance() == 1).isTrue(); }

RandomNumber 객체 - randomNumber를 validate해주기 위해 validateRandomNumber() 메서드 추가 - 객체에게 움직일 수 있냐는 메세지를 보내기 위해 isMovable() 메서드 추가

jeonghoon1107

안녕하세요 빙봉!
아쉬운 점이 있어서 피드백 남겼습니다!
확인해주세요 :)

jeonghoon1107 · 2020-02-17T14:02:00Z

src/main/java/racingcar/domian/RandomNumber.java

+        return randomNumber >= RANDOM_MIN_LIMIT_NUMBER && randomNumber < RANDOM_MAX_LIMIT_NUMBER;
+    }
+
+    public static boolean isMovable() {


움직임에 대한 판단은 RandomNumber가 하는 것이 맞나요?

isMovable() 메서드는 Car의 Position에게 물어보는 것이 맞다고 판단되어 수정했습니다!

jeonghoon1107 · 2020-02-17T14:03:58Z

src/main/java/racingcar/domian/RandomNumber.java

+
+    public RandomNumber() {
+        int randomNumber = generateRandom();
+        if(validateRandomNumber(randomNumber)){


유효하지 않으면 어떻게 되나요?

Random 클래스에서 생성해주는 범위가 이미 있기 때문에 굳이 validate 해주지 않아도 될 것 같다고 판단하여 validate 메서드를 삭제했습니다!

jeonghoon1107 · 2020-02-17T14:05:55Z

src/main/java/racingcar/domian/RandomNumber.java

+
+import java.util.Random;
+
+public class RandomNumber {


상태값을 가질 필요가 없는 것 같은데 매번 인스턴스를 생성할 필요가 있을까요?

말씀대로 상태값을 가질 필요가 없을 것 같아 RandomNumber의 속성을 지우고 랜덤 번호를 생성해주는 메서드만 만들어 호출하도록 했습니다!

상태 값을 갖지 않는 클래스로 변경되었는데 RandomNumber라는 클래스 이름도 명시적으로 변경되면 좋을 것 같아요!
하나의 랜덤한 숫자같은 의미처럼 보여요!

jeonghoon1107 · 2020-02-17T14:07:23Z

src/main/java/racingcar/domian/Cars.java

+public class Cars {
+
+    private static final String EMPTY_STRING = "";
+    private static final CarFactory carFactory = new CarFactory();


CarFactory가 상태값을 가지고 있나요?
상태값을 갖는 것과 갖지 않는 것에 대해 구분할 필요가 있어보여요!

CarFactory가 상태값을 가지지 않고 있고 팩토리 패턴을 사용한 것도 아니며 메소드를 뺀 것에 불과한 것 같아 삭제했습니다!

jeonghoon1107 · 2020-02-17T14:10:20Z

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

+
+public class CarTest {
+
+    @DisplayName("랜덤 생성된 값이 4이상일 때 이동 테스트")


4 이하일 때의 케이스를 추가해야할 것 같아요.
또한, 해당 테스트를 보면 car에 대한 테스트가 아니고 position에 대한 테스트 같네요 :)

move()에 대해서 코드를 고치고 테스트케이스를 수정했습니다!

jeonghoon1107 · 2020-02-17T14:12:19Z

src/main/java/racingcar/domian/Cars.java

+        }
+    }
+
+    public String getResult() {


우승자를 관리하는 클래스를 따로 분리하면 어떨까요?

Winners 라는 클래스를 만들어 따로 분리했습니다! 다만 제 Winners 클래스에 보면 매개변수로 모두 cars를 받기때문에 상태값이 없는 클래스가 되었는데 이렇게 메소드만으로 정의한 클래스를 클래스 분리로 생각해도 좋을지 여쭙고 싶습니다! 메소드가 우승자에 관련되어 있다는 것만으로 우승자라는 클래스로 분리하는 것이 괜찮은 생각인지 궁금합니다!

public class Winners { public static List<Car> findWinner(List<Car> cars) { int maxPosition = 0; for(Car car : cars) { maxPosition = car.getBiggerPosition(maxPosition); } return getWinners(cars, maxPosition); } private static List<Car> getWinners(List<Car> cars, int maxPosition) { List<Car> winners = new ArrayList<>(); for(Car car : cars) { if(car.isWinner(maxPosition)){ winners.add(car); } } return winners; } }

…는 메서드 삭제

…View에 ","를 joining하도록 구현, isNotEmpty() 메서드 삭제

jeonghoon1107

안녕하세요 빙봉!
깔끔하네요 👍
아쉬운 점이 있어서 피드백 남겼으니 확인해주세요!

jeonghoon1107 · 2020-02-19T11:10:01Z

src/main/java/racingcar/domian/car/Car.java

+    }
+
+    public void move(int number) {
+        if(position.isMovable(number)) {


position이 움직임을 판단하는 것보다 car가 판단하는 것은 어떨까요?

객체에 역할에 따라 판단해야된다고 생각해 Car 객체에 isMovable() 메서드를 넣도록 했습니다!

jeonghoon1107 · 2020-02-19T11:12:35Z

src/main/java/racingcar/domian/Cars.java

+    private final List<Car> cars;
+
+    public Cars(String inputNames) {
+        String[] names = inputNames.split(DELIMITER);


메서드로 추출하면 조금 더 깔끔해질 것 같아요!

split과 아래 for문 모두 수정했습니다!

jeonghoon1107 · 2020-02-19T11:21:12Z

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

+        cars.getCars()
+                .add(new Car(new Position(maxPosition), "user5"));
+
+        assertThat(cars.getCars()


테스트안에 비즈니스 로직에 해당하는 부분이 별도로 구현되어있네요!

CarTest에 이미 isWinner를 테스트하고 있었는데 못 봤던 것 같습니다! 테스트하려는 메소드 자체가 없어서 삭제했습니다. 테스트 안에 비즈니스 로직을 넣지 않도록 염두하겠습니다!

jeonghoon1107 · 2020-02-19T11:21:34Z

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

+                .stream()
+                .filter(car -> car.isWinner(maxPosition))
+                .findFirst()
+                .get()


get이 어떻게 구현되어있는지 보셨나요? get의 사용은 지양하는 것이 좋을 것 같아요.

public T get() { if (value == null) { throw new NoSuchElementException("No value present"); } return value; }

jeonghoon1107 · 2020-02-19T11:28:37Z

src/main/java/racingcar/domian/RandomNumber.java

+
+import java.util.Random;
+
+public class RandomNumber {


상태 값을 갖지 않는 클래스로 변경되었는데 RandomNumber라는 클래스 이름도 명시적으로 변경되면 좋을 것 같아요!
하나의 랜덤한 숫자같은 의미처럼 보여요!

jeonghoon1107

안녕하세요 빙봉!
깔끔하네요 💯
미션 진행하느라 고생 많으셨습니다 :)

chomily added 26 commits February 11, 2020 14:57

feat: 문자열 덧셈 계산기 기능 구현

028e826

feat: 메인 메소드 추가 및 예외 발생 시 재입력 기능 추가

ee92c98

- 커스텀 구분자 방식 변경("//#\n1#2#3" -> "//#//woowacourse#1#2#3")

refactor: 하드코딩 값 상수화

a9f2813

refactor: 유틸성 클래스로 변경

cff2c0a

docs: README 구현 기능 목록 업데이트

c364c75

feat: input값이 4이상이면 이동하는 메서드 추가

fab5e0c

feat: null or empty로 입력 값이 들어왔을 때 예외처리 기능

3c0330f

feat: 입력 이름 길이 유효성 검사 기능 추가 및 네이밍 수정

5fc0d3e

feat: 입력 횟수 10번 초과 처리 기능

009be64

feat: 입력 횟수 변환 및 예외 처리

0dc98d4

feat: 자동차 5대 초과 예외 처리 기능

8a9fa72

feat: 0 - 9 사이 랜덤 숫자 생성 기능 추가

9245252

feat: Max Position 을 반환하는 메서드 추가

a15cda0

feat: 우승자 찾는 기능 추가

1314136

- Car생성자에 이름(String) 매개변수 추가

feat: 경주할 자동차 이름 입력받고 게임 셋팅기능 추가

cbca8c2

refactor/style: 접근제한자 수정 및 필드 변수 상수화

840abc9

feat: 게임 횟수 입력 기능 추가

aaf461d

fix: validateCount 메서드 범위 수정, 입력값 trim() 적용

c7a6a83

refactor: RandomNumberGenerator 클래스 분리

99f815b

feat: 중간 결과 출력 기능 및 라운드 실행 기능 추가

34c1578

fix: 입력갑 트림 버그 수정

374ee37

feat: 우승자 확인 및 출력 기능

0c52d23

- refactor: 메서드명 변경 printResult -> printNotice

refactor: 하드코딩값 상수화

6d51639

refactor: validate 메서드 이동(InputHandler -> 실사용 클레스 내부), 테스트 코드 변경

96d4a57

refactor: 실행 로직을 RacingCarApplication main으로 이동 및 구조 변경

df72afd

- 입력 round 값 RacingCar 인스턴스 변수화 - 메서드 구조, 네이밍 수정 - 변경에 따른 테스트 코드 수정

refactor: Displayname 어노테이션 추가, 불필요한 static 제거, 코드 정렬

8968419

jeonghoon1107 suggested changes Feb 14, 2020

View reviewed changes

aegis1920 added 3 commits February 15, 2020 10:27

refactor : convention 수정 및 Random값에 final 추가

98b670b

refactor : 메서드명 리펙토링(move -> moveByRandom), feat : isNotEmpty 메서드 추가

e6543c2

refactor : 경계값 기반 테스트로 수정

ebe42b9

aegis1920 added 5 commits February 16, 2020 16:28

feat : position 객체, refactor : RandomNumber 객체 수정

f43f605

RandomNumber 객체 - randomNumber를 validate해주기 위해 validateRandomNumber() 메서드 추가 - 객체에게 움직일 수 있냐는 메세지를 보내기 위해 isMovable() 메서드 추가

feat : CarFactory 클래스 추가

21672d1

refactor : Position 클래스의 변수명 수정, GameRound 클래스 추가

3eb3f79

refactor : 접근제한자 및 컨벤션 수정

2fe069d

fix ; 테스트 코드 오류 수정

817136a

jeonghoon1107 suggested changes Feb 17, 2020

View reviewed changes

aegis1920 added 4 commits February 18, 2020 19:15

fix : CarFactory 클래스 삭제

12f5986

refactor : isMovable() 메서드 및 move() 메서드 로직 수정, RandomNumber 클래스에서 필요없…

ed85fe0

…는 메서드 삭제

refactor : isWinner 및 MaxPosition인지 묻는 메서드 재구현, Winner 클래스 생성, Output…

78ae77a

…View에 ","를 joining하도록 구현, isNotEmpty() 메서드 삭제

refactor : 메서드명 변경 및 테스트코드 수정

52c50bf

jeonghoon1107 suggested changes Feb 19, 2020

View reviewed changes

aegis1920 added 5 commits February 20, 2020 19:32

refactor : 가장 먼 자동차의 거리(우승자 찾기)를 구하는 테스트 삭제

625464f

refactor : RandomNumber 클래스명을 RandomNumberGenerator 클래스명으로 수정

7b869e3

refactor : isMovable 메서드를 Car 객체로 옮김

7dddf5a

refactor : CarTest 수정

735fcd3

feat : WinnersTest 클래스 추가 및 테스트코드 작성

995af6f

jeonghoon1107 approved these changes Feb 23, 2020

View reviewed changes

jeonghoon1107 merged commit 2d8643d into woowacourse:aegis1920 Feb 23, 2020


		public class CarTest {

		@DisplayName("랜덤 생성된 값이 4이상일 때 이동 테스트")

[빙봉] 자동차 경주 미션 제출합니다. #109

[빙봉] 자동차 경주 미션 제출합니다. #109

Uh oh!

Conversation

aegis1920 commented Feb 13, 2020

Uh oh!

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

jeonghoon1107 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

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!

jeonghoon1107 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeonghoon1107 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants