-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(step3): 자동차 경주 #5332
base: chyjeon
Are you sure you want to change the base?
feat(step3): 자동차 경주 #5332
Conversation
#tag @neojjc2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 창열님 🙇
3단계 진행 잘 해주셨습니다 💯
소소한 의견 몇 가지 드렸습니다.
한번 보시고 개선 검토 해주시면 좋을 것 같아요 😄
그럼 재 리뷰요청 기다리고 있겠습니다. 🙏
src/main/java/racingcar/IOTest.java
Outdated
protected void systemIn(byte[] inputBytes) { | ||
System.setIn(new ByteArrayInputStream(inputBytes)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좀 더 생동감(?) 있는 테스트를 작성하기 위해서 실제 System.in
을 활용해주신 것 같네요 😄
특정 값을 입력받았다고 가정하고 테스트 작성해주셔도 될 것 같습니다 🙇
모든 로직에 단위 테스트를 구현한다. 단, UI(System.out, System.in) 로직은 제외
return toInts((new Scanner(System.in)).nextLine()); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드의 마지막은 마지막은 개행으로 끝나야 합니다 🙏
} | |
} | |
|
||
public static int numberOfRound(){ | ||
System.out.println("시도할 회수는 몇 회 인가요?\n"); | ||
return toInts((new Scanner(System.in)).nextLine()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RacingCar
가 toInts
를 통해 RacingGame
을 위해 필요한 입력 값들에 대한 검증을 다 하고 있는 구조입니다 😄
입력 값에 대한 기본 검증을 하는 Validator
를 통해 분리하시거나, 각 객체들이 초기화 할 때 필요한 값을 검증할 수 있도록
적절히 위임해보시는 건 어떨까요?? 🤔
사실 RacingCar
입장에서는 시도활 회수가 몇 번 인지, 참가한 자동차 대수가 몇 대 인지 알 필요가 없습니다 🙇
return result; | ||
} | ||
|
||
public static int toInts(String values){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위에서도 의견 드렸지만 공통 유틸 같은것으로 분리되면 좋을 것 같습니다 😄
|
||
public String resultOfRacing(){ | ||
String result = ""; | ||
for (int i=0;i<this.position;i++){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (int i=0;i<this.position;i++){ | |
for (int i = 0; i < this.position; i++){ |
for (int i=0;i<this.position;i++){ | ||
result += HYPHEN; | ||
} | ||
return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RacingCar
고유 기능이 아닌 단지 출력 포맷이나 양식 (지금은 HYPHEN
이지만, csv
형식의 콤마라던지 json
형식으로 변경되어야 한다던지)이 변경된다면 이 부분은 요구사항 때문이 아니라 출력 변경에대한 요구사항 때문에 계속 수정이 발생할 수 밖에 없습니다. 이 부분은 OutputView
쪽에서 담당하는게 맞는 것 같습니다 🙇
핵심 로직을 구현하는 코드와 UI를 담당하는 로직을 구분한다.
UI 로직을 InputView, ResultView와 같은 클래스를 추가해 분리한다.
private final List<RacingCar> records = new ArrayList<>(); | ||
|
||
public void carRegister(int carNum){ | ||
for (int i=0;i<carNum; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (int i=0;i<carNum; i++) { | |
for (int i = 0; i < carNum; i++) { |
public void runOneRound(){ | ||
|
||
for (RacingCar racingCar: records){ | ||
racingCar.moveRacingCar(racingCar.genRandomNum()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RacingCar
에게 getRandomNum
을 요청해서 자신을 움직이게 하는 구조네요 🤔
자율주행차 같은 모습입니다 😅
가정을 하나 해보면, 만약 게임에 대한 Car
의 위치를 체크해야 하는 Car
들이 필요하고 (예를들면 경기를 중계하는 차들이라고 해볼
께요 😅) 5위치, 10위치에 세워둬야 하는 요구사항이 생긴다면, 지금 구조에서는 불가능합니다.
또한 RacingGame
중에 문제가 발생해서 진행중인 차들을 다시 시작 점 (position=0
) 으로 옮겨야 하는 상황이 생긴다면 지금구조에선 방법이 없습니다.
저희가 운전을 할 때도 가속페달을 밟거나, 브레이크를 밟아서 가고 서는 부분만 결정하고 나머지는 자동차가 알아서 하는 것 처럼요 😅
전략패턴을 한번 적용해 볼 시점이 온 것 같습니다 🙇
요 내용 한번 참고해보시면 좋을 것 같습니다. (잘 이해 안되는 부분 있으시면 언제든 문의 주세요)
public void runRace(int rounds){ | ||
OutputView outputView = new OutputView(); | ||
outputView.resultTitle(); | ||
for (int round =0; round<rounds; round++){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (int round =0; round<rounds; round++){ | |
for (int round = 0; round < rounds; round++){ |
}); | ||
|
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트가 많이 부족합니다 🙇
만약에 테스트 작성이 어렵다면, 객체들의 역할과 책임이 분명하지 않거나, 비즈니스 로직과 UI로직이 잘 분리가 안되서 그럴 수 있을것 같아요 😄
이 부분 좀 더 개선 고민해주시면 좋을 것 같습니다 🙇
안녕하세요 리뷰어님!
step3 자동차 경주 리뷰 요청드립니다!
먼가 테스트를 많이 해보려 했는데 마음처럼 되지 않네요.. :(
이번에도 잘 부탁드립니다!
감사합니다 :)