[자동차 경주] 김선호 미션 제출합니다. #486

haero77 · 2023-10-31T10:56:11Z

No description provided.

skylar1220

전체적으로 객체 분리와 생성자 주입이 잘 되고 있다는 느낌입니다!
객체별 책임에 대해서 고민이 드는 점 위주로 리뷰를 남겨보았습니다! 수고 많으셨어요😉

skylar1220 · 2023-11-01T16:56:23Z

src/main/java/racingcar/domain/numberpicker/SystemRandomNumberPicker.java

+
+public class SystemRandomNumberPicker implements RandomNumberPicker{
+
+    private static final int MIN_RANDOM_DIGIT = 0;


클린코드 책에 나와있는 내용으로 알려져있는데
함수나 변수 이름을 축약하지 않는 것을 권장하는 측면에서 MIN은 통용되는 축약어인가 궁금합니다!

저도 축약어를 최대한 지양하는 편이긴 합니다. MIN, MAX 같은 경우 Integer.MAX_VALUE 처럼 Java API 에서 많이 활용되고 있고, Java API 에서 사용하는 네이밍 정도면 다른 사람들이 봤을 때도 부담없이 읽을 수 있지 않을까 생각해서 위처럼 사용했습니다!

또, 이건 저만의 방법인데 네이밍 짓기가 애매한 경우 Spring 이나 Java 에 선언된 네이밍을 최대한 따르려고 하는 편입니다!

skylar1220 · 2023-11-01T17:00:07Z

src/main/java/racingcar/io/input/InputManager.java

+
+    private List<String> parseByDelimiter(String input) {
+        return Arrays.stream(input.split(CAR_NAME_DELIMITER))
+                .collect(Collectors.toList());


Suggested change

.collect(Collectors.toList());

.toList();

를 쓰는 방법도 있습니다! 내부에서 unmodifiableList도 함께해주더라고요!

default List<T> toList() { return (List<T>) Collections.unmodifiableList(new ArrayList<>(Arrays.asList(this.toArray()))); }

오 꿀팁이네요! 배워갑니다

skylar1220 · 2023-11-01T17:04:56Z

src/main/java/racingcar/io/input/InputManager.java

+                    "Some elements of %s are blank character.".formatted(Arrays.toString(literals.toArray()))
+            );
+        }
+    }


parseByDelimiter, validateBlankCharacterNotIncluded와 같은 convert, validate 메소드가 Manager 객체에 있어도 될까?하는 생각이 드는 것 같아요!
변환, 검증 객체에서 가져와서 여기서는 호출만 하는게 Manager의 역할에 맞지 않을까 싶어서요!
지금은 변환, 검증 책임을 다 갖고 있는 객체인가?라는 생각이 듭니다!

지금은 변환, 검증 책임을 다 갖고 있는 객체인가?라는 생각이 듭니다!

맞습니다. 변환, 검증 책임을 부여했습니다!
말 그대로 Input 을 Managing 하기 때문에, 입력에 대한 convert, validate 에 대한 책임을 부여해서, Manager 라는 역할을 설계했기 때문에 이렇게 보면 위 메서드들을 갖는게 자연스럽지 않을까요? 변환, 검증하는 책임이 훨씬 많아진다면 객체로 분리하면 좋겠지만 지금 같이 간단한 경우 굳이 객체로 분리해야할까 싶기도 합니다. 단일 책임 원칙을 위반하는 것 아닐까 싶지만, 이 객체의 책임 자체를 input 을 관리한다. 라고 보면 OCP를 지키는 것 같기도 하고요. 확실히 어떻게 책임을 분배할지에 대한 고민은 항상 어려운 것 같군요 😂

skylar1220 · 2023-11-01T17:08:22Z

src/main/java/racingcar/domain/car/CarName.java

+    public static List<CarName> from(List<String> names) {
+        return names.stream()
+                .map(CarName::from)
+                .toList();
+    }
+


String 리스트를 받아서 CarName 객체로 만들어 리스트로 담아주는 것을 CarName의 부생성자 from이 해야하는 일이 맞을까 하는 생각이 듭니다!
목적이 String 리스트를 받아서 CarName 객체로 반환이면 받는 쪽인 inputManager 쪽에서 처리를 해서 컨트롤러로 바로 넘겨도 될 것 같고요!

CarName 객체 생성을 CarName 이 하는 것은 굉장히 응집도가 높은 설계라고 생각합니다. 팩토리 메서드가 해당 클래스의 인스턴스를 만들어서 리턴하듯이, 여러 CarName 인스턴스를 만드는 팩터리 메서드 역시 해당 클래스에 위치해야한다고 생각해요. (별도의 CarGenrator 등 팩토리 클래스가 있지 않을 때에 한해서요.).

