[자동차 경주 게임] 박진훈 리뷰 부탁드립니다! by jinhoon227 · Pull Request #522 · woowacourse/java-racingcar-precourse

jinhoon227 · 2022-12-06T22:16:32Z

No description provided.

기존에는 printStackTrace 로 출력하였으나, 메시지 출력방식을 원해서 변경 TryCommand 클래스에서 String->int 형변환시 예외 catch 를 NumberFormatException 으로 수정

Cars 객체 생성시 불변값으로 설정

상수화가 안된 메시지가 있어서 수정

khj1

안녕하세요! 좋은 코드 잘 보고 갑니다!
특히 커스텀 예외를 사용하신 것이 인상적이었어요! :)
항상 화이팅입니다 진훈님!!!

khj1 · 2022-12-07T01:47:54Z

+    private static final int MOVABLE_MIN_NUMBER = 4;
+    private final String name;


컨벤션에 따라서 상수와 인스턴스 변수를 새 줄로 구분해주시는 게 좋을 것 같습니다!!

감사합니다! 놓치고있던 부분이네요..

khj1 · 2022-12-07T01:58:01Z

+    private Cars(List<Car> cars) {
+        this.cars = Collections.unmodifiableList(cars);
+    }


불변 객체를 만들 때 생성자 단계에서는 방어적 복사를 고려해주시면 더 좋을 것 같아요!!

방어적 복사와 Unmodifiable Collections

오... 좋은글 감사합니다. 방어적복사는 처음 접해보는 개념이네요!

khj1 · 2022-12-07T02:03:27Z

+public class Winner {
+
+    private final List<String> winner;


리스트의 네이밍은 복수형이 더 적절하다고 생각합니다!
Winner 클래스 자체도 승리자들의 이름을 담고있는 일급 컬렉션이기 때문에 Winners 라는 네이밍을 더 추천드립니다!

khj1 · 2022-12-07T02:11:35Z

+    private String convertMoveMark(int position) {
+        return Stream.generate(() -> MOVE_MARK).limit(position).collect(Collectors.joining());
+    }


String에서 제공하는 repeat() 메서드를 활용해보시는 것도 추천드립니다!

이런,, 스트림을 너무 남발했네요.. repeat() 라는 좋은 함수가 있는데 말이죠 ㅠ

khj1 · 2022-12-07T02:17:44Z

+
+    public void move(CarMoveNumberGenerator carMoveNumberGenerator) {
+        for (Car car : cars) {
+            car.move(carMoveNumberGenerator);


CarMoveNumberGenerator 를 Car 객체까지 넘겨주지 않아도 될 것 같습니다!
그러면 car.move() 메서드를 테스트하기도 훨씬 간편해질 것 같아요!

khj1 · 2022-12-07T02:23:04Z

@@ -0,0 +1,22 @@
+package racingcar.domain;
+
+public class RacingCarGame {


RacingCarGame 클래스의 메서드들이 대부분 Cars 클래스의 메서드를 호출하는 용도로만 사용되는 것 같아요!
RacingCarGame 클래스가 정말 필요한지, 아니면 어떤 식으로 RacingCarGame 만의 새로운 책임을 부여할 수 있을 지
생각해보시면 좋을 것 같아요!!

원래 RacingCarGame 에서 시도횟수를 가지고있어서 Cars 를 시도횟수만큼 움직이는 로직이있었는데 controller 쪽으로 해당 로직을 옮기는바람에 쓸모없는 클래스가 되었네요.. 좋은 지적 감사합니다..!

khj1 · 2022-12-07T02:26:39Z

+
+    private static final int MIN_TRY = 1;
+    private static final int MAX_TRY = 100000;
+    private int tryCount;


값 객체의 내부 상태를 final 로 선언하는 방법도 있더라구요! 참고해보시면 좋을 것 같습니다!

Value Object, Reference Obejct

불변으로 해도 값을 변경하는 방법이 있었군요..
DDD 에서 나오는 개념이라 MVC 에는 어떻게 적용할지는 고민을 해봐야겠네요..
좋은글 감사합니다!

khj1 · 2022-12-07T02:28:03Z

+        for (String carName : words) {
+            cars.add(new Car(carName));
+        }
+        return new Cars(cars);


스트림으로 로직을 바꾸시면 빈 리스트를 선언하지 않고 더 깔끔하게 구현할 수 있을 것 같아요!

khj1 · 2022-12-07T02:37:57Z

@@ -0,0 +1,10 @@
+package racingcar.exception;
+
+public class CarNameLengthException extends IllegalArgumentException{


특별히 커스텀 예외를 사용하신 이유가 있을까요?? 궁금합니다!

커스텀예외

커스텀 예외들은 장단점이 있습니다! 그래서 정답은 없지만 제가 사용한 이유는
첫째로클래스 이름으로 어떤 예외인지 바로 알 수 있고
둘째로 보통 도메인 코드를 볼때 중요한것은 그 도메인 역할과 관련된 로직이지, 예외처리를 어떻게 하는지가 중요한게 아니기때문에 예외를 따로빼내어 좀 더 보기 편한게 만들기 위함입니다!
셋째로 대부분 Spring MVC 프로젝트에서는 커스텀 예외를 사용하기에 연습차원에서 커스텀 예외를 사용하고 있습니다.

BuildTools added 16 commits December 7, 2022 02:26

docs: 추가 기능목록

0556a70

feat: 추가 skeleton

8f1220e

fix: 변경 자바버전 설정

7935b9f

feat: 추가 자동차에 이름을 입력받는 기능

db4cb02

test: 추가 자동차에 이름을 입력받는 기능테스트

ca3eda7

feat: 추가 주어진 횟수를 입력받는 기능

c117b2b

test: 추가 주어진 횟수를 입력받는 기능테스트

134df50

feat: 추가 자동차는 전진 또는 멈춤 기능

3fc2bae

test: 추가 자동차는 전진 또는 멈춤 기능 테스트

9f8abc4

feat: 추가 주어진 횟수 동안 n대의 자동차는 전진 또는 멈춤 기능

76ad6d5

test: 추가 주어진 횟수 동안 n대의 자동차는 전진 또는 멈춤 테스트기능

c3f9689

feat: 추가 자동차 경주 게임을 완료한 후 누가 우승했는지 출력하는 기능

d001728

test: 추가 자동차 경주 게임을 완료한 후 누가 우승했는지 출력하는 기능테스트

6cc73d5

fix: 변경 예외처리 메세지 출력방식

5a0b432

기존에는 printStackTrace 로 출력하였으나, 메시지 출력방식을 원해서 변경 TryCommand 클래스에서 String->int 형변환시 예외 catch 를 NumberFormatException 으로 수정

refactor: 수정 Cars 객체생성

e5e4263

Cars 객체 생성시 불변값으로 설정

refactor: 수정 outputView

1f07840

상수화가 안된 메시지가 있어서 수정

khj1 reviewed Dec 7, 2022

View reviewed changes

		private static final int MOVABLE_MIN_NUMBER = 4;
		private final String name;

		@@ -0,0 +1,22 @@
		package racingcar.domain;

		public class RacingCarGame {

		@@ -0,0 +1,10 @@
		package racingcar.exception;

		public class CarNameLengthException extends IllegalArgumentException{

Conversation

jinhoon227 commented Dec 6, 2022

Uh oh!

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

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!

2 participants