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

Step4 - 자동차 경주(우승자) #5490

Open
wants to merge 19 commits into
base: jinnie-j
Choose a base branch
from

Conversation

jinnie-j
Copy link

자동차 경주 4단계 리뷰 요청 합니다.

Copy link
Member

@testrace testrace left a comment

Choose a reason for hiding this comment

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

안녕하세요 지은님 😃
4단계도 잘 구현해 주셨네요
PR이 머지된다면 rebase 이후 새로운 브랜치로 PR을 생성하시면 좋을 것 같아요
자세한 내용은 리뷰 요청 가이드 3단계를 참조해 주세요
몇 가지 코멘트 남겨두었는데요 확인해 주시고 다시 리뷰 요청 부탁드려요

Comment on lines +24 to +29
- [x] 자동차 이름 길이 5 제한
- [x] 0-9 사이가 아닌 숫자 또는 빈값을 입력하는 경우 오류가 발생한다.
- [x] 0-9 사이의 랜덤 값을 생성하여 4이상일 경우 전진한다.
- [x] 시도 횟수만큼 모든 자동차의 상태를 출력한다.
- [x] 경주 종료 후 우승를 출력한다.
Copy link
Member

Choose a reason for hiding this comment

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

기능 목록 정리 👍

Comment on lines +16 to +23
public static List<Car> makeCarList(String names) {
List<Car> carList = new ArrayList<>();
for (String name : splitNames(names)) {
checkCarName(name);
carList.add(new Car(name, 0));
}
return carList;
}
Copy link
Member

Choose a reason for hiding this comment

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

이 메서드외에 Cars의 활용도가 없네요 😢
Cars를 어떤 용도로 만드셨는지 궁금합니다 🤔
Cars 클래스가 경주에 참가자한 자동차들의 목록이라면 자동차들에게 이동하라고 메시지를 보내거나
우승자를 구하는 책임을 수행하면 어떨까요?

private void numberCheck(String input) {
Integer number = Integer.parseInt(input);
if (number < MIN_DISTANCE) {
throw new RuntimeException("0-9 사이의 숫자를 입력해주세요.");
Copy link
Member

Choose a reason for hiding this comment

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

메시지를 보면 9 를 초과하는 숫자는 입력할 수 없을 것 같지만
실제로는 9 보다 큰 숫자를 입력할 수 있는데요
기능이 변경되었다면 예외 메시지도 함께 변경해 주시면 좋을 것 같아요

import java.util.ArrayList;
import java.util.List;

public class Record {
Copy link
Member

Choose a reason for hiding this comment

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

클래스명은 기록인데, 우승자를 구하는 로직만 포함되어 있네요
기록이라는 클래스명에 맞게 자동차 경주 결과를 상태로 관리하면 어떨까요?
자동차의 목록을 외부에서 받기 보다 내부의 상태 값으로 우승자를 구할 수 있다면 어떨까요?

@@ -20,4 +22,8 @@ public static void printCarResult(Car car) {
public static void printResultMessage() {
System.out.println("실행 결과");
}

public static void printWinners(List<String> winnerList) {
System.out.println(String.join(",", winnerList) + "가 최종 우승했습니다.");
Copy link
Member

Choose a reason for hiding this comment

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

join 👍

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

public class RacingGameTest {
Copy link
Member

Choose a reason for hiding this comment

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

지난 코멘트의 의도가 제대로 전달되지 않은 것 같아요
테스트 코드의 이름을 변경을 요청드린 것은 아니었구요
자동차의 움직임을 테스트하는 코드는 CarTest.java 에서,
자동차의 우승자를 구하는 로직을 테스트 코드는 RecordTest.java에서 수행할 수 있도록 클래스 분리를 하시면 좋을 것 같아요

추가로, 자동차의 움직임에 대한 테스트가 사라졌는데요

요구사항
모든 로직에 단위 테스트를 구현한다. 단, UI(System.out, System.in) 로직은 제외

위 요구사항을 충족하기 위해 테스트 코드를 작성해 주시면 좋을 것 같아요

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