[1단계 - 자동차 경주 구현] 히이로(문제웅) 미션 제출합니다. #499

MoonJeWoong · 2023-02-09T07:56:40Z

안녕하세요 제이온님.
이번 자동차 경주 미션은 페어와 함께 진행하면서 '일급 컬렉션을 객체답게 잘 사용해보자'는 것을 메인 목표로 진행했습니다.
일급 컬렉션 클래스에 대한 getter 메소드를 최대한 지양하는데 집중한 시간이 가장 많았습니다.
부족하지만 리뷰 잘 부탁드리겠습니다!

세부사항 자동차의 위치를 증가 시킬 수 있는 기능 추가 자동차 이름을 심표로 구분하는 기능을 당담 클래스 수정

세부사항 - random 하게 0~9 범위 내에 한 자리 숫자를 반환한다.

세부사항 - 자동차 대수는 1대 이상이어야 한다. - 자동차 이름은 쉼표로 구분한다.

세부사항 자동차 대수는 1대 이상이어야 한다.

세부사항 - 총 시도 횟수는 1 이상의 양의 정수 값이어야 한다. - 자동차 경주 클래스를 전 경기를 진행하는 것이 아니라 한 라운드를 진행하는 것으로 역할 수정

세부사항 - 자동차 이름에 대한 사용자 입력을 받는다. - 총 시도 횟수에 대한 사용자 입력을 받는다. - 자동차 경주를 진행한다. - 경주 결과를 출력하고 프로그램을 종료한다.

세부사항 - RacingGame 클래스에서 담당하던 자동차 이동 기능을 Car 클래스로 이전

세부사항 - RacingGame 클래스 제거 - RacingGame 클래스 기능을 Cars로 이관 - Car 클래스 내 getter 사용 지양을 위한 기능 추가 - domain 클래스 리팩토링으로 인한 controller 및 view 수정

pjy1368

안녕하세요! 이번 리뷰어를 맡은 제이온입니다 🙇‍♂️
getter를 지양하면서 객체에 메시지를 던지려고 노력하신 모습이 인상적이었습니다!
첫 미션치고 굉장히 깔끔하게 코드를 작성해주신 것 같아서 소소한 피드백 위주로 남겨뒀습니다. 반영해보시면서 코멘트 달아주세요~

docs/README.md

src/main/java/racingcar/domain/NumberGenerator.java

pjy1368 · 2023-02-09T15:12:21Z

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

+    public String getMovePosition() {
+        StringBuilder stringBuilder = new StringBuilder();
+        for (int i = 0; i < position; i++) {
+            stringBuilder.append("-");
+        }
+        return stringBuilder.toString();
+    }


해당 메서드는 View에게 전달하기 위한 출력값을 반환하는 용도로 보입니다.
Domain이 View를 알게 되면 어떠한 단점이 있을까요?

해당 부분은 View 단에서는 단순 입력과 출력만을 담당해야 한다고 생각하고 페어와 진행하다보니 과하게 Domain으로 책임이 전가된 부분이라고 생각합니다. 프리코스 때 똑같은 실수를 했었는데 아쉽네요... 😂

아래는 제이온의 질문을 받고 제가 정리해본 생각들입니다.

단순히 출력을 위한 로직이 도메인에 위치하게 된다면 처음 코드를 읽는 사람으로 하여금 "이 부분이 왜 도메인 로직일까?" 라고 생각을 하게 될 소지가 생기게 된다는 것이 단점이라고 생각합니다.

만약 view가 model에게 알려지게 된다면 이후에 model에서 로직이 수정되어야 할 때 model만 수정해야 하는 것이 아니라 view까지 수정해야 하는 강한 의존성이 생겨버리게 되는 것이 단점이라고 생각합니다.

훌륭합니다! 바로 이해하시네요~
2번째 단점에서 추가로 view가 수정되면 model도 수정되어야 한다는 점도 있습니다 ㅎㅎ
예를 들어 포지션만큼 출력을 하는 정책이 현재는 '-'지만, 추후 '*'로 바뀌게 된다면 도메인 로직도 바뀌어야 하는 것이죠!

