1주차 - Step2 자동차 경주 구현 #319

Integerous · 2019-06-18T10:29:10Z

안녕하세요!
Step1에서 피드백 해주신 부분들 감사했습니다.
일정이 조금 촉박해서 Step1을 리팩토링하지는 못했습니다.
앞으로 이어질 단계들에서 적용해보겠습니다.

Step2에서는 테스트 코드를 많이 작성하지 못했습니다.
어떤 로직을 테스트해야할지 감이 잘 잡히지 않았습니다.

전체적으로 구조가 조잡하고 역할이 명확하지 않은 것 같아서
몇 가지를 제외하고는 질문을 따로 작성하지 않았습니다..
문제가 될만한 부분은 사소한 것이라도 간략하게라도 지적해주시면 감사하겠습니다!

그리고, 객체지향원칙 중에 일급 콜렉션을 사용하라는 부분이 있어 적용해보고 싶었는데, (실패했습니다ㅜ)
이번 구현에서 어떤 부분에 일급 콜렉션을 사용했으면 좋았을 지 힌트라도 주시면 감사하겠습니다!

바쁘신 와중에 좋은 피드백 감사합니다 👍 👍

brainbackdoor · 2019-06-18T13:20:07Z

src/main/java/step2/domain/CarRace.java

+    }
+
+    private List<Car> makeCars(int numberOfCars) {
+        List<Car> cars = new ArrayList<>();


Cars를 일급콜렉션으로 구성하면 좋을거 같네요.

brainbackdoor · 2019-06-18T13:20:32Z

src/main/java/step2/domain/RaceExecutor.java

+
+import java.util.List;
+
+public class RaceExecutor {


RaceExecutor 객체의 역할이 모호한거 같아요.
자동차의 이동가능 여부는 Car 객체가 가지고 있어야 자연스러울거 같구요.
randomNumberGenerator 의존성을 주입하기 위해서 인자로 넣으신거라면 NumberGenerator 등의 interface를 작성하셔서 넘기시는게 좋을거 같습니다.
Cars 일급콜렉션까지 구성한다면 이 클래스는 없어질 수도 있겠네요.

brainbackdoor · 2019-06-18T13:20:50Z

src/main/java/step2/domain/RaceExecutor.java

+        for (Car car : this.cars) {
+            moveIfPossible(car);
+        }
+        new OutputView(this.cars); // TODO: 결과화면을 여기서 생성하는 것이 어색한데 문제 없을지 궁금합니다.


도메인 모델에는 비즈니스로직만 남아있어야 합니다.
출력과 관련된 부분은 말씀하신대로 뷰모델에 위임하셔야 합니다.
현재 구조에서 변경하려면 각 trial별로 결과 데이터를 List 등에 담아서 리턴하는 것이 좋을 거 같네요.

brainbackdoor · 2019-06-18T13:22:34Z

src/main/java/step2/view/InputView.java

+        final int numberOfTrials = scanner.nextInt();
+        inputNumberValidation(numberOfTrials);
+        scanner.close();
+        System.out.println();


이런 부분들도 printEmptyLine()등의 메소드를 통해 의미를 분명히 드러내는 것이 좋다고 생각합니다.

brainbackdoor · 2019-06-18T13:23:21Z

src/main/java/step2/view/InputView.java

+    public int getNumberOfTrials() {
+        System.out.println(ASK_NUMBER_OF_TRIALS);
+        final int numberOfTrials = scanner.nextInt();
+        inputNumberValidation(numberOfTrials);


두 메소드간에 중복코드가 있네요.

brainbackdoor · 2019-06-18T13:25:09Z

src/test/java/step2/CarTest.java

+    @ParameterizedTest
+    @ValueSource(ints = {1, 2, 3, 4, 5})
+    void 자동차가_주어진_거리만큼_이동한다(int numberOfMoves) {
+        for(int i=0; i<numberOfMoves; i++) {


공백 컨벤션 위반입니다. IDE에서 컨벤션을 자동으로 맞춰주는 shortcut이 있으니 확인해보세요.
intelliJ 는 option + command + l 입니다.

8.2 blank spaces 항목을 확인해보세요.
https://myeonguni.tistory.com/1596

brainbackdoor · 2019-06-18T13:25:52Z

src/test/java/step2/CarTest.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class CarTest {


자동차 이동조건과 관련된 테스트가 없네요.

brainbackdoor · 2019-06-18T13:26:49Z

src/main/java/step2/domain/CarRace.java

+    }
+
+    public void raceExecute() {
+        System.out.println("실행 결과");


출력과 관련된 건 OutputView에 위임하는게 좋을거 같네요.

brainbackdoor · 2019-06-18T13:30:12Z

src/main/java/step2/domain/CarRace.java

+    public CarRace(int numberOfCars, int numberOfTrials) {
+        this.cars = makeCars(numberOfCars);
+        this.numberOfTrials = numberOfTrials;
+        this.raceExecutor = new RaceExecutor(this.cars); //TODO : RaceExcutor는 CarRace가 생성되어야만 필요하니까 이렇게 생성했는데 문제없을까요?


생성자 내에서 생성하는 것과 관련된 물음이라면, 문제는 없다고 생각합니다.
다만, RaceExecutor에 cars를 전달하여 로직을 수행하는 것은 객체지향적이지는 않다고 생각합니다.

brainbackdoor · 2019-06-18T13:32:03Z

src/main/java/step2/view/OutputView.java

+public class OutputView {
+    private static final String POSITION_OF_CAR = "-";
+
+    public OutputView(List<Car> cars) {


Car 객체를 OutputView에 직접 전달하면, 뷰모델에서 객체의 상태를 변경시킬 수 있지 않을까요.
뷰모델에는 Value Object만 넘기는 것이 좋다고 생각합니다.

brainbackdoor · 2019-06-18T13:35:43Z

src/test/java/step2/CarTest.java

+
+    @BeforeEach
+    void init() {
+        car = new Car();


별다른 로직이 없다면 private Car car = new Car();로 구현해도 되지 않을까요/

brainbackdoor

안녕하세요!
몇가지 피드백 남겼으니 확인해보시고 궁금한 점 있으면 DM 남겨주세요.
자동차 객체의 이동조건에 대한 경계값 테스트가 없네요.
그리고 값을 받는 부분에서 유효성 검사를 해준다면, 이 부분에 대한 테스트도 유의미하다고 생각합니다.

brainbackdoor · 2019-06-18T23:47:09Z

src/test/java/step2/CarTest.java

+
+    @Test
+    void 입력된_숫자만큼_자동차가_생성된다() {
+        //TODO


TODO에 앞으로 할 작업 목록을 작성해주는 것이 좋습니다.
IDE에서 TODO로 달린 주석 리스트를 확인하실 수 있습니다.

brainbackdoor · 2019-06-18T23:58:16Z

src/main/java/step2/domain/CarRace.java

+    public static CarRace raceStart(int numberOfCars, int numberOfTrials) {
+        numberOfCarsValidation(numberOfCars);
+        numberOfTrialsValidation(numberOfTrials);
+        return new CarRace(Cars.makeCars(numberOfCars));


정적 팩토리 메서드를 사용하셨네요! 👍

brainbackdoor · 2019-06-19T00:00:42Z

src/main/java/step2/domain/Cars.java

+        for (Car car : cars) {
+            car.move(numberGenerator.getRandomNumber());
+        }
+        return this;


일급 콜렉션은 불변성을 보장해야 합니다.

https://jojoldu.tistory.com/412

brainbackdoor · 2019-06-19T00:11:38Z

src/main/java/step2/domain/Cars.java

+    }
+
+    public List<Car> getCars() {
+        return cars;


DTO를 사용하지 않는다면 값을 getter하는 부분에서 read-only 혹은 불변객체로 리턴하는 것이 좋다고 생각합니다.

https://www.geeksforgeeks.org/collections-unmodifiablelist-method-in-java-with-examples/

brainbackdoor · 2019-06-19T00:12:29Z

src/main/java/step2/domain/Cars.java

+        this.cars = cars;
+    }
+
+    public List<Car> getCars() {


method 순서에 대해서도 한번 고민해보세요

https://stackoverflow.com/questions/4668218/are-there-any-java-method-ordering-conventions

brainbackdoor

피드백 반영하여 잘 작성해주셨네요! 👍
이번에 작성하지 못한 테스트코드는 추후에 꼭 진행해주세요~
몇가지 피드백 남겼으니 확인해보시고, 궁금한 점 있으면 DM 남겨주세요
이번 Step은 여기서 Merge할게요~

Integerous added 14 commits June 17, 2019 23:07

docs #step2 - 구현할 기능 목록 추가

0a1c04e

feat #step2 - 사용자로부터 값을 입력받고 출력하는 main 메서드 추가

1bce695

refactor #step2 - Main 메서드의 UI 로직을 InputView로 분리

303459f

feat #step2 - Car 클래스 추가, 기능구현계획 수정

29901aa

test #step2 - Car 유닛테스트 추가, Car 클래스의 move() 메서드 수정

c2597d3

feat #step2 - 사용자 입력값 Validation 추가

acd03f3

refactor #step2 - Scanner를 생성자로 초기화하도록 수정

cef19ce

feat #step2 - 자동차들을 생성하고 화면에 출력

e4fa12b

feat #step2 - 전진 여부를 결정하는 로직 추가(리팩토링 필요)

808574d

feat #step2 - 시행 횟수만큼 자동차를 전진시키는 메서드 추가, 각 시행마다 화면에 출력

1d55df4

refactor #step2 - 자동차 이동 테스트 로직 변경

9a8150f

refactor #step2 - 경주를 실행하는 RaceExcutor 클래스 분리

c848ead

refactor #step2 - 패키지 분리(domain, utils, view)

c702fcf

refactor #step2 - 자동차의 전진 여부를 결정하는 로직을 MoveIndicator 클래스로 분리

9188b75

brainbackdoor reviewed Jun 18, 2019

View reviewed changes

brainbackdoor requested changes Jun 18, 2019

View reviewed changes

Integerous added 4 commits June 18, 2019 23:44

feedback #step2 - CarTest의 @beforeach 대신 Car 객체 인스턴스화

3f5431f

feedback #step2 - 일급 컬렉션 객체(Cars) 생성, CarRace 클래스에 Cars 사용

ff8683b

feedback #step2 - Car 객체가 자동차의 이동가능 여부 가지도록 수정

522d860

feedback #step2 - Cars 컬렉션을 Cars에서 생성하도록 변경, 게임 시작 메서드 변경

e9b8399

Integerous added 9 commits June 19, 2019 02:26

feedback #step2 - NumberGenerator 인터페이스 생성

e62c955

feedback #step2 - 자동차들 이동 로직을 Cars 클래스로 이전, 각 경주 시행결과 반환을 main 메서드로 이전

ea19507

feedback #step2 - numberOfTrials를 main메서드에서만 사용

3334a79

feedback #step2 - NumberGenerator를 CarRace의 필드로 사용

7b3fa9e

feedback #step2 - printEmptyLine() 추가, 출력과 관련된 것을 OutputView에 위임

5cd9174

feedback #step2 - Cars를 OutputView의 메서드로 전달하도록 수정

846a85a

feedback #step2 - InputView의 입력값 검증 로직을 CarRace 클래스로 이전

afdd1e6

feedback #step2 - (오류 수정) numberGenerator 초기화 및 전달 범위 수정

7928fad

feedback #step2 - 테스트코드 //TODO 세팅

48612a7

brainbackdoor reviewed Jun 18, 2019

View reviewed changes

brainbackdoor reviewed Jun 19, 2019

View reviewed changes

brainbackdoor approved these changes Jun 19, 2019

View reviewed changes

brainbackdoor merged commit 96c0380 into next-step:Integerous Jun 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1주차 - Step2 자동차 경주 구현 #319

1주차 - Step2 자동차 경주 구현 #319

Integerous commented Jun 18, 2019

brainbackdoor Jun 18, 2019

brainbackdoor Jun 18, 2019

brainbackdoor Jun 18, 2019

brainbackdoor Jun 18, 2019

brainbackdoor Jun 18, 2019

brainbackdoor Jun 18, 2019

brainbackdoor Jun 18, 2019

brainbackdoor Jun 18, 2019

brainbackdoor Jun 18, 2019

brainbackdoor Jun 18, 2019

brainbackdoor Jun 18, 2019

brainbackdoor left a comment

brainbackdoor Jun 18, 2019

brainbackdoor Jun 18, 2019

brainbackdoor Jun 19, 2019

brainbackdoor Jun 19, 2019

brainbackdoor Jun 19, 2019

brainbackdoor left a comment


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

		public class CarTest {

1주차 - Step2 자동차 경주 구현 #319

1주차 - Step2 자동차 경주 구현 #319

Conversation

Integerous commented Jun 18, 2019

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

brainbackdoor 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

brainbackdoor left a comment

Choose a reason for hiding this comment