[1단계 - 자동차 경주 구현] 케빈(박진홍) 미션 제출합니다. by xlffm3 · Pull Request #165 · woowacourse/java-racingcar

xlffm3 · 2021-02-06T05:16:12Z

안녕하세요?

페어와 함께 다음 내용을 고민했습니다.

Cars는 String(자동차 이름들)을 인자로 받는 정적 팩토리 메서드만 public이고 기본 생성자(List cars)는 private입니다.
Cars의 move는 랜덤값을 생성해 리스트에 속한 자동차들을 움직이게 하는데, 이 때문에 Cars에서 우승한 자동차의 명단을 추출하는 테스트를 작성하기 곤란한 상황입니다.
기본 생성자를 public으로 두고 아래와 같이 테스트를 작성해볼 수 있을 것 같은데요.

@Test
void getWinnerNames_우승자는_pobi_brown이다() {
	 Car pobi = new Car("pobi");
	 Car brown = new Car("brown");
   Car jiko = new Car("jiko");
	 List<Car> cars = new Cars(Arrays.asList(pobi, brown, jiko));

   pobi.move(4);
   brown.move(4);
   List<String> winnerNames = cars.getWinnerNames();
   
   assertThat(winnerNames).containsExactly("pobi", "brown");
}

프로덕션 코드에서 사용하지 않는 기본 생성자를 테스트를 위해 public으로 두는 것이 맞을지 고민입니다. 테스트 하기 어려운 구조라는 것은 아직 리팩토링할 여지가 많다고 생각되는데, 이에 대해 리뷰어님의 의견을 듣고싶습니다!

쉼표 혹은 콜론을 구분자로 가지는 문자열을 구분자를 기준으로 분리한 다음 숫자의 합을 반환한다.

구분자와 숫자를 제외한 문자 혹은 음수가 문자열에 포함되어 있으면 RuntimeException을 던진다

test 클래스에 있던 StringCalulator를 main클래스로 이동한 후, 테스트 확인 완료

10라인 이상의 메소드를 수정하였고, 메소드가 한가지 일만 하도록 수정함

Cars 클래스 생성 테스트

tryCount가 0이면 false를 반환한다.

tdd를 진행하며 사용했던 클래스트들을 test에서 삭재함.

Car 클래스 접근지정자 변경

Livenow14

- 메서드 인라인화 할 수 있는 부분은 인라인화한다. - 인스턴스 변수는 클래스 선언 사이에 공백을 추가한다. - 정적 팩토리 메서드 네이밍 규칙에 맞게 of를 from으로 변경한다.

- 콘솔의 화면을 담당하는 Screen 추상 클래스 및 하위 클래스를 삭제한다. - 기존 InputView와 OutputView가 해당 스크린 클래스들의 역할을 수행한다.

choihz

안녕하세요, 케빈! 리뷰어 코니입니다.
첫 미션을 아주 잘 구현해 주셨네요 🏅 컨벤션 지키기, 메서드 분리, 의미 있는 메서드 네이밍, 기능 목록 정리와 기능별 커밋 모두 훌륭합니다 👍 남겨드린 피드백은 생각해 보시고 다음 단계에서 반영해 주세요.

말씀하신 대로 Cars가 RandomUtils와 강하게 결합되어 있어 테스트 코드 작성이 어려운 상황이네요. 물론 케빈의 제안대로 테스트를 위해 생성자를 오픈할 수도 있겠지만 이번 미션의 경우엔 더 좋은 방법이 있을 것 같아요! 피드백에 남겨드린 힌트를 참고해서, 어떻게 하면 이 결합을 분리할 수 있을지 고민해 보세요.

더 궁금한 점이 있으시다면 언제든 코멘트나 DM 주세요~

choihz · 2021-02-06T06:09:39Z

src/test/java/racing/domain/CarsTest.java

