3단계 - 자동차 경주(2) 코드 리뷰 요청 드립니다. by sogoagain · Pull Request #301 · next-step/java-racingcar

sogoagain · 2019-06-17T15:07:18Z

안녕하세요 :) 3단계 리뷰 요청드립니다.

2단계 피드백 반영 내용

foreach문 컨벤션 수정 하였습니다.
자동차의 이동 판별을 위해 숫자를 생성하는 부분을 ‘NumberGenerationStrategy’ 인터페이스로 추출하여 원하는 숫자만 나오는 객체를 통해 RacingDrivingRule 클래스를 테스트 하였습니다.
racingGame.run()이 정상 수행여부를 boolean값으로 반환하도록 수정 하였습니다.

자동차 경주 3단계 주요 구현 내용

Car에 name 필드를 넣으니 한 클래스에 멤버변수가 3개가 되어 ‘이름, 위치’를 CarInformation 클래스로 추출 하였습니다.
기존 position을 int형으로 나타내었으나 별도의 클래스인 Position으로 추출 하여 의미를 명확히 하였습니다.
Referee클래스를 통해 우승자 선별 기능을 구현하였습니다.

질문

Getter/Setter와 관련된 고민입니다.

Car의 정보 출력을 위해 불가피하게 getter로 CarInformation을 반환하도록 하였습니다.
CarInformation 클래스는 VO의 개념으로 getter와 setter를 구현하였습니다.
구현을 하다 보니 위와같이 불가피하게 getter와 setter를 사용하게 되었는데 객체지향 생활 체조 규칙을 벗어나는 것 같아 고민이 됩니다.
toString도 고려하였으나, toString의 내용을 그대로 Print문에 쓰는 것은 View가 콘솔이 아닌 웹 등 다른 것으로 변경되는 경우를 고려했을 때를 생각하면 별로 좋지 않은 느낌이 들었습니다. 또한, 출력 형태의 책임은 OutputView에서 갖는게 좋을 것 같아 getter를 사용하였는데, toString() 사용이 더 좋을까요?

감사합니다. :)

- 변경 내용: 1. for-each문 컨벤션 수정 2. OutputView 개행 출력 메서드 삭제

…e 클래스 이름 및 구조 변경 변경 내역: - 기존 RandomDrivingRule 클래스에서 랜덤한 숫자를 추출하던 로직을 RandomNumberGenerator로 추출 - NumberGenerationStrategy 인터페이스를 정의하여 원하는 숫자를 생성하는 클래스 구현 가능 -> RacingDrivingRule에서 이동 여부를 판단하는 메서드에 대한 테스트 가능 - RacingDrivingRule은 더이상 싱글턴이 아님 -> 숫자를 생성하는 여러 방법을 선택할 수 있기에 싱글턴 패턴 적용 해제

변경 내역: - RacingGame의 run 메서드가 정상 종료되면 true를 반환하고 그렇지 않으면 false를 반환하도록 변경

자동차 경주(2)에서 추가된 기능 목록: 1. 자동차 이름 부여 2. 자동차 이름 출력 기능 3. 경주할 자동차들의 이름 입력 기능 4. 우승자 판별 기능

변경 내용: - Car는 String 형태의 이름을 갖는다.

변경 내용: 1. CarInformation - 자동차의 이름과 위치를 표현 2. Position 클래스 - 위치 정보를 추상화

Referee 제공 기능: - judgeWinners: 1등을 판별한다.

young891221

좋습니다. 적극적인 자료구조 활용을 통해 문제를 해결하는 방법 💯

각 객체가 더 알맞은 역할을 하도록 수정한다면 훨씬 가독성있는 코드가 될 것 같습니다ㅎㅎ

피드백 반영 부탁드립니다~!

young891221 · 2019-06-17T23:41:45Z