오히려 input 로직에서 도메인 객체까지 생성하는 책임을 가지면 너무 많은 책임을 지는 것은 아닐 지 고민되네요. input 는 적절히 null 처리, 파싱 등 컨트롤러에 필요한 문자열 정보만 넘겨주고, 컨트롤러에서 본격적으로 도메인 객체 등을 만들어 비즈니스 로직을 전개하는 방식의 책임 분배를 하는 방식이 적절히 책임이 분리된게 아닐까하고 생각합니다. 리뷰어님은 모델 객체 생성의 책임까지 전부 InputView 가 책임을 지는게 좋다고 생각하시는 건지 궁금합니다 :)

skylar1220 · 2023-11-01T17:14:45Z

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

+    public void moveForward(RandomNumberPicker randomNumberPicker) {
+        if (randomNumberPicker.pickOneDigit() >= MIN_REQUIRED_DIGIT_TO_MOVE_FORWARD) {
+            this.forwardCount.increaseByOne();
+        }
+    }


if 절 안의 가독성을 위해 메소드 분리를 해도 좋을 것 같습니다!

Suggested change

public void moveForward(RandomNumberPicker randomNumberPicker) {

if (randomNumberPicker.pickOneDigit() >= MIN_REQUIRED_DIGIT_TO_MOVE_FORWARD) {

this.forwardCount.increaseByOne();

}

}

public void moveForward(RandomNumberPicker randomNumberPicker) {

if (isOverMoveNumber(randomNumberPicker)) {

this.forwardCount.increaseByOne();

}

}

private static boolean isOverMoveNumber(RandomNumberPicker randomNumberPicker) {

return randomNumberPicker.pickOneDigit() >= MIN_REQUIRED_DIGIT_TO_MOVE_FORWARD;

}

이렇게 하면 확실히 이름을 통해 의도를 드러낼 수 있겠군요!
저도 이런 리뷰를 많이 남기는 편인데 저 조차도 깜빡할 때가 많�네요.. 🤣 꼼꼼한 리뷰 감사합니닷 :D

skylar1220 · 2023-11-01T17:17:59Z

src/main/java/racingcar/domain/RacingGame.java

+    private void decreaseMovingCountByOne() {
+        movingCount--;
+
+        if (movingCount == 0) {
+            this.gameStatus = GameStatus.END;
+        }
+    }


실행한 시도횟수가 늘어나면서 입력한 시도횟수가 되었을 때 종료하는 흐름이 자연스럽다고 생각하는데,
반대로 줄어들게 하는 방식으로 할 경우 movingCount가 나타내는 것이 잔여 시도횟수라는 것이 명시적이지 않는다면 헷갈릴 수 있다는 생각이 들어요!

생각나는 대로 컨트롤러의 내용을 적어보자면 이렇게요!

while (racingGame.isCountUnderInput()) { racingGame.moveOnce(this.randomNumberPicker); ioManager.notifyMoveResult(racingGame.getCars()); }

컨트롤러가 게임이 끝났는지 직접 판단할 필요 없이, 게임이 끝났는지(혹은 진행 중인지)만 물어보면되게끔 설계했습니다.
racingGame.isCountUnderInput() 라는 메서드는 도메인 로직 자체에 컨트롤러가 너무 깊게 관여하는 것이 아닌지 고민되네요. 컨트롤러가 유저한테 받아온 count 의 상태를 계속 확인하면서 count 를 계속 신경 쓰는 것은 컨트롤러의 범위를 넘어난 것이 아닐까하고 생각합니다!

skylar1220 · 2023-11-01T17:26:24Z

src/main/java/racingcar/io/output/ConsoleWriter.java

+    private String toForwardCountOutputFormat(long count) {
+        StringBuilder stringBuilder = new StringBuilder();
+
+        for (int i = 0; i < count; i++) {
+            stringBuilder.append(CharacterSymbol.HYPHEN.getLiteral());
+        }
+
+        return stringBuilder.toString();
+    }


Suggested change

private String toForwardCountOutputFormat(long count) {

StringBuilder stringBuilder = new StringBuilder();

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

stringBuilder.append(CharacterSymbol.HYPHEN.getLiteral());

}

return stringBuilder.toString();

}

private String toForwardCountOutputFormat(long count) {

return CharacterSymbol.HYPHEN.getLiteral().repeat((count));

}

repeat를 사용해서 줄일 수도 있습니다!

헉.. 이런 API가 있었군요..!
꿀팁 감사합니당 🐝

skylar1220 · 2023-11-01T17:29:46Z

src/main/java/racingcar/io/output/ConsoleWriter.java

+            return WINNER_RESULT_FORMAT.formatted(winnerCars.getUniqueWinnerName());
+        }
+        return WINNER_RESULT_FORMAT.formatted(String.join(", ", winnerCars.getWinnerNames()));
+    }


제가 알고있기로는 String.join이 알아서 list 속 요소가 하나면 구분자를 안넣고 요소만 출력하는 것으로 알고있는데,
uniqueWinner인 경우를 따로 지정해줘하는 이유가 있을까요?

제가 join 의 동작원리를 정확히 파악하지 못한 상태에서 활용해 생긴 실수입니다🤣
말씀대로 리스트 원소의 개수 상관없이 바로 출력하는 편이 더 깔끔하겠네요!

skylar1220 · 2023-11-01T17:31:17Z

src/main/java/racingcar/domain/judge/WinnerJudge.java

+    private long findMaxForwardCount() {
+        return this.cars.getCars()
+                .stream()
+                .mapToLong(Car::getForwardCount)


이 부분은 getter를 사용해서 car의 count를 모두 가져온 후 최댓값을 비교하는 방식으로 보이는데요!
getter로 객체 내의 요소를 가져오는 것보다는 메시지를 보내서 최댓값을 비교하는 방식은 어떻까요?

Car 객체 하나가 다른 객체의 상태를 모두 알게끔 해서 최댓값을 비교하는 것은 한 개의 Car 객체에서 판단할 수 있는 범위를 벗어나는 것이 아닐까 하고 생각합니다. 승리자를 가려내는 심판 말고 어느 객체가 최댓값을 가려내는 책임을 가지면 좋을까요?

또, 조금 다른 얘기지만 getter 는 메시지를 보내는 것이 아니라고 생각하시는 건지 궁금합니다.
심판 객체가 최댓값을 계산하는 책임을 완수하기 위해 자동차 객체에게 '전진 횟수를 알려달라'라는 메시지를 보내서 해당 책임을 완수하는 로직인데, 개인적으로는 이 과정에서 객체간 협력이 충분히 이루어진 것이 아닐까라고 생각해서요!

생각해보니, 현지님 의견대로 Cars 객체에 findMaxForwardCount() 를 정의해도 괜찮았을 것 같습니다!
심판의 경우 최고 전진 횟수를 직접 찾기 보다, 최고 전진 횟수를 기준으로 누가 우승자인지 가려내기만 해도 괜찮았을 것 같네요! 처음에는 심판이 직접 최고 전진 횟수 찾아야만 하는 걸로 설계했는데, 구현에서 디미터의 법칙을 위배한 구현이 되고 말았군요 😅

skylar1220 · 2023-11-01T17:32:13Z

src/main/java/racingcar/domain/judge/WinnerCars.java

+    }
+
+    public List<Car> getCars() {
+        return Collections.unmodifiableList(this.cars);


unmodifiableList를 적용하셨군요😉 저도 이번주에 적용해보려고 해서 눈에 들어오네요!

찾아보니까 unmodifiableList 역시 능사는 아닌 것 같더라고요. 🤣. 이 부분 같이 공부해보시죳~!

skylar1220 · 2023-11-01T17:43:19Z

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

+    @ValueSource(ints = {0, 1, 2, 3})
+    void moveForwardForNumberLessThan4(int randomDigit) {
+        // given
+        RandomNumberPicker randomNumberPicker = new FakeRandomNumberPicker(randomDigit);


Suggested change

RandomNumberPicker randomNumberPicker = new FakeRandomNumberPicker(randomDigit);

RandomNumberPicker randomNumberPicker = () -> randomDigit;

인터페이스에 메소드가 하나밖에 없으면 이렇게 반환값을 바로 지정해줄 수 있습니다!

물론 인터페이스에 추상메서드가 하나만 있으면 문법적으로 함수형 인터페이스로 볼 수 있습니다. 또, 람다식으로 간결하게 구현하면 가독성도 높아지고요(약간 멋있는 것은 덤이고요 ㅋㅋ).

그런데, 매번 이렇게 '구현'하는 것이 좋을 지는 조금 고민해볼만 합니다. 이 같은 구현이 TC마다 반복될 텐데,
저는 구현이 반복되면 중복이라고 생각하거든요.

또, 결국 중복을 줄일 수 있는 방법은 이미 해당 코드를 구현한 클래스를 만들어 재사용성을 높이는 것이 객체지향의 장점 중 하나라고 생각합니다. 이런 관점에서, 저는 일급 객체인 람다식을 쓸 때는 과연 정말로 유용할지 고민을 하고 적용하는 편입니당 :)

hyeon0208 · 2023-11-02T00:59:34Z

src/main/java/racingcar/io/input/InputManager.java

+public class InputManager {
+
+    private static final String CAR_NAME_DELIMITER = CharacterSymbol.COMMA.getLiteral();
+


Enum에서 COMMA 상수를 선언하고 가져오고, 해당 클래스에서 또 COMMA 값을 갖는 매직넘버 필드를 선언하셨는데
이미 상수화된 값을 다시 해당 클래스의 상수로 정의하신 이유가 궁금합니다!

이름을 통해 의도를 드러내기 위해서입니다!
이름을 붙이기 전까지는 단순히 COMMA는 문자 기호에 불과했지만, CAR_NAME_DELIMITER라는 이름을 붙여서 '차 이름의 구분자로 콤마를 사용하는구나' 라는 의도를 드러내기 위해서입니다.😆

hyeon0208 · 2023-11-02T01:18:06Z

src/main/java/racingcar/io/output/ConsoleWriter.java

+    public void printMoveResult(Cars cars) {
+        cars.getCars().forEach(car -> printLine(toMoveResultFormat(car)));
+        printEmptyLine();
+    }


I/O는 작업은 비용이 큰 작업으로 알고 있습니다
반복문을 통해 매번 이동 결과를 출력하는 것 보다 출력 결과를 모아서 한 번에 출력하는 것은 어떨지 의견 드립니다!

Suggested change

public void printMoveResult(Cars cars) {

cars.getCars().forEach(car -> printLine(toMoveResultFormat(car)));

printEmptyLine();

}

public void printMoveResult(Cars cars) {

StringBuilder stringBuilder = new StringBuilder();

cars.getCars().forEach(car -> stringBuilder.append(toMoveResultFormat(car)).append("\n"));

printLine(stringBuilder.toString());

}

확실히 출력 결과를 한 번에 모아서 출력해주나, 반복하며 출력하나 출력 결과에는 차이가 없을 것 같네요. 저 역시 이 방법이 더 좋은 것 같습니다! 인사이트 감사합니닷 🤠

src/main/java/racingcar/io/utils/StringLiteralValidator.java

src/main/java/racingcar/io/UserIoManager.java

src/main/java/racingcar/domain/car/CarName.java

hyeon0208 · 2023-11-02T01:51:09Z

src/main/java/racingcar/controller/RacingGameController.java

+    private RacingGame setUpNewGame() {
+        Cars racingCars = setUpRacingCars();
+        long movingCount = ioManager.readMovingCount();
+        return RacingGame.newGame(racingCars, movingCount);
+    }
+
+    private Cars setUpRacingCars() {
+        List<CarName> carNames = CarName.from(ioManager.readCarNames());
+        return new Cars(Car.namesOf(carNames));
+    }


오피셜로 정해진 문서를 찾진 못했지만
일반적으로 set으로 시작되는 메서드는 어떤 필드 값을 설정하는 역할을 하는 것으로
이러한 set 메서드는 반환 값이 없는 void 타입으로 알고 있습니다!
해당 메서드 네임으로만 봐서는 값을 세팅한다고 생각이 드는데
객체를 생성하고 반환하는 것으로 보아 set 대신 create, init으로 시작하는게 어떨지 선호님의 의견이 궁금합니다!

사실 set 메서드 라기 보다는 prefix � 를 setUp으로 설계한 메서드이긴 합니다. '준비해라' 라는 뉘앙스를 주고 싶었어요.

현준님께서 헷갈려하신 걸 보니 아마 다른 분들도 헷갈릴 수도 있겠다는 생각이 들고 말씀대로 조금 다른 표현을 할 필요가 있는 것 같아요.

다만 초기화나 생성에 포커스를 맞추기보다는 게임을 시작하기 위한 '준비' 라는 뉘앙스를 주고 싶어서 beReady, prepare 등의 prefix를 사용하고 싶은데 현준님 의견은 어떨지 궁금합니다!

사실 워낙 잘 작성하셔서 리뷰할 요소를 찾기 힘들어 억지로 제안드린 부분이긴 합니다만 .. ㅋㅋ
setUp으로 시작하지만 개인적으로는 set이라는 단어가 접두사로 들어감으로써 setter같은 의미로 보일 수도 있다고 판단했습니다!
저 또한 컨트롤러의 입력값을 가지고 객체를 생성하는 메서드의 이름을 게임 시작 전 준비 단계라는 이름을 부여하고 싶어 처음에는 set() 으로 지었었다가 setter는 아니지만 setter의 느낌이 있어서 다른 적당한 네이밍이 뭐가 있을까 고민되더라구요..
결국엔 get*()으로 시작했지만 이 이름도 역시 좋은 방법은 아닌 것 같습니다..
말씀해주신 접두사중 개인적으로 prepare 맘에드네요 !!
(메서드 네이밍 참 쉽지 않네요 .. 😂)

hyeon0208 · 2023-11-02T01:54:13Z

src/test/java/racingcar/mock/FakeRandomNumberPicker.java

+public class FakeRandomNumberPicker implements RandomNumberPicker {
+
+    private final int randomDigit;
+
+    public FakeRandomNumberPicker(int randomDigit) {
+        this.randomDigit = randomDigit;
+    }
+
+    @Override
+    public int pickOneDigit() {
+        return this.randomDigit;
+    }
+
+}


이런식으로 인터페이스를 활용하신 것을 보고
이번 미션에서 인터페이스를 적용한 장점이 확실히 들어남을 느꼈습니다!
테스트하기 어려운 부분을 이렇게 해결할 수 있군요 배워 갑니다!

향로님의 2. 테스트하기 좋은 코드 - 제어할 수 없는 코드 개선 포스팅에 더 좋은 방식들이 소개되어있으니 이를 참고해보셔도 좋을 것 같습니다 ☺️

hyeon0208 · 2023-11-02T02:02:54Z

src/main/java/racingcar/domain/car/ForwardCount.java

+public class ForwardCount {
+
+    private long forwardCount;


21억번 이상의 이동이 있다면 long 타입이 적절한 것 같지만
21억번 이상의 이동을 하기 위해서는 21억번 이상의 경주가 이루어져야하는데
21억번 이상의 반복문이 실행되기는 어렵다고 생각해서 바이트 수가 적은 int 타입이 적절하지 않나 의견드립니다!
자동차가 이동하는 수를 long으로 정하신 특별한 이유가 있을까요??

저도 이 부분은 궁금합니다! long 형이어야 하는 이유가 있을까요?

일단 요구사항에 이동 횟수의 제한이 없는 것은 맞습니다.

이 경우 int 보다 long 을 사용하게 되면 겨우 4바이트를 더 쓰는 건데, 대응할 수 있는 범위는 (2^63-1) - (2^32 - 1) 만큼 늘어나게 되죠. (비교할 수 없이 훨씬 많은 유즈케이스가 커버되겠죠.)

21 억번 이상의 연산이 없다고 단정해서 int 를 사용하는 것과, 그 이상의 유즈케이스를 고려하는 것은 분명한 차이가 있을 것 같습니다!

hyeon0208

자동차 이름과 이동 원시값을 포장해서 검증한 것도 인상깊었고 전체적으로 많이 보고 배워갈 수 있었습니다!
2주차도 수고많으셨습니다 ㅎㅎ

boompatron

코드 정말 잘 봤습니다!
전체적으로 의존성 주입, 정적 팩토리 패턴 등 배울 점이 정말 많다고 생각이 드는 코드입니다.
DTO 및 일급 컬렉션도 적절하게 사용하신 느낌도 들었고요!
2주차 정말 수고하셨습니다!!

boompatron · 2023-11-04T07:16:52Z

src/main/java/racingcar/Application.java

+        RacingGameController racingGameController = new RacingGameController(randomNumberPicker, userIoManager);
+        racingGameController.run();
+
+        Console.close();


콘솔을 닫는 것 까지 신경 쓰셨군요! 저도 이 부분은 참고해서 다음 과제에서 사용하면 좋을 것 같네요!
그리고 해당 부분은 UserIOManager 내부 메소드로 들어가도 어색하지 않을 것 같습니다!

UserIoManager 에서는 입출력을 담당하긴 하는데, 입력을 받기 위한 자원에 대해 신경쓰는 책임을 갖게하는 것은 고민되네요.
애플리케이션에 실행에 필요한 자원들을 main 에서 초기화해주고 있어서, 애플리케이션을 종료할 때 필요없는 자원을 반납하는 것도 main 에서하는 것도 나름 자연스럽다고 생각했거든요. 여기에 대한 보섭님의 의견은 어떤지 궁금합니다 :)

src/main/java/racingcar/domain/RacingGame.java

boompatron · 2023-11-04T07:28:16Z

src/main/java/racingcar/domain/car/Cars.java

+    }
+
+    public List<Car> getCars() {
+        return Collections.unmodifiableList(this.cars);


이런식으롤 불변 객체를 넘겨줄 수도 있군요! 참고하겠습니다.!

선호님은 도메인 클래스에서의 getter에 대해서 어떻게 생각하시나요? 저는 이번 과제에서 무식하게 getter를 지양하려고 해보았지만 능력 부족으로 코드가 엉키게 되었습니다ㅠ 오히려 getter를 너무 열어준 느낌이랄까요... 어디까지 열어 줘야 적당한 수준인지 아직은 고민중입니다....

먼저, 왜 getter를 지양해야하는지 를 고민해보시면 좋을 것 같습니다. (디미터의 법칙에 대해 찾아보시면 좋을 것 같아요!)
저는 getter 를 지양해야하는 이유 에 해당하는 케이스가 아니라면 편의성을 위해 사용해도 괜찮다고 생각합니다. (물론 이 경우, 확실히 해당 케이스가 아닌지 많은 고민을 합니다!)

출력 로직에서는 어쨌든 도메인의 상태를 출력해야하는 것이고, 이 상태를 알기 위해서는 getter 를 쓸 수 밖에 없어서 이런 경우는 getter 를 사용합니다.

도메인 로직에서 getter 를 사용할 때는 디미터의 법칙을 위반하지 않는지, getter 없이 구현할 수 있는지 고민하고 사용합니다.

src/main/java/racingcar/domain/car/ForwardCount.java

boompatron · 2023-11-04T07:41:46Z

src/main/java/racingcar/domain/car/ForwardCount.java

+public class ForwardCount {
+
+    private long forwardCount;


저도 이 부분은 궁금합니다! long 형이어야 하는 이유가 있을까요?

src/main/java/racingcar/io/input/InputManager.java

src/main/java/racingcar/io/utils/StringLiteralValidator.java

src/test/java/racingcar/ApplicationTest.java

siyeonSon

기능에 따른 클래스 분리가 인상적인 코드였어요! 덕분에 테스트도 꼼꼼하게 작성할 수 있는 것 같아 저도 다음 미션에 적용해 보고 싶네요👍 이번 미션도 고생하셨어요 :)

siyeonSon · 2023-11-04T08:30:30Z

src/main/java/racingcar/io/output/OutputMessage.java

@@ -0,0 +1,19 @@
+package racingcar.io.output;
+
+public enum OutputMessage {


저는 입출력 할 문자열을 enum 보다는 상수로 관리 했어요!
개인적인 생각이지만, 열거 타입을 사용할 때마다 OutputMessage.CAR_NAME_INPUT_GUIDE.getLiteral()으로 메서드를 호출하는 게 가독성이 떨어진다고 생각했기 때문이에요. 출력할 문자열을 저장하는 방식으로 사용했기 때문에, 열거 타입의 장점을 극대화하기 애매하다고 느꼈어요.

저는 상수 타입을 사용하는 것이 조금 더 깔끔하다고 생각했는데, 선호님의 의견이 궁금해요!

확실히 단순 문자열 모음인데 왜 enum 까지 써야하는지에 대한 근거가 좀 부족한 것 같긴하네요. 다만 '열거형' 클래스라는 것 자체가 열거형 상수의 모음이기 때문에(enum=enumration=열거, 목록, 일람표) enum 그 자체만으로 관련된 상수들을 묶어놓았구나 라는 컨텍스트를 줄 수 있을 것 같아요. 만약 상수의 모음인 것을 클래스로 표현하려면 OutputMessageConstant 처럼 Suffix 로 Constant 를 명시해야 할 것 같기도 하네요 🤔.

OutputMessage.CAR_NAME_INPUT_GUIDE.getLiteral()의 가독성이 떨어지는지는 예상치 못한 부분인데, 이 부분이 OutputMessage.CAR_NAME_INPUT_GUIDE 또는 CAR_NAME_INPUT_GUIDE 처럼 줄여쓴다고 해서 가독성이 좋아질 지는 확실치 않을 것 같습니다 🫠

siyeonSon · 2023-11-04T09:58:51Z

src/main/java/racingcar/io/output/ConsoleWriter.java

+    private String toMoveResultFormat(Car car) {
+        return MOVE_RESULT_FORMAT.formatted(car.getName(), toForwardCountOutputFormat(car.getForwardCount()));
+    }


도메인에서 View로 데이터를 전달할 때 DTO로 한번 감싸는 건 어떤가요?
변경에 대해 더 안전해 질 수 있을 것 같아요

저 역시 View 레벨에서 DTO를 사용하는 것에 대해 많이 고민해봤고 아래 질문들에 대해 스스로 물어보고, 나름대로의 기준을 세웠습니다.

DTO 로 감싼다고 해서, 변경에 대해 더 안전해지는 것을 보장할 수 있을까요?

새로운 필드가 추가되고, 이를 View 에서 보여줘야한다면 결국 도메인을 의존하는 DTO가 변경되고, DTO를 의존하는 View 도 변경이 될 것 같아요.

도메인이 변경되면 반드시 View가 변경되는 것일까요?

도메인의 내부 필드가 원시타입에서 원시 타입을 포장한 래퍼클래스로 변경되었다고 해도, View 의 getter 로직은 수정하지 않고 도메인에서 해당 래퍼클래스의 내부 값을 리턴해주면 View 는 변경을 하지 않아도 될 것 같아요.

이 외에도 과연 그 변경 자체가 자주 일어날 것인가에 대해서도 고민해봐야할 것 같아요.

위 같은 내용들을 고민해본 바, 현재로서 프리코스 미션에서 DTO를 쓰는 것은 오버엔지니어링이라고 판단했습니다. 실제로 얻는 이득에 비해(실제로 얻는 이득이 없을 수도 있어요!) 복잡도가 올라가는 것은 자명하니까요.

siyeonSon · 2023-11-04T13:29:20Z

src/main/java/racingcar/domain/RacingGame.java

+    public Cars getCars() {
+        return this.cars;
+    }
+
+    public boolean isEnd() {
+        return this.gameStatus.isEnd();
+    }
+
+    public GameStatus getGameStatus() {
+        return this.gameStatus;
+    }


this 키워드를 생략해도 될 것 같은데, 항상 추가하신 이유가 궁금해요!

물론 this 키워드를 생략해도 됩니다!

다만 생략하지 않으면 안 되는 경우도 있습니다.

혹은 강조를 위해서 생략해도 되지만 그냥 쓰는 경우도 있을 거고요.

(강조를 위해서 쓸 수도 있지만, 과연 얼마나 강조될까라는 생각도 있습니다.)

내부 필드 하나를 참조하는데 위 같은 고민을 매번할 필요없이 일관되게 this를 사용하는 코드 스타일을 가져가면 리소스를 줄일 수 있고, 더 중요한 것에 리소스를 투자할 수 있을 거라고 생각해요! (실제로도 그러고 있고요.☺️)

강조를 위해서 사용하신 것으로 이해하면 될까요?!

siyeonSon · 2023-11-04T13:44:01Z

src/main/java/racingcar/domain/RacingGame.java

+    public void moveOnce(RandomNumberPicker randomNumberPicker) {
+        this.cars.moveOnce(randomNumberPicker);
+        decreaseMovingCountByOne();
+    }
+
+    private void decreaseMovingCountByOne() {
+        movingCount--;
+
+        if (movingCount == 0) {
+            this.gameStatus = GameStatus.END;
+        }
+    }
+
+    public boolean isInProgress() {
+        return this.gameStatus.isInProgress();


moveOnce()의 코드를 보면, movingCount을 하나씩 줄여가면서 0이 될 때까지 자동차 경기를 하는 것으로 보여요. 현재 기능 요구사항으로는 GameStatus를 사용하지 않고도 구현할 수 있을 것 같은데 사용하신 이유가 궁금해요!

GameStatus 를 사용하지 않으면,

컨트롤러에서 이 count 에 대해서 계속 물어보면서 숫자가 0인지 아닌지를 확인해서 게임을 종료할지 말지를 스스로 판단을 해야해요. 이동 횟수와 같은 중요한 정보를 컨트롤러가 이렇게 깊이 신경쓰는게 과연 좋은 설계일지는 고민되네요.

반면에 GameStatus 라는 필드를 추가함으로써, 이제 컨트롤러는 더 이상 Count 를 신경쓰지 않고 단순히 게임이 끝났어? 혹은 아직 게임 중이니? 라고 물어보기만 하면 돼요. 중요한 정보는 감추면서, 필요한 정보만 알게 하는 것이 좋은 설계라고 생각하기 때문에 GameStatus 필드를 추가했습니다 ☺️

siyeonSon · 2023-11-04T13:46:04Z

src/main/java/racingcar/domain/judge/WinnerJudge.java

+                .stream()
+                .mapToLong(Car::getForwardCount)
+                .max()
+                .orElse(0L);


max() 연산이 실패한 경우 0L를 취해 틀린 답을 만드는 것보다 예외를 발생시키는 게 어떨까요?

ssosee

2주차 미션 정말로 고생많으셨습니다~!
선호님 코드를 보면 많은 것을 배우는 것 같아요!!
3주차 미션도 화이팅 합시당

ssosee · 2023-11-05T12:57:00Z

docs/README.md

+
+---
+
+## 유즈 케이스(시나리오)


코드를 읽기 전에, 시나리오를 통해서 어떤 프로그램인지 배경지식이 생기게 되서 좋은 것 같아요~!

ssosee · 2023-11-05T13:02:54Z

src/main/java/racingcar/controller/RacingGameController.java

깔끔한 코드에 감탄이 나오네요! 🫢

ssosee · 2023-11-05T13:13:45Z

src/main/java/racingcar/domain/RacingGame.java

+        return new RacingGame(cars, movingCount, GameStatus.IN_PROGRESS);
+    }
+
+    public void moveOnce(RandomNumberPicker randomNumberPicker) {


RandomNumberPicker을 인자로 사용하게 되면, 테스트 하기에 조금 어려운 코드가 될 수 있다고 생각합니다.
물론 mocking하면 될 것 같긴한데,,(FakeRandomNumberPicker를 사용하셔서 테스트 하셨군요!)

RandomNumberPicker을 주입받은 이유가 있을까요?
저는 쉬운 테스트 코드를 작성하기 위해 primitive 타입의 파라미터를 사용했습니다.!!

ssosee · 2023-11-05T13:15:21Z

src/main/java/racingcar/domain/RacingGame.java

+    public Cars getCars() {
+        return this.cars;
+    }
+
+    public boolean isEnd() {
+        return this.gameStatus.isEnd();
+    }
+
+    public GameStatus getGameStatus() {
+        return this.gameStatus;
+    }


강조를 위해서 사용하신 것으로 이해하면 될까요?!

ssosee · 2023-11-05T13:19:31Z

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

+
+    private static final int MIN_REQUIRED_DIGIT_TO_MOVE_FORWARD = 4;
+
+    private final CarName name;


String 타입을 사용하지 않고, CarName을 사용하신 것이 인상 깊네요!! 👍
일급컬렉션을 의도하신것 같은데,, 저도 한 번 공부해봐야겠어용

ssosee · 2023-11-05T13:29:36Z

src/main/java/racingcar/global/CharacterSymbol.java

상수로 작성할 수 있을 것 같은데, 혹시 enum 클래스로 작성하신 이유가 있을까요?.?

ssosee · 2023-11-05T13:32:17Z

src/main/java/racingcar/io/input/ConsoleReader.java

오! Reader를 상속받는 방법이...!
이건 생각지도 못했네요.!
배워갑니당~!

ssosee · 2023-11-05T13:36:12Z

src/main/java/racingcar/io/output/ConsoleWriter.java

+
+public class ConsoleWriter {
+
+    private static final String MOVE_RESULT_FORMAT = "%s : %s";


와.. 포맷 코드 잊고 살았는데..ㅋㅋㅋ
다시 기억나게 해주셔서 감사합니당

haero77 added 30 commits October 28, 2023 21:03

initial commit

be4a197

docs: 기능 명세서 1차 작성

422eac2

feat: 자동차 이름은 5자 이하만 가능하다.

5bf8677

feat: 차를 전진 시키면 전진 카운트가 1만큼 증가한다.

35de81e

feat: 난수가 4 이상일 때만 자동차가 전진한다.

c5c6e68

test: 난수가 4 미만일 경우 자동차는 전진하지 않는다.

63ad4a2

docs: 기능 명세서 업데이트

4ecda53

feat: 이동 횟수를 모두 소진하면 게임 상태는 END이다.

224fb7b

feat: 자동차 이름을 입력 받는다.

801ddec

feat: 이동 횟수를 입력받는다.

040daf7

refactor: 의미가 모호한 메서드명을 명확하게 변경

7905cfe

refactor: 문자열이 숫자인지 검증하는 로직 수정

a9397e0

feat: 이동 횟수를 입력 받을 시 양의 정수인지 검증한다.

17e80d8

feat: 이동을 1회 진행하고, 이동 결과를 출력한다.

5307149

feat: 심판이 차의 전진 횟수를 토대로 우승자를 가려낸다.

aae88a1

feat: 게임 종료 시 우승자를 출력한다.

6a1bcfb

docs: 기능 명세서 업데이트

b042a14

refactor: Scanner 자원 할당을 해제하는 로직 추가

23e688f

chore: 불필요한 주석 제거

559fb28

refactor: 사용자 입력 기능을 추상화한다.

d331fe4

test: 자동차 이름 중 공백이 있을 경우 예외가 발생한다.

06c36df

test: 입력받은 이동 횟수가 숫자가 아니거나, 0 이하의 정수라면 예외가 발생한다.

9657a51

docs: 기능 명세서 업데이트

f2a7b49

refactor: 게임 종료 후 승자를 공지하는 로직을 메서드로 분리한다

e60b782

refactor: 게임 시작 시 경주용 차를 준비하는 책임을 메서드로 분리한다.

45025bf

style: 논리적 순서를 나타내기 위한 개행 변경

a95edfc

test: 게임을 새로 생성하면 게임 상태는 IN_PROGRESS 이다.

f3692d9

test: '이동 횟수를 모두 소진하면 게임 상태는 END이다.' TC 추가

9cb9f76

docs: 기능 명세서 업데이트

d1f0c53

refactor: 게임 진행을 하기 위한 초기 세팅 로직을 메서드로 분리

ee27da3

refactor: 단독 우승자, 이름을 확인하는 로직을 캡슐화

4197436

skylar1220 reviewed Nov 1, 2023

View reviewed changes

hyeon0208 reviewed Nov 2, 2023

View reviewed changes

src/main/java/racingcar/io/utils/StringLiteralValidator.java Show resolved Hide resolved

hyeon0208 reviewed Nov 2, 2023

View reviewed changes

src/main/java/racingcar/io/UserIoManager.java Show resolved Hide resolved

hyeon0208 reviewed Nov 2, 2023

View reviewed changes

src/main/java/racingcar/domain/car/CarName.java Show resolved Hide resolved

hyeon0208 reviewed Nov 2, 2023

View reviewed changes

boompatron reviewed Nov 4, 2023

View reviewed changes

siyeonSon reviewed Nov 4, 2023

View reviewed changes

ssosee reviewed Nov 5, 2023

View reviewed changes

haero77 closed this by deleting the head repository Apr 22, 2024


		public class SystemRandomNumberPicker implements RandomNumberPicker{

		private static final int MIN_RANDOM_DIGIT = 0;

	RandomNumberPicker randomNumberPicker = new FakeRandomNumberPicker(randomDigit);
	RandomNumberPicker randomNumberPicker = () -> randomDigit;

		public class InputManager {

		private static final String CAR_NAME_DELIMITER = CharacterSymbol.COMMA.getLiteral();

		@@ -0,0 +1,19 @@
		package racingcar.io.output;

		public enum OutputMessage {


		private static final int MIN_REQUIRED_DIGIT_TO_MOVE_FORWARD = 4;

		private final CarName name;


		public class ConsoleWriter {

		private static final String MOVE_RESULT_FORMAT = "%s : %s";

[자동차 경주] 김선호 미션 제출합니다. #486

[자동차 경주] 김선호 미션 제출합니다. #486

Conversation

haero77 commented Oct 31, 2023

skylar1220 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haero77 Nov 4, 2023 • 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

haero77 Nov 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haero77 Nov 4, 2023 • 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

haero77 Nov 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haero77 Nov 4, 2023 • 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

hyeon0208 Nov 2, 2023 • 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

hyeon0208 left a comment • edited Loading

Choose a reason for hiding this comment

boompatron 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

siyeonSon 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

haero77 Nov 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssosee 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

skylar1220 left a comment •

edited

Loading

haero77 Nov 4, 2023 •

edited

Loading

haero77 Nov 4, 2023 •

edited

Loading

haero77 Nov 4, 2023 •

edited

Loading

haero77 Nov 4, 2023 •

edited

Loading

haero77 Nov 4, 2023 •

edited

Loading

hyeon0208 Nov 2, 2023 •

edited

Loading

hyeon0208 left a comment •

edited

Loading

haero77 Nov 4, 2023 •

edited

Loading