Skip to content
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

Open
wants to merge 2 commits into
base: chyjeon
Choose a base branch
from
Open

Conversation

chyjeon
Copy link

@chyjeon chyjeon commented Mar 9, 2024

안녕하세요 리뷰어님!
step3 자동차 경주 리뷰 요청드립니다!
먼가 테스트를 많이 해보려 했는데 마음처럼 되지 않네요.. :(
이번에도 잘 부탁드립니다!
감사합니다 :)

@neojjc2 neojjc2 self-requested a review March 9, 2024 23:58
@neojjc2 neojjc2 self-assigned this Mar 9, 2024
@neojjc2
Copy link

neojjc2 commented Mar 9, 2024

#tag @neojjc2

Copy link

@neojjc2 neojjc2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 창열님 🙇

3단계 진행 잘 해주셨습니다 💯
소소한 의견 몇 가지 드렸습니다.
한번 보시고 개선 검토 해주시면 좋을 것 같아요 😄

그럼 재 리뷰요청 기다리고 있겠습니다. 🙏

protected void systemIn(byte[] inputBytes) {
System.setIn(new ByteArrayInputStream(inputBytes));
}
}
Copy link

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());
}

}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드의 마지막은 마지막은 개행으로 끝나야 합니다 🙏

Suggested change
}
}


public static int numberOfRound(){
System.out.println("시도할 회수는 몇 회 인가요?\n");
return toInts((new Scanner(System.in)).nextLine());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RacingCartoInts를 통해 RacingGame을 위해 필요한 입력 값들에 대한 검증을 다 하고 있는 구조입니다 😄
입력 값에 대한 기본 검증을 하는 Validator를 통해 분리하시거나, 각 객체들이 초기화 할 때 필요한 값을 검증할 수 있도록
적절히 위임해보시는 건 어떨까요?? 🤔
사실 RacingCar입장에서는 시도활 회수가 몇 번 인지, 참가한 자동차 대수가 몇 대 인지 알 필요가 없습니다 🙇

return result;
}

public static int toInts(String values){
Copy link

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++){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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;
Copy link

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++) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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());
Copy link

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) 으로 옮겨야 하는 상황이 생긴다면 지금구조에선 방법이 없습니다.

저희가 운전을 할 때도 가속페달을 밟거나, 브레이크를 밟아서 가고 서는 부분만 결정하고 나머지는 자동차가 알아서 하는 것 처럼요 😅

전략패턴을 한번 적용해 볼 시점이 온 것 같습니다 🙇
요 내용 한번 참고해보시면 좋을 것 같습니다. (잘 이해 안되는 부분 있으시면 언제든 문의 주세요)

https://jackjeong.tistory.com/108

public void runRace(int rounds){
OutputView outputView = new OutputView();
outputView.resultTitle();
for (int round =0; round<rounds; round++){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (int round =0; round<rounds; round++){
for (int round = 0; round < rounds; round++){

});

}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트가 많이 부족합니다 🙇
만약에 테스트 작성이 어렵다면, 객체들의 역할과 책임이 분명하지 않거나, 비즈니스 로직과 UI로직이 잘 분리가 안되서 그럴 수 있을것 같아요 😄
이 부분 좀 더 개선 고민해주시면 좋을 것 같습니다 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants