박순용 - 자동차 경주 #2

gzcxadfzc · 2023-01-14T18:07:56Z

-중점적으로 생각한 것

최대한 model과 view가 서로 직접 호출하지 않도록
기능을 최대한 쪼개서 클래스 생성
테스트코드를 단계적으로 만들어 나가기
원시타입 감싸기

-어려웠던 것
-controller로 분류한 클래스들간의 의존성이 높아지는 것을 어떻게 막을 수 있을지 모르겠음
-model 과 controller 기능 분리

repeatUpdateStatus 추가 currentStatus 삭제 테스트코드 수정

Byeongju-Kong

불필요하게 public인 메서드가 존재합니다! 저번에 몇번이나 강조드렸는데, 아쉽네요~!
읽기 좋은 메서드 순서가 아닌 것 같아요. 호출되는 순서대로 위에서 아래로 메서드를 정렬해주세요~
개행 부분도 신경써주세요. 더 이상 클린코드 측면에서의 리뷰를 드리기 보다는 설계, OOP 리뷰를 드려야 순용님이 더 성장하실 수 있습니다. 이런 부분은 신경써주셨으면 좋겠어요.

객체지향의 사실과 오해 책을 사셨다고 들었는데요. 읽으시면서 리팩토링하시면 좋을 것 같아요. 꼭 읽으시면서 리팩토링하시길 바랍니다!

Byeongju-Kong · 2023-01-15T06:39:13Z

src/main/java/gmbs/model/Car.java

+    public Car move(int position) {
+        Car car = new Car(this.name);
+        car.position = position;
+        return car;
+    }


Suggested change

public Car move(int position) {

Car car = new Car(this.name);

car.position = position;

return car;

}

public Car move(int position) {

return new Car(name, position);

}

해당 메서드는 자동차가 전진하는 로직이예요. 지금 position이라는 값을 받아서, position이 바뀐 새로운 Car객체를 생성해서 반환하는데요. 불변을 사용하려는 시도는 좋았습니다.
하지만, 현재 move를 할 때, 외부에서 변경될 position 값을 받기만 하네요.
객사오를 조금 읽으시면 아시겠지만, 객체지향의 세계에서는
행동의 주체가 본인이 되어야 합니다. 본인이 전진할지 말지를 결정하는 것이 좋아보여요.

현재 4이상이면 전진이라는 조건이 있는데, 4 이상이면 전진하고 아니면 전진하지 않는다라는 행동의 주체가 Car가 되어야 한다는 말입니다, .
고민해보시면 좋을 것 같습니다~

Byeongju-Kong · 2023-01-15T06:42:03Z

src/main/java/gmbs/controller/CarRaceOperator.java

+    private List<String> scanCarNames() {
+        List<String> carNames;
+        while (true) {
+            try {
+                carNames = inputManager.carNameInput(userInput.userInput());
+                break;
+            } catch (IllegalArgumentException e) {
+                System.out.println(e.getMessage());
+            }
+        }
+        return carNames;
+    }
+
+    private int scanRepetition() {
+        int repetition;
+        while (true) {
+            try {
+                repetition = inputManager.repetitionInput(userInput.userInput());
+                break;
+            } catch (IllegalArgumentException e) {
+                System.out.println(e.getMessage());
+            }
+        }
+        return repetition;
+    }


depth 2 입니다!!!!!!!!!!!!!!!!!!!!!!!!!!! 1로 변경해주세요!

Byeongju-Kong · 2023-01-15T06:42:39Z

src/main/java/gmbs/controller/CarRaceOperator.java

+        List<String> carNames;
+        int repetition;


먼저 선언해놓을 필요가 있나요?

Byeongju-Kong · 2023-01-15T06:45:26Z

src/main/java/gmbs/controller/CarManager.java

