[1단계 - 자동차 경주 구현] 저문(유정훈) 미션 제출합니다. #519

jeomxon · 2023-02-09T08:28:33Z

객체가 일하도록 하는 것에 집중하여 개발을 진행해봤는데 생각보다 쉽지 않았습니다...
게터를 지양하려고 노력해보았는데 결국 사용하게 되었던 점이 아쉬워서 한번 고쳐보고 싶습니다.

테스트 작성의 범위가 어디까지인지 궁금해졌습니다.
작은 단위의 테스트를 진행했을 때, 그를 포함하는 큰 단위의 테스트를 진행하는 것이 실질적으로 의미가 있는 것인지 궁금합니다.

반복문을 사용할 때 인덱스를 사용하지 않는 경우가 있는데, foreach문 또한 적용이 안되는 경우라면 어떤 방식으로 풀어나가야하는지 고민입니다. 인덱스를 없애려고 while을 사용하게 되어도 변수를 통해서 무언가를 해야함을 느꼈습니다.
이와 같은 경우는 어떤 방법을 통해 해결하는 것이 좋을지 궁금합니다.

가장 전체적이고 큰 고민은 설계입니다. 어떤 설계가 맞다, 틀리다는 없다고 생각하지만 좀 더 효율적이고 좋은 설계는 분명히 존재한다고 생각합니다. 프리코스부터 이어서 매번 미션을 진행하면서 가장 크게 고민했던 부분 중 하나였습니다.
클래스를 최대한 작게 유지하고 싶고, 패키지를 더 잘게 쪼개고 싶은데, 생각대로 이루어지지 않았던 것 같습니다.
어떤 부분을 얼마나 작게 나눠야하는지, 전체적인 프로그램의 흐름은 어떤 방식이 효율적인지 결론이 서지 않습니다.
단지 저만의 설계를 가지고 있으면 되는 걸까요? 아니라면 어떤 방식으로 생각하는 것이 도움이 될지 궁금합니다.

감사합니다 :)

iphwade

고민의 흔적이 보이는군요! 고생 많이 하셨습니다-!
미션 정말 잘 수행해주셨어요!
첫 미션임에도 설계에 대해 잘 정리해주셨습니다. 👍

간단하게 고민할 수 있는 고민거리들로 리뷰드렸습니다! 혹여 내용 중 질문이 있거나 하면 편하게 슬랙 메세지주세요! :)

테스트 작성의 범위가 어디까지인지 궁금해졌습니다.
-> TDD를 진행하다보면 도메인 안쪽부터 구현을 시작하는 방법, 도메인의 제일 바깥 쪽에서부터 구현을 시작하는 방법들이 있습니다. (In-out, out-in 방식) 보통 도메인 이해도가 높지않은 상태에서 구현을 시작할 때는 바깥 쪽에서 구현을 시작하는 경우가 많은데요. 이런 경우에는 자연스럽게 큰 단위의 테스트부터 구현이 시작되기 때문에 고민할 필요가 없지만 반대의 경우에는 고민할 수도 있겠네요. 개발자가 경중에 따라 선택할 문제로 보입니다. 저 같은 경우에는 시간이 허락하는 한 되도록이면 채워 넣는 편입니다-! 긴 시간 애플리케이션을 관리하다보면 오랜 기간이 지난 후, 또는 내가 아닌 누군가가 작업을 할텐데 하위 로직들이 어떻게 변경될지는 모르는 것이니까요!

반복문을 사용할 때 인덱스를 사용하지 않는 경우
-> 반복문의 역할에 대해 고민해보면 좋을 것 같네요. 왜 인덱스를 사용하는 것이 좋지않다고 판단하신 걸까요?

설계
-> 설계는 저도 매일 고민하는 난제입니다...! 본인의 설계에 대한 근거를 가지고 있다면 정답에 가까운 설계가 되지 않을까요? 지금은 미션을 수행하며 설계에 대한 근거를 쌓는 연습을 하신다고 생각하시면 좋을 것 같습니다. 첫 미션으로도 너무 잘 수행하셨으니 깊은 고민 보다는 부딪혀가며 배워보는 것이 좋아보입니다! :)

iphwade · 2023-02-09T16:04:53Z

