[자동차경주] 유재건 미션 제출합니다. #523
Conversation
| } | ||
|
|
||
| private void validate(String name) { | ||
| if (name.length() > maxNameSize || name.length() < NAME_MIN_LENGTH.getNumber()) { |
There was a problem hiding this comment.
MIN_LENGTH 보다는 null check 혹은 StringUtils.isEmpty(); 를 사용하는건 어떨까요
| } | ||
|
|
||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
관련해서 비슷한 내용의 리뷰가 있어 공유드립니다
참고
| @Override | ||
| public String toString() { | ||
| StringBuilder print = new StringBuilder(name); | ||
| appendPosition(print); |
There was a problem hiding this comment.
StringJoiner를 사용하면 더 간결하게 표현할 수 있습니다
// ex
StringJoiner joiner = new StringJoiner(INFIX.getMessage());
String positionToMessage = doSomething();
joiner.add(name, positionToMessage);|
|
||
| private void appendPosition(StringBuilder print) { | ||
| print.append(INFIX.getMessage()); | ||
| print.append(IntStream.range(0, position).mapToObj(i -> POSITION.getMessage()).collect(Collectors.joining())); |
There was a problem hiding this comment.
저도 이번에 처음알았는데 Java 11에서 String.repeat()라는 함수가 있더라고요
String positionToMessage = "-".repeat(position);이렇게 하면 해당 문자열을 position번 반복할수있습니다!
| this.carAmount = carAmount; | ||
| } | ||
|
|
||
| public List<Integer> tempPosition() { |
There was a problem hiding this comment.
만약에 해당 행위를 묶어서 관리한다면 CarsOperator가 더 적절한 이름이 될거같네요
이건 개인적인 의견이지만 List로 움직임의 여부를 한꺼번에 반환하기보다는 하나씩 반환해주는 것도 괜찮은 방법인것 같아요.
getMoveFlag 정도의 역할을 하는 메소드로 만드는거죠
return numberGenerate.generate();
이렇게하면 위에서 선언하셨던 moveCars 메소드도 한층 읽기 수월해질것 같네요
// 변경전
public void moveCars(CarOperator operator) {
List<Integer> nowMove = operator.tempPosition();
IntStream.range(0, cars.size()).forEach(index -> {
cars.get(index).move((nowMove.get(index)));
});
}
// 변경후
public void moveCars(CarOperator operator) {
IntStream.range(0, cars.size()).forEach(index -> {
cars.get(index).move(operator.getMoveFlag()));
});
}| String input = Console.readLine(); | ||
| String[] splitInput = input.split(DELIMITER.getMessage()); | ||
| List<Car> cars = new ArrayList<>(); | ||
| makeCars(splitInput, cars); |
There was a problem hiding this comment.
Cars의 생성자가 해당 역할을 담당하는건 어떨까요?
| public void roundEndTest() throws Exception{ | ||
| Round round = new Round(2); | ||
| round.next(); | ||
| Assertions.assertEquals(round.isFinish(), false); |
There was a problem hiding this comment.
Assertions.assertTrue()
Assertions.assertFalse()
True, False 를 비교할때 해당 메소드를 사용할 수 있습니다
| } | ||
|
|
||
| public boolean isFinish() { | ||
| if (tempRound == finalRound) return true; |
There was a problem hiding this comment.
return (tempRound == finalRound) 로 축약해도될거같아요!
| public String getWinCar() { | ||
| int max = getMaxPosition(); | ||
| StringJoiner joiner = new StringJoiner(DELIMITER.getMessage() + " "); | ||
| IntStream.range(0, cars.size()) |
There was a problem hiding this comment.
stream을 이렇게 사용할 수도 있을것같아요
cars.stream()
.filter(car -> car.getPosition() == max)
.forEach(car -> joiner.add(car.getName());
| public class Cars { | ||
| private final List<Car> cars; | ||
|
|
||
| public Cars(List<Car> cars) { |
There was a problem hiding this comment.
클래스를 분할해서 List 만을 가지는 일급컬렉션으로 구현하신게 너무 좋은것 같습니다.
Car가 Cars에만 참조된다면 더 좋을꺼같아요
생성자에서 List가 아닌 파라미터를 다른 자료형으로 받았으면 조금 더 깔끔했을것 같네요!
| view.printResult(print.toString()); | ||
| whoIsWinner(carVenueService); |
There was a problem hiding this comment.
결과에는 항상 최종우승자 목록도 따라오니 하나로 합치는 방법도 있을거같아요.
view.printResult나 carVenueService.printWinner에서. '실행결과\n최종 우승자 : ' 로 출력했으면
조금은 더 깔끔해졌을꺼 같아요
No description provided.