+public class CarManager {
+
+    private static final CarPositionGenerator positionGenerator = new CarPositionGenerator();
+
+    public List<Car> createCar(List<String> carNames) {
+        List<Car> cars = new ArrayList<>();
+        for (String name : carNames) {
+            cars.add(new Car(name));
+        }
+        return cars;
+    }
+
+    public List<Car> updateStatus(List<Car> currentStatus) {
+        List<Car> updateStatus = new ArrayList<>();
+        for (Car car : currentStatus) {
+            updateStatus.add(positionGenerator.updatePosition(car));
+        }
+        return updateStatus;
+    }
+}


자동차를 생성하고, 전진시키는 로직을 수행하고 있네요.
view에 대한 의존도 없는데 controller보다는 model에 위치하는게 좋은 것 같아요.
어떤 이유로 controller로 위치시켰나요?

또한, 해당 객체가 List carNames를 받아서 List를 반환하는 것도 방법이지만
List 객체를 가지고 있으면 안될까요? 왜 List를 생성하고 전진한 List를 반환하는 객체로 설계했나요?

Byeongju-Kong · 2023-01-15T06:46:53Z

src/main/java/gmbs/controller/InputManager.java

+        List<String> carNames = new ArrayList<>(Arrays.asList(userInput.split(SPLIT_VALUE)));
+        if (!validator.isValidNames(carNames)) {
+            nameOverflowException();
+        }
+        return carNames;


이름에 대한 검증을 여기서 하고 있네요. 이렇게 하는 것도 방법이겠지만, 꼭 입력에 대한 검증을 입력을 하는 순간 바로 해야한다는 생각을 버리셔도 됩니다. Car 객체에 name이라는 필드가 있는데요. CarName이라는 객체를 만들고 생성하면서 이름에 대한 검증을 하는 건 어떨까요?

Byeongju-Kong · 2023-01-15T07:24:02Z

src/test/java/gmbs/model/ForwardStatusGeneratorTest.java

+    public void randomCarStatus(int rangeStart,int rangeEnd) {
+        HashSet<Integer> actual = new HashSet<>();
+        HashSet<Integer> expected = new HashSet<>();
+        int testCase = 1000;


테스트가 1000번째까지 성공하다가 1001번째에서 실패한다면 1000이라는 숫자는 유의미한 숫자인가요?

Byeongju-Kong · 2023-01-15T07:24:57Z

src/test/java/gmbs/model/ForwardStatusGeneratorTest.java

+        for (int i = 0; i < testCase; i++) {
+            if(statusGenerator.isMoveForward(rangeStart, rangeEnd, referenceCondition)) {
+                trueCount ++;
+            }
+        }        float actual = trueCount/testCase;
+        float expected = (float)(rangeEnd+1-referenceCondition)/(float)(rangeEnd-rangeStart+1);
+        assertThat(actual).isCloseTo(expected, Percentage.withPercentage(10));
+    }


Suggested change

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

if(statusGenerator.isMoveForward(rangeStart, rangeEnd, referenceCondition)) {

trueCount ++;

}

} float actual = trueCount/testCase;

float expected = (float)(rangeEnd+1-referenceCondition)/(float)(rangeEnd-rangeStart+1);

assertThat(actual).isCloseTo(expected, Percentage.withPercentage(10));

}

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

if (statusGenerator.isMoveForward(rangeStart, rangeEnd, referenceCondition)) {

trueCount++;

}

}

float actual = trueCount / testCase;

float expected = (float) (rangeEnd + 1 - referenceCondition) / (float) (rangeEnd - rangeStart + 1);

assertThat(actual).isCloseTo(expected, Percentage.withPercentage(10));

이건 대체 무슨 정렬일까요... 다음부터 이런 리뷰는 없도록 신경써주세요.

Byeongju-Kong · 2023-01-15T07:27:13Z

src/test/java/gmbs/model/InputValidatorTest.java

+                Arguments.of(Arrays.asList("name1", "name2", "name33"), false),
+                Arguments.of(Arrays.asList("name1", "name2", "name3"), true),
+                Arguments.of(Arrays.asList("name1"), true),
+                Arguments.of(Arrays.asList("  "), false),
+                Arguments.of(Arrays.asList(""), false),
+                Arguments.of(Arrays.asList(), false)


List.of 으로 모두 변경하면 좋을 것 같네요~
List.of과 Arrays.asList의 차이를 알아보아요~

