첫번째 자동차 경주 #16

NDjust · 2020-04-30T12:29:17Z

첫번째 자동차 경주입니다!

피드백 부탁드립니다!

pci2676

나단님 1단계 구현하느라 고생하셨습니다!
생각해보실만한 부분에 대한 리뷰를 남겼습니다!

수정하시면서 2단계 진행하시면 될것 같아요~

pci2676 · 2020-05-10T14:07:17Z

src/main/java/racing/domain/GameInfo.java

+    public void isRemainRound() {
+        if (roundCount <= 0) {
+            throw new ArithmeticException("남은 라운드가 없습니다.");
+        }
+    }
+
+    private void hasRacingCars() {
+        if (racingCarCount == 0) {
+            throw new ArithmeticException("경주에 참여하는 자동차가 없습니다.");
+        }
+    }


해당 값들에 대한 검증로직이 InputValue에서 한차례 검증을 거치고 온값으로 보이는데요!
같은 의미를 가지는 값들을 나누어 검증하고 있는것 같아 보입니다!
나누어 검증하는 만큼 테스트 코드의 양이 더 늘어날것 같아요!
제 생각에는 차라리 GameInfo쪽에 검증 로직을 전부 구현하고 InputValue를 지워도 될 것 같기도 하네요!

pci2676 · 2020-05-10T14:09:18Z

src/main/java/racing/RacingGameApplication.java