src/test/java/domain/CarTest.java

+    @DisplayName("랜덤 값이 3보다 큰 경우 전진한다.")
+    @Test
+    void moveWhenRandomNumberIsOverThree() {
+        //given


iphwade · 2023-02-09T16:10:17Z

src/test/java/domain/CarTest.java

+        RandomNumberGenerator numberGenerator = new RandomNumberGenerator() {
+            @Override
+            public int generate() {
+                return 4;
+            }
+        };


테스트가 어려운 영역을 이렇게 분리하여 테스트를 하셨군요! 👍 👍 👍

위 내용을 Interface로 만들고 익명 객체로 구현하는 것과 위 방향처럼 클래스의 메서드를 오버라이드하는 방식의 차이는 무엇일까요?

후자의 방법에서 위처럼 간단히 4를 리턴하는 메서드로 오버라이드 되는 것이 아닌 super.generate() 가 사용되는 경우에는 어떤 문제가 있을 수 있을까요 ?

인터페이스로 익명 객체를 구현하면 강제적으로 내부의 메소드를 오버라이딩해야하지만,
클래스의 메서드를 오버라이드 하게 되면 그렇지 않습니다.
전자의 경우는 강제성을 띈다고 할 수 있습니다.

따라서 후자의 방법에서 super.generate()가 사용되면 테스트가 제대로 되지 않을 수 있습니다.

추가로 move메서드의 파라미터를 boolean타입으로 바꾸니 해당 부분의 테스트를 바꿀 필요가 있다고 판단하여 익명객체를 사용하지 않게 되어서 인터페이스로 변경하지는 않았습니다.

위와 같은 경우가 다음에 발생하거나 추후 리팩터링 과정에 필요성이 생기면 인터페이스를 사용하여 구현해야한다고 느꼈습니다.

넵! 나중에 같은 상황이 오면 한번 더 고민해보면 좋을 것 같습니다! :)

iphwade · 2023-02-09T16:14:15Z

src/main/java/controller/RacingGameController.java

