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

[자동차 경주] 김주언 제출합니다 #1482

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

wndjs803
Copy link

No description provided.

Copy link

@jinu0328 jinu0328 left a comment

Choose a reason for hiding this comment

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

전체적으로 함수 분리와 가독성이 좋아서 쉽게 코드를 이해할 수 있었습니다 👍
고생하셨습니다!

- [x] 전진하는 조건은 0에서 9 사이에서 무작위 값을 구한 후 무작위 값이 4 이상일 경우이다.
- [x] 자동차 경주 게임을 완료한 후 누가 우승했는지를 알려준다. 우승자는 한 명 이상일 수 있다.
- [x] 우승자가 여러 명일 경우 쉼표(,)를 이용하여 구분한다.
- [ ] 사용자가 잘못된 값을 입력할 경우 IllegalArgumentException을 발생시킨 후 애플리케이션은 종료되어야 한다.

Choose a reason for hiding this comment

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

앗 마지막 체크는 실수로 빼먹으신걸까요?

System.out.println("시도할 횟수는 몇 회인가요?");
return Integer.parseInt(Console.readLine());
}
}

Choose a reason for hiding this comment

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

다른 함수들은 모두 동사형이지만 여기 두 메소드 이름만 명사입니다!
getCarNames()와 getTryCount()로 바꾸는 게 좋을 것 같아요 😃

추가로 문자열로 시도 횟수를 여기서 바로 정수로 parse하여 반환하고 있는데 문자열을 정수로 변환하는 것이 과연 View의 역할일까..? 조심스레 의문을 남기고갑니다


public void init() {
String input = inputView.inputCarNames();
String[] carNames = input.split(",");

Choose a reason for hiding this comment

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

Car 모델을 묶는 일급컬렉션 Cars를 만들어주신 것 처럼 String[]을 사용하기보다는 List을 멤버로 가지는 일급 컬렉션 CarNames를 만들어서 사용해주시는 게 좋을 것 같습니다!

Copy link

Choose a reason for hiding this comment

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

"배열 대신 컬렉션을 사용한다"는 우테코의 1주차 피드백을 보시면 좋을 것 같아요!

String[] carNames = input.split(",");
carNameValidator.validate(carNames);
cars = new Cars(carNames);
tryCount = inputView.inputTryCount();

Choose a reason for hiding this comment

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

다른 View와 Validator는 생성자 주입으로 받아서 사용하는데 Cars만 컨트롤러에서 직접 생성해주는 이유가 있을까요?

Comment on lines +11 to +13
public int inputTryCount() {
System.out.println("시도할 횟수는 몇 회인가요?");
return Integer.parseInt(Console.readLine());
Copy link

@lsh1215 lsh1215 Oct 29, 2024

Choose a reason for hiding this comment

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

Integer로 바로 형 변환을 하게되면 숫자가 아닌 문자가 입력됐을 때 에러 처리가 힘들거 같아요

Copy link

@lsh1215 lsh1215 left a comment

Choose a reason for hiding this comment

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

테스트 코드도 추가해보시면 좋을 것 같습니다. 이번 요구 조건에 기능 단위 테스트 코드에 대한 내용이 있었어요!😁

Copy link

@Kimbobae1 Kimbobae1 left a comment

Choose a reason for hiding this comment

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

기능별로 클래스와 메서드가 잘 분리되어 있어서 읽기 좋았습니다!
잘 보고 갑니다 👍👍


public class RandomNumber {
public static int getRandomNumber() {
return Randoms.pickNumberInRange(0, 9);
Copy link

@Kimbobae1 Kimbobae1 Oct 30, 2024

Choose a reason for hiding this comment

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

0(최솟값), 9(최댓값) 과 같은 매직 넘버(의미있는 이름의 상수로 대체될 수 있는 숫자)는 상수로 관리하는 것도 좋을 것 같습니다.
주언님은 이부분에 대해 어떻게 생각하시나요?

Comment on lines +18 to +35
public void printWinners(List<String> winners) {
String message = "최종 우승자 : ";
if (winners.size() == 1) {
printSingleWinner(message, winners);
return;
}
printMultipleWinners(message, winners);

}

private void printSingleWinner(String message, List<String> winners) {
System.out.println(message + winners.get(0));
}

private void printMultipleWinners(String message, List<String> winners) {
System.out.println(message + String.join(", ", winners));
}
}

Choose a reason for hiding this comment

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

String.join() 메서드는List의 요소가 하나일 때 그 요소만 반환하는 것으로 알고 있습니다.
혹시 SingleWinner와 MultipleWinner를 따로 나누어 놓으신 이유가 따로 있으실까요?


public String getResult() {
StringBuilder result = new StringBuilder(name + " : ");
result.append("-".repeat(Math.max(0, forward)));

Choose a reason for hiding this comment

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

코드에서 forward를 0으로 초기화하셨는데, Math.max 메서드로 다시 0과 비교하신 이유가 궁금합니다 :)

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.

4 participants