자동차 경주 게임 제출합니다! by jjanggyu · Pull Request #6 · biforest/java-racingcar

jjanggyu · 2021-03-11T19:04:18Z

No description provided.

chae-heechan

미션 진행하시느라 수고 많으셨습니다!
모든 코드를 한번에 commit하셔서 진행 순서를 볼 수 없어서 아쉽네요😥
CarFactory 부분과 다른 부분들을 보고 리뷰하며 공부가 많이 됐습니다! 고생 많으셨어요!!

chae-heechan · 2021-03-13T08:17:09Z

src/main/java/domain/Car.java

+    private void validateName() {
+        if (name.length() > 5) {
+            throw new IllegalArgumentException(name + ": 자동차 이름은 5글자 이하여야 합니다.");
+        }
+
+        if (name.isBlank()) {
+            throw new NotBlankException("공백으로만 이루어진 이름을 생성할 수 없습니다.");
+        }


입력에 대한 예외를 더욱 세분화하면 사용자 입장에서 더 편리할것 같아요✔

입력이 없을 경우

쉼표가 연속으로 있을 경우

영어 이외의 문자가 입력될 경우

쉼표로 시작할 경우

쉼표로 끝날 경우

이름의 길이가 6자 이상일 경우

같은 이름이 입력될 경우

저는 이렇게 이름 입력에 대한 예외를 7가지로 정했습니다!

chae-heechan · 2021-03-13T08:47:52Z

src/main/java/ui/Receiver.java

+    public Receiver() {
+        receiveCarNames();
+        receiveRound();
+    }


이름을 입력받고 라운드를 입력받기 전에 이름 입력에 대한 예외 처리를 먼저 해주면 더 좋을 것 같아요.

이름 입력에 오류가 있으면 라운드 입력을 받지 않고 진행할 수 있어서 더 효율적인 프로그램이 될 것 같아요!

chae-heechan · 2021-03-13T08:56:48Z

src/main/java/domain/Car.java

+    private void validateName() {
+        if (name.length() > 5) {
+            throw new IllegalArgumentException(name + ": 자동차 이름은 5글자 이하여야 합니다.");
+        }
+
+        if (name.isBlank()) {
+            throw new NotBlankException("공백으로만 이루어진 이름을 생성할 수 없습니다.");
+        }


이름 입력에 대한 예외를 throw만 해주고 받아주는 곳이 없네요
그러다 보니까 에러 메세지만 띄우고 프로그램이 끝나버려 프로그램이 불친절한 것 같아요.

valiateName 메서드를 Car 객체를 생성할 때 호출하는 것 보다 Receive 클래스에서 실행시켜주면 예외 처리하고 반복적으로 입력을 받기 편할 것 같아요!

chae-heechan · 2021-03-13T09:12:12Z

src/main/java/domain/CarFactory.java

+    }
+
+    private String[] splitCarNames(String input) {
+        String s = input.replaceAll(BLANK, "");


"" 이 부분도 상수로 선언해 주는 게 통일감 있어 보일 것 같아요!

chae-heechan · 2021-03-13T09:13:49Z

src/main/java/utils/RandomUtils.java

+    public static int nextInt(final int startInclusive, final int endInclusive) {
+        if (startInclusive > endInclusive) {
+            throw new IllegalArgumentException();
+        }
+
+        if (startInclusive < 0) {
+            throw new IllegalArgumentException();
+        }
+
+        if (startInclusive == endInclusive) {
+            return startInclusive;
+        }
+
+        return startInclusive + RANDOM.nextInt(endInclusive - startInclusive + 1);
+    }


이 메서드에서도 예외를 throw만 해주고 받아주는 부분이 없네요!

chae-heechan · 2021-03-13T09:18:04Z

src/main/java/domain/Car.java

+    public int compareTo(Car o) {
+        return o.position - position;
+    }


사용되지 않는 메서드같아요!

이런 부분을 줄이려면 README에서 필요한 클래스와 메서드를 정리해가며 진행하는 것이 좋은것 같아요😃

chae-heechan · 2021-03-13T09:36:50Z

src/test/java/domain/RacingCarGameTest.java

+        racingCarGame.printWinners();
+    }
+}


Receiver 부분을 테스트하지 못하신 것 같아요.
저희도 입력 부분을 어떻게 처리해야 하나 고생했습니다😂

찾아보니 ByteArrayInputStream()를 사용해서 Test에서 입력을 넣어줄 수 있습니다!
저 부분을 참고해서 Receiver 부분도 테스트 코드를 작성해 보시기 바랍니다!

chae-heechan · 2021-03-13T09:42:33Z

src/main/java/domain/CarFactory.java