+class CarsTest {
+
+    @Test
+    void Cars_쉼표로_구분된_문자열_받으면_객체가_생성된다() {


메서드 이름은 영어로 간단히 짓고, 처음에 학습 테스트를 할 때 사용했던 @DisplayName으로 한글 이름을 보여주면 어떨까요? 매번 _를 쓸 수고도 줄어들고 가독성도 더 좋아질 것 같네요!

choihz · 2021-02-06T06:10:28Z

src/test/java/racing/domain/CarsTest.java

+    void Cars_쉼표로_구분된_문자열_받으면_객체가_생성된다() {
+        String carNames = "pobi,brown";
+        assertThatCode(() -> Cars.generate(carNames))
+                .doesNotThrowAnyException();


실행한 코드가 어떤 예외도 던지지 않는다는 사실만으로 "객체가 생성"됨을 보장할 수 있을까요?

확실히 추가적인 테스트가 필요하겠네요!!

choihz · 2021-02-06T06:15:17Z

src/main/java/racing/domain/dto/CarDto.java

@@ -0,0 +1,26 @@
+package racing.domain.dto;


dto는 domain의 하위 개념이 아니에요! 패키지 위치가 부적절한 것 같네요~

choihz · 2021-02-06T06:16:16Z

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

+    public Car(String name) {
+        this.name = name;
+        this.position = DEFAULT_POSITION;
+        validateName();


생성자 안에서 입력값을 검증하고 있군요 👍 하지만, 입력값을 먼저 검증한 이후에 인스턴스 변수에 할당해 주세요!

페어와 방어적 복사에 대한 개념을 잘못 이해하고 유효성 검증 코드를 짰네요... 😓😓

choihz · 2021-02-06T06:21:22Z

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

+        }
+    }
+
+    public boolean move(int randomNumber) {


Car 입장에서, 받는 숫자가 "random"인지 알 필요가 있을까요?

또한 현재 이 메서드는 자동차가 움직였을 때 true, 움직이지 않았을 때 false를 리턴하고 있네요. 그리고 이 반환값은 테스트에서만 쓰이고 있어요! 테스트를 다른 방법으로 할 수 있다면, 이 메서드도 이름이 암시하는 것처럼 움직이는 한 가지 일만 할 수 있지 않을까요?

choihz · 2021-02-06T06:25:04Z

src/main/java/racing/domain/Cars.java

+    public List<String> findWinnerNames() {
+        int maxPosition = getMaxPosition();
+        return cars.stream()
+                .filter(car -> car.getPosition() == maxPosition)


람다와 스트림을 쓰셨군요 👍
여기서는 Car의 위치 정보를 가져와 maxPosition과 직접 비교하고 있는데요, 이 역할을 Car에게 위임하면 어떨까요? 이것을 "메시지를 보낸다"고 표현하곤 하는데요, 이렇게 메시지를 보내면 어떤 장점이 있을지도 생각해 보아요.

choihz · 2021-02-06T06:27:40Z

src/main/java/racing/docs/요구사항.md

기능 목록 정리 및 기능별 커밋 👍👍

choihz · 2021-02-06T06:31:42Z

src/main/java/racing/domain/Cars.java

+    }
+
+    public void race() {
+        cars.forEach(car -> car.move(RandomUtils.getRandomNumber(START_NUMBER, END_NUMBER)));


Cars가 RandomUtils와 강하게 결합하고 있네요~ 이렇게 되면 Cars의 테스트가 굉장히 어려워지죠!
다음 단계에서 이 결합을 어떻게 분리시킬지 고민해 보고, 테스트 코드도 작성해 보아요! (힌트: 전략 패턴)

choihz · 2021-02-06T06:38:30Z

src/test/java/racing/domain/CarTest.java

+    void Car_유효하지_않은_이름_예외가_발생한다(String name) {
+        assertThatThrownBy(() -> {
+            new Car(name);
+        }).isInstanceOf(IllegalArgumentException.class);


예외를 던질 때 예외 상황에 맞는 메시지를 담아주면, 아래와 같이 더 정밀하게 검증할 수 있겠죠?

assertThatThrownBy(() -> { // ... }).isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("이름은 5자 이하만 가능합니다.");

choihz · 2021-02-06T06:42:13Z

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

+        }
+        Matcher matcher = PATTERN.matcher(this.name);
+        if (!matcher.matches()) {
+            throw new IllegalArgumentException();


이름 유효성 검증 👍
하지만 예외에 어떤 메시지도 담겨있지 않네요. 그래서 실제로 프로그램을 실행해 잘못된 이름을 입력했을 때, 사용자는 영문도 모른 채 영어로 된 이상한 메시지만 보게 돼요. 상황에 맞는 적절한 예외 메시지를 담아주면 어떨까요?

Livenow14 and others added 30 commits February 3, 2021 15:04

feat: String 클래스에 대한 학습 테스트

c811e2b

feat: Set Collection에 대한 학습 테스트

19ac667

docs: 문자열 계산기 요구사항 정리

f336109

feat: null 또는 반문자가 들어올 시 0을 반환한다

1b2447b

feat: 숫자 하나를 문자열로 입력할 경우 해당 숫자를 반환한다

73a50a5

feat: 쉼표구분자

48b5b7d

feat: 쉼표 혹은 콜론구분자

1af69a3

쉼표 혹은 콜론을 구분자로 가지는 문자열을 구분자를 기준으로 분리한 다음 숫자의 합을 반환한다.

feat: 커스텀 구분자 지정

78d3eb7

feat: 예외 처리 구현

042b56d

구분자와 숫자를 제외한 문자 혹은 음수가 문자열에 포함되어 있으면 RuntimeException을 던진다

feat: StringCalulator클래스 main패키지로 이동

436eed1

test 클래스에 있던 StringCalulator를 main클래스로 이동한 후, 테스트 확인 완료

refactor: 메소드 길이, 역할 수정

9c0733a

10라인 이상의 메소드를 수정하였고, 메소드가 한가지 일만 하도록 수정함

refactor: Code Convention

39b7eb7

docs: 자동차 경주 구현할 기능 목록 정리

d7590ac

feat: 자동차 객체 생성시 이름 유효성 체크

a99f843

feat: 쉼표를 기준으로 구분된 문자열을 받으면 Cars를 생성한다

ba3fd5c

Cars 클래스 생성 테스트

feat: 쉼표가 아닌 다른 구분자가 들어오면 에러가 발생한다

bc4eae5

feat: 전진조건에 맞을 경우 Car객체의 포지션이 증가한다

518b49a

feat: 랜덤값을 생성하는 객체 구현

f64324e

feat: Cars에 속한 자동차 객체를 파라미터값에 의해 움직이게 하는 기능 구현

51d30d5

feat: 게임 시도 횟수 입력 구현

cb0fb0c

feat: 게임이 한번 실행될때 tryCount가 줄어든다.

b10a043

tryCount가 0이면 false를 반환한다.

feat: 시도 횟수가 양수가 아니면 에러가 발생한다.

81eb58b

feat: Car객체가 하나일 경우 에러가 발생한다.

350f4fe

feat: 우승자 탐색 서비스 구현 완료

a731e2b

feat: 각 회차별 실행 결과를 출력한다.

7a80bfa

feat: 가장 멀리 이동한 1명 이상의 우승자 명단을 탐색해 출력한다.

38be250

refactor: test 클래스 내의 클래스 삭제

69419e6

tdd를 진행하며 사용했던 클래스트들을 test에서 삭재함.

refactor: CarDto 객체 생성 방식 변경

b6fc770

Car 클래스 접근지정자 변경

refactor: GameScreen 네이밍 수정

57b1c8e

Merge pull request #1 from Livenow14/livenow14

0be9843

Livenow14

xlffm3 added 3 commits February 5, 2021 09:33

style: 포맷을 통일한다

cf45d87

- 메서드 인라인화 할 수 있는 부분은 인라인화한다. - 인스턴스 변수는 클래스 선언 사이에 공백을 추가한다. - 정적 팩토리 메서드 네이밍 규칙에 맞게 of를 from으로 변경한다.

refactor: Screen 역할을 InputView와 OutputView로 이동

5d839e5

- 콘솔의 화면을 담당하는 Screen 추상 클래스 및 하위 클래스를 삭제한다. - 기존 InputView와 OutputView가 해당 스크린 클래스들의 역할을 수행한다.

style: 패키지명 camel case 제거

40efac1

choihz approved these changes Feb 6, 2021

View reviewed changes

choihz merged commit 7c3a084 into woowacourse:xlffm3 Feb 6, 2021

Conversation

xlffm3 commented Feb 6, 2021

Uh oh!

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

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