-
Notifications
You must be signed in to change notification settings - Fork 4
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
[자동차 경주] 박성민 미션 제출합니다. #4
base: main
Are you sure you want to change the base?
Conversation
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.
고생하셨습니다...!!!
src/main/java/racingcar/Model.java
Outdated
public boolean move() { | ||
int random = Randoms.pickNumberInRange(0, 9); | ||
return random >= 4; | ||
} |
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.
이 메서드는 움직이는 메서드라기보단 "움직일 수 있는지?" 여부에 대한 기능을 담고 있기 때문에
메서드명을 isMovable()로 바꾸거나, random >= 4일 때 실제로 carProgress에 "-"이 더 추가되도록 하면 좋을 것 같습니다.
근데 사실 문자열은 연산 자체가 복잡한 감이 있어서 그냥 변수를 int 타입으로 하고, 자동차의 이동거리를 화면에 출력할 때만 "-" 으로 변환해서 출력하는 게 제일 좋을 것 같긴 합니다!
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.
문자열을 사용하면 로직에서 길이를 정수형으로 받아오는 과정이 추가되는데, 확실히 정수형을 사용하면 훨씬 간단해지는군요! 올바른 타입을 결정하는 것도 중요하다는 것을 알게되었습니다
src/main/java/racingcar/Model.java
Outdated
public void gameProgress(List<String> carNames, List<String> carProgress) { | ||
for (int i = 0; i < carNames.size(); i++) { | ||
String progress = carProgress.get(i); | ||
if (move()) { | ||
progress += '-'; | ||
carProgress.set(i, progress); | ||
} | ||
} | ||
} |
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.
위에서 언급한 것처럼 이 부분이 좀 어색한 것 같습니다.
if (move()) {
// 자동차가 움직이는 로직
}
직독하면 "만약 움직인다면 -> 움직인다"인데 "만약 움직일 수 있다면 -> 움직인다" 가 훨씬 자연스러운 것 같습니다!
src/main/java/racingcar/View.java
Outdated
String str = st.nextToken(); | ||
cars.add(str); | ||
if (str.length() > 5) { | ||
throw new IllegalArgumentException(); |
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.
여기서 new IllegalArgumentException()
안에 에러 메시지까지 넣어주면 좋을 것 같습니다!
"자동차의 이름은 5글자 이내여야 합니다." <- 이런 식으로용
src/main/java/racingcar/View.java
Outdated
public void printResult(List<String> carNames, List<String> carProgress) { | ||
List<String> winner = new ArrayList<>(); | ||
int max = 0; | ||
for (int i = 0; i < carProgress.size(); i++) { // 최댓값 찾기 | ||
if (carProgress.get(i).length() >= max) { | ||
max = carProgress.get(i).length(); | ||
} | ||
} | ||
for (int i = 0; i < carProgress.size(); i++) { // 우승자 찾기 | ||
if (carProgress.get(i).length() >= max) { | ||
winner.add(carNames.get(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.
최댓값을 찾고 이를 이용해 우승자를 찾는 부분은 Model의 책임인 것 같아서,
이 메서드의 파라미터를 받을 때 애초에 Model에서 우승자 목록을 넘겨주고 printResult에서는 출력만 하는 게 올바른 구조인 것 같습니다!
try { | ||
Application.main(new String[]{}); | ||
} catch (IOException e) { | ||
throw new RuntimeException(e); | ||
} |
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.
만약 위에서 new IllegalArgumentException()
을 던질 때 에러 메시지를 추가한다면,
여기서 에러를 캐치할 때 콘솔창에 에러 메시지를 print문을 통해 출력하면 좋을 것 같습니다!
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.
수고하셨습니다!
return progress; | ||
} | ||
|
||
public boolean isWin() { |
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.
isWin을 넣었는데 사용하지 않게 된 이유가 뭔가용
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.
삭제하는 것을 까먹었네요 🤦🏻
StringTokenizer st = new StringTokenizer(carNames, ","); | ||
while (st.hasMoreTokens()) { | ||
String str = st.nextToken(); | ||
Car car = new Car(str, 0, false); |
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.
0과 false는 고정이니까 입력하지 않게 만들어놔도 되지 않을까용?
public void findWinner() { | ||
int max = 0; | ||
|
||
for (int i = 0; i < cars.size(); 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.
Math.max() 메서드를 사용하는것도 좋아보여요
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.
좋은 메서드가 내장되어 있었네요 !
model.frequency = view.inputFrequency(); | ||
for (int i = 0; i <= model.frequency; i++) { | ||
model.moveCars(); | ||
view.printCarProgress(model.cars, model.cars.size()); |
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.
model.car.size()를 매개변수로 전달해준 이유가 있을까요?
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.
출력 기준에 맞춰 입력받은 자동차의 수 만큼 출력 해야하기 위해서 입니다!
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 void printCarProgress(List<Car> cars, int length) { | ||
for (int index = 0; index < length; index++) { | ||
System.out.print(cars.get(index).getName() + " : "); | ||
int progress = cars.get(index).getProgress(); | ||
for (int i = 0; i < progress; i++) { | ||
System.out.print("-"); | ||
} | ||
System.out.println(); | ||
} | ||
System.out.println(); | ||
} | ||
|
||
public void printWinner(List<String> winner) { | ||
System.out.print("최종 우승자 : "); | ||
for (int i = 0; i < winner.size(); i++) { | ||
System.out.print(winner.get(i)); | ||
if (i + 1 < winner.size()) { | ||
System.out.print(","); | ||
} | ||
} | ||
} |
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.
InputView, OutView로 분리 했으면 관리가 더 편했을거 같아요
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.
좋은 방법 같습니다 참고할게요 🤝🏻
try { | ||
Application.main(new String[]{}); | ||
} catch (IOException e) { | ||
throw new RuntimeException(e); | ||
} |
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.
테스트코드 대응 굿
MODEL
splitNames()
findWinner()
View
inputCarNames()
inputFrequency()
printCarProgress()
printResult()
Controller
startGame()