+public class CarFactory {
+    public static final String COMMA = ",";
+    public static final String BLANK = " ";
+    private final List<Car> cars;
+    private final String[] carNames;
+
+    public CarFactory(String input) {
+        this.carNames = splitCarNames(input);
+        cars = createCars();
+    }


CarFactory 클래스를 따로 만드신게 너무 좋은 것 같아요!👍
Car들를 따로 관리해 줄 클래스가 필요하다고 생각했는데 이런 식으로 하면 되겠군요!

begaonnuri

깔끔하게 잘 구현해주셨네요 💯
몇가지 변경사항 남겼으니 반영해주시고, 궁금한 사항 편하게 질문 주세요 :)

begaonnuri · 2021-03-14T12:59:58Z

src/main/java/ui/Receiver.java

+public class Receiver {
+    public static final String REQUEST_CAR_NAME = "경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)";
+    public static final String REQUEST_ROUND = "시도할 횟수는 몇회인가요?";
+    private static final Scanner scanner = new Scanner(System.in);


상수 컨벤션을 지켜주세요!

begaonnuri · 2021-03-14T13:00:24Z

src/main/java/ui/Receiver.java

+    public static final String REQUEST_CAR_NAME = "경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)";
+    public static final String REQUEST_ROUND = "시도할 횟수는 몇회인가요?";


해당 클래스에서만 사용하는데 public하게 관리한 이유가 있을까요?

begaonnuri · 2021-03-14T13:02:34Z

src/main/java/ui/Receiver.java

+    private void receiveCarNames() {
+        System.out.println(REQUEST_CAR_NAME);
+        carNames = scanner.nextLine();
+    }
+
+    private void receiveRound() {
+        System.out.println(REQUEST_ROUND);
+        round = scanner.nextInt();
+    }


여기서 next()를 통해 받은 값을 return한다면 getter와 멤버 변수가 사라질 것 같아요!

begaonnuri · 2021-03-14T13:02:45Z

src/main/java/utils/RandomUtils.java

+import java.util.Random;
+
+public class RandomUtils {
+    private static final Random RANDOM = new Random();


static 관리 👍🏻

begaonnuri · 2021-03-14T13:03:32Z

src/main/java/utils/RandomUtils.java

+    private RandomUtils() {
+    }


유틸 클래스의 private 생성자 활용 👍🏻

begaonnuri · 2021-03-14T13:09:57Z

src/main/java/domain/CarFactory.java

+    public static final String COMMA = ",";
+    public static final String BLANK = " ";
+    private final List<Car> cars;
+    private final String[] carNames;


배열 보다는 컬렉션을 사용하는 것은 어떨까요?
그리고 이 경우엔 굳이 인스턴스 변수로 관리할 필요가 없을 것 같네요!

begaonnuri · 2021-03-14T13:10:52Z

src/main/java/domain/CarFactory.java

+    }
+
+    private String[] splitCarNames(String input) {
+        String s = input.replaceAll(BLANK, "");


이 기능은 String의 trim()이란 메소드로 해결할 수 있습니다!
api를 찾아보는 습관을 가져보세요 :)

begaonnuri · 2021-03-14T13:14:01Z

src/main/java/domain/RacingCarGame.java

+
+import utils.RandomUtils;
+
+public class RacingCarGame {


RacingCarGame에선 어떤일을 할까요?

랜덤 숫자를 만들고(makeRandomValue)

자동차를 전진시키고(movePosition)

게임을 진행시키고(proceed)

상태를 출력하고(print)
다양한 기능을 하고있네요! 하지만 일관된 기능들은 아닌 것 같아요.
SRP(Single Responsibility Principle)을 지키도록 변경해보면 어떨까요?

begaonnuri · 2021-03-14T13:14:46Z

src/main/java/domain/Car.java

+        }
+
+        if (name.isBlank()) {
+            throw new NotBlankException("공백으로만 이루어진 이름을 생성할 수 없습니다.");


이 부분에 대해서만 커스텀 익셉션을 정의한 이유가 있을까요?
일관된 예외처리가 아니라면 무슨 차이인지 혼동을 줄 수 있을 것 같아요.

begaonnuri · 2021-03-14T13:17:22Z

src/main/java/domain/RacingCarGame.java

+        System.out.print(cars.get(0).getName());
+
+        for (int i = 1; i < cars.size(); i++) {
+            if (cars.get(i).getPosition() < max) {


객체의 값을 꺼내는 것이 아닌 메시지를 던지는 방식으로 변경해보면 어떨까요?

feat: 자동차 경주 게임 제출

3887b57

chae-heechan reviewed Mar 13, 2021

View reviewed changes

begaonnuri self-requested a review March 14, 2021 04:30

begaonnuri requested changes Mar 14, 2021

View reviewed changes

refactor: 리뷰 일부 반영

a6810a2

		public static final String REQUEST_CAR_NAME = "경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)";
		public static final String REQUEST_ROUND = "시도할 횟수는 몇회인가요?";

Conversation

jjanggyu commented Mar 11, 2021

Uh oh!

chae-heechan 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!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants