자동차 경주 게임 PR #1

DevRunner21 · 2022-03-14T20:08:09Z

이번에 자동차 경주게임 미션을 진행해봤습니다.
야구게임을 다 완성하진 못했었는데...
그 미션은 재성님의 답지를 봐버려서 그런지 강의 그대로 코딩하게 되버려서...
일단 주제가 비슷한 것 같아서 바로 다음 미션으로 넘어왔습니다. ㅜㅜ

자동차 경주 게임도 아직 다 완성은 못했지만
주요 기능은 작업을 해서 PR 보내봅니다.

혹시나 시간 되실 때, 가차없는 피드백 부탁드립니다. ㅎㅎ

PR 포인트

입출력 및 결과산정 로직을 제외한 메인로직 부분만 작업이 되어있습니다.
Getter 없이 TDD 개발방법으로 작업을 진행했습니다.
메인로직의 흐름도는 아래와 같습니다.
RandomNumber에 따른 경주게임 테스트를 위해 RandomNumberFactory를 만들고
이 객체의 Fake 객체로 FakeRandomNumberFactory 를 만들어서 사용했습니다.

고민사항

이 부분은 코멘트로 남겨놓겠습니다.

- "자동차 이름"은 5자를 초과 할 수 없다.

- 시도 횟수는 음수 일 수 없다.

- 0 이상 9 이하의 랜덤 값을 받는다.

- 모든 자동차 이동 기능 구현 - RandomNumber 생성을 위한 Factory Class 구현 - 테스트를 위해 Fake RandomFactory 생성

DevRunner21 · 2022-03-14T20:28:57Z

src/test/java/FakeRandomNumberFactory.java

+public class FakeRandomNumberFactory implements AbleToProduceRandomNumber {
+
+    public final int MOVE_NUMBER = 4;
+    public final int STOP_NUMBER = 3;
+    public final int MOVE_TARGET_CAR_COUNT = 2;
+
+    private List<Integer> numberPool;
+    private int currentIndex;
+
+    public FakeRandomNumberFactory(int carsLength) {
+        this.currentIndex = 0;
+        this.numberPool = new ArrayList<>();
+
+        settingFakeData(carsLength);
+    }
+
+    private void settingFakeData(int carsLength) {
+        for (int i = 0; i < carsLength - MOVE_TARGET_CAR_COUNT; i++) {
+            this.numberPool.add(STOP_NUMBER);
+        }
+        for (int i = carsLength - MOVE_TARGET_CAR_COUNT; i < carsLength; i++) {
+            this.numberPool.add(MOVE_NUMBER);
+        }
+    }
+
+    @Override
+    public RandomNumber produce() {
+        int numberPoolIndex = currentIndex % numberPool.size();
+        plusCurrentIndex();
+        return new RandomNumber(numberPool.get(numberPoolIndex));
+    }
+
+    private void plusCurrentIndex() {
+        currentIndex++;
+    }
+
+}


[고민사항]
Fake 객체를 위와 같이 구성해봤습니다.
조금 설명을 해보자면

numberPool이라는 List에 자동차 개수 만큼 데이터를 넣어 놓고
produce()(RandomNumber를 생성하라는 메시지)를 받으면
numberPool에서 저장된 값을 순차적으로 빼오는 식으로 구현했습니다.

여기서 numberPool에 들어가 있는 데이터는 자동차 목록 중 끝에 두개의 자동차만 전진 시킬 수 있는 숫자를 할당하도록 구성했습니다.

이렇게 한 이유는
Test 코드를 작성 할 때, RandomNumber를 TestCode에서 컨트롤 할 수 있어야 테스트가 가능할텐데...
현재 CarsTest.class에서는 외부에서 만들어진 RandomNumber를 주입할 방법이 없기 때문에
Factory에서 아예 설정된 값을 다 세팅해 두도록 할 수 밖에 없었습니다.

일단 이렇게 구성해 봤는데,
여러분은 어떻게 생각하시나요? ( 뭔가 엄청 찜찜해서.. ㅜ )

해당 부분에 대해서 예전에 재원님꼐서 리뷰를 주신 것 같아요!!
random한 값들에 대한 test를 어떻게 진핼할 것인지!!
@darkant99 스승님 한번 예시를 보여주시죠!

저의 경우에는 fake객체도 만들지 않고 진행합니다!
그 이유는 관리 포인트가 늘어나는 것을 막기 위함이에요!

Fake 객체를 사용해 랜덤 값을 직접 지정해 테스트를 진행 해주셨네요.
여기서 '아침에는 움직이지 않고 밤에는 움직이는 자동차' 와 같은 요구 사항으로 변경되면 굉장히 난감 해질 것 같아요.
Car 객체가 RandomNumber에 의존하고 있는데, 이 의존 관계를 개선 해보면 어떨까요?😊

현재 설계는 난수에 너무 집중되어 있는것 같아요!

주어진 요구사항의 구현에만 초점을 맞추다 보니
요구사항이 변경되는 경우에 대해서는 생각을 못한 것 같습니다.
크~ 역쉬 재원님!!!

아래서 말씀해주신 것처럼 move() 라는 행위를 별도의 전략 인터페이스로 만들고
RandomNumber를 통해 움직이는 전략 구현체를 따로 만들어봐야 겠군요.
그러면 변경에도 유연해지고 메인로직의 소스도 좀 더 깔끔해질 것 같습니다!

다만 전략이 바뀔경우 테스트를 위해 만든 Fake 객체 또한 전략마다 생성을 해줘야겠군요...
현재 샘플 데이터를 난수에 맞게 세팅해놓았으니...
관리 포인트가 늘어난다는 동건님의 말씀이 확 공감이 가는 부분이네요 ㅋㅋ

Fake객체 없이는 테스트가 불가능 할 거라는 생각을 했는데
동건님은 없이 하셨다니... 피드백 받은 내용 적용해 보고 Fake객체 없이 테스트 할 방법도 한번 고민해봐야겠습니다. ㅎㅎ

DongGeon0908

잠깐 점심시간이 되서 리뷰 남겨요!!

엄청 예쁘게 작성하셔서 놀랐습니다~~ 뭔가 여러 사람들의 코드를 보다면 큰 틀에서는 비슷하게 작성되고 있는데 세부적으로는 정말 각기다른 개성을 가지고 있는 것 같아 신기하기도 하네요 ㅋㅋㅋ

고생하셨습니다 지훈님~~ @DevRunner21

DongGeon0908 · 2022-03-15T03:02:17Z

src/main/java/AbleToProduceRandomNumber.java

@@ -0,0 +1,7 @@
+import java.util.ArrayList;
+
+public interface AbleToProduceRandomNumber {


해당 부분에 대해 궁금한 점이 있습니다!
Factorty라고 하셨는데, naming을 AbleTo라고 하신 이유가 있을까요??

인터페이스에 AbleTo를 붙인건 처음봐서용

인터페이스 자체가 어떤 기능에 대한 명세이다 보니
"어떤 역할를 할 수 있다~" 라는 네이밍이 어울릴 것 이라고 생각했습니다.
Java의 Serializable 인터페이스 처럼 말이죠.

그래서 Producable이나 Generatable 로 네이밍 하려헀는데
저런 단어는 없더라구요.. ㅜㅜ

일단 저렇게 네이밍 하기는 했는데
저도 좀 어색해서 뭔가 보편적인 네이밍 방법을 좀 더 찾아봐야 할 것 같습니다! ㅎㅎ

해당 클래스의 경우에는 생성하는 기능만을 재공하기 때문에 더 어색하게 느껴졌던것 같아용

DongGeon0908 · 2022-03-15T03:03:00Z

src/main/java/Car.java

+
+public class Car {
+
+    public final int MOVE_PERMISSION_MIN_NUMBER = 4;


외부에서도 쓰인다면 static으로 선언하는게 좋을 것 같아요!

일단은 내부에서만 사용 할 필드로 선언한거라서
static으로 선언하지 않았습니다. ㅎㅎ

현재 소스가 상수들이 중복된게 많아서.. 나중에 싹 다 정리해야 할 것 같아요 ㅜ

넵넵 그렇다면 해당상수는 private으로 처리하시죠!!

DongGeon0908 · 2022-03-15T03:03:38Z

src/main/java/Car.java

+
+    private void validate(Name name, Position position) {
+        if (Objects.isNull(name) || Objects.isNull(position)) {
+            throw new IllegalArgumentException("Name and Position must be not null");


새롭게 exception을 정의해서 사용해도 좋을 것 같아요!

커스텀 Exception은 사실 귀찮아서 안만들었었는데,
주요 기능을 다 완성하고 만들어보겠습니다! ㅎㅎ

ㅋㅋㅋ 넵넵!

DongGeon0908 · 2022-03-15T03:07:37Z

src/main/java/Cars.java

+        }
+    }
+
+    public void move(AbleToProduceRandomNumber factory) {


인터페이스의 명치 뭔가 아쉽게 느껴집니다~
able to : ~ 할 수 있는으로 알고 있어서용

사실 ~할 수 있는 이라고 보여지는 걸 의도하긴 했는데,
제가 봐도 좀 어색하네요 ㅜㅜ

좀 더 보편적인 네이밍을 찾아보고 수정해보겠습니다! ㅎㅎ

DongGeon0908 · 2022-03-15T03:08:24Z

src/main/java/Name.java

+    private static final int MAX_NAME_LENGTH = 5;
+    private static final String NAME_LENGTH_OVER_MESSAGE = "자동차 이름은 5자를 초과할 수 없다.";


위에서는 상수를 public으로 처리하셨는데, 여기서는 다르게 처리되고 있네요! 차이점이 있을까요?

아 사실 제가 의도한 건 전부 private로 하는게 맞습니다.

저 상수들은 명시적으로 의미를 표시하기 위해서 뺴놓은 거고
사용되는 범위가 해당 클래스 내부로 한정해 놓을 생각이었습니다.

이후에 공통되는 상수들은 별도의 클래스로 정리할 생각이었죠.

public으로 되어있는 부분은 IDE에서 자동 생성기능을 쓰면 자동으로 public이 되다보니
제가 미처 수정을 못했나봅니다. ㅎㅎ
다음 PR에서 수정해서 올리겠습니다!

DongGeon0908 · 2022-03-15T03:14:35Z

src/main/java/Race.java

+    }
+
+    public void start() {
+        int currentTryCount = TryCount.MIN_TRY_COUNT;


TryCount에서 상수를 가져와서 사용하고 있는데, 생성자로 tryCount를 받아야 하는 이유가 있을까요??
위에서는 생성자로 최고하 해주는데, 밑에서는 static으로 사용하다보니 해석에 어려움이 있는 것 같아용

저기다가 TryCount에서 정의한 상수를 쓰다보니 가독성에 혼란이 생기는 군요.. ㅜ
사실 아래와 같이 0을 쓰는게 맞습니다.

// int currentTryCount = TryCount.MIN_TRY_COUNT; int currentTryCount = 0;

currentTryCount 는 반복문을 돌리기 위한 현재 Index의 역할입니다 ㅎ
저기다가 왜 저 상수를 넣었는지..ㅜㅜ
새벽에 무지성으로 넣었나봐요 ㅜㅜ ( 새벽코딩의 폐혜군요.. 반성합니다.. ㅜ )
수정해서 다시 올려 놓겠습니다! ㅎㅎ

여기서 그럼 TryCount를 생성자로 받아서 필드로 넣지 말고

public void start(TryCount tryCount);

이런식으로 메서드의 인자로 전달해도 되지 않느냐 에 대해서도 고민해봤는데,
사실 메서드의 인자로 넘겨줘도 동작에 지장은 없습니다.

하지만 저는 TryCount는 Race라는 객체에게 딱 1개만 할당되어서 변하지 않아야 할 필드라고 생각했습니다.
Race(경기)라는건 처음에 설정된 일정대로 수행되고 일정이 다 끝나면 그대로 종료되는 것이니까요.
그래서 Cars(참가자), TryCount(경기 라운드 수) 는 Race를 구분하는 상태 라고 생각해서 필드로 구성하였습니다.
( 적다보니 그럼 Factory는 필드로 들어가있는게 좀 애매하군요.. 큼.. 이부분은 좀 더 고민해봐야 할 것 같습니다. )

상당히 많은 부분에서 고민하셨군요!
이미 정답을 찾으신듯합니다❤️

DongGeon0908 · 2022-03-15T03:15:20Z

src/test/java/CarTest.java

+    @ParameterizedTest
+    @DisplayName("move()는 4이상의 수가 입력될 경우 1칸 전진하고 아닐 경우 그대로 정지합니다.")
+    @MethodSource("provideRandomNumberForMoving")
+    void test_move(RandomNumber randomNumber, Car expected) {
+        car.move(randomNumber);
+
+        assertThat(car).isEqualTo(expected);
+    }
+
+    private static Stream<Arguments> provideRandomNumberForMoving() {
+        int stoppedPositionNumber = 0;
+        int oneStepMovedPositionNumber = 1;
+
+        Car stoppedCar = new Car(DEFAULT_CAR_NAME, stoppedPositionNumber);
+        Car oneStepMoveCar = new Car(DEFAULT_CAR_NAME, oneStepMovedPositionNumber);
+
+        return Stream.of(
+            Arguments.of(RANDOM_NUMBER_LESS_THAN_FOUR_OBJECT, stoppedCar),
+            Arguments.of(RANDOM_NUMBER_MORE_THAN_FOUR_OBJECT, oneStepMoveCar)
+        );
+    }


오!! 신기하네요!!! 저도 나중에 사용해야겠어영

expected 값으로 객체를 넘기고 싶어서 구글링하다가 발견했습니다. ㅋㅋ

DongGeon0908 · 2022-03-15T03:15:51Z

src/test/java/CarsTest.java

+    }
+
+    @Test
+    @DisplayName("move()는 랜덤 숫자에 따라 Car를 전진시킵니다.")


요즘 추세가 DisplayName을 사용하지 않고 메서드에 바로 한글로 작성하는 것 같습니다!

오오! 하긴 같은 내용을 굳이 영어, 한글로 둘 다 적을 필요는 없겠군여 ㅋㅋ
꿀팁 감사합니다!

DongGeon0908 · 2022-03-15T03:17:28Z

src/test/java/FakeRandomNumberFactory.java

+public class FakeRandomNumberFactory implements AbleToProduceRandomNumber {
+
+    public final int MOVE_NUMBER = 4;
+    public final int STOP_NUMBER = 3;
+    public final int MOVE_TARGET_CAR_COUNT = 2;
+
+    private List<Integer> numberPool;
+    private int currentIndex;
+
+    public FakeRandomNumberFactory(int carsLength) {
+        this.currentIndex = 0;
+        this.numberPool = new ArrayList<>();
+
+        settingFakeData(carsLength);
+    }
+
+    private void settingFakeData(int carsLength) {
+        for (int i = 0; i < carsLength - MOVE_TARGET_CAR_COUNT; i++) {
+            this.numberPool.add(STOP_NUMBER);
+        }
+        for (int i = carsLength - MOVE_TARGET_CAR_COUNT; i < carsLength; i++) {
+            this.numberPool.add(MOVE_NUMBER);
+        }
+    }
+
+    @Override
+    public RandomNumber produce() {
+        int numberPoolIndex = currentIndex % numberPool.size();
+        plusCurrentIndex();
+        return new RandomNumber(numberPool.get(numberPoolIndex));
+    }
+
+    private void plusCurrentIndex() {
+        currentIndex++;
+    }
+
+}


해당 부분에 대해서 예전에 재원님꼐서 리뷰를 주신 것 같아요!!
random한 값들에 대한 test를 어떻게 진핼할 것인지!!
@darkant99 스승님 한번 예시를 보여주시죠!

DongGeon0908 · 2022-03-15T03:18:10Z

src/test/java/NameTest.java

+    @Test
+    @DisplayName("이름의 길이는 5자를 초과 할 수 없다.")
+    void test_name_validate_over_max_length() {
+        assertThatThrownBy(() -> new Name(INVALID_NAME))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessage(NAME_LENGTH_OVER_MESSAGE);
+    }
+
+    @Test
+    @DisplayName("길이 5이하의 문자열을 통해 Name 객체를 생성한다.")
+    void test_name_validate_valid() {
+        success(new Name(VALID_NAME));
+    }


조금 더 다양한 데이터로 테스트를 진행하면 좋을 것 같아요!!
테스트케이스가 한정적으로 사용되네용

여기도 ParameterizedTest를 진행하도록 수정해보겠습니다! ㅎㅎ

darkant99 · 2022-03-15T04:16:11Z

src/main/java/Car.java

+
+    private final Name name;
+    private final Position position;
+


Car 객체가 불변이기 때문에 Position 객체는 불변성을 확보하지 못한것 같아요.
저는 Car 보단 Position 객체의 불변성을 가져오고, 인스턴스를 재활용 할것 같아요.
Car객체가 불변인 상태로 필드가 계속해서 늘어난다면 불변성을 지키면서 확장하기도 매우 어려울것 같아요 😊

말씀해주신 내용에 대해서 제가 이해한 내용을 말씀드려보자면

정리

현재 코드 상황을 정리해 보자면 Car는 불변객체이고 Position은 불변성을 확보하지 못한 상태입니다.
그 이유는 Position의 값이 전진 할 때 마다 변경되어야 하는데,
결국엔 값의 변경이 일어나다 보니 Car의 Position인스턴스를 교체하던지, Position의 필드 값을 바꿔야 하게되죠.
즉, 무조건 둘 중 하나는 불변성을 잃어야하는 상황입니다.

여기서 저는 Position 값의 변경은 Position 본인이 책임져야 할 부분이라고 생각해서
Position의 불변성을 없애고 내부에서 상태를 변경 시킬 수 있도록 했습니다.
이렇게 했을 때, 객체간의 결합도를 낮출 수 있다고 생각했죠.

하지만 재원님의 말씀을 듣고보니
만약 이렇게 할 경우 Name을 변경 해야 할 경우 Name 객체의 불변성을 없앨 것이고,
다른 필드들이 추가되고 그 필드의 내용을 수정해야 할 경우 전부 불변성을 잃은 객체로 만들게 되겠군요.

즉 수정이 필요한 필드마다 불변성을 없애게 될 테니, 확장에 있어서 지속적인 불변성을 지키기가 어렵겠군요!
Car객체가 불변인 상태로 필드가 계속해서 늘어난다면 불변성을 지키면서 확장하기도 매우 어려울 것 같아요 😊

그래서 어차피 완전한 불변성을 지키기 어려운 경우라면
여러 개의 가변 객체가 생기는 것 보다는 Car 객체 하나를 가변객체로 두는 것이 확장에 있어서
불변성을 지키기가 더 수월하다! 라고 이해했는데,
제가 이해한 내용이 맞나요?

질문

그렇다면 Car를 가변객체로 만들 경우 아래와 같이 Position의 인스턴스를 Car에서 만들어서 갈아끼우게 될텐데

public void moveToOneStep(){ this.position = new Position(1); }

이때, Position의 내부구조가 Car에 노출되다보니 결국 결합도가 올라간다는 단점이 있을 것 같습니다.
결국 불변성 vs 객체간의 결합도 중 트레이드 오프를 고민해야 하는데,
재원님께서는 불변성을 더 중요하게 생각하신 이유가 있을까요? @darkant99
(
재원님의 피드백을 보고 저도 불변성을 지키는게 더 나을 것 같은게
객체간의 결합도 문제는 Runtime에서 문제가 생기지는 않지만
불변성에 대한 부분은 동시성 이슈로 인해 Runtime에서 연쇄적인 이슈가 발생할 가능성이 높아서
둘중 하나를 고르라고 하면 불변성을 지키는게 운영상 더 이득일거라고 생각합니다. ㅎㅎ
)

move 메서드에서도 동시성 이슈가 발생할거라고 판단되는데 Car는 이미 가변 객체 일수도 있어요🤔
Car를 Entity 와 비유 해보면 어떨까요?
Car가 자신의 상태에 대해서 결합도를 낮춰야 할까요? 저는 아니라고 생각해요.
불변성과 결합도 중 트레이드 오프를 고려하는것보단
아래처럼 만들어 불변성과 응집도를 챙길 수 있다고 생각해요 😇

public void moveToOneStep() {
this.position = position.moveOneStep();
}

Car를 Entity 와 비유 해보면 어떨까요? Car가 자신의 상태에 대해서 결합도를 낮춰야 할까요?

오! 이렇게 보니 딱 이해가 됩니다.
자신의 상태와 결합도를 낮출 필요는 없겠군요!

그냥 생성자를 가져다 쓰는 것만 생각했는데
아래와 같이 응집도까지 챙길수 있다니~ 크~~ 👍👍👍👍

public void moveToOneStep() { this.position = position.moveOneStep(); }

darkant99 · 2022-03-15T04:25:02Z

src/main/java/RandomNumber.java

+    }
+
+    private int generate() {
+        return new Random().nextInt(MAX_RANDOM_NUMBER);


Random 객체가 계속 생성되고 있네요 😢

앗! 이 부분은 너무 무지성이었네요 ㅎㅎ
생각도 못하고 있었습니다! 크흐~
난수 생성 로직이 Factory Class로 옮겨야겠네요
피드백 감사합니다! ㅎㅎ

darkant99 · 2022-03-15T04:49:27Z

저도 점심 시간을 활용 했습니닷 😎
코드 작성은 깔끔하게 잘 하신것 같아요👍
난수는 언제든 변경될 수 있는 요구 사항이고, 테스트에 의해서도 변경 되어야 하는 요구 사항인데 설계의 초점이 난수에만 맞추어져 있는것 같아요!
이와 관련해서 전략 패턴을 활용 해보시면 좋을것 같아요

- Random 객체 생성 부분을 RandomFactory로 분리해서 매번 인스턴스가 생성되지 않도록 조치함 - 변경에 유연하게 대처하기 위해 기존 RamdomNumber를 추상화 하여 공통 로직을 분리함 ( AbstractRandomNumber ) - 인터페이스 네이밍 변경 ( AbleToProduce -> RandomNumberFactory )

leeheefull

제가 너무 늦었네요..ㅜㅜ
다른 분들이 열심히 리뷰 해주셔서 제가 할 부분이 많이 없네요!!
다음에는 열심히 리뷰하겠습니다 ㅎㅎ
수고하셨습니다

leeheefull · 2022-03-16T06:00:55Z

src/main/java/Position.java

+    public void moveOneStep() {
+        step++;
+    }


Position의 불변성을 유지하기 위해서 아래와 같이 구현하면,

public Position moveOneStep() { return new Position(this.step + 1); }

step도 불변해 질 수 있을 것 같은데 어떻게 생각하시나요!?

오! 좋은 방법이네요!
재원님이 피드백 주신 내용이랑 접목하면 완벽해지겠군여!
피드백 감사합니다! ㅎㅎ

- TryCount의 상수를 Race 객체에서 직접 사용하고 있던 부분 수정

- validation로직과 값세팅 순서를 잘못 세팅해놔서 제대로 Validation 체크가 이루어지지 않았음

- 클래스 네이밍 변경 : RandomNumber -> RangeableRandomNumber

darkant99 · 2022-03-19T08:04:52Z

src/main/java/Car.java

+        }
+    }
+
+    public void moveByStrategy() {


메서드명에 전략을 사용한다고 명시해줄 필요까지는 없을것 같습니다!

Class명에 자료구조 이름을 넣지 않는 것과 같은 원리군요.
뭔가 코딩하면서 입에서 나오는데로 네이밍을 하다보니 이렇게 된 것 같습니다.
이 부분은 그냥 move() 가 낫겠네요 ㅋㅋ 피드백 감사드립니다! ㅎㅎ

darkant99 · 2022-03-19T08:08:46Z

src/main/java/Cars.java

+
+    public Cars(List<Car> cars) {
+        validate(cars);
+        this.cars.addAll(cars);


cars를 직접 대입하지 않고 요소들의 참조를 각각 추가 해주셨네요 이유가 있을까요? 🤔

이 부분의 경우 원래 생성자의 인자로 List<Car>가 아니라 Car의 이름들을 담은 List<String> names 를 받다가
나중에 수정했는데,
인자값만 바꾸고 대입로직은 수정을 안했었네요 ㅜㅜ

피드백 감사합니다!!
코드 변경시 꼼꼼히 확인해야겠군여 ㅜㅜ

darkant99 · 2022-03-19T08:15:06Z

src/main/java/MoveByRandomNumberStrategy.java

@@ -0,0 +1,19 @@
+public class MoveByRandomNumberStrategy implements MoveStrategy{
+
+    private final int MOVABLE_CAR_MIN_NUMBER = 4;


static가 안붙었어요!

이 부분의 경우 해당 클래스 내부에서만 사용하는걸 의도해서 static을 붙이지 않았습니다.
중복되는 상수는 나중에 별도 클래스로 모을 계획이었죠. Constants.class 에 상수를 다 모아놓는 식으로 말이죠 ㅋㅋ

하지만 토비스트링 5장 347페이지를 보니
상수의 의미를 명확하게 전달 할 수 있는 Class에 static 상수로 넣어놓으니
테스트 코드에서까지 사용하는걸 보고서
클래스 마다 있는 상수를 static으로 가지고 있는게 낫겠다는 생각이 들었습니다. ㅋㅋ

이 부분은 수정해서 다시 올리겠습니다!
피드백 감사합니다!

static을 붙여도 private로 지정되있으면 클래스 내부에서만 사용 가능해요!

darkant99 · 2022-03-19T08:28:35Z

src/test/java/CarsTest.java

+                .map(s -> new Car(s, strategy))
+                .collect(Collectors.toList())
+        );
+


Collectors.collectingAndThen 메서드를 활용 해보세요 😃

오! 찾아보니 Collection을 만들고 나서 만들어진 Collection을 통해 메서드를 바로 호출하게 해주는 기능을 하는 메서드네요.

이런게 있는 줄 몰랐는데, 유용할 것 같습니다! ㅋㅋ

그런데 적용해보니 제 코드에는 적용하면 가독성이 떨어질 것 같다는 생각이 들어서 아직 적용을 안했습니다.

<적용전>

Cars cars = new Cars( inputCarNames.stream() .map(s -> new Car(s, strategy)) .collect(Collectors.toList()) ); cars.move();

<적용후>

Cars cars = inputCarNames.stream() .map(s -> new Car(s, strategy)) .collect(Collectors.collectingAndThen(Collectors.toList(), Cars::new)); cars.move();

뭔가 Cars 객체를 만든다는 내용이 <적용전>이 더 알기 쉽다고 생각이 들었습니다. ㅋㅋ

그래도 다른 부분들에서는 활용여지가 많은 메소드인것 같아요!
좋은 정보 감사합니다!! ㅎㅎ

darkant99 · 2022-03-19T08:33:11Z

src/test/java/CarTest.java

+    @MethodSource("provideRandomNumberFactory")
+    void moveByStrategy는_주어진_전략에_따라_자동차를_움직입니다(NumberFactory fakeRandomNumberFactory, Car expected) {
+
+        Car car = new Car(DEFAULT_CAR_NAME, new MoveByRandomNumberStrategy(fakeRandomNumberFactory));


주어진 전략에 따라 행동하는지 테스트를 하는거라면 의도와 달리 MoveByRandomNumberStrategy 테스트도 진행하고 있는것 같아요. 아래와 같이 변경해보면 어떨까요? 😇

Car car = new Car(DEFAULT_CAR_NAME, Position::moveOneStep);

단위테스트를 진행해야하는데, 통합테스트를 하고있었군여 ㅜ
MoveByRandomNumberStrategy 는 이미 테스트 코드로 검증을 했으니 여기서는 moveToStratrgy()의 고유 기능만을 테스트 하는게 맞을 것 같습니다.

public void moveByStrategy() { this.position = strategy.move(this.position); }

moveByStrategy()의 고유기능은 전략에서 반환한 Position 객체를 Car의 필드에 넣어주는 기능입니다.

그래서 저는 무조건 전진시키는 전략을 넣어주고 Position 객체가 바뀌었는지 검증하면 될꺼라 생각했는데..

Car car = new Car(DEFAULT_CAR_NAME, new AbsoluteMoveStrategy());

그러면 또 AbsoluteMoveStrategy.class 검증이 필요하고....
뭔가 결국은 AbsoluteMoveStrategy 로직까지 같이 검증하게되는거니까 단위테스트가 아니게 되고...
아아아아~

재원님이 예시로 들어주신 코드를 보니
Car를 생성하면서 이미 움직인 Position을 넣고 있는데

Car car = new Car(DEFAULT_CAR_NAME, Position::moveOneStep);

이 코드를 보면 굳이 moveToStrategy()에 대한 테스트 보다는 생성자에 대한 검증이라는 생각이 듭니다.

사실moveByStrategy()의 고유기능은 전략에서 반환한 Position 객체를 Car의 필드에 넣어주는 것 뿐이라면
굳이 테스트 코드로 검증을 해야 할까 라는 생각이 듭니다.

그래서 재원님이 적어주신 예시 코드로 생성자 테스트만 하면 충분할 것 같다는 생각이 드는데
어떻게 생각하시나요? @darkant99

예시는 메서드 참조를 사용해서 함수형 인터페이스를 주입하는 방식 입니다😇
'전략에서 반환한 Position 객체를 Car의 필드에 넣어주는 기능'을 테스트 하기 위해 예시를 들어드렸어요!

darkant99 · 2022-03-19T08:37:27Z

src/test/java/MoveByRandomNumberStrategyTest.java

+
+        assertThat(result).isEqualTo(expect);
+    }
+


MethodSource를 사용 함으로써 테스트 케이스가 정확히 어떤것인지 한눈에 들어오지는 않는것 같아요 😭

"여러개의 Parameter에 대한 테스트을 한번에 진행" + "기대값이 객체"

이 두가지를 모두 만족시키기 위해서 MethodSource를 사용했는데
확실히 한눈에 들어오지가 않는군요..

이 경우에는 여러가지 파라미터에 대한 테스트를 그냥 테스트 메서드에 다 적는 수 밖에 없을 것 같은데
그럴 경우 테스트 메서드가 너무 길어져서 그것또한 보기 불편할 것 같다는 생각이 듭니다. ㄷㄷ

@Test void move는_4이상의_수를_입력하면_1칸_움직인_Position을_반환합니다() { Position position = new Position(); MoveStrategy strategy = new MoveByRandomNumberStrategy(new FakeRandomAbstractNumberFactory(4)); Position result = strategy.move(position); Position expectedMove = new Position(1); assertThat(result).isEqualTo(expectedMove); position = new Position(); strategy = new MoveByRandomNumberStrategy(new FakeRandomAbstractNumberFactory(3)); result = strategy.move(position); Position expectedStop = new Position(); assertThat(result).isEqualTo(expectedStop); }

재원님께서는 이럴 경우 어떻게 테스트 하시나요?? @darkant99

저는 이경우 ParameterizedTest를 사용하지 않고 Test를 사용해 코드 내에서 직접 테스트할것 같아요.
중복을 허용 하더라도 '무엇을' 테스트 하는지가 한눈에 보이도록 테스트 코드를 작성하는게 좋은것 같습니다 😇

darkant99 · 2022-03-19T08:39:07Z

src/test/java/RaceTest.java

+    }
+
+    @Test
+    void start는_tryCount번_반복해서_자동차들을_Strategy에_따라_전진시킵니다() {


TryCount의 테스트를 RaceTest에서 진행하고 있네요🤔

여기서도 단위테스트가 아니라 통합테스트를 하고 있었군요.. ㅜㅜ
TryCount와 Race의 책임을 다시 분리하면서 이 부분도 수정하겠습니다.

여기서 질문이 있습니다.
현재 객체간의 메시지 구조는 아래와 같이 Race에서는 TryCount의 메서드를 그대로 호출하고 있습니다.
( 자신만의 고유 기능이 없어서 단위테스트로 분리할 기능이 없는 경우죠 )

public class Race { private TryCount tryCount; private Cars cars; public void start() { tryCount.startTryUpToTryCount(cars); } }

이런 구조 자체가 이형적이긴 하지만...
전략패턴을 사용할 경우, 이런 구조가 많이 보일 것 같습니다.

만약에 이런 구조의 메서드가 있을 경우 테스트를 진행하시나요?
( 제 생각에는 이런 구조의 메서드는 굳이 테스트를 할 필요가 없을 것 같다는 생각입니다. )
@darkant99 @DongGeon0908 @leeheefull @HwangHarim

굳이 테스트할 필요가 없는 코드는 역할이 명확하지 않은 경우가 많은것 같습니다.
저같은 경우 이럴때는 리팩터링을 고민 해봅니다.

darkant99 · 2022-03-19T08:43:04Z

src/main/java/AbstractNumber.java

@@ -0,0 +1,17 @@
+public abstract class AbstractNumber {
+


추상화 수준을 낮춘다면 조금 더 이해하기 쉬운 코드가 될것 같아요.

DongGeon0908

재원님께서 엄청나게 많은 리뷰를 진행해주셨군요!! 크흐~~

지금 달린 리뷰만 수정 진행하고, 다음 단계를 진행하는 건 어떨까요??

DongGeon0908 · 2022-03-18T10:04:25Z

src/main/java/AbstractNumber.java

@@ -0,0 +1,17 @@
+public abstract class AbstractNumber {


해당 클래스가 존재해야 하는 이유는 무엇일까요??
어떤 점을 염두하고 만드신건지 궁금해요!!!!

이 부분은 move()의 조건이 되는 숫자의 유형이 바뀌는 걸 염두했습니다.
move를 위해 주어지는 수가 0~9까지의 난수 일수도 있고, 짝수일 수도 있고, 홀수일 수도 있죠.

그래서 일단 숫자 라는 걸 추상객체로 만들고 ( AbstractNumber )
그 하위 클래스로는 조건이 부여된 숫자를 만드는 걸 생각했습니다.

move의 조건이 "0~9까지의 난수" 에서 "짝수"라고 바뀌어도 변경이 없게 말이죠.

DongGeon0908 · 2022-03-23T10:44:17Z

src/main/java/Position.java

+
+public class Position {
+
+    private final int ZERO = 0;


static으로 선언하지 않은 이유는 왜일까요?
다른 부분에서는 static으로 선언하신 것 같아서요!

저 변수를 해당 인스턴스 안에서만 사용하려고 static을 쓰지 않았습니다.
다른부분에 static이 들어간건 그 부분은 제가 실수로 고치지 않은 부분이었죠.. ㅎㅎ

근대 토비스프링 5장을 읽어보니 static 상수로 정의해놓고
테스트에서 그 상수를 사용하면 더 가독성이 좋더라구요!

그래서 테스트에서 사용할 상수의 경우 static 상수로 바꿀 생각입니다! ㅎㅎ

DongGeon0908 · 2022-03-23T10:45:36Z

리뷰를 남긴줄 알았는데, comment를 하지 않았네요ㅜㅠㅠㅠ

- 가변객체이더라도 불변성을 가지고 가야하는 부분에 대해서는 final을 기입하자

- AbstractNumber -> AbstractRandomNumber로 교체

DevRunner21 · 2022-03-24T12:20:12Z

안녕하세요!
마지막 PR 올려봅니다!
이제 슬슬 답지가 보고싶어서.. ㅎㅎ
디자인패턴 때문에 머리가 아픕니다 ㅜㅜ

언급해주신 내용에 대해서 수정한 내용을 올려봤습니다!
고민해보고 고치긴 했는데...
시간 괜찮으실때 리뷰 한번만 부탁드립니다!

반영사항은 아래와 같습니다.

자세한 내용이 필요한 부분은 코멘트로 남겨놓겠습니다.

피드백 반영사항

AbstractNumber의 추상화 수준을 낮춰봤습니다.
생성자에 중복 코드가 있던 부분을 this 키워드를 통해서 수정했습니다.
TryCount에서 Car를 움직이고 있던 부분에 대해서
저 역시도 너무 과한 책임이 부여된 것 같아서 수정했습니다.
Race, Cars, Car에서 Strategy에 대한 테스트까지 같이 하고 있던 부분을 재원님 말씀데로 함수형 인터페이스를 통해서 수정해봤습니다.
final이 빠진 부분에 대해서 "아무리 가변객체라도 불변을 지향해야 한다"는 말씀을 듣고 가변적일 필요가 없는 필드에 final 을 붙였습니다.
Class 내의 상수를 static 으로 바꾸고 테스트 코드에서 이를 활용하도록 수정했습니다.

DevRunner21 · 2022-03-24T12:24:24Z

src/main/java/Car.java

+    public Car(String name, int position, MoveStrategy strategy) {
+        this(name, strategy);
+        this.position = new Position(position);
    }

-    public Car(String name, int position, MoveStrategy strategy) {
+    public Car(String name, MoveStrategy strategy) {
        validateMoveStrategy(strategy);
        this.name = new Name(name);
-        this.position = new Position(position);
+        this.position = new Position();


재원님께서 언급주셨던 내용을 이렇게 고쳐봤는데,
언급해주신데로 잘 고친게 맞을까요? ㅎㅎ

테스트 코드 떄문에 어쩔수 없이 생성자가 2개 다 public으로 열려있어야 해서
생성자가 아예 1개로 줄지는 않았지만 중복코드는 제거했습니다!
@darkant99

public Car(Name name, MoveStrategy strategy, Position position) { this.name = name; this.strategy = strategy; this.position = position; } public Car(Name name, MoveStrategy strategy) { this(name, strategy, new Position(0)) }

이렇게 하면 조금 더 깔끔할것 같아요

아! 이렇게 바꿀수도 있군요!
왜 이 생각이 안났는지.. ㅜㅜ
피드백 감사합니다!!

DevRunner21 · 2022-03-24T12:27:16Z

src/main/java/TryCount.java

+//    public TryCount(int count, int currentCount) {
+//        this(count);
+//        this.currentCount = currentCount;
+//    }

    public TryCount(int count, int currentCount) {
        validate(count);
        this.count = count;
        this.currentCount = currentCount;
    }

+    public TryCount(int count) {
+        validate(count);
+        this.count = count;
+        this.currentCount = DEFAULT_CURRENT_TRY_COUNT;
+    }


이 부분도 생성자에 대한 중복코드를 제거하려고 시도했는데..
currentCount 필드를 불변적으로 가져가다 보니 이 부분은 중복코드를 제거하지를 못했습니다 ㅜㅜ

중복코드보다는 객체의 불변성을 가져가는게 더 나을꺼라고 생각하여
TryCount의 생성자는 일단 그대로 두었습니다.

이것도 아래처럼 할 수 있겠네용

public TryCount(int count, int currentCount) { validate(count); this.count = count; this.currentCount = currentCount; } public TryCount(int count) { this(count, DEFAULT_CURRENT_TRY_COUNT); }

DevRunner21 · 2022-03-24T12:47:25Z

기존 AbstractNumber의 추상화 수준을 AbstractRandomNumber로 낮춰봤습니다.

어차피 MoveByRandomNumberStrategy에서는 RandomNumber만 사용 할 텐데
거기서 굳이 Number라는 걸로 한번 더 추상화 할 필요는 없겠다는 생각이 들었습니다.

하지만 RandomNumber는 "범위가 있는 RandomNumber", "짝수인 RandomNumber", "홀수인 RandomNumber" 등으로
변경 될 수 있기 때문에 추상클래스를 두는 방식을 그대로 두었습니다.

여기서 굳이 RandomNumber를 또 한번 추상화 할 필요가 있을지에 대해서는 저도 고민해봤는데..

사실 전략패턴을 도입했는데,
그 전략(MoveStrategy)에 사용되는 필드( RandomNumber )를 또 한번 추상화 할 필요가 있을까에 대해서는
그냥 전략을 바꾸면 되지 유동적으로 바뀌는 전략에 또 추상화 필드를 가지는 건 좀 오버같다고 생각했습니다.

"랜덤숫자 별로 움직이는 전략"에서 "짝수마다 움직이는 전략"으로 바뀐다면
그냥 전략객체를 새로 생성하고 바꾸면되지 굳이 기존 전략객체를 그대로 쓸 필요는 없겠다는 생각이 들었습니다.

일단은 추상화를 고생해서 해놨는데
아예 없애기는 아까워서 그대로 두었습니다. ㅎㅎ

DevRunner21 · 2022-03-24T13:05:00Z

여기서 RandomNumber에 Factory를 사용할 필요가 있었나...에 대해서 고민해봤는데,
재원님 말씀대로 FactoryMhethod패턴이나 AbstractFactory 패턴이 어울리는 상황은 아닌것 같습니다.
지금 제가 사용한 방식은 그냥 생성자를 Factory라는 걸로 뺴놨을 뿐이죠.

RandomNumber를 Factory를 써서 생성한 이유는 아래와 같습니다.

테스트 시에 RandomNumber를 조작하기 위해서
RandomNumber에서 생성자를 통해 생성하면 Java Random 객체를 매 인스턴스마다 생성해야 하기 떄문에
별도의 클래스로 생성에 대한 책임을 분리 할 필요가 있었다.

뭔가 다른 좋은 방법이 있을 것도 같은데...
제 머리로는 아직 모르겠군요 ㅜㅜ
재성님의 해설을 보면 좀 보일 것 같아서 일단 고민은 여기까지 하는걸로 하려고 합니다 ㅎㅎ

darkant99 · 2022-03-29T05:43:11Z

src/main/java/RangeableRandomNumber.java

+        this.min = DEFAULT_MIN_RANDOM_NUMBER;
+        validateNumber(number);
+    }
+


public RangeableRandomNumber(int number) { this(number, DEFAULT_MAX_RANDOM_NUMBER, DEFAULT_MIN_RANDOM_NUMBER); }

😇

darkant99 · 2022-03-29T05:54:44Z

src/main/java/RangeableRandomNumber.java

@@ -0,0 +1,30 @@
+public class RangeableRandomNumber extends AbstractRandomNumber {
+


테스트 코드에서만 사용하고 있는 클래스인데, 테스트 패키지로 옮겨보면 어떨까요?

사실 Input, Output을 안만든상태라 그렇습니다.. ㅎㅎ
원래로직대로면 메인 비즈니스 로직에서 사용될 클래스입니다 ㅎㅎ

darkant99 · 2022-03-29T05:58:14Z

src/main/java/AbstractRandomNumber.java

@@ -0,0 +1,17 @@
+public abstract class AbstractRandomNumber {
+


AbstractRandomNumber보다 기본 자료형을 사용하는게 더 간단하지 않을까요? 🧐

이 부분을 클래스로 만든 이유는 RandomNumber의 종류가 더 확장될 수 있을 것 같아서 입니다.
무작위 RandomNumber가 있으면 현재 사용중인 범위가 있는 RandomNumber도 있고
짝수만 나오는 RandomNumber 등등 다양한 확장이 가능할 것 같아서 추상화를 한번 거치게 되었습니다.

이 부분은 좀 너무 많이 나간 확장일까요??

AbstractRandomNumber와 AbstractRandomNumberFactory의 역할이 계속해서 헷갈리네요.
둘중에 하나만 존재해도 될것 같아요.🧐

darkant99 · 2022-03-29T05:59:15Z

src/main/java/AbstractRandomNumberFactory.java

@@ -0,0 +1,5 @@
+public abstract class AbstractRandomNumberFactory {
+


abstract class보단 interface가 더 어울릴것 같아요 😇

동의합니다!
저의 경우에는 생성자로 무언가를 강제하는 수단이 없다면 가능한 인터페이스를 쓰는 것을 선호합니다.

이 부분의 경우 기능 명세만 되어있다보니 인터페이스가 어울리겠군여 ㅋㅋ
사실 추상 팩토리 패턴을 적용하면서
예제코드가 상속구조를 통해 구현이 되었다보니 무지성으로 따라하게 되었네요 ㅎㅎ
이 부분 수정하겠습니다!

darkant99 · 2022-03-29T06:01:41Z

src/main/java/Race.java

+
+    public void start() {
+        while (!this.tryCount.isComplete()) {
+            cars.move();


부정보단 긍정이 더 잘 읽힌다고 합니다 😎
한번 시도 해보시면 재미있을것 같아요.

아! 예전에 피드백 주셨던 내용이군요!
isNotComplete() 같은 형태가 확실히 술술 읽히겠군요!
수정하겠습니다!

darkant99 · 2022-03-29T06:07:16Z

src/test/java/CarsTest.java

+    }
+
+    @Test
+    void move_메서드는_주어진_전략에_따라_Car들을_전진시킵니다() {


공백줄로 개념을 분리 해보세요.
List를 ArrayList로 감싸는 실수도 발견해서 제가 수정 해봤어요 😎

// given MoveStrategy fakeAbsoluteMoveStrategy = Position::moveOneStep; List<String> inputCarNames = Arrays.asList("a", "b", "c", "d", "e"); // when Cars cars = new Cars( inputCarNames.stream() .map(s -> new Car(s, fakeAbsoluteMoveStrategy)) .collect(Collectors.toList()) ); cars.move(); // then Cars expertCars = new Cars( inputCarNames.stream() .map(s -> new Car(s, 1, fakeAbsoluteMoveStrategy)) .collect(Collectors.toList()) ); assertThat(cars).isEqualTo(expertCars);

하다보니 이런 디테일을 항상 놓치고 가는군요 ㅜㅜ
재원님 덕분에 한번 더 짚고갑니다! 감사합니다! ㅎㅎ

darkant99 · 2022-03-29T06:09:31Z

src/test/java/NameTest.java

+
+    private final int MAX_NAME_LENGTH = Name.MAX_NAME_LENGTH;
+
+    @Test


@ParameterizedTest를 사용하기 적절한 상황인것 같아요 😍

오! 이 부분을 놓쳤군요!
수정해서 반영하겠습니다! ㅎㅎ

darkant99 · 2022-03-29T06:14:48Z

요새 정신이 없어 리뷰가 늦었네요 😢
테스트 코드에서 많은 고민을 하신게 느껴집니다 👍

DongGeon0908

고생하셨어요!!
이제 진짜진짜 다음 과정을 진행하시면 될 것 같슴다!!

DongGeon0908 · 2022-03-29T07:29:33Z

src/main/java/AbstractRandomNumber.java

@@ -0,0 +1,17 @@
+public abstract class AbstractRandomNumber {


해당 클래스를 만드신 이유는 RandomNumber를 생성하는 로직이 앞으로 더 추가될 수 있기 때문인가요?

네! 그렇습니다!
자동차의 Move 기능을 전략패턴으로 사용한 만큼
그 전략이 조건이 되는 RandomNumber 또한 다양한 조건들로 확장이 가능하도록 설계해봤습니다.

DongGeon0908 · 2022-03-29T07:30:13Z

src/main/java/AbstractRandomNumberFactory.java

@@ -0,0 +1,5 @@
+public abstract class AbstractRandomNumberFactory {
+


동의합니다!
저의 경우에는 생성자로 무언가를 강제하는 수단이 없다면 가능한 인터페이스를 쓰는 것을 선호합니다.

DongGeon0908 · 2022-03-29T07:31:37Z

src/main/java/Cars.java

+        if (Objects.isNull(cars)) {
+            throw new IllegalArgumentException("cars는 Null일 수 없습니다.");
+        }
+        if (cars.isEmpty()) {
+            throw new IllegalArgumentException("cars는 Enpty 일 수 없습니다.");


넘어오는 데이터에 대한 방어로직일까요??

네! 맞습니다!
이번에 생성자 마다 되도록이면 validationCheck를 확실하게 하고 싶어서 ㅎㅎ

DongGeon0908 · 2022-03-29T07:34:24Z

src/main/java/RangeableRandomNumberFactory.java

+
+    @Override
+    public AbstractRandomNumber produce() {
+        return new RangeableRandomNumber(random.nextInt(max+min) + min, max, min);


인자로 들어가는 값들이 어떤 역할을 하는지 명확하게 명시하는게 좋을 것 같아요!
현재 상태에서는 구체적으로 random.nextInt(max+min) + min, max, min 해당 값들이 어떤 역할을 하는지 모르겠어요!

max, min 보다는 random.nextInt(max+min) + min 요부분을 별도의 변수로 네이밍 하는게 좋겠군요.
별생각 없이 코딩했는데, 가독성을 생각하면 확실히 더 좋아질 것 같습니다!
굳굳~!~!

DevRunner21 added 11 commits March 14, 2022 18:16

docs: ReadMe를 작성하라

97202cd

feat: 자동차 이름 생성 기능을 구현하라

1da6266

- "자동차 이름"은 5자를 초과 할 수 없다.

feat: Try 생성 기능을 구현하라

c502a69

- 시도 횟수는 음수 일 수 없다.

feat: RandomNumber 생성 로직을 구현하라

6b068d3

- 0 이상 9 이하의 랜덤 값을 받는다.

feat: 자동차를 움직이는 기능을 구현하라

f21aefc

refactor: Car의 생성인자로 객체가 아닌 Literal 타입으로 수정하라

57472e2

docs: ReadMe 내용을 수정하라

6567a14

feat: 모든 자동차를 이동시키는 기능을 구현하라

6533332

- 모든 자동차 이동 기능 구현 - RandomNumber 생성을 위한 Factory Class 구현 - 테스트를 위해 Fake RandomFactory 생성

refactor: 시도 횟수 저장을 위해 만들었던 Try Class를 TryCount Class로 변경하라

aaf7f35

fix: 잘못 표기된 상수명을 변경하라

9687f38

feat: "시도 횟수" 만큼 자동차들을 움직이는 기능을 구현하라

6221dff

DevRunner21 requested review from darkant99, DongGeon0908, leeheefull and HwangHarim March 14, 2022 20:08

DevRunner21 self-assigned this Mar 14, 2022

DevRunner21 commented Mar 14, 2022

View reviewed changes

DongGeon0908 added the review label Mar 15, 2022

DongGeon0908 approved these changes Mar 15, 2022

View reviewed changes

darkant99 reviewed Mar 15, 2022

View reviewed changes

leeheefull reviewed Mar 16, 2022

View reviewed changes

DevRunner21 added 6 commits March 16, 2022 15:49

refactor: 잘못된 상수 사용을 수정하라

30f7917

- TryCount의 상수를 Race 객체에서 직접 사용하고 있던 부분 수정

fix: RandomNumber 범위값(MAX, MIN) 세팅 로직을 수정하라

870ab11

- validation로직과 값세팅 순서를 잘못 세팅해놔서 제대로 Validation 체크가 이루어지지 않았음

refactor: 잘못 세팅된 접근제어자를 수정하라

b923bec

refactor: RandomNumber 로직을 FactoryMethod 패턴으로 변경하라

b9400bc

refactor: Car의 move 기능을 전략패턴으로 수정하라

2f80191

refactor: Car를 가변객체로 수정하고 Position을 불변객체로 수정하라

83f4dca

- 클래스 네이밍 변경 : RandomNumber -> RangeableRandomNumber

darkant99 reviewed Mar 19, 2022

View reviewed changes

DongGeon0908 approved these changes Mar 23, 2022

View reviewed changes

DevRunner21 added 6 commits March 24, 2022 18:01

refactor: TryCount에서 직접 Car를 move 하고 있던 역할을 Race에게 옮겨라

2ca4abb

refactor: final이 빠진 부분에 final을 기입하라

bf114f9

- 가변객체이더라도 불변성을 가지고 가야하는 부분에 대해서는 final을 기입하자

refactor: 생성자 중복코드에 대해서 this키워드를 이용해서 중복코드를 제거하라

a25c6a9

refactor: AbstractNumber의 추상화 수준을 낮춰라

6455b38

- AbstractNumber -> AbstractRandomNumber로 교체

refactor: 클래스 내의 상수를 static 상수로 변경하라

8cb2640

refactor: 함수형인터페이스를 활용하여 Race, Cars, Car 단위테스트를 Strategy에서 독립적으로 수정하라

38a82c8

DevRunner21 commented Mar 24, 2022

View reviewed changes

darkant99 reviewed Mar 29, 2022

View reviewed changes

DongGeon0908 approved these changes Mar 29, 2022

View reviewed changes

		@@ -0,0 +1,7 @@
		import java.util.ArrayList;

		public interface AbleToProduceRandomNumber {


		public class Car {

		public final int MOVE_PERMISSION_MIN_NUMBER = 4;

		private static final int MAX_NAME_LENGTH = 5;
		private static final String NAME_LENGTH_OVER_MESSAGE = "자동차 이름은 5자를 초과할 수 없다.";

		@@ -0,0 +1,19 @@
		public class MoveByRandomNumberStrategy implements MoveStrategy{

		private final int MOVABLE_CAR_MIN_NUMBER = 4;

		@@ -0,0 +1,30 @@
		public class RangeableRandomNumber extends AbstractRandomNumber {

		@@ -0,0 +1,17 @@
		public abstract class AbstractRandomNumber {

		@@ -0,0 +1,5 @@
		public abstract class AbstractRandomNumberFactory {


		private final int MAX_NAME_LENGTH = Name.MAX_NAME_LENGTH;

		@Test

자동차 경주 게임 PR #1

Are you sure you want to change the base?

자동차 경주 게임 PR #1

Conversation

DevRunner21 commented Mar 14, 2022 • edited Loading

PR 포인트

고민사항

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

DongGeon0908 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

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

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

darkant99 commented Mar 15, 2022

leeheefull 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

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

DevRunner21 Mar 21, 2022 • 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

DongGeon0908 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

DongGeon0908 commented Mar 23, 2022

DevRunner21 commented Mar 24, 2022

피드백 반영사항

DevRunner21 commented Mar 14, 2022 •

edited

Loading

DevRunner21 Mar 21, 2022 •

edited

Loading