src/main/java/racingcar/controller/RacingGame.java

 public class RacingGame {
-    private Cars cars;
+    private List<String> carNames;
+    private int numberOfTries;


일급 객체를 만들기 위한 조건중 필드를 1개만 가지도록 즉, 자신의 속성값과 역할이 한 가지로 일치하도록 지향해야 합니다.
RacingGame의 경우는 레이싱 게임을 하기 위해 필요한 요소만 있으면 될 듯 합니다. 뭐만 있으면 될까요?? 고민해 보시고 수정 부탁드립니다. 힌트는 뷰는 뷰데로 외부에서 컨트롤하도록 떼어내버리고 내부에는 레이싱 게임만을 위한 요소만 있으면 해결됩니다.
여기서는 carNames도 그 필드값의 속성이겠네요.
아래쪽에 다른 부분들에 대해서는 아주 잘 작성해 주셨는데요. 이 부분을 수정하면 더 완벽할 듯 합니다ㅎㅎ

일급 객체를 만들기 위한 조건중 필드를 1개만 가지도록 즉, 자신의 속성값과 역할이 한 가지로 일치하도록 지향해야 합니다.
RacingGame의 경우는 레이싱 게임을 하기 위해 필요한 요소만 있으면 될 듯 합니다. 뭐만 있으면 될까요?? 고민해 보시고 수정 부탁드립니다. 힌트는 뷰는 뷰데로 외부에서 컨트롤하도록 떼어내버리고 내부에는 레이싱 게임만을 위한 요소만 있으면 해결됩니다.
여기서는 carNames도 그 필드값의 속성이겠네요.
아래쪽에 다른 부분들에 대해서는 아주 잘 작성해 주셨는데요. 이 부분을 수정하면 더 완벽할 듯 합니다ㅎㅎ

넵! Cars 객체만 갖도록 수정하였습니다. :)
UI관련 기능은 Main으로 위임하였습니다.

young891221 · 2019-06-17T23:43:21Z

src/main/java/racingcar/controller/RacingGame.java

