[자동차 경주] 김정아 미션 제출합니다. #1461

squidjiny · 2024-10-28T14:50:42Z

java-racingcar-precourse

주요 기능

자동차 이름을 입력받는 기능:

사용자가 경주할 자동차들의 이름을 쉼표로 구분하여 입력받는다.
자동차 전진 판단 기능:

랜덤 수 생성기를 사용하여 4 이하인 경우 전진하고, 그렇지 않은 경우 0을 출력한다.
전진 상황 출력 기능:

각 회차별로 자동차들이 얼마나 전진했는지 출력한다.
최종 우승자 선정 기능:

전진 회차가 종료된 후 가장 멀리 전진한 자동차를 선정한다. 동점자가 있을 경우 공동 우승자로 인정한다.

예외처리 기능

이름 입력, 전진 횟수 입력에서 null이나 빈칸이 입력으로 들어왔을 경우 IllegalArgumentException을 발생시킨다.
이름 입력에서 자동차의 이름이 5자가 넘어갈 경우 IllegalArgumentException 을 발생시킨다.
이름 입력에서 동일한 이름이 입력 될 경우 IllegalArgumentException 을 발생시킨다.
전진 횟수 입력에서 숫자가 아닌 값이 들어올 경우 IllegalArgumentException 을 발생시킨다.
전진 횟수 입력에서 양의 정수가 아닌 값이 입력되면 IllegalArgumentException 을 발생시킨다.

… 이름 입력 클래스(InputView) 구현

seogwoojin

고생 많으셨습니다!
스트림 메소드 간결해서 보기 좋았던 것 같습니다
의존성 관련해서 코멘트 많이 달아서 확인해주시면 좋을 것 같아요 😊

seogwoojin · 2024-10-31T06:32:24Z

src/main/java/racingcar/common/IOMessage.java

+
+public enum IOMessage {
+
+    INPUT_CAR_NAMES("경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)"),


혹시 상수가 아닌 Enum 클래스를 사용한 이유가 따로 있을까요?
개인적인 생각으론 인자가 문자열 하나만 가지고 있다면, static 상수로 관리하는 게 더 보기 좋다고 생각합니다!

하나의 데이터를 저장하기에는 enum은 차지하는 크기가 static 상수에 비해 크기 때문에 static 상수를 활용해도 좋다 생각해요

InputView와 OutputView 모두 기본 메시지(입력 멘트, 출력 멘트)가 필요하기 때문에 enum으로 한 곳에서 관리할 수 있다면 좋겠다고 생각했는데, 생각해보니까 enum 말고 static으로 띄우면 되는거더라고요🤣🤣 Limit에는 잘 적용을 해놨는데 제출을 하고 아 여기도 쓰면 되는구나 깨달았습니다... 집어주셔서 제가 실수한게 맞다는걸 다시 한번 깨달아서 좋네요ㅠㅠ

그런데 상수 선언이라는 의미를 명확하게 드러내기 위해선 enum을 쓰는것도 좋은 것 같아요!!

seogwoojin · 2024-10-31T06:37:21Z

src/main/java/racingcar/common/Limit.java

+
+
+    private Limit() {
+    }


생성자 제한하는 건 좋은 것 같습니다 👍

추상 클래스로 정의하는 방법도 활용해 볼 수 있을 것 같아요. 생성자 제한을 안해도 될 것 같습니다 :)

추상 클래스는 서브클래스에서 추가적으로 구현하도록 하는 용도로 사용되는 것으로 알고 있어서 추상 클래스를 사용할 경우 자칫하면 추가적으로 구현할, 혹은 구현 될 항목이 있다 라는 의미로 비춰져서 리뷰어 혹은 동업자에게 혼란을 줄 수 있고, Limit가 제공하는 역할에 맞지 않다고 생각했습니다.

그래서 생성자 제한을 두어서 상수만을 제공하는 클래스(유틸리티 클래스와 비슷한 구조)로 만들었습니다.

seogwoojin · 2024-10-31T06:40:52Z

src/main/java/racingcar/common/Validator.java

