[자동차 경주 1단계] 카피 pr 요청 드립니다 #687

tkdgur0906 · 2024-02-15T07:42:36Z

안녕하세요 카피입니다!

[클래스 설명]
● Car : 자동차 정보와 moveCar()를 통해 이동 여부를 정할 수 있습니다.
● RandomGenerator : 0~9 사이의 랜덤한 숫자를 반환하는 역할을 합니다.
● GameManager : 자동차 경주 프로그램을 실행합니다.

[궁금한 점]
인텔리제이에서 단축키로 함수를 추출하면 자동으로 private static 메서드로 만들어지는 것을 확인했습니다! 페어와 그 이유에 대해 얘기해 보았는데 명확한 답을 찾지 못하였습니다.
private을 쓰는 이유는 외부로의 접근을 금지하는 것이고, static을 쓰는 이유는 해당 메서드가 인스턴스가 아닌 클래스에 종속되게 하기 위함이다 라고 생각하는데, static이 아닌 기존 public 메서드에서 추출한 private 메서드를 static으로 바꿔주는 이유를 납득하지 못하였습니다. 이와 관해서 제가 잘못 생각하는 부분이 있다면 인사이트를 얻고 싶습니다!!

yxxnghwan

안녕하세요 카피! 이번 미션 리뷰하게 된 알렉스입니다.
미션 잘 구현해주셨네요!

전체적으로 테스트코드가 많이 비어있는 것 같아요. 테스트 코드를 작성하기 까다로운 부분도 있었을 것 같은데 왜 테스트코드 작성이 어려웠을지 고민해보면 좋을 것 같습니다!

리뷰 내용중 이해하기 어려운 부분이 있다면 DM 남겨주세요! 😁

yxxnghwan · 2024-02-15T17:39:05Z

README.md

+## 기능 구현 목록
+- [X] 자동차 이름 입력 받는다.


체크리스트 작성 👍🏿

yxxnghwan · 2024-02-15T17:42:03Z

build.gradle

+    // https://mvnrepository.com/artifact/org.mockito/mockito-core
+    testImplementation group: 'org.mockito', name: 'mockito-core', version: '5.10.0'


이거 없이 테스트할 수 있는 방법은 없을까요?
RandomGenerator 때문에 사용하신 것 같은데 다른 방법을 고민해봐도 좋을 것 같아요!

랜덤한 수를 생성하는 부분을 테스트하기 위해서 명확한 이해 없이 사용하였습니다😢
오히려 mock을 사용해서 좀 더 좋은 코드를 작성하기 위한 고민을 못한 것 같습니다!
한번 mock을 사용하지 않고 테스트할 수 있는 방법을 고민해보겠습니다!!

yxxnghwan · 2024-02-15T17:47:32Z

src/main/java/racingcar/GameManager.java

