[자동차 경주] 베디 #1

dpudpu · 2019-11-17T09:27:28Z

생각보다 어렵네요.
지난번보다 잘하고 싶은 마음에 너무 포장하다보니 복잡해진 감도 있네요.
부담없이 편하게 피드백 주시면 감사하겠습니다.

- 현재 자동차 상황 - 현재 선두 자동차 구하기

wbluke

안녕하세요 베디 :)
역시 잘 짜시네요ㅎㅎ 구조도 저랑 비슷해서 신기했어요. 다른 분들은 어떠실지 모르겠지만ㅎㅎ
모든 리뷰는 다 반영하실 필요 없고 동의하시는 부분만 반영 부탁드립니다!

wbluke · 2019-11-17T12:03:10Z

src/main/java/racingcar/domain/Position.java

+package racingcar.domain;
+
+public class Position {
+    private static final int UNIT_INCREASE = 1;


👍👍👍

wbluke · 2019-11-17T12:06:23Z

src/main/java/racingcar/domain/Position.java

+    static Position newInstance() {
+        return new Position(DEFAULT_POSITION);
+    }
+
+    Position increase() {
+        return new Position(position + UNIT_INCREASE);
+    }


Position 객체로 감싸면서 같은 position값을 가진 인스턴스가 많이 생성될 수도 있을 것 같은데요,
pool을 만들어 캐싱해서 관리해 보는 것은 어떨까요?

미처 생각못했는데 좋은 피드백 감사합니다 ! ㅎㅎ

wbluke · 2019-11-17T12:07:34Z

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

+    public boolean isSamePosition(int position) {
+        return getPosition() == position;
+    }


사용하지 않는 메소드인 듯 한데, 남겨둔 의도가 있으실까요?

깜박했네요 ㅎㅎ; 지우겠습니다.

wbluke · 2019-11-17T12:08:52Z

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

+    public Cars(final String names) {
+        this.cars = createCars(names);
+        if (cars.size() < MINIMUM_NAMES) {
+            throw new IllegalArgumentException("2명 이상 입력해주세요.");


MINIMUM_NAMES 상수와 메시지를 연동하면 좋을 것 같아요 :)

다른 예외 메시지들도 비슷한 것 같은데, 같이 적용해 보면 어떨까요ㅎㅎ 상수가 변경되었을 경우에 동적으로 메시지도 변하도록!

맞습니다! 변경할게요 좋은 피드백 감사합니다 ㅎㅎ

wbluke · 2019-11-17T12:16:56Z

src/main/java/racingcar/service/dto/CarDto.java

+
+import java.util.Objects;
+
+public class CarDto {


wbluke · 2019-11-17T12:19:21Z

src/main/java/racingcar/domain/RaceStatus.java

+    public List<CarDto> getCurrentWinners() {
+        final int maxOfPosition = carDtos.stream()
+                .max(Comparator.comparingInt(CarDto::getPosition))
+                .get().getPosition();


.get() 대신에
isPresent() 확인 후 .get()을 하거나
orElseThrow()로 예외를 던지는 것은 어떨까요?

empty 상황은 없을거라고 생각하고 처리해주지 않았는데, 이야기 듣고보니 앞에서 그런 검증도 없고 있더라도 처리해주는 것이 맞는거 같네요.

좋은 피드백 감사합니다 ㅎㅎ

wbluke · 2019-11-17T12:26:54Z

src/main/java/racingcar/console/App.java

+        final MoveStrategy moveStrategy = new RandomNumberMoveStrategy();
+        final RacingService racingService = new RacingService(moveStrategy);
+
+        final String names = InputView.inputNames();
+        final Cars cars = racingService.createCars(names);
+
+        final int repeatNumber = InputView.inputRepeatNumber();
+        final RaceResult raceResult = racingService.startRace(repeatNumber, cars);


실행 결과 예외 상황이 조금 더 있을 수도 있을 것 같은데요.
제가 조금 빡빡하게 예외를 체크하는 편이라 그렇지만

중복된 자동차 이름이 있을 경우

red, blue, green 이런 식으로 공백을 포함하여 사용자가 이름을 입력하는 경우

이런 경우에 대해서는 어떻게 생각하세요?
크게 동의하지 않으시면 넘어가주셔도 좋습니다 :)

그리고 현재 예외가 발생하면 그냥 런타임 에러가 콘솔에 표시되면서 터지는데요.
에러 로그를 날것으로 보여주는 것보다는
try-catch로 잡아서 OutputView를 통해 예외 상황을 사용자에게 알려주는 것은 어떨까 제안드려 봅니다 :)

1.중복된 자동차 이름이 있을 경우 일부러 허용해줬습니다.
2. red, blue, green 공백은 오랜만에 해서 감을 잃었는지 미처 생각을 못했네요.
3. 제가 예외처리에 소홀했네요.

좋은 피드백 감사합니다 ㅎㅎ

wbluke · 2019-11-17T12:30:19Z

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

+        if (cars.size() < MINIMUM_NAMES) {
+            throw new IllegalArgumentException("2명 이상 입력해주세요.");
+        }


Cars 일급콜렉션에 2대 이상의 car가 들어있어야 한다는 제약 조건이 있는데요.
제 생각에는 일급콜렉션 자체의 제약이라기 보다는 비즈니스 로직 상의 제약 조건 같아서
외부의 다른 곳에서 체크하는 것이 더 맞다고 생각했어요.
Cars 일급콜렉션도 비어있는 리스트나 Car가 한 대만 들어있는 일급콜렉션도 충분히 있을 수 있을 것 같아서요.
이 부분은 어떻게 생각하세요? :)

저는 일급콜렉션을 사용하는 의도 중 하나가 비지니스에 종속적인 자료구조 라고 생각해요.
단순히 포장만 해준다면 래퍼클래스라고 부르는 게 맞지 않을까? 생각합니다. (사실 이부분은 맞는 표현인지는 모르겠네요.)

자세한 설명은 제가 말하는 것보다는 갓동욱님의 글을 남겨볼게요.
https://jojoldu.tistory.com/412 의 1. 비지니스에 종속적인 자료구조 부분을 보시면 좋을거 같아요.

오 이해가 되었습니다ㅎㅎ 감사해요

Deocksoo

안녕하세요 베디 :)
전반적으로 코드가 탄탄하고, 읽기 쉽게 잘 작성해주신 것 같아요 👍
커밋이력도 깔끔하게 잘 남겨주셔서 따라가기가 편했어요!
몇 군데 피드백 드렸는데, 베디 의견은 어떤지 궁금해요. 확인 부탁드려요~