+    }
+
+    private static boolean hasDuplicated(List<String> items) {
+        return items.size() != items.stream().distinct().count();


스트림 메소드로 이렇게도 중복을 체크할 수 있네요 배워갑니다!

제가 봤던 중복 확인 코드 중에 가장 깔끔하네요. 잘 배우고 갑니다!

seogwoojin · 2024-10-31T06:47:04Z

src/main/java/racingcar/domain/CarGroup.java

+                .toList();
+    }
+
+    public void printCarPositions() {


CarGroup 클래스에서 OutputView 기능을 맡고 있는 것 같은데
기능의 분리도 되지 않고, View 와 Model 사이의 의존성이 생겨서 MVC 패턴의 장점이 퇴색될 것 같습니다

코딩을 하던 도중, getter를 쓰면 캡슐화가 깨진다는 글을 보고 심취해서 이렇게 했었는데, 막상 하고나니 제 구조에는 맞지 않는 선택이였던 것 같습니다. 심지어 글을 잘못 이해한 것이였습니다.., 만약 다시 리펙토링한다면 CarGroup에서는 Position에 대한 계산만 하고, 출력부는 OutputView로 넘길 것 같습니다.

seogwoojin · 2024-10-31T06:49:15Z

src/main/java/racingcar/view/OutputView.java

+        System.out.println();
+    }
+
+    public void printWinners(List<Car> winners) {


여기서도 Car 객체를 인자로 받기보다 원시 자료형 String의 리스트를 받는 게 어떨까 생각합니다!

이렇게 하면 의존성도 줄고, 테스트 하기도 편하겠네요! 감사합니다!!

TaegeunYou

코드가 워낙 명확하고 깔끔해서 정말 편하게 읽을 수 있었습니다. 제가 추구하는 스타일과 매우 비슷해서 많이 배우고 갑니다!

TaegeunYou · 2024-10-31T11:13:06Z

src/main/java/racingcar/common/Validator.java

+            throw new IllegalArgumentException();
+        }
+        for (String name : carNames) {
+            if (name.length() < Limit.MIN_NAME_LENGTH || name.length() > Limit.MAX_NAME_LENGTH) {


이 부분도 hasDuplicated처럼 별도의 메서드로 분리할 수 있을 것 같습니다. 혹은, 검증과 예외 처리를 하나의 메서드로 묶어 checkCarNameLength 같은 이름으로 구현해도 좋을 것 같습니다. 그러면 validateCarNames 함수가 어떤 검증들을 하는지 조금 더 쉽게 파악할 수 있을 것 같네요!

원래는 다른 검증 메서드를 여러개 만든 후 한 메서드에서 모두 선언해서 예외 처리 메서드를 한번 호출하는것으로 모든 예외검증이 가능하도록 할 생각이였는데, 이상하게 구현한걸 이제 깨달았네요... 다음에는 의도한대로 신경써서 만들어 봐야겠네요! 감사합니다!

TaegeunYou · 2024-10-31T11:13:36Z

src/main/java/racingcar/common/Validator.java

+    }
+
+    private static boolean hasDuplicated(List<String> items) {
+        return items.size() != items.stream().distinct().count();


제가 봤던 중복 확인 코드 중에 가장 깔끔하네요. 잘 배우고 갑니다!

TaegeunYou · 2024-10-31T11:13:40Z

src/main/java/racingcar/controller/RacingGame.java

+        }
+
+        outputView.printWinners(carGroup.getWinners());
+    }


이 함수만 봐도 자동차 게임의 진행 흐름을 쉽게 파악할 수 있어, 정말 깔끔하고 좋은 코드인 것 같습니다!

TaegeunYou · 2024-10-31T11:18:15Z

src/main/java/racingcar/common/Validator.java