Byeongju-Kong · 2023-01-15T07:27:48Z

src/test/java/gmbs/model/InputValidatorTest.java

+
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+
+public class InputValidatorTest {


테스트 클래스에는 public이 존재하지 않아도 됩니다. 모든 테스트 클래스의 public 접근제어를 삭제해주세요~

Byeongju-Kong · 2023-01-15T07:28:10Z

src/test/java/gmbs/model/InputValidatorTest.java

+    @ParameterizedTest
+    @DisplayName("리스트 원소들이 모두 5글자 이하이면 true를 반환하는지 확인한다")
+    @MethodSource("generateNames")
+    public void isValidNames(List<String> inputNames, boolean expected) {


테스트 메서드에는 public이 존재하지 않아도 됩니다. 모든 테스트 메서드의 public 접근제어를 삭제해주세요~

Byeongju-Kong

순용님~ 코드가 훨씬 깔끔해지고 전 코드보다 객체지향적으로 변경되었어요! 좋습니다!
테스트 코드들도 잘 작성해주셨어요~~~ 몇가지 리뷰 남겨두었으니 반영하고 리뷰 요청 해주세요~

Byeongju-Kong · 2023-01-19T06:11:08Z

src/main/java/gmbs/controller/CarRaceOperator.java

+
+    private static final Display display = new Display();
+    private static final UserInput userInput = new UserInput();
+    private static final String SPLIT_VALUE = ",";


상수는 클래스의 맨 위에 위치시켜주는게 좋을 것 같아요! 저는 상수. 변수, 생성자, 메서드 순으로 선언하는데요.
위 순서가 아니더라도, 본인이 생각하기에 좋은 순서를 정해서 모든 코드들에 순서를 일치시키는 것이 좋아보입니다~

Byeongju-Kong · 2023-01-19T06:17:18Z

src/main/java/gmbs/controller/CarRaceOperator.java

+        int repetition;
+        try {
+            repetition = Integer.parseInt(new RepetitionNumber(userInput.userInput()).number());
+        } catch (IllegalArgumentException e) {
+            System.out.println(e.getMessage());
+            repetition = scanRepetition();
+        }
+        return repetition;


RepetitionNumber 객체에 사용자 입력 값을 받아서, 유효성 검증을 한 후에 바로 number()를 통해 int 값을 빼는데요.
RepetitionNumber 객체를 계속 사용하면 어떨까요? 아래 repeatRace 함수에서도 꺼낸 int 값을 사용하지 않고, RepetitionNumber 객체를 사용하는 방식으로요! 생성된 객체를 그대로 사용하는 것이 더 좋아보입니다~!

지금 읽고 계신 객체지향의 사실과 오해에 값을 꺼내서 사용하지말고, 해당 객체에게 요청을 하자고 나와있지 않나요~?😀

Byeongju-Kong · 2023-01-19T06:22:16Z

src/main/java/gmbs/controller/CarRaceOperator.java

+import java.util.Arrays;
+import java.util.List;
+
+public class CarRaceOperator {


opreate 메서드를 메서드들 중에 가장 위로 위치시키고, 그 아래로는 operate 메서드 내부에서 호출되는 순서대로 정렬하면 좋을 것 같아요~ 사람은 위에서 아래로 읽기 때문에 훨씬 코드를 읽기가 쉬워지겠죠? 모든 클래스에 반영하시면 좋을 것 같습니다~

Byeongju-Kong · 2023-01-19T06:23:58Z

src/main/java/gmbs/controller/CarRaceOperator.java

+    private static final String SPLIT_VALUE = ",";
+    private static final RandomNumberGenerator generator = new RandomNumberGenerator(0, 9);
+
+    private RacingCars racingCars;


해당 필드 없이 충분히 메서드 내부 변수들로 로직을 진행할 수 있는 것으로 보여요!
또한, CarRaceOperator는 생성자에서 RacingCars가 초기화되지 않기 떄문에(생성자가 없기 때문에) RacingCars라 null인 시점이 존재하네요. null을 최대한 피하는 것이 좋습니다~!

Byeongju-Kong · 2023-01-19T06:24:58Z

src/main/java/gmbs/controller/CarRaceOperator.java

+    private List<String> requestCarNames() {
+        List<String> carNames;
+        try {
+            carNames = splitInput(userInput.userInput());
+        } catch (IllegalArgumentException e) {
+            System.out.println(e.getMessage());
+            carNames = requestCarNames();
+        }
+        return carNames;
+    }


Suggested change

private List<String> requestCarNames() {

List<String> carNames;

try {

carNames = splitInput(userInput.userInput());

} catch (IllegalArgumentException e) {

System.out.println(e.getMessage());

carNames = requestCarNames();

}

return carNames;

}

private List<String> requestCarNames() {

try {

return splitInput(userInput.userInput());

} catch (IllegalArgumentException e) {

System.out.println(e.getMessage());

return requestCarNames();

}

}

위와 같이 변경하면 더 깔끔한 것 같은데 어떻게 생각하세요~?

좋은 방법인것 같습니다

depth를 줄이기 위해 재귀를 통하여 함수를 호출하는 방법은 어떻게 생각하시나요
재귀적 호출을 지양하는 것이 좋다고 생각하는데 미션의 요구사항을 지키기 위해서 부득이하게 이용햇습니다
다른 좋은 방법을 찾아봐야겠어요

Byeongju-Kong · 2023-01-19T06:40:00Z

src/test/java/gmbs/model/CarNameTest.java

+    @ParameterizedTest
+    @DisplayName("이름이 1~5자가 아니면 예외가 발생하는지 확인한다")
+    @ValueSource(strings = {"", "123456"})
+    void CarName_ExceptionByNameOverflow(String name) {
+        assertThatThrownBy(() -> new CarName(name))
+                .isInstanceOf(IllegalArgumentException.class).hasMessage("[Error] 이름은 1자 이상 5자 이하여야 한다.");
+    }


Suggested change

@ParameterizedTest

@DisplayName("이름이 1~5자가 아니면 예외가 발생하는지 확인한다")

@ValueSource(strings = {"", "123456"})

void CarName_ExceptionByNameOverflow(String name) {

assertThatThrownBy(() -> new CarName(name))

.isInstanceOf(IllegalArgumentException.class).hasMessage("[Error] 이름은 1자 이상 5자 이하여야 한다.");

}

@ParameterizedTest

@DisplayName("이름이 1~5자가 아니면 예외가 발생하는지 확인한다")

@ValueSource(strings = {"", "123456"})

void CarName_ExceptionByNameOverflow(String name) {

assertThatThrownBy(() -> new CarName(name))

.isInstanceOf(IllegalArgumentException.class)

.hasMessage("[Error] 이름은 1자 이상 5자 이하여야 한다.");

}

한줄에 .은 하나만~

Byeongju-Kong · 2023-01-19T06:41:21Z

src/test/java/gmbs/model/CarTest.java

+    @DisplayName("4 이상의 수를 넣으면 carposition에 1이 더해진다")
+    @CsvSource(value = {"4, 0", "5, 1"})
+    void moveByCondition(int input, int expectedPositionDifference) {
+        Car testCar = new Car("test");


testCar라는 변수명에 대해 이야기하고 싶은데요. 이 차가 test를 위한 Car인게 중요할까요..?
여기는 누가 봐도 테스트이기 떄문에 test를 위한 Car인걸 알 수 있다고 생각합니다! 그냥 car로 해도 무방할 것 같은데,
testCar로 명명하신 이유가 있나요~?

좀 더 명확히 하고 싶어서 그랫습니다

이런 식의 명명은 오히려 더 이해하기 힘들게 만드는 걸까요?

굳이 test를 명시하지 않아도 테스트에서 사용하기 때문에 이해가 될 것 같습니다~

Byeongju-Kong · 2023-01-19T06:42:27Z

src/test/java/gmbs/model/CarNameTest.java

+    @ParameterizedTest
+    @DisplayName("이름에 공백이 있으면 예외가 발생하는지 확인한다")
+    @ValueSource(strings = {" ", "1 6"})
+    void CarName_ExceptionByBlank(String name) {
+        assertThatThrownBy(() -> new CarName(name))
+                .isInstanceOf(IllegalArgumentException.class).hasMessage("[Error] 공백 금지");
+    }
+}


위의 테스트 코드가 name값과 예외 메시지만 다르고 나머지는 다 똑같은데 테스트 메서드를 두개로 분리하신 이유는 뭔가요~?
순용님의 의도를 물어보는 것이니, 잘못됐다고 생각하지 마시고 답변해주세요~

각각의 예외조건에 따라 메시지가 다르게 나오는지 확인하려고 했습니다

검증 순서 때문에 두 예외조건을 모두 만족한다면 먼저 검증하는 쪽의 예외가 먼저 터지는데 이런 식으로 되어도 괜찮은지 궁금합니다

아니면 그냥 모두 하나의 예외로 처리하는 것이 좋을까요?

검증 순서에 따라서 발생하는 예외가 달라지지 않도록 할 수는 없을까요?

Byeongju-Kong · 2023-01-19T06:46:18Z

src/test/java/gmbs/model/CarTest.java

+class CarTest {
+
+    @ParameterizedTest
+    @DisplayName("4 이상의 수를 넣으면 carposition에 1이 더해진다")


carPosition에 1이 더해진다 라고 작성해놓으셨는데요.
필드이름이 바뀐다면 @DisplayName도 함께 바꿔줘야 할 것 같아요.
이렇게 하면 필드이름과 DisplayName이 엮이는데요. 다르게 표현할 수 있는 방법이 있다면 엮지 않는 것이 좋습니다.
변경에 대한 부수효과를 최소화하는 것이 좋아요~!

Byeongju-Kong · 2023-01-19T06:47:20Z

src/test/java/gmbs/model/RacingCarsTest.java

+
+class RacingCarsTest {
+
+    private final List<String> names = Arrays.asList("레드불", "페라리", "메르세데스");


Arrays.asList를 사용하셨네요~
ArrayList로 생성할 수도 있고, List.of로도 생성할 수 있는데요. 어떤 차이가 있을까요? 알아보시고 노션이나 블로그에 정리해서 다음 시간에 공유해주세요~

gzcxadfzc added 30 commits January 12, 2023 23:27

docs : readme 필요한 기능 기술

8915e01

feat : 메인 클래스, car 객체 생성

63b094c

feat : 자동차 전진 상태 생성기 기능 추가

71af93f

Test : ForwardStatusGeneratorTest 테스트코드 추가

ff268b1

fix : isMoveForward 메서드 기준 이상의 난수가 나오면 true 반환

19c477e

feat : car클래스 메서드 추가

53e92e9

feat : race클래스 추가

a73db1b

refactor : carPostionGenerator 로 클래스 이름 변경

b2e4e40

feat : CarManager 클래스 생성

93e0ead

Test : CarManagerTest 테스트코드 추가

7ff81a2

fix : updatePosition에서 같은 값이 계속 할당되는 것 수정

0081e71

feat : CarManager 기능 업데이트

410e553

repeatUpdateStatus 추가 currentStatus 삭제 테스트코드 수정

feat : CarStatusReader 기능 추가

d17cc15

test : CarStatusReaderTest 추가

5b9a954

refactor : CarStatusReader 메서드 반환타입 변경

43a5da4

refactor : CarStatusReader 메서드 반환타입 변경

4268f1c

refactor : carManager 불필요한 메서드 삭제

2fd6a47

feat : RaceManager, Display 클래스 추가

ea756a9

feat : RaceManager startRace 메서드 추가

6adda7a

feat : inputValidator 클래스 추가,테스트 추가

2d9eec4

fix : inValidNames 메서드 빈 리스트를 넘길 때 false를 반환하게 수정

2c8c70f

feat : isNaturalNumber 메서드 추가

6b83254

fix : 공백 입력을 검증하지 못함 수정

8b374bb

refactor : 메서드명 변경

daca70f

feat : inputManager 클래스 추가

99db019

feat : Display 클래스 showInputDisplay 메서드 추가

d1aad69

feat : userInput 클래스 추가

1be5af9

feat : userInput 메서드 추가

adbb41d

feat : CarRaceOperator 클래스 추가

6166ebb

test : InputValidatorTest 테스트 케이스 추가

f438be9

gzcxadfzc added 4 commits January 15, 2023 02:25

fix : inputValidator 검증값 정규식 수정

70793d9

style : 코드 수정

ca5f3f8

style : 코드 수정

001c658

refactor : 불필요한 클래스 삭제

db1d02d

Byeongju-Kong requested changes Jan 15, 2023

View reviewed changes

gzcxadfzc added 14 commits January 17, 2023 14:51

docs : gitkeep 삭제

5f43565

refactor : carName 클래스 분리에 따른 리팩터

68c1b86

test : carName 예외 테스트 추가

ea27d7c

test : repitionNumber 예외 테스트 추가

6232c31

fix : 반복횟수 0 예외처리

b98cf79

refactor : 불필요한 클래스 삭제

95ebb9d

fix : 반복횟수 검증 조건 추가

37441d4

refactor : 클래스 통합 및 분리

07fa6b8

refactor : 변수명 변경

5a9d8b9

refactor : 클래스 통합 및 분리

56c447f

refactor : 클래스 이름 변경

f374007

feat : 숫자생성 인터페이스 추가

b467473

refactor : 인터페이스 추가에 따른 리팩터링

1e2d6ae

test : RacingCarsTest 추가

85b1e39

Byeongju-Kong requested changes Jan 19, 2023

View reviewed changes

gzcxadfzc added 10 commits January 24, 2023 16:25

refactor : RepetitionNumber 사용 리팩터링

519c800

fix : car 이동 조건 수정

22f8b37

style : 코드 수정

e55d809

refactor : 난수생성 리팩터링

bb54f08

test : carTest 구현 수정

d40658d

refactor : carName 검증 순서 변경

308b0f0

style : 코드 변경

2860da5

refactor : vo사용 리팩터

db92cb8

test : 리패터링에 따른 테스트코드 수정

ce77c53

style : 불필요한 코드 삭제

88a7774

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

박순용 - 자동차 경주 #2

박순용 - 자동차 경주 #2

gzcxadfzc commented Jan 14, 2023

Byeongju-Kong left a comment

Byeongju-Kong Jan 15, 2023

Byeongju-Kong Jan 15, 2023

Byeongju-Kong Jan 15, 2023

Byeongju-Kong Jan 15, 2023

Byeongju-Kong Jan 15, 2023

Byeongju-Kong Jan 15, 2023

Byeongju-Kong Jan 15, 2023

Byeongju-Kong Jan 15, 2023

Byeongju-Kong Jan 15, 2023

Byeongju-Kong Jan 15, 2023

Byeongju-Kong Jan 15, 2023

Byeongju-Kong Jan 15, 2023

Byeongju-Kong left a comment

Byeongju-Kong Jan 19, 2023

Byeongju-Kong Jan 19, 2023

Byeongju-Kong Jan 19, 2023

Byeongju-Kong Jan 19, 2023

Byeongju-Kong Jan 19, 2023

gzcxadfzc Jan 23, 2023

Byeongju-Kong Jan 19, 2023

Byeongju-Kong Jan 19, 2023

gzcxadfzc Jan 23, 2023

Byeongju-Kong Jan 25, 2023

Byeongju-Kong Jan 19, 2023

gzcxadfzc Jan 23, 2023

Byeongju-Kong Jan 25, 2023

Byeongju-Kong Jan 19, 2023

Byeongju-Kong Jan 19, 2023


		import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

		public class InputValidatorTest {


		class RacingCarsTest {

		private final List<String> names = Arrays.asList("레드불", "페라리", "메르세데스");

박순용 - 자동차 경주 #2

Are you sure you want to change the base?

박순용 - 자동차 경주 #2

Conversation

gzcxadfzc commented Jan 14, 2023

Byeongju-Kong 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

Byeongju-Kong 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