[자동차 경주 2단계] 카피 step2 미션 제출합니다 #789

tkdgur0906 · 2024-02-18T16:21:06Z

지난 피드백을 반영하면서 한 클래스가 여러 역할을 하는 부분을 분리하려고 노력했습니다!

키워드로 주신 일급 컬렉션을 공부하여 적용해보려고도 하였는데, 아직 미숙하여 부족한 부분이 있는지 의견을 묻고 싶습니다!

yxxnghwan

안녕하세요 카피, 2단계 구현도 수고하셨습니다! 이미 1단계에서 MVC 구조로 시도했어서 큰 구조 변경이 존재하진 않네요! 👍🏿
고민해볼만한 부분들에 대해서 코멘트 남겼으니 확인 부탁드려요! 💪

yxxnghwan · 2024-02-18T16:30:09Z

src/main/java/racingcar/message/ErrorMessage.java

+public enum ErrorMessage {
+    MESSAGE_NO_DUPLICATE_CAR_NAMES("중복된 자동차 이름은 사용할 수 없습니다."),
+    MESSAGE_LENGTH_OF_CAR_NAME("자동차 이름은 1자 이상 5자 이하여야 합니다."),
+    MESSAGE_NO_SPACE("중복된 자동차 이름은 사용할 수 없습니다."),
+    MESSAGE_ONLY_NATURAL_NUMBER("시도할 회수는 자연수를 입력해 주세요."),


에러 메시지를 enum으로 하셨네요! 어떤 장점이 있으셨나요?

가장 먼저 에러 메시지를 구조화 할 수 있다는 장점이 있는 것 같습니다. 상수의 타입을 정의하여 컴파일 시에 데이터의 값을 체크할 수 있습니다!

에러 메시지를 enum 클래스로 분리하면서 가독성이 좋아진 것 같습니다. 에러 메시지에 대한 관리를 할 때 하나의 클래스만 신경써주면 되기 때문에 유지보수도 좋을 것 같습니다!

yxxnghwan · 2024-02-18T16:31:00Z

src/main/java/racingcar/message/ErrorMessage.java

+    MESSAGE_ONLY_NATURAL_NUMBER("시도할 회수는 자연수를 입력해 주세요."),
+    MESSAGE_ONLY_NUMBER("시도할 회수는 숫자여야만 가능합니다.");
+
+    private String message;


재할당 해도 되는 변수일까요?

에러 메시지는 실행될 때 변경이 필요가 없을 것 같습니다!
final을 추가하여 의도치 않은 변경을 막을 수 있을 것 같습니다😀

yxxnghwan · 2024-02-18T16:32:10Z

src/main/java/racingcar/controller/GameManager.java

+    private Cars makeNewCars(List<String> carNames) {
+        return new Cars(carNames.stream()
+                .map(Car::new)
+                .toList());
    }


이름 list를 받아서 Cars 를 생성하는 로직이네요! Cars가 해당 책임을 가져가는건 어떨까요?

Cars를 생성하는 로직은 Cars가 책임을 가져가는 것이 좋을 것 같습니다.
새로 Cars를 생성하는 로직이기 때문에 static 메서드로 생성하여 클래스를 통해 직접 호출할 수 있게 하였습니다!

yxxnghwan · 2024-02-18T16:40:13Z

src/main/java/racingcar/validator/InputValidator.java

+
+import static racingcar.message.ErrorMessage.*;
+
+public class InputValidator {


각각의 검증이 필요한 도메인 클래스들이 검증 로직을 나눠 가져가는게 좀 더 응집성 있는 역할분리가 되지 않을까요?

input, output 같은 view에 대한 검증 클래스를 각각 생성하는 것보다 이는 공용 Validator 클래스를 생성하고, 도메인을 검증하기 위한 추가 검증 클래스를 생성하는 것이 역할 분리 측면에서 좋을 것 같습니다!

yxxnghwan · 2024-02-18T16:41:30Z

src/main/java/racingcar/domain/Cars.java

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


만약 이 메소드의 응답을 받은 곳에서 cars에 add()나 remove() 메소드를 호출하면 어떻게 되나요? Cars의 상태가 변경되는걸 방지할 방법은 없을까요?

현재 방식으로는 외부에서 반환받은 cars 리스트에 원소를 추가, 삭제 할 수 있어 상태가 변경 될 가능성이 있습니다.
따라서 반환받은 리스트에 대한 수정을 막기 위해 copyOf 메서드를 추가했습니다!

yxxnghwan · 2024-02-18T16:44:06Z

src/main/java/racingcar/controller/GameManager.java

+    private void moveCars(Cars cars) {
+        for (Car car : cars.getCars()) {
+            moveCar(car);
+        }
+    }


이 동작도 Cars가 가져갈 수 있지 않을까요?

Cars에 메서드를 포함하게 하는 것이 구조적으로 맞는 것 같습니다!
코드를 바꾸면서 moveCar 메서드를 Car에 같이 포함시켰습니다.
하지만 기존에 Car의 move 메서드에 random을 포함시키지 않고 distance++ 자체만을 하게 하였는데, 랜덤하게 움직이는 moveRandomly 메서드를 Car에 추가하는 것이 괜찮은 방법인지에 대한 의문이 들어 여쭤봅니다!

moveRandomly 메소드가 Car에 있으면 Car가 랜덤에 의해 움직인다는걸 알게 되는거 아닐까요? 차가 굳이 랜덤에 의해 움직인다는걸 알 필요가 없을 것 같아요!

핵심 도메인인 Car가 moveRandomly를 아는 것 보다 차라리 일급 컬렉션인 Cars가 가지고 있는 것이 좋을 것 같아 수정하였습니다! 완벽하게 만족하는 건 아니지만 수정해봤습니다!

yxxnghwan · 2024-02-18T16:44:40Z

src/main/java/racingcar/controller/GameManager.java

+    private void moveCar(Car car) {
+        if (RandomGenerator.generateRandomNumber(MAX_RANDOM_NUMBER + 1) >= MINIMUM_MOVEMENT_CONDITION) {
+            car.moveCar();
        }


해당 동작을 테스트하려면 어떻게 하면 좋을까요? 🤔
힌트를 드리자면 인터페이스를 활용해보면 좋을 것 같아요!

MoveCondition이라는 인터페이스를 만들어 자동차의 움직임 여부를 주입받도록 변경하였습니다.
Random 숫자를 테스트 하기 어렵기에 인터페이스로 분리해봤는데 정상적으로 테스트는 완료되지만 주먹구구식으로 구현하여서 구조상으로는 완벽하지 않은 것 같습니다😥

yxxnghwan · 2024-02-18T16:46:42Z

src/main/java/racingcar/controller/GameManager.java


 public class GameManager {

    private static final int MAX_RANDOM_NUMBER = 9;
+    private static final int MINIMUM_MOVEMENT_CONDITION = 4;
+
    OutputView outputView = new OutputView();
    InputView inputView = new InputView();

    public void run() {
        outputView.printCarNameInputMessage();


입력을 받기 위한 메소드인 것 같은데 InputView가 가져가는게 어떨까요?

InputView로 해당 메서드를 분리하여 같이 사용하게 하였습니다!

yxxnghwan · 2024-02-18T16:49:05Z

src/main/java/racingcar/message/ErrorMessage.java

+public enum ErrorMessage {
+    MESSAGE_NO_DUPLICATE_CAR_NAMES("중복된 자동차 이름은 사용할 수 없습니다."),
+    MESSAGE_LENGTH_OF_CAR_NAME("자동차 이름은 1자 이상 5자 이하여야 합니다."),
+    MESSAGE_NO_SPACE("중복된 자동차 이름은 사용할 수 없습니다."),


에러 메시지가 이게 맞을까요?

혹시 어떤 부분을 고치면 좋을지 조금 더 알려주실 수 있나요?

Enum의 이름은 MESSAGE_NO_SPACE 인데 에러 메시지는 이름이 중복되었다는 내용인 것 같아서요!

헉 이런 실수를.. 지적해주셔서 감사합니다!

yxxnghwan · 2024-02-18T16:51:24Z

src/main/java/racingcar/view/InputView.java

@@ -1,19 +1,17 @@
 package racingcar.view;

+import racingcar.message.ErrorMessage;


사용하지 않는 import문은 제거해주세요! 보이스카우트 규칙!

yxxnghwan · 2024-02-19T15:23:58Z

src/main/java/racingcar/domain/Cars.java

+    public List<Car> getCars() {
+        return List.copyOf(cars);
+    }
+
+    public int calculateLongestDistance() {


일반적인 관례상 getter의 경우 다른 public 메소드들보다 아래에 선언하곤 합니다!
메소드 나열하는 순서에 대해서도 고민해보면 좋을 것 같아요!

그런 관례가 있었군요! 앞으로 getter는 마지막에 위치시켜야겠습니다😀

yxxnghwan · 2024-02-19T15:24:43Z

src/main/java/racingcar/domain/Cars.java

+    public static Cars makeNewCars(List<String> carNames) {
+        return new Cars(carNames.stream()
+                .map(Car::new)
+                .toList());
+    }


정적 팩토리 메소드의 위치도 어디에 있으면 좋을지 고민해봐요!

정적 팩토리 메서드는 사용자가 어떻게 사용하는지 빠르게 알려주면 좋기 때문에 메서드 정의 부분의 맨 윗부분에 위치하는게 좋을 것 같습니다!

yxxnghwan · 2024-02-19T15:27:34Z

src/main/java/racingcar/controller/GameManager.java

+
+import java.util.List;
+
+import static racingcar.domain.Cars.makeNewCars;


static 메소드를 import 해주셨네요! 정적 팩토리 메소드의 경우 어떤 클래스의 팩토리 메소드인지를 호출 부분에서 파악하는게 가독성이 좋다고 생각하는데 카피의 생각은 어떠신가요?

정적 팩토리 메서드는 코드를 봤을 때 한번에 어떤 클래스에 속해있는지 바로 파악할 수 있도록 import 하는게 좋을 것 같습니다! 무의식적으로 import를 한 것 같습니다😅

yxxnghwan · 2024-02-19T15:32:32Z

src/main/java/racingcar/domain/Cars.java

+public class Cars {
+
+    private static final String MESSAGE_NOT_EXIST_CAR = "생성된 자동차가 없습니다.";
+    private List<Car> cars;


이 인스턴스 변수는 재할당해도 괜찮도록 설계된 클래스인가요?

경주할 자동차는 처음 입력에서 정해지기 때문에 final을 붙여 불변으로 하는 것이 좋을 것 같습니다!

yxxnghwan · 2024-02-19T15:34:40Z

src/main/java/racingcar/controller/GameManager.java

+    private void moveCars(Cars cars) {
+        for (Car car : cars.getCars()) {
+            moveCar(car);
+        }
+    }


moveRandomly 메소드가 Car에 있으면 Car가 랜덤에 의해 움직인다는걸 알게 되는거 아닐까요? 차가 굳이 랜덤에 의해 움직인다는걸 알 필요가 없을 것 같아요!

yxxnghwan

리뷰 반영 수고하셨습니다 카피! 고민해볼만한 부분들 몇 가지 남겨두었으니 확인 부탁드려요!😁

yxxnghwan

첫 미션 수고하셨습니다 카피! 이번 미션에서 고민할만한 것들은 충분히 하셨다고 보여서 이만 머지하도록 할게요! 앞으로의 우테코 생활도 화이팅입니다!💪

tkdgur0906 added 9 commits February 18, 2024 23:28

refactor : controller 패키지 분리

430487c

refactor : Car가 움직일 때 움직이는 여부를 스스로 정하지 않게 수정

6b1d50e

refactor : 자동차가 움직일 때 거리가 음수인지 검증하는 코드 삭제

db7ea78

refactor : RandomGenerator로 랜덤한 숫자를 생성하도록 수정

6161ec1

refactor : 자동차를 만드는 스트림 코드 간단하게 수정

436012f

refactor : Car 리스트에 대한 일급 컬렉션을 만들어 자동차들의 최장거리, 우승자 찾는 메서드 분리

6421c65

refactor : Input을 검증하는 로직 검증 클래스로 분리

93ddb73

test : 자동차들의 최대 길이 계산 및 우승자 찾는 테스트 작성

b7032e1

refactor : 실행 결과 출력 포맷 올바르게 수정

495237e

yxxnghwan requested changes Feb 18, 2024

View reviewed changes

tkdgur0906 added 10 commits February 19, 2024 10:07

refactor : 에러 메시지의 값을 변경하지 못하게 final 추가

6b35c9b

refactor : Cars를 새로 생성하는 로직에 대한 책임을 Cars로 분리

c084a26

refactor : InputValidator를 Validator로 이름 변경

1cecf05

refactor : get한 Cars 리스트에 대한 수정 불가하게 수정

5c6bbd1

refactor : Cars가 움직이는 기능을 Cars 클래스에 분리

8594d90

style : 미사용 import 삭제 및 라인 정리

b16972a

refactor : 사용하지 않는 변수 제거

9dbc258

refactor : outputView의 입력 받는 문구 출력하는 메서드 InputView로 분리

8874692

refactor : 자동차가 움직이는 여부를 MoveCondition 인터페이스로 분리하여 주입하도록 수정

82fda9c

test : 랜덤한 숫자에 따라 자동차가 움직이는지 테스트

5b1d392

yxxnghwan reviewed Feb 19, 2024

View reviewed changes

yxxnghwan requested changes Feb 19, 2024

View reviewed changes

tkdgur0906 added 7 commits February 20, 2024 13:28

refactor : get 메서드의 위치를 public 메서드 아래에 위치

f4c8b84

refactor : 정적 팩토리 메서드 위치 변경

717971c

refactor : 정적 팩토리 메서드 호출 부분에서 import 취소

1388507

refactor : car List를 재할당 하지 못하도록 final 추가

4dfa252

refactor : moveRandomly 메서드 Cars로 분리

7dfe01d

refactor : 미사용 변수 제거

34eea60

refactor : 에러 메시지 수정

342d609

yxxnghwan approved these changes Feb 20, 2024

View reviewed changes

yxxnghwan merged commit 3b1a6ea into woowacourse:tkdgur0906 Feb 20, 2024


		import static racingcar.message.ErrorMessage.*;

		public class InputValidator {

		@@ -1,19 +1,17 @@
		package racingcar.view;

		import racingcar.message.ErrorMessage;


		import java.util.List;

		import static racingcar.domain.Cars.makeNewCars;

[자동차 경주 2단계] 카피 step2 미션 제출합니다 #789

[자동차 경주 2단계] 카피 step2 미션 제출합니다 #789

Conversation

tkdgur0906 commented Feb 18, 2024

yxxnghwan 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

tkdgur0906 Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdgur0906 Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdgur0906 Feb 19, 2024 • 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

tkdgur0906 Feb 20, 2024 • 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

yxxnghwan left a comment

Choose a reason for hiding this comment

yxxnghwan left a comment

Choose a reason for hiding this comment

tkdgur0906 Feb 19, 2024 •

edited

Loading

tkdgur0906 Feb 20, 2024 •

edited

Loading

tkdgur0906 Feb 19, 2024 •

edited

Loading

tkdgur0906 Feb 20, 2024 •

edited

Loading