Deocksoo · 2019-11-17T13:48:31Z

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

+    @DisplayName("이동")
+    void shouldMoveTest2() {
+        // given
+        int expected = car.getPosition() + 1;


이 부분에서 상수를 하드코딩하는 대신에, Position에 정의해두신 상수를 사용하면 어떨까요?
CarsTest나 PositionTest에서도 마찬가지로요!

그러네요 !! 감사합니다 ㅎㅎ

Deocksoo · 2019-11-17T13:50:02Z

src/test/java/racingcar/domain/CarsTest.java

+import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+class CarsTest {


정상적인 문자열 입력에 대한 테스트도 있으면 좋을 것 같아요!

감사합니다.
이런 상황일때 드는 생각인데, 정상 입력 테스트는 어떻게 해야하나? 예외처리가 발생하지 않는지 확인하면 될까?

@Test void 정상_이름_입력_성공() { assertDoesNotThrow(()-> new Cars("pobi, crong, pol")); }

흐음.. 이 부분은 제가 너무 추상적으로 생각했네요. 약간 까다로운 부분이군요. 지금 떠오르는 방법은 아래 세가지정도인데요,

말씀해주신대로 예외 발생하지 않는지 확인

나중에 사용할 거라고 생각하고 cars에 contains나 size같은 public 메서드를 만든다. 그런데 contains를 만들려면 car간의 equality에 대한 정의도 되어있어야 할것같으니 좀더 생각해봐야 할거같고..

문자열 입력과 관련된 부분이 테스트가 어려우니, 문자열을 parsing하고 자동차들의 list를 만들어내는 기능을 cars로부터 분리해낸다.

3번과 같은 방법에 대해서는 어떻게 생각하시나요? cars는 자기가 가지고 있는 객체들을 관리하는 책임만 가지고 있고, 객체의 생성과 관련된 로직은 분리하면 어떨까 싶어서요.

말씀해주신 부분 다 좋은 방법이라고 생각합니다.

다만 3번 같은 경우 저는 개인적으로 선호하지 않는데요. 생성하는 방법이 다양하지 않으면 (예를 들어 로또처럼 자동, 수동인 경우) 같이 관리해주는 방식이 낫더라고요.

예전에 WAS때 모든 부분에 대하여 parser 클래스를 만들어봤는데 복잡해지더라고요.
HeaderParser, CookieParser, RequestLineParser 등...

그래서 저는 1번이나 2번이 괜찮은 방법 같아요.
이부분은 저도 아직 잘 모르겠네요..ㅎㅎ;

좋은 피드백 감사합니다.!

Deocksoo · 2019-11-17T14:17:25Z

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

+        return new Car(name);
+    }
+
+    public void shouldMove(final MoveStrategy moveStrategy) {


shouldMove는 반드시 움직여야 한다는 느낌을 주는것 같아요. 메서드 이름으로 tryMove는 어떤가요?

제가 생각하는 should는 해야한다 도 있지만 (아마) …일 것이다(예상·추측을 나타냄) 라는 의미도 있다고 생각해서 했는데.
다시 생각해보니 이건 의미가 좀 다른거 같네요 try가 더 나은 거 같아요. 감사합니다.

Deocksoo · 2019-11-17T15:01:07Z

src/test/java/racingcar/domain/CarsTest.java

+        // given
+        Cars cars = new Cars("pobi,crong,honux");
+        int movedPosition = Position.DEFAULT_POSITION + 1;
+        List<Integer> expectedPosition = List.of(movedPosition, movedPosition, movedPosition);


혹시 List.of를 jdk 1.9에서부터 지원하나요? 1.8에서는 없는 메서드로 뜨는데, 버전 차이가 맞겠죠..?

9 버전부터가 맞네요!

네. 말씀을 드려야했는데, 죄송합니다 ㅠ
저는 Arrays.asList() 쓰는 게 불편해서 자바9 사용하고 있습니다.

Deocksoo · 2019-11-18T01:50:58Z

src/main/java/racingcar/domain/movestrategy/MoveStrategy.java

+import racingcar.domain.car.Car;
+
+public interface MoveStrategy {
+    boolean isAvailableMove(final Car car);


메서드 인자로 car를 넣어주는 이유가 있나요?

처음엔 테스트를 수월하게 하기 위해서 넣었습니다. (car의 이름이 pobi일 때만 true) 이런식으로 하기 위해서요. 하지만 테스트를 위한 코드라서 다시 수정 하려다가 나중에 경우에 따라서 car가 필요할 수 도 있겠다 싶어서 유지했습니다.

Deocksoo · 2019-11-18T02:00:26Z

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

+                .collect(Collectors.toList());
+    }
+
+    public List<Car> shouldMove(final MoveStrategy moveStrategy) {


메서드가 CarDto List를 리턴하게 하면 어떨까요?

저도 확실한 불변을 위해서 고민했는데, CarDto를 반환하면 CarDto에 의존이 생겨서 과연 맞을까?
entity가 dto에 의존하면 안되듯이 Cars에서 하는게 맞을까? 고민하다가 서비스에서 처리해줬습니다.

이 부분에 대해서는 어떻게 생각하시나요?

아하, 의존성의 방향을 고려하면 베디가 말씀해주신게 맞는것 같아요! 덕분에 또 배웠네요 :)

아닙니다. 아직 이부분은 저도 확실하지 않아서요.
모든 부분에 정답은 없고 가끔은 개발 편의성이 더 중요하다고 생각해서 DTO를 반환하는 방식도 좋다고 생각합니다. 아니면 새로운 Domain을 만드는 방법도 있을거 같고요.

요구사항의 수정이 잦은 서비스영역인 경우에 dto를 반환하는 방식이 좋지 않겠네요. 내려주는 데이터를 변경할 때마다 매번 도메인 로직을 변경해야 할테니까요. 이 부분은 상황을 조금 더 고려해야겠네요.

Conatuseus

안녕하세요. 베디 ✋
자소서 쓰느라 늦었네요 죄송해요.
그리고 앞에서 다들 리뷰를 잘 해 주셔서 해드릴게 별로 없네요.
DTO 사용, final, 모든 원시값 포장 등이 인상적이고 잘 구현하셨네요.
질문과 리뷰 남겼습니다. 확인 부탁드릴게요. 💯

Conatuseus · 2019-11-20T09:10:45Z

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

+    private InputView() {
+    }


private 기본 생성자를 써준 이유를 알 수 있을까요?

모든 메소드가 static 이여서 인스턴스를 생성할 필요가 없습니다. 이를 방지하기 위해서 private으로 해줬습니다.

Conatuseus · 2019-11-20T09:11:29Z

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

+        final String joinedWinners = winners.stream()
+                .map(CarDto::getName)
+                .collect(Collectors.joining(", "));


joining 사용 👍

Conatuseus · 2019-11-20T09:12:15Z

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

+    private final Name name;
+    private Position position;


포장 👍

Conatuseus · 2019-11-20T09:15:40Z

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

+    }
+
+    private void validateCarSize() {
+        if (cars.size() < MINIMUM_NAMES) {


저는 if 조건을 메서드화 시키는게 좋다고 생각하는데,
어떻게 생각하시나요?
예를들어 위 cars.size() < MINIMUM_NAMES 같은 경우엔
isInvalidCarSize 라던지...

Conatuseus · 2019-11-20T09:16:53Z

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

+    }
+
+    private void validateLengthOfName() {
+        if (this.name.length() > MAX_BOUNDARY || this.name.length() < MIN_BOUNDARY) {


여기에도 아까 말씀 드렸듯이 if 조건을 메서드로 분리 시키면 어떨까요?

메소드가 복잡하면 좋은 방법이라고 생각하지만, 현재는 이 로직밖에 없는데 또 분리시키면 복잡하지 않을까요? 현재 메소드명으로 충분히 설명이 가능하다고 생각해요

Conatuseus · 2019-11-20T09:23:48Z

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

+        // given
+        int expected = car.getPosition();
+
+        // when
+        car.tryMove(x -> false);
+        int actual = car.getPosition();
+
+        // then


given, when, then 👍

CarDto가 도메인에 침투 코드여서 변경함 CarDto 대신 Car로 대체하고 완벽한 불변객체로 수정

dpudpu added 20 commits November 16, 2019 15:48

docs: 간단한 설계 작성

250dc42

feat: Name VO

1f1b185

feat: Position VO

33bceb2

feat: Car

bcbe5b5

feat: Cars 문자열로 List<Car> 생성 구현

42111ac

refactor: MoveStrategy.isAvailableMove() 파라미터 추가

81b6874

feat: Cars 이동 및 우승 구하기

7ef12bb

feat: Cars 이름 2명 미만 입력시 예외처리

e538057

refactor: Car, Cars move() -> shouldMove()로 이름 변경

6b0ff66

feat: RepeatNumber (시도할 회수값 포장)

1323fbe

refactor: Cars.shouldMove() 리턴타입 void -> List<Car> 변경

b738501

feat: RaceStatus, CarDto

f45db21

- 현재 자동차 상황 - 현재 선두 자동차 구하기

refactor: Cars.getWinners() 제거 -> RaceStatus 로 위임

48d6c6e

feat: RaceResult

5aa31ed

feat: RaceResult Iterator 구현

7f9766b

feat: RacingService

5061815

feat: RandomNumberMoveStrategy

0dd39e3

feat: View, Main 메소드 구현

0942191

style: 포맷팅

531f355

refactor: OutputView 메소드 추출

5d58965

dpudpu requested review from skygl, wbluke and Deocksoo November 17, 2019 10:59

dpudpu assigned skygl, Deocksoo and wbluke Nov 17, 2019

dpudpu removed request for skygl, Deocksoo and wbluke November 17, 2019 11:04

dpudpu unassigned skygl Nov 17, 2019

dpudpu assigned Conatuseus Nov 17, 2019

wbluke requested changes Nov 17, 2019

View reviewed changes

dpudpu added 5 commits November 17, 2019 22:57

refactor: domain 패키지 세분화

b083a2b

feat: RaceStatus Empty 검사 추가

7a452ed

feat: Name 공백처리 추가

6e81dd1

refactor: 상수와 에러메시지 연동

83db448

feat: Position 캐싱 추가

4afecb7

Deocksoo reviewed Nov 18, 2019

View reviewed changes

dpudpu added 6 commits November 18, 2019 11:35

refactor: Test코드 매직넘버 프로덕션 코드의 상수로 변경

24b7e46

test: CarsTest 이름 정상 입력 테스트 추가

cc83425

refactor: Car, Cars 의 shouldMove() -> tryMove() 이름 변경

2fb4281

feat: Name null 검사 추가

65b3ba2

feat: RepeatNumber null 검사 추가

b0ca4ab

style: Position 공백

ec4a347

Conatuseus reviewed Nov 20, 2019

View reviewed changes

dpudpu added 2 commits November 20, 2019 18:36

feat: 예외 발생시 사용자에게 메시지 표시

afb53fe

refactor: CarDto 제거 -> Car로 대체

edd5e30

CarDto가 도메인에 침투 코드여서 변경함 CarDto 대신 Car로 대체하고 완벽한 불변객체로 수정

wbluke approved these changes Nov 25, 2019

View reviewed changes

dpudpu merged commit 1b471a0 into woowacourse-mission-review:dpudpu Dec 5, 2019

[자동차 경주] 베디 #1

[자동차 경주] 베디 #1

Conversation

dpudpu commented Nov 17, 2019 • edited Loading

wbluke 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpudpu Nov 17, 2019 • 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

Deocksoo left a comment • 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

Deocksoo Nov 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpudpu Nov 18, 2019 • 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

Deocksoo Nov 18, 2019 • 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

Deocksoo Nov 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deocksoo Nov 18, 2019 • edited Loading

Choose a reason for hiding this comment

Conatuseus 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

dpudpu commented Nov 17, 2019 •

edited

Loading

dpudpu Nov 17, 2019 •

edited

Loading

Deocksoo left a comment •

edited

Loading

Deocksoo Nov 18, 2019 •

edited

Loading

dpudpu Nov 18, 2019 •

edited

Loading

Deocksoo Nov 18, 2019 •

edited

Loading

Deocksoo Nov 18, 2019 •

edited

Loading

Deocksoo Nov 18, 2019 •

edited

Loading