+public class RacingGameApplication {
+    public static void main(String[] args) {
+        InputView inputView = new InputView();
+        inputView.run();


입력을 받는 메서드의 이름이 .run()인 것은 조금 어색한 네이밍 같아 보이네요!
또한 InputView가 상태값을 가져야 하는가? 하는 점에 대해서도 조금 의문이 드는데요!
단순하게 static메서드로 입력값을 반환하는 메서드를 작성해도 좋을 것 같아 보입니다!

pci2676 · 2020-05-10T14:10:09Z

src/main/java/racing/domain/RacingCars.java

+
+public class RacingCars {
+
+    List<RacingCar> racingCars = new ArrayList<>();


접근제어자를 빠뜨리셨네요! ㅎㅎ

또한 일급 컬렉션은 final을 붙여 컬렉션 객체 참조값의 불변성을 확실히 해주는 것이 좋을 수 있습니다!

pci2676 · 2020-05-10T14:13:54Z

src/main/java/racing/domain/RacingCars.java

+    public RacingCars(int racingCarCount) {
+        createRacingCars(racingCarCount);
+    }
+
+    public void attendRound(NumberGenerator numberGenerator) {
+        for (RacingCar racingCar : racingCars) {
+            racingCar.attendRound(numberGenerator);
+        }
+    }
+
+    private void createRacingCars(int racingCarCount) {
+        for (int i = 0; i < racingCarCount; i++) {
+            racingCars.add(new RacingCar());
+        }
+    }


Suggested change

public RacingCars(int racingCarCount) {

createRacingCars(racingCarCount);

}

public void attendRound(NumberGenerator numberGenerator) {

for (RacingCar racingCar : racingCars) {

racingCar.attendRound(numberGenerator);

}

}

private void createRacingCars(int racingCarCount) {

for (int i = 0; i < racingCarCount; i++) {

racingCars.add(new RacingCar());

}

}

public RacingCars(int racingCarCount) {

createRacingCars(racingCarCount);

}

private void createRacingCars(int racingCarCount) {

for (int i = 0; i < racingCarCount; i++) {

racingCars.add(new RacingCar());

}

}

public void attendRound(NumberGenerator numberGenerator) {

for (RacingCar racingCar : racingCars) {

racingCar.attendRound(numberGenerator);

}

}

private 메서드는 사용되는 public 메서드와 그 위치를 가까이 두면 가독성에 있어 이점이 있습니다!

오라클 자바 컨벤션 문서에서 3.1.3 Class and Interface Declarations를 한번 읽어보시면 좋을 것 같아요!

pci2676 · 2020-05-10T14:16:26Z

src/main/java/racing/domain/GameInfo.java

+        this.roundCount = inputValue.getRoundCounts();
+    }
+
+    public void isRemainRound() {


is, has등과 같은 메서드 네이밍은 boolean값을 반환하는 네이밍으로 많이 쓰입니다!
검증 로직용으로 사용되는 네이밍은 check, validate와 같은 네이밍이 있습니다.
이외에도 더 있으니 한번 찾아보시는 것도 좋을 것 같아요!

pci2676 · 2020-05-10T14:38:16Z

src/test/java/racing/domain/GameInfoTest.java

+
+    @ParameterizedTest
+    @DisplayName("경기 정보을 가지고 있는지 확인")
+    @CsvSource(value = {"1,2", "2,3","3,4","5,2"})


해당 테스트를 여러번 해야하는 이유가 있을까요??🤔

pci2676 · 2020-05-10T14:40:18Z

src/test/java/racing/domain/MovementTest.java

+    @DisplayName("4보다 작은 값을 생성하면 HOLD 리턴")
+    @ParameterizedTest
+    @ValueSource(ints = {1, 2, 3})
+    public void randomValueTestHold(int number) {
+        Movement movement = Movement.nextMovement(new FixedNumberGenerator(number));
+
+        assertEquals(Movement.HOLD, movement);
+    }
+
+    @DisplayName("4보다 크고 같은 값을 생성하면 HOLD 리턴")
+    @ParameterizedTest
+    @ValueSource(ints = {4, 5, 6, 7, 8, 9})
+    public void randomValueTestMove(int number) {
+        Movement movement = Movement.nextMovement(new FixedNumberGenerator(number));
+
+        assertEquals(Movement.MOVE, movement);
+    }


두개의 테스트는 하나의 테스트로 통합할 수 있을것 같은데요!
@CsvSoruce를 잘 이용해 보시면 두가지 분기점에 대한 테스트를 하나의 테스트 메서드에서 검증 할 수 있습니다!

pci2676 · 2020-05-10T14:41:40Z

src/test/java/racing/domain/RacingCarTest.java

+    @DisplayName("랜덤값이 4이상일때 전진하는지 확인.")
+    @ValueSource(ints = {4, 5, 9})
+    @ParameterizedTest
+    void attendRoundMoveTest(int num) {
+        // Random 으로 값이 잡히는데 테스트를 어떻게 하면 좋을지?...
+        RacingCar racingCar = new RacingCar();
+        racingCar.attendRound(new FixedNumberGenerator(num));
+        assertEquals(2, racingCar.getRoundPosition(1));
+    }
+
+    @DisplayName("랜덤값이 4이하일때 움직이지 않는지 확인.")
+    @ValueSource(ints = {1, 2, 3})
+    @ParameterizedTest
+    public void attendRoundHoldTest(int num) {
+        // given
+        RacingCar racingCar = new RacingCar();
+
+        // when
+        racingCar.attendRound(new FixedNumberGenerator(num));
+        // then
+        assertEquals(1, racingCar.getRoundPosition(1));
+    }


MovementTest 와 같은 상황이네요! 하나의 테스트로 줄일 수 있을것 같아요!

pci2676 · 2020-05-10T14:42:29Z

src/test/java/racing/domain/RacingCarsTest.java

+
+    @ParameterizedTest
+    @DisplayName("입력된 레이싱카 숫자만큼 레이싱카가 생성되었는지 확인.")
+    @ValueSource(ints = {1, 2, 3, 4, 5, 6, 10})


이 테스트 또한 너무 많이 테스트를 진행하고 있는것 같습니다!
1과 10만 테스트해도 충분히 검증 되었다고 볼 수 있을것 같아요!

pci2676 · 2020-05-10T14:43:33Z

src/test/java/racing/domain/RacingCarsTest.java

+    @ParameterizedTest
+    @DisplayName("자동차들이 들어온 랜덤 값이 조건에 맞게 움직였는지 확인")
+    @ValueSource(ints = {4, 6})
+    public void attendRoundMoveTest(int num) {
+        RacingCars racingCars = new RacingCars(5);
+        racingCars.attendRound(new FixedNumberGenerator(num));
+        for (RacingCar racingCar : racingCars.getRacingCars()) {
+            assertEquals(2, racingCar.getRoundPosition(1));
+        }
+    }
+
+
+    @ParameterizedTest
+    @DisplayName("자동차들이 들어온 랜덤 값이 조건에 맞게 멈춰있는지 확인")
+    @ValueSource(ints = {1, 2, 3})
+    public void attendRoundHoldTest(int num) {
+        RacingCars racingCars = new RacingCars(5);
+        racingCars.attendRound(new FixedNumberGenerator(num));
+        for (RacingCar racingCar : racingCars.getRacingCars()) {
+            assertEquals(1, racingCar.getRoundPosition(1));
+        }
+    }


ㅎㅎ 이 테스트도 하나로 줄여보는게 좋을것 같아요!
그리고 테스트에서 for loop는 사용하지 않는게 좋습니다!
테스트 코드는 한번에 읽히는 것이 좋은데 반복문이 들어가면 코드를 해석해야하는 어려움이 생기게 됩니다!

nathanH32 added 2 commits April 30, 2020 21:28

first racing-game

5f9c4b7

InputValue 응집도 수정 & RacingGame 책임 분리 & 랜덤값 테스트 코드 추가

f492fd9

pci2676 requested changes May 10, 2020

View reviewed changes

nathanH32 added 6 commits May 15, 2020 19:59

feedback 반영

09afd33

Add FacotryPattern

7688fdf

객체 분리

5baf21f

2단계 요구사항 구현

1af0ddd

feedback 반영

6e9a684

2단계 요구사항 테스트 코드

e731a52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

첫번째 자동차 경주 #16

첫번째 자동차 경주 #16

NDjust commented Apr 30, 2020

pci2676 left a comment •

edited

Loading

pci2676 May 10, 2020

pci2676 May 10, 2020

pci2676 May 10, 2020

pci2676 May 10, 2020

pci2676 May 10, 2020

pci2676 May 10, 2020

pci2676 May 10, 2020

pci2676 May 10, 2020

pci2676 May 10, 2020

pci2676 May 10, 2020

pci2676 May 10, 2020


		public class RacingCars {

		List<RacingCar> racingCars = new ArrayList<>();

첫번째 자동차 경주 #16

Are you sure you want to change the base?

첫번째 자동차 경주 #16

Conversation

NDjust commented Apr 30, 2020

pci2676 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

Choose a reason for hiding this comment

pci2676 left a comment •

edited

Loading