pjy1368 · 2023-02-09T15:17:26Z

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

+
+    public List<String> readCarNames() {
+        System.out.println(READ_CARS_NAME_MESSAGE);
+        Scanner scanner = new Scanner(System.in);


readCarNames() 메서드를 호출할 때마다 Scanner 객체를 생성하게 되는데, InputView 필드에 Scanner 객체를 두는 건 어떨까요?

inputView 내부에 사용자 입력 값 validation기능을 추가하고 나서 해당 기능을 테스트하기 위해 불가피하게 Scanner 객체의 scope를 메소드 안으로 넣어줄 수 밖에 없다고 판단했던 상황이었습니다. (입출력이 포함된 기능에 대한 테스트)

처음부터 입력값 검증 기능을 view에 위임하지 않고 도메인 클래스 내부에서 검증하거나 차선책으로 validation 클래스를 따로 만들어서 테스트를 진행할 수 있도록 했으면 어땠을까 하는 아쉬움이 남습니다.

InputView 생성자에 scanner를 받을 수 있게 하고, 테스트 코드에서는 scanner 객체를 만들어 InputView 객체에 넣어주면 어떨까요? System.in 테스트는 아래 아티클을 참고하시면 좋을 듯합니다.
https://steadyjay.tistory.com/m/10

그리고 입력값 검증은 View와 Domain이 나눠 가져 2차 검증을 해야하므로 View에서도 검증은 이루어져야하며, 개인적으로 validation 클래스는 굳이 만들기보다는 View, Domain 내부에서 진행해도 된다고 생각합니다~

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

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

pjy1368 · 2023-02-09T15:26:56Z

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

+        for (Car car : cars) {
+            car.goForward();
+        }
+        return Collections.unmodifiableList(cars);


오호 unmodifiableList를 사용하셨군요. 단순히 cars를 반환하지 않고, unmodifiableList()는 사용하면 어떠한 장단점이 있을까요?

moveEachCar 메소드는 컬렉션 내 모든 car 인스턴스들을 전진 시키고 결과 출력을 위해 해당 컬렉션을 반환하는 메소드입니다. 외부에서 경주에 참여 중인 자동차들을 담고 있는 리스트가 의도치 않게 수정되는 것을 방지하기 위해 unmodifiable을 사용하였습니다.

unmodifiableList를 사용하면 외부에서 의도치 않은 원본 컬렉션에 대한 수정 연산을 방지할 수 있는 것이 장점이라 생각합니다.

그리고 단점이라고 한다면 원본 컬렉션에 대한 완전한 불변을 보장하지 않는다는 것 정도로 생각이 듭니다.

좋은 의견입니다 👍
추가로 단점을 말씀드리자면, unmodifiableList로 반환된 List는 수정이 발생하면 예외를 던지기 때문에 예외 핸들링을 필수로 해야 합니다. 하지만 같이 협업하는 동료 개발자는 단순히 getCars()라는 네이밍만 보았을 때 수정 가능한 가변 List로 생각할 확률이 높으므로 예외가 발생했을 때 당황할 확률이 높답니다 ㅎㅎ (참고로 코틀린은 이런 문제를 해결하기 위해 List와 MutableList 타입이 구분되어 있습니다!)

pjy1368 · 2023-02-09T15:27:55Z

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

+
+    public Cars(List<Car> cars) {
+        validateCarsSize(cars);
+        this.cars = new ArrayList<>(cars);


단순히 this.cars = cars가 아닌 new ArrayList<>(cars)로 리스트를 재할당한 이유가 있을까요?

Cars 클래스는 생성자 매개변수로 외부에서 생성되어 전달되는 list를 가집니다. 지금은 그런 위험이 없지만 이후 프로젝트 규모가 커지게 되면 혹시 외부에서 실수로 Cars 생성자 매개변수로 전달한 리스트를 그대로 놔뒀다가 수정하는 일이 발생할 수도 있다고 생각했습니다. 이렇게 되면 Cars 내부의 list 변수에도 영향을 줄 것이라 생각해서 재할당을 수행해줬습니다. 혹시 제가 잘못 생각한 부분이 있다면 피드백 주시면 좋겠습니다!

전체적인 흐름은 맞습니다!
방어적 복사에 대해 좀 더 자세히 알고 싶으시다면 아래 아티클을 읽어보시면 좋을 것 같습니다.
원래 여기까지는 피드백 안 드리는데 방어적 복사와 Unmodifiable Collection을 사용하셨길래 추가 학습 권유드립니다~

https://steady-coding.tistory.com/559
https://tecoble.techcourse.co.kr/post/2021-04-26-defensive-copy-vs-unmodifiable/

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

세부사항 - 게임을 진행하고 결과를 보여주는 기능을 알려줄 수 있는 이름으로 변경

수정사항 - 자동차 위치 출력을 위한 문자열을 만들지 않고 position을 반환하도록 수정

수정사항 - cars.decideWinner()의 반환 값을 외부에서 받도록 리팩토링

세부사항 - 기존 decideWinner 메소드 명이 우승자를 찾는 것이 아니라 우승자를 결정하는 로직을 수행하는 것처럼 느껴지는 이유로 findAllWinner로 수정 - 가장 멀리 간 자동차 객체를 찾는 기능을 따로 메소드를 분리

MoonJeWoong

첫 리뷰 감사합니다 제이온! 페어 프로그래밍을 처음 하면서 놓쳤던 부분들을 다시 볼 수 있었어서 좋았습니다!

MoonJeWoong · 2023-02-10T16:39:38Z

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

+    public String getMovePosition() {
+        StringBuilder stringBuilder = new StringBuilder();
+        for (int i = 0; i < position; i++) {
+            stringBuilder.append("-");
+        }
+        return stringBuilder.toString();
+    }


해당 부분은 View 단에서는 단순 입력과 출력만을 담당해야 한다고 생각하고 페어와 진행하다보니 과하게 Domain으로 책임이 전가된 부분이라고 생각합니다. 프리코스 때 똑같은 실수를 했었는데 아쉽네요... 😂

아래는 제이온의 질문을 받고 제가 정리해본 생각들입니다.

단순히 출력을 위한 로직이 도메인에 위치하게 된다면 처음 코드를 읽는 사람으로 하여금 "이 부분이 왜 도메인 로직일까?" 라고 생각을 하게 될 소지가 생기게 된다는 것이 단점이라고 생각합니다.

만약 view가 model에게 알려지게 된다면 이후에 model에서 로직이 수정되어야 할 때 model만 수정해야 하는 것이 아니라 view까지 수정해야 하는 강한 의존성이 생겨버리게 되는 것이 단점이라고 생각합니다.

MoonJeWoong · 2023-02-10T16:45:31Z

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

+
+    public Cars(List<Car> cars) {
+        validateCarsSize(cars);
+        this.cars = new ArrayList<>(cars);


Cars 클래스는 생성자 매개변수로 외부에서 생성되어 전달되는 list를 가집니다. 지금은 그런 위험이 없지만 이후 프로젝트 규모가 커지게 되면 혹시 외부에서 실수로 Cars 생성자 매개변수로 전달한 리스트를 그대로 놔뒀다가 수정하는 일이 발생할 수도 있다고 생각했습니다. 이렇게 되면 Cars 내부의 list 변수에도 영향을 줄 것이라 생각해서 재할당을 수행해줬습니다. 혹시 제가 잘못 생각한 부분이 있다면 피드백 주시면 좋겠습니다!

MoonJeWoong · 2023-02-10T17:02:28Z

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

+        for (Car car : cars) {
+            car.goForward();
+        }
+        return Collections.unmodifiableList(cars);


moveEachCar 메소드는 컬렉션 내 모든 car 인스턴스들을 전진 시키고 결과 출력을 위해 해당 컬렉션을 반환하는 메소드입니다. 외부에서 경주에 참여 중인 자동차들을 담고 있는 리스트가 의도치 않게 수정되는 것을 방지하기 위해 unmodifiable을 사용하였습니다.

unmodifiableList를 사용하면 외부에서 의도치 않은 원본 컬렉션에 대한 수정 연산을 방지할 수 있는 것이 장점이라 생각합니다.

그리고 단점이라고 한다면 원본 컬렉션에 대한 완전한 불변을 보장하지 않는다는 것 정도로 생각이 듭니다.

MoonJeWoong · 2023-02-10T17:13:37Z

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

+
+    public List<String> readCarNames() {
+        System.out.println(READ_CARS_NAME_MESSAGE);
+        Scanner scanner = new Scanner(System.in);


inputView 내부에 사용자 입력 값 validation기능을 추가하고 나서 해당 기능을 테스트하기 위해 불가피하게 Scanner 객체의 scope를 메소드 안으로 넣어줄 수 밖에 없다고 판단했던 상황이었습니다. (입출력이 포함된 기능에 대한 테스트)

처음부터 입력값 검증 기능을 view에 위임하지 않고 도메인 클래스 내부에서 검증하거나 차선책으로 validation 클래스를 따로 만들어서 테스트를 진행할 수 있도록 했으면 어땠을까 하는 아쉬움이 남습니다.

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

pjy1368

고생많으셨어요 히이로!
전체적으로 코드가 깔끔해서 사실 남겨 둔 피드백도 소소하거나 지금은 제대로 적용하지 못해도 무방한 내용이었습니다. 기존 코멘트에 답변을 남겨두었으니 잘 확인해보시고 반영해보시면 좋을 것 같아요👍
추가로 질문한 점 있으면 언제든지 디엠주세요! 나머지는 미션 2단계에서 만나요~

seokhwan-an added 30 commits February 8, 2023 14:45

docs(readme) : 프로젝트 설명 및 구현할 기능 readme 작성

fc5f84b

feat(domain) 자동차 이름의 길이를 검증하는 기능 추가

da1e355

test(domain) : 자동차 이름의 길이를 검증하는 기능 테스트

531cd4d

docs(readme) : 구현 기능 목록 추가 및 수정

269ef0a

세부사항 자동차의 위치를 증가 시킬 수 있는 기능 추가 자동차 이름을 심표로 구분하는 기능을 당담 클래스 수정

feat(domain) : 자동차의 위치를 증가 시키는 기능 추가

0e5a930

docs(readme) : 구현 기능 목록 수정

5a139f2

세부사항 - random 하게 0~9 범위 내에 한 자리 숫자를 반환한다.

feat(domain) : 0~9사이의 수를 랜덤으로 반환하는 기능 추가

af854ce

docs(readme) : 구현할 기능 목록 추가 및 수정

9a78916

세부사항 - 자동차 대수는 1대 이상이어야 한다. - 자동차 이름은 쉼표로 구분한다.

feat(domain) : 생성된 자동차들의 정보를 저장하고 관리하는 클래스 생성

7fecde8

세부사항 자동차 대수는 1대 이상이어야 한다.

test(domain) : 자동차 대수가 1이상인지 검증하는 기능 테스트

6d91bcd

feat(domain) : 총 시도 횟수가 1이상인지 검증하는 기능 추가

279c900

test(domain) : 총 시도 횟수가 1이상인지 검증하는 기능 테스트

c3655e2

feat(domain) : 자동차의 전진 여부를 판단하는 기능 구현

ceffd3d

docs(readme) : 구현 기능 목록 수정

19179f7

세부사항 - 총 시도 횟수는 1 이상의 양의 정수 값이어야 한다. - 자동차 경주 클래스를 전 경기를 진행하는 것이 아니라 한 라운드를 진행하는 것으로 역할 수정

feat(domain) : 한 라운드 경주를 진행하는 기능 구현

0ac9140

test(domain) : 한 라운드 경주를 진행하는 기능 테스트

ccaa35d

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

461d591

feat(view) : 자동차 레이싱 라운드 입력 기능 구현

5b1efb9

feat(view) : 시도 회차 별 진행상황 출력 기능 구현

794d093

feat(domain) : 우승한 자동차를 제공하는 기능 구현

3a22991

test(domain) : 우승한 자동차를 제공하는 기능 테스트

2ac8fad

feat(view) : 최종 결과 문구를 출력하는 기능 구현

73ddb24

style(all) : 프로젝트 패키지 구조 변경

74924ff

feat(controller) : 레이싱 게임 controller 구현

61b6bc1

세부사항 - 자동차 이름에 대한 사용자 입력을 받는다. - 총 시도 횟수에 대한 사용자 입력을 받는다. - 자동차 경주를 진행한다. - 경주 결과를 출력하고 프로그램을 종료한다.

feat(application) : 프로젝트 구현 완료

1b27db5

refactor(domain) : 자동차 이동 기능 책임 클래스 수정

20895db

세부사항 - RacingGame 클래스에서 담당하던 자동차 이동 기능을 Car 클래스로 이전

style(all) : 불필요한 메소드, 주석 및 공백 제거

ed654e1

test(domain) : displayname을 사용한 가독성 향상

15219c0

test(domain) : 모든 자동차들이 전진을 1회 시도하는 기능 테스트

1ad6b5d

seokhwan-an added 6 commits February 9, 2023 16:01

test(domain) : 자동자 전진 여부를 판단하는 기능 테스트

9381ba5

refactor(domain) : Car 생성자 내 name 변수의 공백문자 제거

27e9b40

feat(view) : 사용자 시도 횟수 입력값이 숫자인지 검증하는 기능 구현

d153a93

test(view) : 사용자 시도 횟수 입력값이 숫자인지 검증하는 기능 테스트

101dd34

style(domain) : 불필요한 변수 제거

aa3997f

docs(readme) : README 최종 수정

06b2ccb

pjy1368 requested changes Feb 9, 2023

View reviewed changes

MoonJeWoong added 8 commits February 10, 2023 20:22

style(controller) : 메소드 이름 변경

aa0d321

세부사항 - 게임을 진행하고 결과를 보여주는 기능을 알려줄 수 있는 이름으로 변경

style(test) : 테스트 코드 파일 패키지 구조 변경

b5e74e7

refactor(domain) : Car 클래스 내 존재하는 view단 책임 소재 메소드 제거

a85f409

수정사항 - 자동차 위치 출력을 위한 문자열을 만들지 않고 position을 반환하도록 수정

docs(readme) : 자동차 위치를 bar 문자열로 변환하는 기능 추가

03a4a46

feat(view) : 자동차 위치를 bar 문자열로 변환하는 기능 구현

d6a4228

rafactor(view) : 객체 대신 객체 반환 값을 매개변수로 받도록 수정

2ce11a0

수정사항 - cars.decideWinner()의 반환 값을 외부에서 받도록 리팩토링

refactor(view) : stream과 joining을 통한 상수 제거 및 가독성 향상

265469e

MoonJeWoong commented Feb 10, 2023

View reviewed changes

style(all) : 코드 스타일 및 가독성 개선

fcb46fa

pjy1368 approved these changes Feb 11, 2023

View reviewed changes

pjy1368 merged commit 0308287 into woowacourse:moonjewoong Feb 11, 2023

[1단계 - 자동차 경주 구현] 히이로(문제웅) 미션 제출합니다. #499

[1단계 - 자동차 경주 구현] 히이로(문제웅) 미션 제출합니다. #499

Uh oh!

Conversation

MoonJeWoong commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pjy1368 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Uh oh!

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

Uh oh!

Uh oh!

pjy1368 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MoonJeWoong commented Feb 9, 2023 •

edited

Loading