+            carNames = InputView.inputCarNames();
+            numberOfTries = InputView.inputNumberOfTries();
+        } catch (InputMismatchException exception) {
+            return false;


에러처리에 대해 boolean으로 조정하기 보다는 에러를 throw하는 방식으로 처리하면 무엇이 장점일까요??ㅎㅎ

에러처리에 대해 boolean으로 조정하기 보다는 에러를 throw하는 방식으로 처리하면 무엇이 장점일까요??ㅎㅎ

지난번 피드백에서 run()메서드에 반환값이 없다는 의견을 주셔서 억지로 boolean을 반환하도록 하여 어색한 감이 있었네요 ㅎㅎ
현재는 RacingGame의 책임을 명확히 하니 각각의 메서드가 반환값을 갖게 되었습니다.

제 생각엔 에러처리를 boolean으로 하면 에러에 대한 의미가 명확하지 않은 것 같아요. Exception을 throw하는 방식이 왜 발생한 에러이고 후속 조치를 어떻게 해야하는지 명확히 나타내는것 같습니다.

넵, 제가 의도했던 바는 의미있는 반환값을 말씀드리고 싶었는데 넘 뜬구름식으로 얘기했던거 같네요ㅠㅠ;;
여튼 마지막으로 생각하신 바는 정확합니다. 앞으로도 throw로 처리하도록 부탁드립니다~ㅎㅎ

young891221 · 2019-06-17T23:44:29Z

src/main/java/racingcar/controller/RacingGame.java

-        OutputView.printNewLine();
+    private boolean inputRaceSetting() {
+        try {
+            carNames = InputView.inputCarNames();


이렇게 생각해 보면 어떨까요.
Main에서 InputView, OutputView를 처리하고 RacingGame은 정말 게임만 수행한다는 컨셉.
각 객체는 고유의 역할만 수행한다.

이렇게 생각해 보면 어떨까요.
Main에서 InputView, OutputView를 처리하고 RacingGame은 정말 게임만 수행한다는 컨셉.
각 객체는 고유의 역할만 수행한다.

네! 피드백 참고하여 수정하였습니다.

young891221 · 2019-06-17T23:45:52Z

src/main/java/racingcar/model/Car.java

 public class Car {
    private DrivingRule drivingRule;
-    private int position = 0;
+    private CarInformation carInformation;


CarInformation 객체 굿입니다. DrivingRule 이것도 CarInformation의 속성이 되지 않을까요??ㅎㅎ

CarInformation 객체 굿입니다. DrivingRule 이것도 CarInformation의 속성이 되지 않을까요??ㅎㅎ

네, DrivingRule도 CarInformation에서 갖도록 하였습니다.
그런데, 그럼 CarInformation의 속성이 3개가 되어 객체지향 생활체조에 어긋나는데... 괜찮을까요?

'모든 객체가 일급 객체가 아닌 의미있는 객체가 일급 객체다'라고 컨셉을 잡아두셔도 좋을것 같아요. CarInformation or CarParameter로 하던 getter용 파라미터 객체는 실제 프로젝트에서도 항상 필요하니까요ㅎㅎ

young891221 · 2019-06-17T23:46:20Z

src/main/java/racingcar/model/CarInformation.java

+        return position;
+    }
+
+    public void setPosition(Position position) {


setter는 지양해 주세요.
저희 코딩원칙중 한 가지입니다.

setter는 지양해 주세요.
저희 코딩원칙중 한 가지입니다.

네, 수정하였습니다.

young891221 · 2019-06-17T23:47:14Z

src/main/java/racingcar/model/Cars.java

@@ -6,22 +6,22 @@
 public class Cars {


Cars는 일급객체군요! 👍

young891221 · 2019-06-17T23:52:46Z

src/main/java/racingcar/model/Referee.java

+
+public class Referee {
+    public static List<CarInformation> judgeWinners(List<CarInformation> carInformationList) {
+        TreeMap<Position, List<CarInformation>> informationMap = new TreeMap<>();


TreeMap을 사용해서 우선순위 정렬하는 방법 굿입니다. 💯

TreeMap을 사용해서 우선순위 정렬하는 방법 굿입니다. 💯

감사합니다. :)

변경 내역: - RacingGame에서 UI 관련 호출 부분 제거 - Game 진행을 위한 인터페이스만 제공

변경 내역: - CarInformation의 setPosition 제거

변경 내역: - RacingGame 테스트 작성 - Cars 생성 방식 추가 - Car, CarInformation equals 구현

변경 내용: - CarInformation은 VO로 getter만 제공 - CarInformation에서 DrivingRule 속성을 갖는 것으로 변경

변경 내용: - CarName 클래스는 자동차 이름을 나타낸다.

young891221

아주 훌륭한 피드백 반영이었습니다. 💯

코드가 훨씬 깔끔해 졌네요.

바로 다음 단계 진행 부탁드립니다~!

sogoagain added 10 commits June 16, 2019 22:58

style(컨벤션수정): for-each문 컨벤션 수정 및 OutputView 개행 출력부분 수정

76c2af7

- 변경 내용: 1. for-each문 컨벤션 수정 2. OutputView 개행 출력 메서드 삭제

refactor: RacingGame의 run 메서드가 boolean값을 리턴하도록 변경

b0ba97b

변경 내역: - RacingGame의 run 메서드가 정상 종료되면 true를 반환하고 그렇지 않으면 false를 반환하도록 변경

docs(README.md): 자동차 경주(2) 기능 목록 작성

1dc9892

자동차 경주(2)에서 추가된 기능 목록: 1. 자동차 이름 부여 2. 자동차 이름 출력 기능 3. 경주할 자동차들의 이름 입력 기능 4. 우승자 판별 기능

feat(자동차이름추가): Car 객체에 이름 부여

2a99142

변경 내용: - Car는 String 형태의 이름을 갖는다.

feat(클래스분리): CarInformation 클래스 분리, Position 클래스 분리

8a9bcdf

변경 내용: 1. CarInformation - 자동차의 이름과 위치를 표현 2. Position 클래스 - 위치 정보를 추상화

refactor: 출력을 위해 Car에서 CarInformation을 제공하도록 변경

764442b

test(테스트코드작성): CarInformation, RandomNumberGenerator의 테스트 코드 작성

e2d6df4

feat(1등판별기능): 1등을 판별하는 Referee 클래스 생성

fad510b

Referee 제공 기능: - judgeWinners: 1등을 판별한다.

feat(1등출력): 1등 출력기능 구현

e4b9457

young891221 suggested changes Jun 17, 2019

View reviewed changes

sogoagain added 5 commits June 18, 2019 15:33

refactor: RacingGame을 일급객체로 변경

35316ec

변경 내역: - RacingGame에서 UI 관련 호출 부분 제거 - Game 진행을 위한 인터페이스만 제공

refactor: CarInformation의 setter 제거

abd0d40

변경 내역: - CarInformation의 setPosition 제거

test(RacingGame): RacingGame 테스트 작성

761b95a

변경 내역: - RacingGame 테스트 작성 - Cars 생성 방식 추가 - Car, CarInformation equals 구현

refactor: Car의 DrivingRule 속성을 CarInformation으로 이동

1805127

변경 내용: - CarInformation은 VO로 getter만 제공 - CarInformation에서 DrivingRule 속성을 갖는 것으로 변경

feat(자동차이름객체): CarName 클래스 생성

c30ecb2

변경 내용: - CarName 클래스는 자동차 이름을 나타낸다.

young891221 approved these changes Jun 18, 2019

View reviewed changes

young891221 merged commit ec1ad40 into next-step:sogoagain Jun 18, 2019

Conversation

sogoagain commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

young891221 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sogoagain commented Jun 17, 2019 •

edited

Loading

young891221 left a comment •

edited

Loading