+
+    }
+
+    public static void validateCarNames(List<String> carNames) {


자동차 이름을 검증하는 역할은 Car나 CarGroup에게 넘겨서 객체를 생성할때 확인해줘도 좋은 방법일 것 같습니다. 그러면 Car와 관련된 모든 로직이 한곳에 응집됨으로써 도메인의 응집성을 높일 수 있습니다. 그래서 저는 Validator 클래스를 따로 만들지 않고 검증에 대한 메소드는 각 도메인에 구현하는 편입니다!

예외처리를 따로 구현해서 책임 분리를 확실하게 하고, 한 곳에소 좀 더 편리하게 유지보수 하자! 가 목표였는데, 이런 작은 프로젝트에서 Validator를 분리하는 것은 확실히 과투자라는 생각이 드네요... 좋은 의견이에요!

혹시 클래스 내부에 선언할때 클래스 안에 static으로 Validator를 선언하여 예외 로직 부분을 분리하는것은 어떻게 생각하세요?? 이 방법과 따로 파일을 분리하는것을 고민하다가 이렇게 했습니다.

static으로 선언하려는 이유는 뭔가요?

TaegeunYou · 2024-10-31T11:20:46Z

src/main/java/racingcar/domain/CarGroup.java

+
+    public void printCarPositions() {
+        for (Car car : cars) {
+            String positionMarker = Symbol.POSITION_MARKER.getSymbol().repeat(car.getPosition());


repeat 함수를 사용해 반복되는 문자를 깔끔하게 구현하셨네요! Java API 활용을 정말 잘하시는 것 같아 많이 배워갑니다.

koosco

2주차 고생 많으셨습니다! 리뷰가 너무 늦었네요. 변수명 선정을 직관적으로 너무 잘 하시는 것 같습니다.
값의 검증은 어디에서 이뤄지는게 좋다고 생각하는지 궁금해요. 정답은 없는 것 같고 다들 생각이 다르시니.. 타당한 이유가 있다면 어떤 방법이든 좋다고 생각합니다. 저는 도메인에서 관리하는 편인데 어떻게 생각하시는지 궁금해요. 아 Limit이라는 클래스명은 조금 변경해도 좋을 것 같다는 생각을 했습니다.
3주차도 화이팅하세요 :)

koosco · 2024-11-01T14:52:32Z

src/main/java/racingcar/common/IOMessage.java

+
+public enum IOMessage {
+
+    INPUT_CAR_NAMES("경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)"),


하나의 데이터를 저장하기에는 enum은 차지하는 크기가 static 상수에 비해 크기 때문에 static 상수를 활용해도 좋다 생각해요

koosco · 2024-11-01T14:53:13Z

src/main/java/racingcar/common/IOMessage.java

+    INPUT_ROUND_COUNT("시도할 횟수는 몇 회인가요?"),
+    RESULT_NOTICE("실행 결과"),
+    WINNER_ANNOUNCEMENT("최종 우승자 : "),
+    CAR_POSITION_FORMAT("%s : %s");


형식 지정자를 사용한 것 좋은 것 같아요. 변수 이름이 명확히 어떤 데이터를 저장하고 있는지 잘 나타내고 있는거 같아요.

다른 분도 비슷한 코멘트를 달아주셔서 생각을 해 봤는데, 차지하는 크기(효율)를 생각하면 static으로 유틸리티 클래스를 만들기가 베스트인것 같고, 상수라는 느낌을 강하게(?) 명확하게(?) 주는것은 enum이 베스트인것 같습니다!

이 경우에서는 규모가 작아서 유틸리티 클래스를 쓰는게 나은 것 같아요! 좋은 의견이에요😊
유틸리티 클래스와 enum을 본의아니게 섞어써버려서 리펙토링을 한다면 1순위 대상일거같네요...

koosco · 2024-11-01T14:55:17Z

src/main/java/racingcar/common/Limit.java

+
+
+    private Limit() {
+    }


추상 클래스로 정의하는 방법도 활용해 볼 수 있을 것 같아요. 생성자 제한을 안해도 될 것 같습니다 :)

koosco · 2024-11-01T14:55:38Z

src/main/java/racingcar/common/Symbol.java

@@ -0,0 +1,17 @@
+package racingcar.common;
+
+public enum Symbol {


이 부분도 static 상수를 고려해보셔도 좋을 것 같아요

위에서도 비슷한 의견이 나왔어요!

추상 클래스는 서브클래스에서 추가적으로 구현하도록 하는 용도로 사용되는 것으로 알고 있어서 추상 클래스를 사용할 경우 자칫하면 추가적으로 구현할, 혹은 구현 될 항목이 있다 라는 의미로 비춰져서 리뷰어 혹은 동업자에게 혼란을 줄 수 있고, Limit가 제공하는 역할에 맞지 않다고 생각했습니다.

그래서 생성자 제한을 두어서 상수만을 제공하는 클래스(유틸리티 클래스 비슷한 구조)로 만들었습니다.

koosco · 2024-11-01T14:56:00Z

src/main/java/racingcar/common/Symbol.java

+package racingcar.common;
+
+public enum Symbol {
+    SEPARATE_MARKER(","),


변수들의 의미를 갖는 이름이 좋은 것 같습니다

koosco · 2024-11-01T14:57:17Z

src/main/java/racingcar/common/Validator.java

+    }
+
+    public static void validateCarNames(List<String> carNames) {
+        if (carNames == null || carNames.isEmpty()) {


isEmpty 대신 isBlank를 사용하면 공백 처리까지 할 수 있을 것 같습니다!

원래 isBlank를 사용하는데 실수했네요... 감사합니다!!

koosco · 2024-11-01T14:58:27Z

src/main/java/racingcar/controller/RacingGame.java

+    public void run(){
+        CarGroup carGroup = new CarGroup(inputView.readCars());
+        int roundCount = inputView.readRoundCount();
+        for(int i = 0; i<roundCount; i++){


cargroup에게 책임을 할당하고 결과를 한 번에 출력하는 방법은 어떨까요?

분리 해야지 해야지 해놓고 시간이 모자라서 분리를 못 했네요ㅠㅠ
감사합니다!

koosco · 2024-11-01T14:59:30Z

src/main/java/racingcar/domain/Car.java

+
+public class Car {
+    private final String name;
+    private int position = 0;


원시 타입도 도메인에 맞게 클래스로 묶어서 사용하면 좋다고 하더라구요.
찾아보셔도 좋을 것 같아요 !

분리를 할지 말지 고민을 했었는데, 규모가 작아서 분리를 하면 오히려 과투자 일 것 같아서 분리를 하지 않았습니다.
하지만 테스트 방면이나 확장성에서는 좋은 선택이라고 생각해요! 다음 미션에서 한번 사용해보겠습니다😊

koosco · 2024-11-01T15:00:07Z

src/main/java/racingcar/domain/Car.java

+    }
+
+    private boolean isMove() {
+        int number = Randoms.pickNumberInRange(Limit.MIN_RANDOM_VALUE, Limit.MAX_RANDOM_VALUE);


랜덤한 숫자 생성을 private 메서드로 넣으면 테스트하기 힘들 수도 있지 않을까요?

아... 테스트를 많이 만들어보지 않아서 이걸 고려를 못했네요... 감사합니다!!

저는 이런 경우 private 메소드가 관여되어 있는 move() 메소드를 통해 간접적으로 테스트를 하곤 합니다
무조건적으로 이렇게 해야된다는 아닌 것 같습니다!

koosco · 2024-11-01T15:01:18Z

src/main/java/racingcar/domain/CarGroup.java

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


일급 컬렉션처럼 사용하신 것 같은데 맞을까요?
일급 컬렉션을 의도하신거면 불변성에 대해 어떻게 생각하시는지 궁금합니다.

일급 컬렉션 비슷한것을 의도한것은 맞습니다! 하지만 주된 이유는 분리를 하면 테스트에 용이하다는 것이였는데, 그와 관련된 테스트 작성을 못했습니다... 만약 완벽하게 일급컬렉션으로 선언하려면 반환하려는 리스트를 방어적 복사를 한 후 결과값을 반환하는 로직이 추가로 필요합니다.

final로 선언을 한 것으로는 문제가 없을 줄 알았는데 Collections.unmodifiableList() 라는것으로 리스트를 또 불변 처리를 해야하는것은 공부하면서 처음 알았네요...!

저도 다른 분 코드 보면서 알게된거라! 다음에는 활용해보려구요!

changbill · 2024-11-04T13:59:04Z

src/main/java/racingcar/common/Validator.java

+
+    public static void validateRoundCount(int roundCount) {
+        if (roundCount < 1) {
+            throw new IllegalArgumentException();


Exception에 메세지를 담으면 이게 무슨 에러인지 리뷰어 입장에서 와닿을 것 같아요
물론 디버그 시에도 어떤 문제인지 명시되어 있으면 알아차리기 쉬운 장점도 있습니다

changbill · 2024-11-04T14:06:26Z

src/test/java/racingcar/ApplicationTest.java

전체적으로 단위테스트하기 좋은 메소드들이 많은데 기능 완성하실때마다 테스트도 추가해주시면 좋을 것 같아요

changbill

이미 좋은 피드백들이 있어서 없는 내용만 추가했습니다
전체적으로 코드량이 적고 깔끔해서 보기 좋았던 것 같습니다
수고많으셨습니다!!

squidjiny added 18 commits October 28, 2024 02:41

Docs: 초기 기능 명세와 예외처리 기능 작성

65d4451

Chore: gitignore에 build, gradle파일 추가

ec38a02

Test: 초기 테스트케이스 설정

b06fd84

Docs: 추가된 예외처리 기능에 대한 명세 추가

6a6a76f

Feat: enum class(IOMessage, Symbol)를 통한 전역적인 메시지, 문자열 구분 기호 관리 구현, 차량…

d5cb4c2

… 이름 입력 클래스(InputView) 구현

Feat: Car class 추가

d0cbccf

Feat: 상수 값을 선언해놓은 클래스(Limit) 생성

f51f72d

Feat: Car 클래스에서 Position 계산을 위한 랜덤수 생성 함수 선언

2fe989a

Refactor: 요구 사항에 맞게 Random 생성 API 변경

32b49b9

Feat: Car 객체 생성의 용이함을 위해 CarGroup 클래스 생성

48b2d8e

Feat: RacingGame에서 Car의 이름을 입력받아서 Car 객체를 생성

500a325

Feat: 경기 실행 횟수 입력기능 생성

8cf39a5

Feat: 각 Car들의 라운드가 끝난 후의 위치를 출력하는 기능 개발

dd98b05

Feat: 매 라운드마다 car의 포지션을 계산하고, 출력하는 기능 개발

82611c7

Feat,Fix: 승리자 출력 기능 생성, 요구 포맷에 맞게 자동차 위치 출력 변경

e8d8a9e

Feat,Fix: 승리자 출력 기능 생성, 요구 포맷에 맞게 자동차 위치 출력 변경

66e76bf

Fix, Refactor: enum값이 제대로 출력되지 않는 버그 수정

4deefb8

Feat: 유효성검사 추가

f3d23f4

squidjiny changed the title ~~[자동차 경주] 김정 미션 제출합니다.~~ [자동차 경주] 김정아 미션 제출합니다. Oct 28, 2024

seogwoojin reviewed Oct 31, 2024

View reviewed changes

TaegeunYou reviewed Oct 31, 2024

View reviewed changes

koosco reviewed Nov 1, 2024

View reviewed changes

changbill reviewed Nov 4, 2024

View reviewed changes


		public enum IOMessage {

		INPUT_CAR_NAMES("경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)"),


		}

		public static void validateCarNames(List<String> carNames) {

		@@ -0,0 +1,17 @@
		package racingcar.common;

		public enum Symbol {

[자동차 경주] 김정아 미션 제출합니다. #1461

Are you sure you want to change the base?

[자동차 경주] 김정아 미션 제출합니다. #1461

Conversation

squidjiny commented Oct 28, 2024

java-racingcar-precourse

주요 기능

예외처리 기능

seogwoojin left a 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

squidjiny Nov 2, 2024 • 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

squidjiny Nov 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TaegeunYou left a 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

koosco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squidjiny Nov 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squidjiny Nov 2, 2024 • 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

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

changbill left a comment

Choose a reason for hiding this comment

squidjiny Nov 2, 2024 •

edited

Loading

squidjiny Nov 2, 2024 •

edited

Loading

squidjiny Nov 2, 2024 •

edited

Loading

squidjiny Nov 2, 2024 •

edited

Loading