Skip to content

Step4 #6065

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

Merged
merged 6 commits into from
Mar 23, 2025
Merged

Step4 #6065

merged 6 commits into from
Mar 23, 2025

Conversation

cm0x0x0x0
Copy link

@cm0x0x0x0 cm0x0x0x0 commented Mar 20, 2025

리뷰 요청드립니다.

결과화면
스크린샷 2025-03-20 오후 7 56 50

@cm0x0x0x0 cm0x0x0x0 changed the base branch from master to cm0x0x0x0 March 20, 2025 11:00
Copy link

@mskangg mskangg left a comment

Choose a reason for hiding this comment

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

4단계 잘 구현해주셨네요 👍
그런데, 요구사항을 정의하시지 않고 개발하셨네요 😅
우승자 요구사항을 다시 정의하고 TDD 사이클로 적용하는것을 추천드릴게요!

Comment on lines 7 to 8
private String[] carNames;
private int maxCarPosition;
Copy link

Choose a reason for hiding this comment

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

RacingCarView는 입출력을 담당하는 역할인데, 내부적으로 자동차 이름들과 최대 위치 값을 필드로 갖고 있네요 🤔
보통 View 클래스는 출력 형식 정의나 사용자 입력 처리를 주된 역할로 가져야 하고, 게임의 상태나 데이터를 직접 저장하는 것은 지양하는 것이 좋습니다.

RacingCarView는 단순히 게임 상태를 화면에 출력하는 역할만 담당하도록 하고,
자동차의 상태(이름, 위치 등)는 별도의 객체(RacingCar)에 저장하는 것이 더 적절합니다.
그리고 최대 위치 값은 우승자를 구하는 비즈니스를 가진 객체에게 할당하는 건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

0e70bcb 에서 반영했습니다 :)

Comment on lines 56 to 60
for (int i = 0; i < this.carNum; i++) {
if (cars[i].position() == this.maxCarPosition) {
winnerJoiner.add(cars[i].name());
}
}
Copy link

Choose a reason for hiding this comment

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

출력을 담당하는 View 클래스가 우승자를 정하고 있군요 🤔
여기선 우승자를 전달받아 출력만하고 우승자를 구하는 도메인 객체가 필요해보여요.

Copy link
Author

Choose a reason for hiding this comment

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

76b9eb4
7b7b2e8 에서 반영했습니다~

Copy link

@mskangg mskangg left a comment

Choose a reason for hiding this comment

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

피드백 반영 정말 잘해주셨네요!
소소하게 피드백 남겨드렸는데, 이건 다음 단계때 적용 부탁드립니다!
그럼 마지막 단계도 화이팅 🔥

## 요구사항
- [x] 자동차의 이름을 입력 받는다.
- [] 우승자를 구한다.
Copy link

Choose a reason for hiding this comment

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

체크가 빠졌네요 😄

Comment on lines +15 to +21
void findWinners(int maxCarPosition) {
for (RacingCar car : cars) {
if (maxCarPosition == car.position()) {
this.winningCars.add(car);
}
}
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
void findWinners(int maxCarPosition) {
for (RacingCar car : cars) {
if (maxCarPosition == car.position()) {
this.winningCars.add(car);
}
}
}
RacingCar[] findWinners(int maxCarPosition) {
for (RacingCar car : cars) {
if (maxCarPosition == car.position()) {
this.winningCars.add(car);
}
}
return winningCars.toArray(new RacingCar[winningCars.size()]);
}

메서드명이 find 이니 winners 를 찾아서 리턴해주는방향은 어떨까요?
조금더 직관적일것 같아요 🤔

@mskangg mskangg merged commit d8c0fcb into next-step:cm0x0x0x0 Mar 23, 2025
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.

2 participants