TDD 기반의 두번째 시도 - 코드리뷰용 by ca1af · Pull Request #2 · CODE-CLEANERS/java-racingcar-playground

ca1af · 2023-10-08T08:47:14Z

No description provided.

minseon33 · 2023-10-08T11:37:03Z

src/main/java/car/Cars.java

+        this.cars = initCarByNames(carNames);
+    }
+
+    private List<Car> initCarByNames(String carNames) {


일급 컬렉션을 사용해서 처음에 사용자가 입력한 input값으로 초기화를 해준 뒤 활용하는 아이디어가 정말 좋은 것 같습니다.
하지만 그 input값에 대한 검증은 어떻게 처리하면 좋을지도 생각해보면 어떨까요..?

input 값에 대한 처리는 Car의 생성자로 하고 있습니다.

minseon33 · 2023-10-08T11:42:22Z

src/main/java/car/MoveStrategy.java

@@ -0,0 +1,5 @@
+package car;
+@FunctionalInterface
+public interface MoveStrategy {


MoveStrategy를 인터페이스로 나눈 이유는 무엇인가요?

익명 클래스로써 사용하기 위함입니다. 해당 인터페이스를 상속(implement) 하지 않고 필요한 경우 간단히 재정의 가능합니다.

minseon33 · 2023-10-08T11:43:38Z

src/main/java/view/OutputView.java

+import java.util.List;
+
+public class OutputView {
+    private static final String INPUT_NAMES = "경주할 자동차 이름을 입력하세요(이름은 쉼표(,)를 기준으로 구분).";


static 상수값이 계속해서 많아진다면 어떻게 관리하면 좋을지 생각해보면 좋을 것 같습니다.

Enum 으로 관리하면 편할 듯 합니다. 하지만 그정도로 양이 많지 않다고 판단했습니다.

minseon33 · 2023-10-08T11:51:38Z

src/main/java/car/Cars.java

+
+    public List<String> getWinnersNames() {
+        int maxLocation = cars.stream().mapToInt(Car::getLocation).max().orElse(0);
+        return cars.stream().filter(car -> car.getLocation() == maxLocation).map(Car::getName).collect(Collectors.toList());


stream과 ramda를 잘 활용하는 모습이 멋집니다. bb
스트림을 사용하니 코드가 깔끔해지는 효과가 있는것 같습니다.

그러나 현업에서 스트림에 대한 지식이 없는 개발자가 보게될 경우 오히려 더 보기 어려운 코드가 될 수 있는데
스트림을 사용해서 깔끔한 코드를 작성하는것과
조금 지저분해지더라도 신입 또는 후배 개발자를 위해
스트림을 사용하지 않고 코드를 작성하는것 중 어느 것을 선택하는 것이 더 좋을것 같다고 생각하시나요?

stream 에 대한 지식을 갖춰야 합니다.

Java 버전이 업데이트 될수록 비동기 처리에 대한 최적화가 진행되고 있어서, 비동기 방식의 stream이 유리한 경우가 왕왕 생기고 있습니다. 특정 상황 (primitive 타입의 stream 처리 등) 에서 발생하는 오버헤드들을 주의한다면 stream의 이점이 많다고 생각됩니다.

dhlee128

10/08 자동차 경주 리뷰

dhlee128 · 2023-10-08T13:01:04Z

src/main/java/car/Car.java

+    private int location; // 초기값은 0 (primitive)
+    private final String name;
+    public Car(String name) {
+        if (name.length() > 5) throw new IllegalArgumentException();


저는 유효성 체크하는 부분을 따로 작성을 해두었는데,
이유는 유효성 체크 시 코드가 섞여 가독성이 떨어질까 싶어서였습니다.
동균님이 생각하시기엔
따로 유효성 체크하는 코드를 기존 코드와 함께 작성하는 것이 더 클린한 코드일지요?!!

Car 객체의 유효성 체크는 Car 객체의 역할이라는 생각이 있었습니다.

Car 객체의 유효성 체크의 분량이 커진다면 해당 작업을 위해 유효성을 체크하는 인터페이스를 따로 빼서 분리하는 전략 패턴의 사용도 고려해 볼 수 있을 것 같습니다.

다희님이 말씀주신 대로 해당 부분을 매서드로 빼서 validateName 정도의 이름을 가진 매서드로 만들고, 이를 생성자 단에서 활용한다면 아마 더 가독성이 높은 코드가 되지 않을까 싶습니다.

dhlee128 · 2023-10-08T13:04:14Z

src/main/java/car/Car.java

+        this.location++;
+    }
+
+    public void moveByStrategy(int digit, MoveStrategy moveStrategy) {


MoveStrategy 를 파라미터로 받아 자동차를 움직이는 코드에서
if(digit >= 4) 이런식으로 하지 않고 인터페이스를 사용한 이유가 궁금합니다..
4를 static final 로 따로 관리해두어 변수로 사용한다면! 괜찮지 않을까요?!!!

저 인터페이스는 함수형 인터페이스로, 익명 클래스를 이용한 재정의를 통해서 일급 객체(함수를 이용한)로 사용 될 수 있습니다.

즉, 매소드에 패러미터로 사용할 수 있게 되며, 이 경우 호출하는 측에서 재정의해주면 됩니다.

랜덤한 값을 받아서 자동차를 움직이는 로직은 테스트가 매우 어렵습니다. 순수 랜덤값에 대한 테스트는 근사치에 대한 계산 (예를 들어 10의 N승번 실행하면 자동차가 움직일 확률은 50%일 것입니다)만 가능하지만, 해당 방식처럼 해당 차가 움직이기 위한 조건을 때마다 재정의하면 간단히 테스트가 가능합니다.

즉, 랜덤 값 그 자체에 대한 테스트가 아닌 자동차가 움직인다는 로직이 잘 작동하기 위한 테스트를 작성하기 위해서는 해당 방식처럼 매서드를 재정의 해야하고, 해당 방식을 위한 손쉬운 방법이 함수형 인터페이스라고 생각했습니다.

매직 넘버 (이경우 4)에 대한 상수 처리는 다음부터 꼭 하도록 하겠읍니다...넘 귀찮았습니다

dhlee128 · 2023-10-08T13:06:47Z

src/main/java/car/Cars.java

+    }
+
+    public void move(){
+        cars.forEach(car -> car.moveByStrategy(random.nextInt(10), value -> value >= 5));


Random 은 지역보다는 전역으로 많이 선언을 하는 클래스 인가요 ?ㅎㅎ

랜덤 객체는 굉장히 비싼 (무거운, 메모리 많이먹는) 객체입니다.

매서드 단에서 new Random() 과 같은 식으로 인스턴스를 생성하게 된다면 매서드 호출 시마다 메모리를 잡아먹고, 이는 비효율적일 것입니다.

따라서 위와 같은 방식으로 진행했습니다. (말씀하신대로 Random은 전역 선언을 많이 합니다! - 메모리 낭비를 막기 위해)

dhlee128 · 2023-10-08T13:23:53Z

src/test/java/MovingStrategyTest.java

+    //- 전진하는 조건은 0에서 9 사이에서 random 값을 구한 후 random 값이 4이상일 경우이다.
+    private Car car;
+
+    @BeforeEach


BeforeAll 어노테이션으로 테스트 코드 실행 전 한번만 실행을 하는것도 가능하겠죠!?

테스트에서 중요한 점 중 하나는 매 테스트가 독립적이어야 한다는 것입니다. 즉, 한 테스트가 다른 테스트의 결과에 영향을 받으면 안됩니다.

이 경우 테스트마다 Car 객체는 move() 등의 매서드를 통해 위치값이 변화하므로, @beforeeach 를 통해서 매번 초기화 (새 Car 객체로 설정) 해주는 작업이 필요합니다.

예를 들어 move를 통해 위치값(==location)이 1이 되고, 그 객체를 다른 테스트에서 재사용한다면(@BeforeAll을 사용해서), 테스트는 실패할 것입니다. 따라서@ BeforeEach를 사용했습니다.

ca1af

리뷰 끝

ca1af · 2023-10-08T11:55:32Z

src/main/java/car/Cars.java

+        this.cars = initCarByNames(carNames);
+    }
+
+    private List<Car> initCarByNames(String carNames) {


input 값에 대한 처리는 Car의 생성자로 하고 있습니다.

ca1af · 2023-10-08T11:56:04Z

src/main/java/car/MoveStrategy.java

@@ -0,0 +1,5 @@
+package car;
+@FunctionalInterface
+public interface MoveStrategy {


익명 클래스로써 사용하기 위함입니다. 해당 인터페이스를 상속(implement) 하지 않고 필요한 경우 간단히 재정의 가능합니다.

ca1af · 2023-10-08T11:56:22Z

src/main/java/view/OutputView.java

+import java.util.List;
+
+public class OutputView {
+    private static final String INPUT_NAMES = "경주할 자동차 이름을 입력하세요(이름은 쉼표(,)를 기준으로 구분).";


Enum 으로 관리하면 편할 듯 합니다. 하지만 그정도로 양이 많지 않다고 판단했습니다.

ca1af · 2023-10-08T11:57:34Z

src/main/java/car/Cars.java

+
+    public List<String> getWinnersNames() {
+        int maxLocation = cars.stream().mapToInt(Car::getLocation).max().orElse(0);
+        return cars.stream().filter(car -> car.getLocation() == maxLocation).map(Car::getName).collect(Collectors.toList());


stream 에 대한 지식을 갖춰야 합니다.

Java 버전이 업데이트 될수록 비동기 처리에 대한 최적화가 진행되고 있어서, 비동기 방식의 stream이 유리한 경우가 왕왕 생기고 있습니다. 특정 상황 (primitive 타입의 stream 처리 등) 에서 발생하는 오버헤드들을 주의한다면 stream의 이점이 많다고 생각됩니다.

ca1af · 2023-10-08T13:46:34Z

src/main/java/car/Car.java

+    private int location; // 초기값은 0 (primitive)
+    private final String name;
+    public Car(String name) {
+        if (name.length() > 5) throw new IllegalArgumentException();


Car 객체의 유효성 체크는 Car 객체의 역할이라는 생각이 있었습니다.

Car 객체의 유효성 체크의 분량이 커진다면 해당 작업을 위해 유효성을 체크하는 인터페이스를 따로 빼서 분리하는 전략 패턴의 사용도 고려해 볼 수 있을 것 같습니다.

다희님이 말씀주신 대로 해당 부분을 매서드로 빼서 validateName 정도의 이름을 가진 매서드로 만들고, 이를 생성자 단에서 활용한다면 아마 더 가독성이 높은 코드가 되지 않을까 싶습니다.

ca1af · 2023-10-08T13:53:02Z

src/main/java/car/Car.java

+        this.location++;
+    }
+
+    public void moveByStrategy(int digit, MoveStrategy moveStrategy) {


저 인터페이스는 함수형 인터페이스로, 익명 클래스를 이용한 재정의를 통해서 일급 객체(함수를 이용한)로 사용 될 수 있습니다.

즉, 매소드에 패러미터로 사용할 수 있게 되며, 이 경우 호출하는 측에서 재정의해주면 됩니다.

랜덤한 값을 받아서 자동차를 움직이는 로직은 테스트가 매우 어렵습니다. 순수 랜덤값에 대한 테스트는 근사치에 대한 계산 (예를 들어 10의 N승번 실행하면 자동차가 움직일 확률은 50%일 것입니다)만 가능하지만, 해당 방식처럼 해당 차가 움직이기 위한 조건을 때마다 재정의하면 간단히 테스트가 가능합니다.

즉, 랜덤 값 그 자체에 대한 테스트가 아닌 자동차가 움직인다는 로직이 잘 작동하기 위한 테스트를 작성하기 위해서는 해당 방식처럼 매서드를 재정의 해야하고, 해당 방식을 위한 손쉬운 방법이 함수형 인터페이스라고 생각했습니다.

매직 넘버 (이경우 4)에 대한 상수 처리는 다음부터 꼭 하도록 하겠읍니다...넘 귀찮았습니다

ca1af · 2023-10-08T13:54:19Z

src/main/java/car/Cars.java

+    }
+
+    public void move(){
+        cars.forEach(car -> car.moveByStrategy(random.nextInt(10), value -> value >= 5));


랜덤 객체는 굉장히 비싼 (무거운, 메모리 많이먹는) 객체입니다.

매서드 단에서 new Random() 과 같은 식으로 인스턴스를 생성하게 된다면 매서드 호출 시마다 메모리를 잡아먹고, 이는 비효율적일 것입니다.

따라서 위와 같은 방식으로 진행했습니다. (말씀하신대로 Random은 전역 선언을 많이 합니다! - 메모리 낭비를 막기 위해)

ca1af · 2023-10-08T13:56:16Z

src/test/java/MovingStrategyTest.java

+    //- 전진하는 조건은 0에서 9 사이에서 random 값을 구한 후 random 값이 4이상일 경우이다.
+    private Car car;
+
+    @BeforeEach


테스트에서 중요한 점 중 하나는 매 테스트가 독립적이어야 한다는 것입니다. 즉, 한 테스트가 다른 테스트의 결과에 영향을 받으면 안됩니다.

이 경우 테스트마다 Car 객체는 move() 등의 매서드를 통해 위치값이 변화하므로, @beforeeach 를 통해서 매번 초기화 (새 Car 객체로 설정) 해주는 작업이 필요합니다.

예를 들어 move를 통해 위치값(==location)이 1이 되고, 그 객체를 다른 테스트에서 재사용한다면(@BeforeAll을 사용해서), 테스트는 실패할 것입니다. 따라서@ BeforeEach를 사용했습니다.

TDD 기반의 두번째 시도

34c4cfa

minseon33 reviewed Oct 8, 2023

View reviewed changes

dhlee128 reviewed Oct 8, 2023

View reviewed changes

ca1af requested review from dhlee128 and minseon33 October 8, 2023 14:34

ca1af commented Oct 8, 2023

View reviewed changes

Conversation

ca1af commented Oct 8, 2023

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!

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

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants