[1단계 - 자동차 경주 구현] 현구막(최현구) 미션 제출합니다. #181

Hyeon9mak · 2021-02-07T00:12:50Z

안녕하세요 휴! 현구막이라고 합니다.
제출이 많이 늦어졌습니다. 잘 부탁드립니다!

샐리와 헤어진 후 리팩토링을 진행하면서 접근제한자와 final 키워드를 중점적으로 생각했습니다.
특히 얼마전 3기 동기들끼리 java.lang.String 클래스의 내부 구조에 대해 의견을 나누면서
자바 개발자들이 불변을 얼마나 생각하는지를 고심해보면서 최대한 사용하려고 했습니다.

Car 가 GasTank 를 필드로 갖는데,
Car 가 언제든지 난수가 아닌 다른 요소로 이동을 결정하는 로직을 갖게 될 수도 있다는 생각에
난수를 이용하는 역할을 GasTank 로 분리해보았습니다.

4 가지 궁금한 것이 있습니다!

페어인 샐리와 함께 테스트 코드를 작성하면서 난수는 어떻게 테스트 하는가? 를 고민해보았습니다.
저희는 의외로 결정을 쉽게 내렸는데, 난수라는 것 그 자체에서 테스트가 불가능하지 않은가? 라고 결론을 내렸습니다.
이에 대해서 휴는 어떻게 생각하고 계신지가 궁금합니다!
GasTank, Lap 같이, racingcar의 역할에는 어울리지만
실제 로직을 유추하기 어려운 네이밍이 괜찮은 방식인지, 이름을 달리해야할지 궁금합니다!

메소드가 읽히는 순서대로 정리되어야 하는지, 접근제한자 순서? 메인로직 순서? 로 정리되어야 하는지 궁금합니다.

// 1. 메소드가 읽히는 순서대로
private 생성자 {}
private 생성자 보좌 검증 메서드1 {}
private 생성자 보좌 검증 메서드2 {}
public static 정적 팩터리 메서드 {}
public 메인로직 메서드 {}
private 메인로직 보좌 메서드 {}

// 2. 접근제한자 순서
private 생성자 {}
public static 정적 팩터리 메서드 {}
public 메인로직 메서드 {}
private 생성자 보좌 검증 메서드1 {}
private 생성자 보좌 검증 메서드2 {}
private 메인로직 보좌 메서드 {}

정적 팩터리 메서드를 몇몇 클래스에 적용해보자 했는데, '굳이?' 라는 생각이 드는 클래스들의 경우
정적 팩터리 메서드 없이 일반적인 new 생성자만 사용했습니다.
이펙티브 자바를 통해 접한 정적 팩터리 메서드는 좋으면 좋았지 나쁠건 없는 패턴이었는데,
막상 적용하려니 오히려 new 생성자보다 가독성이 더 떨어지는 것 같아 고민되는 부분이 있었습니다.
실제 현업에서는 어떻게 사용되는지 궁금합니다!

감사합니다!

null 검사 메서드와 빈 문자 검사 메서드 분리

패키지 구조 수정

예외처리 완료

기능 구현 목록 업데이트

경주 화면 출력 기능 횟수에 따라 경주 진행 기능 우승자 계산 기능 우승자 출력 기능

예외처리 완료

기능 구현 목록 업데이트

경주 화면 출력 기능 횟수에 따라 경주 진행 기능 우승자 계산 기능 우승자 출력 기능

fix test variable names add final keyword in test variables fix assertj method

fix test variable names

fix test variable names add final keyword in test variables

fix method name from testCharAt() to testSuccessCharAt() fix test variable names in testCharAtException method add final keyword in testCharAtException method

add final keyword in methods parameter fix displayname methods

delete invalid variable 'result2'

fix const variable name from ZERO to NUMBER_ZERO fix validate method name from makePositiveNumber to validatePositiveNumber

add final keyword in methods parameter fix method name from getDelimiters to toString

fix custom delimiter pattern to static add const variable for custom delimiter pattern

delete calculator.Number class add validate number method in calculator.StringCalculator class delete calculator.NumberTest test class

add package car in racingcar.domain move Car from racingcar.domain to racingcar.domain.car move Cars from racingcar.domain to racingcar.domain.car move Position from racingcar.domain to racingcar.domain.car rename Main to Application rename CarName to Name move Name from racingcar.domain to racingcar.domain.car