+    }
+
+    public void run() throws IOException {
+        RacingGame racingGame = inputHandler.readCars();


InputHandler가 RacingGame을 생성하는 기능을 가지고 있으면 MV가 명확하게 분리되는 것처럼 보이지 않습니다!
어떻게하면 분리할 수 있을까요?

controller로 RacingGame객체 생성을 이동하여 분리하는 방법으로 수정하였습니다!

iphwade · 2023-02-09T16:15:46Z

src/main/java/handler/InputHandler.java

+            String inputName = inputView.readCarNames();
+            String[] names = inputName.split(COMMA);
+
+            return new RacingGame(names);


위에서 말씀드려듯 InputHandler가 RacingGame을 생성하는 것이 InputHandler의 역할인지 고민이 필요해보입니다!

위의 컨트롤러에서 객체를 생성하도록 역할을 바꿔주었습니다!

iphwade · 2023-02-09T16:17:13Z

src/main/java/handler/InputHandler.java

+    }
+
+    private void validateInteger(String movingTrial) {
+        if (!movingTrial.matches(REGEX)) {


이건 중요한 것은 아니지만 정규식을 성능면에서 더 잘 사용할 수 있는 방법도 존재합니다! :)

String.matches를 사용할 경우 정규표현식용 Pattern 인스턴스가 한번 쓰고 버려지게 되는데,
변경한 방법으로 사용하였을 경우 Pattern인스턴스를 불변 객체로 생성함으로써 재사용이 가능하다는 장점이 있다는 것을 알았습니다.
따라서 위의 방법으로 리팩터링 하였습니다.

iphwade · 2023-02-09T16:27:49Z

src/main/java/domain/CarGroup.java

+        for (String name : names) {
+            Car car = new Car(name);
+            cars.add(car);
+        }


add 하지 말고 이 메서드에서 List를 반환하는 것은 어떨까요?

stream을 사용하여 List를 반환하도록 변경하니 add를 사용하지 않고 할 수 있었습니다.
또한 이를 통해 생성자에서 setUp메소드가 cars와 가까워지는 효과를 볼 수 있었고,
이로인한 가독성 증가라는 효과도 볼 수 있었습니다!

iphwade · 2023-02-09T16:28:40Z

src/main/java/domain/CarGroup.java

+    private final List<Car> cars;
+
+    public CarGroup(String[] names){
+        this.cars = new ArrayList<>();


this.cars는 초기화되고 setUp의 메서드에서 사용됩니다. 생성된 변수 혹은 객체는 사용하는 곳과 가깝게 배치하면 가독성이 더 좋아집니다!

위의 코멘트에 수정하여 답을 남겼습니다 :)

iphwade · 2023-02-09T16:30:07Z

src/main/java/domain/Car.java

+        this.position = INITIAL_VALUE;
+    }
+
+    public String getName() {


게터 메서드는 컨벤션에서 어디에 위치하는 게 좋을까요-!

처음 저는 필드 -> 생성자 -> 게터 -> 일반 메서드 순서대로 배치하면서, 접근자가 공개적인 순으로 배치하도록 컨벤션을 유지하며 코딩해왔습니다.
public메소드의 가장 마지막으로 위치를 변경하니, 로직이 포함된 메서드들이 더 앞선 위치에 있게 되면서
객체의 역할을 잘 드러내는 것처럼 보였습니다.

일반 메서드, 게터 메서드 중 어떤게 더 중요할까요?

객체의 역할을 생각했을 때 일반 메서드는 객체의 행동이라고 생각합니다.
따라서 일반 메서드가 더 중요한 로직을 가지고, 전체적으로 더 중요한 것 같습니다.

iphwade · 2023-02-09T16:33:17Z

src/main/java/domain/Car.java

+public class Car {
+
+    private static final int INITIAL_VALUE = 0;
+    private static final int MOVING_CONDITION = 4;


잘 구현해주셨습니다!

요 정보는 Car가 알고있을 필요가 있을까요? Car 입장에서는 움직일지 말지만 알도록 구현해보는 방법은 어떤 게 있을까요?

자동차의 입장에서는 움직일지 말지만 알도록 구현하기 위해서 파라미터를 boolean으로 받아서 변경해보았습니다.
하지만 이렇게 변경하면서 다시 생긴 고민이 있습니다.

원래 랜덤 값에 대해서 테스트 하던 부분이 boolean값으로 변경되면서, 원래 있던 조건에 대한 테스트를 진행하기가 어려워졌습니다.
isMovable이라는 메소드가 생겼지만, 해당 메서드의 접근자를 private으로 하는 것이 맞다고 생각했고 이에따라 테스트 진행이 어려워졌습니다.
제가 코일이 의도하신 방향으로 맞게 리팩터링 했는지 궁금합니다.

iphwade · 2023-02-09T16:36:15Z

src/main/java/domain/RacingGame.java

+        }
+    }
+
+    public RacingResult getRacingResult(){


get은 getter 메서드에서 사용되어 혼용되기 쉽습니다. 되도록 다른 메서드명으로 변경해보는 건 어떨까요?

해당 메서드 명은 get을 다른 단어로 수정하였습니다!

iphwade

전체적으로 피드백 드린 부분 많이 반영해주셨네요! 훌륭하게 잘 해주셨습니다! :)
이번 단계는 여기서 merge하도록 하겠습니다! :)

iphwade · 2023-02-12T08:09:24Z

src/main/java/racing/controller/RacingGameController.java

+        String[] carNames = inputHandler.readCars();
+        int movingTrial = inputHandler.readMovingTrial();
+
+        RacingGame racingGame = new RacingGame(carNames);


iphwade · 2023-02-12T08:11:06Z

src/main/java/racing/domain/Car.java

+    private static final int INITIAL_VALUE = 0;
+    private static final int MINIMUM_LENGTH_OF_CAR_NAME = 1;
+    private static final int MAXIMUM_LENGTH_OF_CAR_NAME = 5;
+    private static final String LENGTH_OF_CAR_NAME_ERROR = "[ERROR] 자동차이름의 길이는 1-5자까지 가능합니다.";


iphwade · 2023-02-12T08:12:20Z

src/main/java/racing/domain/CarGroup.java

