-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Step4 #6128
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
base: myj4513
Are you sure you want to change the base?
Step4 #6128
Conversation
Added README.md - add Todo List - add Criteria
Update - CarTest - RacingGameTest - InputView - ResultView -StubInputView - README.md
Update - Car - RacingGame - RacingGameTest - ResultView - README.md
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.
안녕하세요 예준님
4단계 기능 구현 잘 해주셨어요
코멘트 확인 후 재요청 부탁드립니다!
public List<Car> getWinners(List<Car> cars) { | ||
int maxPosition = cars.stream().mapToInt(Car::getPosition).max().orElse(1); | ||
|
||
return cars.stream().filter(car -> car.getPosition() == maxPosition).collect(Collectors.toList()); | ||
} |
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.
👍 Car
객체를 만드신 것 처럼
우승자 처리를 위한 도메인 모델을 만들어 보는 것도 좋을 것 같습니다!
public List<Car> getWinners(List<Car> cars) { | ||
int maxPosition = cars.stream().mapToInt(Car::getPosition).max().orElse(1); | ||
|
||
return cars.stream().filter(car -> car.getPosition() == maxPosition).collect(Collectors.toList()); |
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 cars.stream().filter(car -> car.getPosition() == maxPosition).collect(Collectors.toList()); | |
return cars.stream().filter(car -> car.isEqualPosition(maxPosition)).collect(Collectors.toList()); |
게터를 사용하는 대신 객체에게 메시지를 전달하는 방법은 어떤가요?
메소드 네이밍은 예시이니 참고만 해주세요~
for (int i = 0; i < numberOfCars; i++) { | ||
cars.add(new Car()); | ||
public void initializeCars(String[] carNames) { | ||
cars = Arrays.stream(carNames).peek(this::validateCarNameLength).map(Car::new).collect(Collectors.toList()); |
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 List<Car> getWinners(List<Car> cars) { | ||
int maxPosition = cars.stream().mapToInt(Car::getPosition).max().orElse(1); |
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.
자동차의 기본 위치가 1임을 상수로 정의하면
여러 곳에서 사용될 때 실수 없이 일관되게 처리할 수 있을 것 같아요.
이 부분을 고려해보시면 좋겠습니다!
|
||
@ParameterizedTest | ||
@CsvSource({"2,false", "3,false", "4,true", "5,true", "6,true", "7,true", "8,true"}) | ||
@CsvSource({"3,false", "4,true"}) // 경계를 두고 전후만 테스트 해보자 |
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.
👍 👍
Car car = new Car(); | ||
Car car = new Car("TEST"); | ||
|
||
boolean actual = car.isMovable(random); |
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
을 노출시킬 필요없이
move
로 움직인 후 position
의 값을 게터로 꺼내와 테스트하는 방법도 있을 것 같아요
import racingcar.view.InputView; | ||
import racingcar.view.ResultView; | ||
|
||
public class Main { |
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으로 노출되어 있네요.
불필요한 접근을 제한하면 객체의 캡슐화를 더 잘 지킬 수 있을 것 같습니다
혹시 테스트하기 쉬운 구조를 만들기 위해 모두 public으로 선언하신 걸까요?
} | ||
|
||
public boolean isMovable(int randomNumber) { | ||
return randomNumber >= 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.
4를 상수로 리팩터링하면 어떨까?
No description provided.