refactor Car calss refactor Name class refactor Cars class refactor Position class feat GasTank class

refactor Cars class refactor Name class refactor Position class

rename method in Winners class reduce method in Lap class rename method in Car class add validate null input in Cars class

rename method and variable in RacingCarGameController class add static factory method in Car class

add static factory method in Cars class add static factory method in Lap class rename method in RacingCarGameController class add enterline in Application class

Hue9010

안녕하세요 현구막! 이번 리뷰를 맡은 휴입니다. 😉
굉장히 깔끔히 잘 구현해 주셨네요. 💯
이번 단계는 여기서 머지할게요. 보완하면 좋을 내용과 질문에대한 답변 드렸는데 확인하시고 궁금한 건 언제든 질문 주세요.
고생하셨습니다!

Hue9010 · 2021-02-07T20:49:52Z

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

+
+    private final List<Car> cars;
+
+    private Cars(final String inputNames) {


cars 입장에서는 input이라는 네이밍이 필요없을 것 같아요. 😉

Hue9010 · 2021-02-07T20:51:06Z

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

+
+    private void validateNull(final String inputNames) {
+        if (inputNames == null) {
+            throw new IllegalArgumentException("잘못된 입력입니다.");


메시지 활용 👍

Hue9010 · 2021-02-07T20:53:17Z

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

+        List<String> namesList = new ArrayList<>(Arrays.asList(inputNames.split(COMMA)));
+        Set<String> namesSet = new HashSet<>(namesList);
+        if (namesList.size() != namesSet.size()) {


Collection을 이용한 로직 구현 👍

Hue9010 · 2021-02-07T20:54:57Z

src/main/java/racingcar/domain/car/Name.java

+    Name(final String name) {
+        validateBlank(name);
+        validateLength(name);


꼼꼼하게 validation check를 해주시네요. 👍
Name 스스로도 null 체크를 하면 더 좋을 것 같아요. 😉

접근 제어자를 사용하지 않은 곳들이 꽤 보이는데 특별한 이유가 있을까요?
아니라면 접근 가능한 수준에 맞춰 접근 제어자를 사용해 주세요.

접근 제어자를 사용하지 않은 곳들이 꽤 보이는데 특별한 이유가 있을까요?
아니라면 접근 가능한 수준에 맞춰 접근 제어자를 사용해 주세요.

package-private 접근제어자를 활용해보려고 했습니다!
한 눈에 의도파악이 되지 않았으니, private-private을 포기하고 다른 명시적인 접근제어자를 이용하는게 좋을까요?

Hue9010 · 2021-02-07T21:04:33Z

src/main/java/racingcar/domain/Lap.java

+        try {
+            return validatePositive(Integer.parseInt(inputLap));
+        } catch (NumberFormatException e) {
+            throw new IllegalArgumentException("숫자가 아닌 입력입니다.");


NumberFormatException extends IllegalArgumentException {

NumberFormatException은 IllegalArgumentException을 상속받은 상태랍니다. 😉
알고 계시지만 메시지를 사용하기 위함으로 보이긴 하네요.

Hue9010 · 2021-02-07T21:23:56Z

src/test/java/racingcar/domain/RandomNumberTest.java

+    @Test
+    @DisplayName("지정된 범위의 난수 생성")
+    void random_number_make() {
+        final int minimumNumber = 0;
+        final int maximumNumber = 9;
+        for (int i = 0; i < 999999; i++) {
+            int madeNumber = RandomUtils.nextPositiveInt(maximumNumber, maximumNumber);
+            assertThat(madeNumber).isBetween(minimumNumber, maximumNumber);
+        }
+    }


if (startInclusive > endInclusive) { throw new IllegalArgumentException(); } if (startInclusive < 0) { throw new IllegalArgumentException(); } if (startInclusive == endInclusive) { return startInclusive; }

에 대한 테스트가 있어야 하지 않을까요? 🤔

페어인 샐리와 함께 테스트 코드를 작성하면서 난수는 어떻게 테스트 하는가? 를 고민해보았습니다.
저희는 의외로 결정을 쉽게 내렸는데, 난수라는 것 그 자체에서 테스트가 불가능하지 않은가? 라고 결론을 내렸습니다.
이에 대해서 휴는 어떻게 생각하고 계신지가 궁금합니다!

난수이기 때문이 아니라 java.util.Random이기 때문에 믿고 사용할 수 있다고 생각합니다. 그게아니라면 List, Set, ... 등도 모두 테스트를 해줘야겠죠?
만약 Random이 외부 라이브러리였다면 고민이 됐을텐데 이미 많은 사람들에 의해 검증이 된 라이브러리라면 믿고 사용 할 것 같아요. 그게 아니라면 java.util.Random이 있는데 굳이 신뢰 할 수 없는 라이브러리를 사용 할 지 고민을 하지 않고 Random을 사용할 것 같네요.

Hue9010 · 2021-02-07T21:39:31Z

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

+        this.position = new Position();
+        this.gasTank = new GasTank();
+    }
+
+    static Car enrollWithName(final String name){
+        return new Car(name);
+    }
+
+    public void forward() {
+        if (gasTank.isEnoughGas()) {


GasTank, Lap 같이, racingcar의 역할에는 어울리지만 실제 로직을 유추하기 어려운 네이밍이 괜찮은 방식인지, 이름을 달리해야할지 궁금합니다!

GasTank는 생각치 못했던 방식인데 굉장히 좋아 보여요. 👍
게다가 isEnoughGas라니!
Lap도 충분히 좋아보입니다. turn, round 등의 이름도 가능하긴 할 것 같은데 저도 영어가 엄청 익숙한편이 아니다보니 어느 게 더 어울리는지는 확실치 않지만 기억을 더듬어 보면 레이싱 게임을 할 때 Lap이라는 표현을 봤던걸로 기억했을 때 Lap도 좋아 보입니다. 😉

사실 이 위치에 답변을 드리는 이유가 있는데요.
car.forward를 테스트하기 힘들지 않던가요? 🤔
GasTank 내부적으로 랜더값을 생성하여 그런데 차량의 가스탱크에 주유하는 것 처럼 일정량의 가스를 주입 받는건 어떨까요?(Gas를 만들 필요 없이 int로 주입 받아도 충분해 보입니다. 😉)
외부에서 주입 받게 되면 Car.forward에 대한 테스트도 가능해 질 겁니다. (지금 테스트는 여러번 수행하다보면 한두번씩 실패합니다.)

아...! car.forward 자체가 확률적인 전진이라 테스트가 실패하기도 한다는걸 생각 못했습니다!!!

Hue9010 · 2021-02-07T22:01:41Z

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

+    private Car(final String name) {
+        this.name = new Name(name);
+        this.position = new Position();
+        this.gasTank = new GasTank();
+    }
+
+    static Car enrollWithName(final String name){
+        return new Car(name);
+    }


정적 팩터리 메서드를 몇몇 클래스에 적용해보자 했는데, '굳이?' 라는 생각이 드는 클래스들의 경우
정적 팩터리 메서드 없이 일반적인 new 생성자만 사용했습니다.
이펙티브 자바를 통해 접한 정적 팩터리 메서드는 좋으면 좋았지 나쁠건 없는 패턴이었는데,
막상 적용하려니 오히려 new 생성자보다 가독성이 더 떨어지는 것 같아 고민되는 부분이 있었습니다.
실제 현업에서는 어떻게 사용되는지 궁금합니다!

enrollWithName이 결국 정적 팩터리 메서드가 아닌가요? 아니면 적용해봤는데 살짝 마음에 안드는 걸까요? 🤔
of, from, ...의 네이밍은 결국 이름의 문제일 뿐 해당 이름이 아니라고 정적 팩터리 메서드가 아닌건 아닙니다. 이펙티브 자바의 네이밍 규칙은 수학 공식처럼 불변의 진리가 아닌지라 이펙티브 자바 저자가 권장하는 네이밍 규칙일 뿐입니다.

아이템 1. 생성자 대신 정적 팩터리 메서드를 고려하라. (3판 기준)

아이템 이름에 나와 있다시피 고려하라가 중요할 것 같습니다. 지금은 그렇게 객체를 생성할 때 필요한 값들이 복잡하지 않습니다. 그런면에서 취할 수 있는 장점은 네이밍을 통한 가독성인데 방금 말씀 드린 것처럼 생성 시 복잡할 부분이 없어 네이밍을 통해 얻을 수 있는 이점도 크진 않은 상황으로 보여요. 그런 면에서 단순하게 적용해보지 않고 고민 해보신 점 굉장히 좋네요. 👍

Car 객체의 경우 제 입장에선 String 인자가 자동차의 이름으로 등록된다는 걸 알지만,
다른 사람이 제 코드를 읽을 땐 "자동차에 String을 넘겨서 무얼 하려는거지?"
와 같은 상황이 발생할 수 있을 것 같아 팩터리 메서드를 활용했습니다.

반대로 Name 객체의 경우 객체의 이름 자체가 인자의 용도를 가늠하게 해주는 것 같아
팩터리 메서드 없이 new 생성자만을 이용하도록 해봤습니다.

저는 당장 위의 이유 때문에 저리 구현했지만, 실제 현업에서는 팩터리 메서드로 감싸는 이유가
저와 다르지 않을까 하는 궁금증(조바심? ㅎㅎ..)에 질문을 드렸습니다.

Car 객체의 String 인자는 이름으로 활용될 것이다 라는 걸 프로젝트의 README를 통해 이해하고 코드를 읽는다
혹은
Car 객체의 String 인자를 name 이라고 명명했으므로, 충분히 유추 가능하다

Car 객체의 상황을 위 2가지로 해석한다면 제가 팩터리 메서드로 감싼 것이 사실 불필요한 것이 아니었나 하는 궁금증이 생겨 질문드리게 되었습니다!

Hue9010 · 2021-02-07T22:08:01Z

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

+    private final Position position;
+    private final GasTank gasTank;
+
+    private Car(final String name) {


메소드가 읽히는 순서대로 정리되어야 하는지, 접근제한자 순서? 메인로직 순서? 로 정리되어야 하는지 궁금합니다.

접근 제한자 기준으로 했을 때의 장점이 크게 떠오르지 않네요. 🤔 해당 내용은 Clean Code - 형식 맞추기(Form)에 잘 정리되어 있어서 참고하면 좋을 것 같아요. 😉

Hue9010 · 2021-02-07T22:11:55Z

README.md

+## 🔥 기능 구현 목록
+- [x] 이름 입력받는 기능


깔끔히 잘 정리해주셨네요. 체크 박스 활용까지 👍👍

Yeonwoo Cho and others added 30 commits February 3, 2021 17:23

test: 쉼표가 포함된 문자열 분리 테스트 구현

df750c3

test: 쉼표가 포함되지 않은 문자열 분리 테스트 구현

f3bd6af

test: 문자열 자르기 테스트 구현

c529635

test: 문자열 특정 위치의 값 반환 테스트 구현

53a7c0a

test: 문자열의 올바르지 않은 위치 호출 시 예외 처리 구현

b629d09

chore: 패키지 study 생성

639e5ab

test: Set 테스트 기본 설정 구현

2d87808

test: 집합 크기 확인 테스트 구현

7ed8858

test: 집합 원소 포함 확인 테스트 구현

de66134

test: 집합 원소 포함과 미포함 모두 확인 테스트 구현

1edf266

test: 문자열 덧셈 계산기 테스트 초기 구현

e89964f

feat: 빈 문자 또는 null 입력시 0 반환 코드 구현

7325551

refactor: 빈 문자 또는 null 입력시 0 반환 코드 수정

90c0e46

null 검사 메서드와 빈 문자 검사 메서드 분리

feat: 숫자로만 이루어진 문자열 입력시 숫자 반환 코드 구현

7e430ad

feat: 쉼표 구분자로 분리한 후 합을 반환하는 코드 구현

f7f737c

feat: 여러 구분자로 분리한 후 합을 반환하는 코드 구현

bca9aea

feat: 커스텀 구분자로 분리하는 경우 코드 구현

b790a1b

feat: 음수 혹은 숫자 이외 값 전달 시 예외 처리 코드 구현

0738bab

refactor: Number 객체 분리

8614d2a

docs: 자동차 경주 게임 기능 구현 목록 작성

da756d1

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

4dc75c3

feat: 이름이 중복되는 경우 예외 처리 코드 구현

9d44104

feat: 양 끝에 쉼표가 있는 경우 예외 처리 코드 구현

8381288

feat: 쉼표가 연달아 나오는 경우 예외 처리 코드 구현

770f891

feat: 경주 횟수 생성 코드 구현

85aac29

패키지 구조 수정

횟수 입력받는 기능 구현

f247b97

예외처리 완료

자동차 전진 기능 구현

d998acf

랜덤 넘버 생성 기능 구현

9184481

기능 구현 목록 업데이트

컨트롤러 구현 완료

851d320

경주 화면 출력 기능 횟수에 따라 경주 진행 기능 우승자 계산 기능 우승자 출력 기능

feat: 횟수 입력받는 기능 구현

b912489

예외처리 완료

dusdn1702 and others added 24 commits February 5, 2021 18:50

feat: 자동차 전진 기능 구현

3d8e4ff

feat: 랜덤 넘버 생성 기능 구현

7d3ce4c

기능 구현 목록 업데이트

feat: 컨트롤러 구현 완료

750d086

경주 화면 출력 기능 횟수에 따라 경주 진행 기능 우승자 계산 기능 우승자 출력 기능

Merge remote-tracking branch 'origin/dusdn1702' into dusdn1702

55dab37

test: fix String.split method success test

37dff84

fix test variable names add final keyword in test variables fix assertj method

test: fix String.split method unusual test

25afc3b

fix test variable names

test: fix String.substring method success test

b317f50

fix test variable names add final keyword in test variables

test: fix String.charAt method test

7f1fac9

fix test variable names add final keyword in test variables

test: fix String.charAt, String.charAt exception test

d00bb66

fix method name from testCharAt() to testSuccessCharAt() fix test variable names in testCharAtException method add final keyword in testCharAtException method

test: fix SetTest

d32189c

add final keyword in methods parameter fix displayname methods

test: fix StringCalculator custom delimiter test

0b789af

delete invalid variable 'result2'

refactor: fix Number class

5a3b820

fix const variable name from ZERO to NUMBER_ZERO fix validate method name from makePositiveNumber to validatePositiveNumber

style: add enterline in Number class

2818d30

refactor: fix calculator.Delimiters class

e1abb96

add final keyword in methods parameter fix method name from getDelimiters to toString

refactor: add final keyword in methods parameter

05187f2

refactor: fix calculator.StringCalculator class

090ef35

fix custom delimiter pattern to static add const variable for custom delimiter pattern

refactor: fix calculator package

2509125

delete calculator.Number class add validate number method in calculator.StringCalculator class delete calculator.NumberTest test class

refactor: fix racingcar.car package

79a3063

refactor Car calss refactor Name class refactor Cars class refactor Position class feat GasTank class

refactor: fix racingcar.car package

0f4b45c

refactor Cars class refactor Name class refactor Position class

refactor: fix racingcar package

2671a97

rename method in Winners class reduce method in Lap class rename method in Car class add validate null input in Cars class

refactor: fix racingcar package

9e05fce

rename method and variable in RacingCarGameController class add static factory method in Car class

refactor: fix racingcar package

d8f2899

add static factory method in Cars class add static factory method in Lap class rename method in RacingCarGameController class add enterline in Application class

refactor: add lineSeparator in racingcar.view.OutputView class

aedbfa0

Hue9010 approved these changes Feb 7, 2021

View reviewed changes

Hue9010 merged commit 8187468 into woowacourse:hyeon9mak Feb 7, 2021

Hyeon9mak mentioned this pull request May 8, 2021

[Spring 지하철 노선도 관리 - 1, 2단계] 현구막(최현구) 미션 제출합니다. woowacourse/atdd-subway-map#94

Merged


		private final List<Car> cars;

		private Cars(final String inputNames) {

		## 🔥 기능 구현 목록
		- [x] 이름 입력받는 기능

[1단계 - 자동차 경주 구현] 현구막(최현구) 미션 제출합니다. #181

[1단계 - 자동차 경주 구현] 현구막(최현구) 미션 제출합니다. #181

Uh oh!

Conversation

Hyeon9mak commented Feb 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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!

Uh oh!

Hyeon9mak commented Feb 7, 2021 •

edited

Loading