+public class GameManager {
+    OutputView outputView = new OutputView();
+    InputView inputView = new InputView(outputView);
+    List<Car> cars = new ArrayList<>();


List<Car> 라는 타입으로 선언하고 해당 클래스에서 관련 로직이 존재하는데, 클래스를 분리하여 로직을 응집시킬 수는 없을까요?
일급 컬렉션 에 대해 알아보면 좋을 것 같아요.

더불어서 굳이 cars가 인스턴스 변수일 필요가 있을지도 고민해보면 좋을 것 같습니다.

일급 컬렉션을 알아보았는데, List를 Wrapping하여 그 외 다른 멤버 변수가 없는 상태를 말하는 것이라고 이해했습니다!
하지만 GameManager의 인스턴스 변수일 필요가 있을까에 대해서 고민하였는데, List를 인스턴스 변수가 아닌 메서드 안에서 다루는 방법이 더 좋은 구조라고 생각하였습니다.
이에 따라 makeNewCars 메서드에서 리스트를 반환하는 방식으로 코드를 변경하였습니다😊

yxxnghwan · 2024-02-15T17:50:39Z

src/main/java/racingcar/domain/Car.java

+        if (randomNumber >= 4) {
+            distance++;
+        }
+        if (distance < 0) {


매직넘버는 상수로 분리하면 어떨까요? 상수의 네이밍으로 해당 값의 의미도 나타내줄 수 있을 것 같아요.

처음에는 단순한 숫자를 상수로 분리하는 것이 가독성을 높일 수 있을까 의문이 들어 분리하지 않았습니다.
하지만 그냥 숫자를 영어로 바꾸는 것이 아닌 네이밍을 통해 읽는 사람에게 하여금 명확한 의미를 나타낼 수 있다고 하신 부분에 대해 공감하여 상수로 분리하였습니다!!🤣

yxxnghwan · 2024-02-15T17:53:04Z

src/main/java/racingcar/domain/RandomGenerator.java

+import java.util.concurrent.ThreadLocalRandom;
+
+public class RandomGenerator {
+    private static final ThreadLocalRandom random = ThreadLocalRandom.current();


해당 클래스를 사용하신 이유가 무엇인가요? 🤔

랜덤한 수를 mock을 통해 테스트할 때 fix하기 위해서 생성한 클래스입니다😂
테스트만을 위해 불필요한 클래스를 생성했다고 생각하여, 해당 클래스 없이 테스트를 진행하는 방법을 고민해보고자 합니다!

에고 이 부분 의도가 잘못 전달된 듯 하네요..!😂
저는 랜덤 번호를 뽑아주는 역할을 하는 클래스의 존재 자체는 필요하다고 봤어요!
ThreadLocalRandom 을 사용한 의도를 여쭤본 거였습니다!

yxxnghwan · 2024-02-15T18:00:02Z

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

+        int tryCount;
+        try {
+            tryCount = Integer.parseInt(input);
+            validateNaturalNumber(tryCount);
+        } catch (NumberFormatException e) {
+            System.out.println(MESSAGE_ONLY_NUMBER);
+            return getTryCount();
+        } catch (IllegalArgumentException e) {
+            System.out.println(e.getMessage());
+            return getTryCount();
+        }
+        return tryCount;


얼리 리턴으로 라인 수 줄일 수 있을 것 같네요!

return을 try안에서 하도록 변경하였는데, 제가 한 방식이 제안해주신 방식과 동일한지 궁금합니다!

yxxnghwan · 2024-02-15T18:01:34Z

src/test/java/racingcar/domain/CarTest.java

+        Assertions.assertThat(afterDistance).isEqualTo(beforeDistance);
+    }
+}


파일 끝에 개행문자 추가하는건 어떨까요? EOF에 대해 알아보면 좋을 것 같아요.

POSIX 명세에 파일 마지막에 개행문자가 있어야 한다고 적혀있다고 하네요😮 가끔 깃허브에서 경고 표시가 있었던 것을 신경쓰지 않고 넘겼던게 EOF 였던 것을 알게되었습니다! 앞으로 개행문자를 지우지않고 남겨놓도록 하겠습니다!

yxxnghwan · 2024-02-15T18:06:48Z

src/main/java/racingcar/domain/Car.java

+
+    // 0 ~ 9 까지의 랜덤한 수를 생성해 4이상일 경우 전진, 4미만일 경우 정지한다.
+    public void moveCar() {
+        int randomNumber = RandomGenerator.getRandomNumberUnderTen();


Car가 랜덤 값에 의해서 움직인다는걸 알 필요가 있을까요?
몰라도 된다면 어떻게 코드를 고칠 수 있을까요? 고민해보면 좋을 것 같습니다!

말씀하신 내용을 고민해보았는데, Car가 랜덤한 수를 생성하는 것을 알 필요가 있나? 라는 생각이 들었습니다.
Car는 움직이는 것에만 집중하도록 랜덤한 수를 외부에서 파라미터로 받게 코드를 수정하였습니다!!

yxxnghwan · 2024-02-15T18:16:20Z

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

+    public InputView(OutputView outputView) {
+        this.outputView = outputView;
+    }


InputView에서 OutputView를 들고있는 이유가 있을까요?

입력을 받기 위해서는 입력 메시지를 먼저 출력해야 하기 때문에 이를 하나의 메서드에서 처리하기 위해 InputView가 OutputView를 의존하게 설계하였었습니다!
질문하신 것을 보고 올바른 방법인가 생각해보았는데, InputView는 input만을 관리하는 역할을 하는 것이 바람직하지 않을까 하는 생각이 들어 클래스를 분리하였습니다😊

yxxnghwan · 2024-02-15T18:28:14Z

질문 답변을 놓쳤네요!

[궁금한 점]
인텔리제이에서 단축키로 함수를 추출하면 자동으로 private static 메서드로 만들어지는 것을 확인했습니다! 페어와 그 이유에 대해 얘기해 보았는데 명확한 답을 찾지 못하였습니다.
private을 쓰는 이유는 외부로의 접근을 금지하는 것이고, static을 쓰는 이유는 해당 메서드가 인스턴스가 아닌 클래스에 종속되게 하기 위함이다 라고 생각하는데, static이 아닌 기존 public 메서드에서 추출한 private 메서드를 static으로 바꿔주는 이유를 납득하지 못하였습니다. 이와 관해서 제가 잘못 생각하는 부분이 있다면 인사이트를 얻고 싶습니다!!

https://youtrack.jetbrains.com/issue/IDEA-298640/Extracted-method-is-always-static

한번 찾아보았는데 특정 인텔리제이 버전에서 발현되는 버그로 보이네요. 제 로컬에 설치된 인텔리제이에선 재현되지 않고 있습니다!
따로 설정이 있나 찾아봤는데, 존재하지 않는 것 같아요!

…List<car> 제거

tkdgur0906 · 2024-02-17T09:40:15Z

아직 테스트코드에 익숙치 않고 마감시간에 맞추기 위해서 구현할 때 소홀히 한 부분이 있는 것 같습니다! 다음 미션부터는 구현 레벨에서부터 테스트 코드가 필요한 기능들의 테스트를 작성하면서 미션 수행해보도록 노력해보겠습니다!

함수 추출할 때 static이 붙는 것은 설정 문제인 것 같군요🤔 찾아봐주셔서 감사합니다!

yxxnghwan

수고하셨습니다 카피! 이번 단계는 이만 머지하도록 할게요! 남겨둔 코멘트에 대해서는 다음 단계에서 반영해주시면 될 것 같습니다! 2단계도 화이팅입니다! 🚀

yxxnghwan · 2024-02-17T14:43:49Z

src/main/java/racingcar/domain/Car.java

+        if (number >= MINIMUM_MOVEMENT_CONDITION) {
+            distance++;
+        }


특정 숫자 이상이면 차가 전진한다 라는 규칙은 차가 아는게 좋을까요? 고민해보시면 좋을 것 같아요!

자동차가 전진하는 조건에 대한 규칙을 자동차가 알 필요는 없는 것 같습니다! 자동차는 전진하는 것에 책임만 가지고 메서드를 호출하는 부분에서 특정 숫자인지 검증하는 부분을 추가하였습니다!

yxxnghwan · 2024-02-17T14:45:35Z

src/main/java/racingcar/domain/Car.java

+        if (distance < ZERO) {
+            throw new IllegalArgumentException("이동 거리는 음수가 될 수 없습니다.");
+        }


메소드가 시작할 때 갑자기 차의 상태가 무결한 지를 검증하고 있네요. 해당 메소드로 인해 변하는 부분에 대해서만 검증하면 어떨까요?
distance가 음수인 차는 존재하면 안될 것 같고, distance가 음수가 세팅될 수 있는 코드 부분에서 검증하는게 좋을 것 같아요!

메서드를 만들 때 많은 것을 검증하고자 하여서 불필요한 검증이 포함된 것 같습니다😅
distance가 음수가 될 코드 부분이 없는 것 같아 해당 코드를 삭제하였습니다!

yxxnghwan · 2024-02-17T14:47:26Z

src/main/java/racingcar/GameManager.java

+
+    private void moveCars(List<Car> cars) {
+        for (Car car : cars) {
+            car.moveCar(ThreadLocalRandom.current().nextInt(MAX_RANDOM_NUMBER + 1));


ThreadLocalRandom 을 사용하신 이유가 있나요?

해당 클래스는 Math.random()이나 Randdom 클래스와 어떤 차별점이 있고 뭘 위해 사용하셨나요?

Random은 seed를 설정하지 않으면 컴퓨터의 현재 시간을 seed로 사용합니다.
만약 멀티스레드 환경에서 동일한 시간에 여러 요청이 들어온다면 중복된 난수를 생성하지 않기 위해 스레드끼리 경합을 하게 됩니다. 이는 멀티 스레드 환경에서 성능 저하를 일으킬 수 있기 때문에 ThreadLocalRandom을 사용하였습니다.

ThreadLocalRandom은 Random처럼 하나의 인스턴스를 전역적으로 사용하는 것이 아닌, 스레드 별로 각각의 인스턴스를 할당하게 됩니다. 따라서 Random에서 발생할 수 있는 성능 저하를 해결 할 수 있다는 차별점이 있습니다.

현재 미션에서는 멀티 스레드 환경을 고려하지 않아도 무방하지만, ThreadLocalRandom 대신 Random을 사용할 이유가 없다고 생각하여 사용하였습니다!

yxxnghwan · 2024-02-17T14:49:40Z

src/main/java/racingcar/GameManager.java

+        return carNames.stream()
+                .map(carName -> new Car(carName))
+                .toList();


Suggested change

return carNames.stream()

.map(carName -> new Car(carName))

.toList();

return carNames.stream()

.map(Car::new)

.toList();

yxxnghwan · 2024-02-17T14:51:10Z

src/main/java/racingcar/view/OutputView.java

+    public void printWinners(List<Car> cars) {
+        int longestDistance = cars.stream()
+                .mapToInt(Car::getDistance)
+                .max()
+                .orElseThrow(() -> new IllegalArgumentException(MESSAGE_NOT_EXIST_CAR));
+
+        String winnerNames = String.join(", ",
+                cars.stream()
+                        .filter(car -> car.getDistance() == longestDistance)
+                        .map(Car::getName)
+                        .toList()
+        );
+
+        System.out.println(winnerNames + "가 최종 우승했습니다.");
+    }


이 부분은 승자를 찾는 비즈니스 로직이 View에 존재하는 걸로 보이네요!

List<Car>에 대한 일급 컬렉션을 생성하여 Car들의 최장거리 찾는 로직과 승자를 찾는 로직을 분리하였습니다.
해당 로직은 View가 아닌 Cars가 가지고 있어야 한다고 생각하여 변경하였습니다!!

yxxnghwan · 2024-02-17T14:53:15Z

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

+    private final static String MESSAGE_LENGTH_OF_CAR_NAME = "자동차 이름은 1자 이상 5자 이하여야 합니다.";
+    private final static String MESSAGE_NO_SPACE = "중복된 자동차 이름은 사용할 수 없습니다.";
+    private final static String MESSAGE_ONLY_NUMBER = "시도할 회수는 숫자여야만 가능합니다.";
+    private final static String MESSAGE_ONLY_NATURAL_NUMBER = "시도할 회수는 자연수를 입력해 주세요.";


이 검증은 View에서만 해도 괜찮을까요? View는 코어 로직보다 쉽게 교체될 수 있고, 만약 다른 형태의 View로 교체된다면, 검증 로직이 날라가겠네요..🤔

View에서 직접 검증을 하지 않고 별도의 InputValidator 클래스를 생성하여 InputView의 검증을 하도록 변경하였습니다!
리팩토링을 하면서 에러 메시지도 별도의 클래스가 관리하는 것이 좋을 것 같아 enum으로 관리하도록 하였습니다!

yxxnghwan · 2024-02-17T14:55:54Z

src/main/java/racingcar/domain/RandomGenerator.java

+import java.util.concurrent.ThreadLocalRandom;
+
+public class RandomGenerator {
+    private static final ThreadLocalRandom random = ThreadLocalRandom.current();


에고 이 부분 의도가 잘못 전달된 듯 하네요..!😂
저는 랜덤 번호를 뽑아주는 역할을 하는 클래스의 존재 자체는 필요하다고 봤어요!
ThreadLocalRandom 을 사용한 의도를 여쭤본 거였습니다!

tsulocalize added 19 commits February 14, 2024 16:30

docs : 기능 구현 목록 작성

6a49f58

docs : 기능 구현 목록 추가

a441e90

feat : 자동차 이름 입력 기능 구현

0ab8f8d

feat : 자동차 게임 시도할 회수 입력 기능 구현

492af6c

feat : 랜덤 조건에 따라 자동차를 움직이는 기능 구현

a180b01

feat : 자동차 게임 실행 결과 출력하는 기능 구현

c71801d

게임 완료 후 우승자 출력 기능 구현

79f811c

refactor : 사용자 입력 전, 입력 메세지 출력 기능 추가

8265649

feat: 자동차 게임 실행 기능 구현

d593791

refactor : moveCar 메서드에 작동 조건에 대한 주석 추가

a12bc57

refactor : InputView 클래스의 getCarName() 메서드 분리 및 에러 메시지 상수 처리

fccb4fc

refactor : InputView 클래스의 getTryCount() 메서드 분리 및 에러 메시지 상수 처리

0725d76

refactor : OutputView 클래스의 에러 메시지 상수 처리

0256e7c

refactor : GameManger 클래스 생성 및 게임 실행 기능 분리

f5681ac

refactor : package 분리

a2bb927

refactor : 출력 메세지 추가 및 format 변경

93d4553

refactor : 랜덤 숫자 생성 기능 외부 클래스로 분리

ff58cf0

test : moveCar() 조건에 따라 전진 혹은 정지하는지 테스트

d142726

style : 코드 스타일 정리

e91b81a

tkdgur0906 changed the title ~~[자동차 경주] 우아한 테크코스 백엔드 6기 카피 pr 요청 드립니다~~ [자동차 경주 1단계] 우아한 테크코스 백엔드 6기 카피 pr 요청 드립니다 Feb 15, 2024

yxxnghwan requested changes Feb 15, 2024

View reviewed changes

tkdgur0906 added 8 commits February 17, 2024 16:35

refactor : InputView가 OutputView를 의존하지 않도록 수정

f94ac96

refactor : 랜덤한 수를 생성하는 기능을 Car 내부에서 분리하고 파라미터로 랜덤한 수를 받도록 수정

537396b

refactor : RandomGenerator 클래스 삭제

954493f

refactor : mock을 사용하지 않고 Car가 움직이는지 검증하는 테스트 작성

3d6fa4b

refactor : GameManager의 makeNewCar 메서드가 리스트를 반환하도록 변경하여 클래스의 인스턴스 변수 …

7037759

…List<car> 제거

refactor : 매직넘버를 상수로 분리

69522c6

refactor : 메서드가 try 내부에서 얼리리턴하도록 수정

1cd6a33

refactor : 파일 끝 개행문자 추가

beb56cb

tkdgur0906 added 2 commits February 17, 2024 18:32

test : 자동차 이름 조건을 테스트하는 코드 작성

e7703ab

refactor : 테스트 코드 매직넘버 상수 분리

39abf8b

yxxnghwan approved these changes Feb 17, 2024

View reviewed changes

yxxnghwan merged commit 9982161 into woowacourse:tkdgur0906 Feb 17, 2024

tkdgur0906 changed the title ~~[자동차 경주 1단계] 우아한 테크코스 백엔드 6기 카피 pr 요청 드립니다~~ [자동차 경주 1단계] 카피 pr 요청 드립니다 Sep 16, 2024

		## 기능 구현 목록
		- [X] 자동차 이름 입력 받는다.

		// https://mvnrepository.com/artifact/org.mockito/mockito-core
		testImplementation group: 'org.mockito', name: 'mockito-core', version: '5.10.0'

[자동차 경주 1단계] 카피 pr 요청 드립니다 #687

[자동차 경주 1단계] 카피 pr 요청 드립니다 #687

Conversation

tkdgur0906 commented Feb 15, 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

tkdgur0906 Feb 17, 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

Choose a reason for hiding this comment

tkdgur0906 Feb 17, 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

yxxnghwan commented Feb 15, 2024

tkdgur0906 commented Feb 17, 2024

yxxnghwan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdgur0906 Feb 18, 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

tkdgur0906 Feb 18, 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

tkdgur0906 Feb 17, 2024 •

edited

Loading

tkdgur0906 Feb 17, 2024 •

edited

Loading

tkdgur0906 Feb 18, 2024 •

edited

Loading

tkdgur0906 Feb 18, 2024 •

edited

Loading