+    public CarGroup(String[] names){
+        validateDuplicatedName(names);
+        validateNumberOfCars(names);
+        this.cars = setUp(names);


iphwade · 2023-02-12T08:14:16Z

src/main/java/racing/domain/RacingGame.java

+
+    public RacingGame(String[] names) {
+        this.carGroup = new CarGroup(names);
+        this.numberGenerator = new RandomNumberGenerator();


numberGenerator를 생성자에서 주입받는 형태로 구현하면 어떤 부분이 좋아질까요?

생성자에서 주입받는 형태로 구현하게 되면 테스트 코드 작성이 용이해질 것 같습니다!

@jeomxon 테스트 코드 작성이 용이해지는 이유는 결국 NumberGenerator하는 영역을 쉽게 다른 기능으로 확장할 수 있다는 것을 뜻합니다.

iphwade · 2023-02-12T08:17:13Z

src/main/java/racing/handler/InputHandler.java

+            String input = inputView.readMovingTrial();
+            validateInteger(input);
+            int movingTrial = Integer.parseInt(input);
+            validateTrialRange(movingTrial);


jeomxon

말씀해주신 부분에 대해서 리팩토링을 완료했습니다.
생각지도 못했던 부분이 너무 많아서 큰 부족함을 느꼈던 것 같습니다.
친절하고 꼼꼼하게 리뷰해주셔서 정말 감사드립니다 코일!

jeomxon · 2023-02-12T11:53:48Z

src/main/java/racing/domain/RacingGame.java

+
+    public RacingGame(String[] names) {
+        this.carGroup = new CarGroup(names);
+        this.numberGenerator = new RandomNumberGenerator();


생성자에서 주입받는 형태로 구현하게 되면 테스트 코드 작성이 용이해질 것 같습니다!

iphwade

안녕하세요 저문! ( _ _ )

수정하신 코드 잘 보았습니다-!
많이 고민하신 흔적이 보입니다! 고생하셨습니다-!

제가 리뷰 드린 내용을 고민하다보니 기존에 잘 지켜지던 요구 사항이 지켜지지 않는 부분들이 있네요.
테스트 코드의 중요성을 인지할 수 있는 좋은 경험으로 보입니다. 기존 요구 사항에 대한 테스트 코드가 잘 짜여져 있었다면 코드를 리팩토링하면서 요구 사항을 놓치지 않으셨을 것 같아요! 이런 부분들이 테스트 코드의 중요성이죠-!

테스트 코드가 부족한 부분들을 더 채워주시길 바라며, 또한 제가 코멘트 남긴 부분에 대해 고민해주세요-!

주요 리뷰

생성자를 통한 초기화
테스트 케이스에 대해 더 고민하기
Car.move(boolean isMovable)의 문제점

iphwade · 2023-02-12T15:08:39Z

src/main/java/racing/domain/RacingGame.java

+
+    //TODO: 테스트
+    public void race() {
+        carGroup.race(isMovable());


이 로직은... 요구사항과 맞지않네요. 자동차 개별적으로 판단되는 것이 아닌 라운드에 모든 자동차가 이동하거나 이동하지 않네요...!

리팩터링 도중에 요구사항이 맞지 않다는 사실을 인지하지 못했네요..ㅠㅠ
수정사항 반영하도록 하겠습니다.
테스트 코드를 잘 짜는게 얼마나 중요한지 새삼 깨닫게 되네요!!

iphwade · 2023-02-12T15:11:04Z

src/main/java/racing/domain/Car.java

+    public void move(boolean isMovable) {
+        if (isMovable) {
+            this.position++;
+        }
+    }


boolean을 통해 메서드의 동작을 체크하는 방법보다 더 좋은 방법이 있어보입니다.
위와 같이 매개 변수를 boolean으로 받고 로직을 실행시키는 것은 너무 많은 가능성을 열어둡니다.
Car의 이동 로직이 도메인 내에서 결정되는 특정 숫자 이상일 때에만 이동한다거나 이 범위를 넘어서 애플리케이션 전체에서 무분별한 car position 변경을 야기할 수 있습니다.

변경 조건을 클래스 또는 인터페이스로 좁혀보면 어떨까요?

제가 의도를 잘못 이해하고 올바르지 못한 방향으로 리팩터링 했습니다 ㅠㅠ
인터페이스를 통해 NumberGenerator를 다양한 방식으로 구현할 수 있도록 변경하였습니다.
실제 로직에서는 random으로 생성하고 테스트 시에는 필요한 클래스를 구현하여 하는 방법을 사용해보았습니다.

jeomxon added 26 commits February 8, 2023 15:32

test: 페어와 JUnit 학습하기

71e7457

docs: 기능 목록 초안 정리

c264230

feat: 입력받는 기능 추가

85d02bc

feat: 쉼표를 기준으로 이름을 받아 자동차를 생성하는 기능 추가

372a7d9

feat: 자동차 이름 길이에 대한 예외처리

c3e0bea

test: 자동차 이름 길이 예외처리에 대한 테스트

b02d056

feat: 시도 횟수는 양수로만 가능하도록 예외처리

dbbd054

feat: 랜덤 값을 받아 자동자 위치를 이동하는 기능 추가

887cb06

test: 값에 따른 자동차 위치를 이동하는 테스트

cff8350

feat: 최종 우승 자동차를 확인하는 기능 추가

ceac359

feat: 최종 결과를 가져오는 기능 추가

358029b

feat: 각 자동차별 경주 실행결과 출력 기능 추가

b192e68

feat: 최종 우승 자동차 출력 기능 추가

2cfa842

feat: 컨트롤러 프로그램 실행 로직 추가

d7f9a61

docs: 기능 목록 정리 최신화

c63368a

feat: 자동차 경주 시도 횟수는 1~100으로 제한하는 기능 추가

ef4649d

refactor: 예외 메시지 추가 및 메소드 분리

faafb76

style: 전체적인 코드 스타일 수정

f54dbab

refactor: 하드 코딩된 값을 제거

55363c2

test: CarGroup에 대한 테스트

37fd9d3

test: Car에 대한 테스트

043fe2a

test: RacingResult에 대한 테스트

685613e

docs: 기능 목록 정리 최신화

34274a2

style: 전체적인 코드 스타일 수정

2fb5180

refactor: 예외 발생 시 그 부분부터 다시 입력을 받는 기능 수정

3fbe4dd

refactor: 테스트 코드 접근자 제거

9953e68

iphwade requested changes Feb 9, 2023

View reviewed changes

jeomxon added 3 commits February 10, 2023 14:45

refactor: RandomNumberGenerator를 NumberGenerator를 implement하도록 구현

d2b39b3

refactor: CarTest에서의 Car를 전진하도록 하는 메서드의 설계 변경으로 인한 코드 수정

ee739cc

refactor: RacingGame객체의 생성을 controller로 이전

86c48cf

jeomxon added 8 commits February 10, 2023 23:20

refactor: RacingGame의 race메서드 로직을 CarGroup으로 이전

c457897

refactor: CarGroup의 setUp메소드가 List를 반환하도록 수정

e1f04d5

refactor: Car클래스 getter위치 변경

982802e

refactor: getter와 혼동될 수 있는 메서드 네이밍 변경

fddfc3c

refactor: Car에서 move의 매개변수를 boolean타입으로 변경

3bf3994

refactor: 정규표현식 성능 개선을 위해 메서드 변경

1eb8f17

refactor: NumberGenerator인터페이스 삭제 후 RandomNumberGenerator만으로 운영되도록 변경

79287e6

refactor: CarTest의 test로직 변경에 따라 DisplayName변경

79b852f

iphwade approved these changes Feb 12, 2023

View reviewed changes

iphwade merged commit 9601a85 into woowacourse:jeomxon Feb 12, 2023

jeomxon commented Feb 12, 2023

View reviewed changes

iphwade reviewed Feb 12, 2023

View reviewed changes

[1단계 - 자동차 경주 구현] 저문(유정훈) 미션 제출합니다. #519

[1단계 - 자동차 경주 구현] 저문(유정훈) 미션 제출합니다. #519

Conversation

jeomxon commented Feb 9, 2023 • edited Loading

iphwade 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

jeomxon Feb 11, 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

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

iphwade 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

jeomxon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iphwade 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

jeomxon commented Feb 9, 2023 •

edited

Loading

jeomxon Feb 11, 2023 •

edited

Loading