[자동차 경주 게임] 효오 미션 제출합니다. #40
Conversation
src/main/java/racingcar/Cars.java
Outdated
| } | ||
|
|
||
| Cars(List<String> carNames, List<Integer> positions) { | ||
| Car.instantiateCar(carNames, cars, positions); |
There was a problem hiding this comment.
구현한 Car는 이미 position을 가지고 있습니다.
이를 활용하면 List positions 없이도 충분히 구현 가능하다고 생각합니다(테스트 코드 포함)
|
|
||
|
|
||
|
|
||
|
|
|
|
||
| return CalculatorException.applyCalculationException(); | ||
| } | ||
|
|
There was a problem hiding this comment.
필수는 아니지만, 문자열 계산기는 enum을 활용해서도 구현할 수 있습니다!
enum을 이용한 구현에 대해 고민해보면 어떨까요!?
There was a problem hiding this comment.
enum으로 if없이 구현해보려고 했지만 어려워서 더 공부하고 하려고 합니다ㅜ
src/main/java/racingcar/Car.java
Outdated
|
|
||
| public static int randomNumberGenerator() { | ||
| return (int) (Math.random() * MAX_RANDOM_NUMBER) + MIN_RANDOM_NUMBER; | ||
| } |
There was a problem hiding this comment.
(Math.random() * MAX_RANDOM_NUMBER) 자체로 0~9 사이의 랜덤 값을 생성해 주기 때문에 MIN_RANDOM_NUMBER(0) 는 더하지 않아도 될것 같아요!
추가로 랜덤 넘버 생성의 경우, Car 객체 보다는 Util 혹은, Car 외부에서 담당하는것이 더 좋을 것 같습니다!
자동차는 단순히 자신이 움직일 수 있는가?에 대해서만 확인 후 움직이거나 움직이지 않는 역할로 충분하지 않을까요?
There was a problem hiding this comment.
넵 피드백 주신 내용 반영했고
Util에서 따로 만들어주니 더 깔끔해지는 효과가 있었습니다!
src/test/java/racingcar/CarTest.java
Outdated
| public class CarTest { | ||
| @Test | ||
| void randomNumberGeneratorTest() { | ||
| int random = Car.randomNumberGenerator(); |
There was a problem hiding this comment.
테스트 메서드 이름은 "메소드명_테스트하려는 상태_기대하는 동작" 과 같이 무엇을 테스트 하려고 하는지 명확하게 나타내도록 작성하는 것이 더 좋을 것 같다고 생각합니다.
There was a problem hiding this comment.
RacingCar에서 test 메소드명을 계속 "메소드명" + "Test"로 짓고 있었고,
한글로 메소드명을 명확하게 지어주니 어떤 부분을 테스트하는지 명확하게 알 수 있었습니다!
| cars.add(new Car("a", 101)); | ||
| cars.add(new Car("b", 101)); | ||
| cars.add(new Car("c", 99)); | ||
| testCars = new Cars(cars); |
There was a problem hiding this comment.
add와 같은 방식도 좋지만 Arrays.asList()를 사용하는 것은 어떨까요?
| return this.name; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
null을 리턴하는 것은 굉장히 위험합니다!
sameMaxPositionCarName를 Winner 관련 클래스에서 사용하고 있는데, 아래에 피드백 남기겠습니다!
대안으로, isSamePosition과 같은 메서드를 통해 maxPosition인지 아닌지를 boolean 타입 등으로 확인하면 어떨까요?
There was a problem hiding this comment.
이번 미션을 통해서 null 값을 리턴하는 것이 위험하다고 깨달았고
메시지를 던져서 maxPosition인지 아닌지 확인하는 방식으로 리팩토링 하였습니다!
There was a problem hiding this comment.
비교할 로직이 복잡하지 않은 경우, 위와 같이 단순하고 명료한 경우 return this.position == maxPosition 같이 한줄로 구현할 수 있어요!
| Winners winners = new Winners(cars); | ||
| OutputView.printWinners(winners); | ||
| System.exit(0); | ||
| } |
There was a problem hiding this comment.
Play 클래스의 역할과 CarGameLauncher 역할이 섞여 있는 것 같아요!
CarGameLauncher는 인풋을 받고, 경주를 실행하기 위해 필요한 인풋을 전달하고(Play 클래스로), 우승자를 구하고, 출력하기
Play는 전달 받은 인풋에 대해 validation을 확인 후 자동차를 생성/경주를 진행하기(하지만 출력도 함께 진행하고 있어요!)
가 각각 꼭 담당해야하는 역할이라고 생각해요
public static void main(String[] args) {
//input for car names and round count
Play play = new Play(carNames, tryCount);
Winners winners = null;
while(play.hasNextRound()) {
winners = play.move();
// print
}
// print
}
구현에 정답은 없지만, 힌트는 위에 첨부한 코드와 같습니다!
There was a problem hiding this comment.
피드백 주신대로 play에서 자동차를 생성/ 경주를 진행하고
CarGameLauncher에서 단순히 인풋을 전달하고 출력만 하는 기능으로 최대한 나누어 보았습니다.
There was a problem hiding this comment.
추후에는 for (int i = 0; i < totalTurns; i++) 이 부분도 Play가 담당하도록 구현해봐도 좋을 것 같아요! 👍
| int maxPosition = decideMaxPosition(carsState); | ||
| decideWinners(carsState, maxPosition); | ||
| } | ||
|
|
There was a problem hiding this comment.
Winners 클래스는 listOfWinners를 String으로 가지고 있는 대신, List를 가지고 있는 것은 어떨까요!?
또한 Winners 생성자에서 우승자를 구하는 것이 아닌,
public String getWinners() {
}
와 같이 메서드로 분리하는 것은 어떨까요?
그럼 우승자 String을 구하는 로직이 조금 더 간단해질 것 같아요!
(구현하며 만약 Car의 name을 get 해야 하는 경우가 발생한다면 허용하겠습니다!)
| } | ||
| return carState; | ||
| } | ||
|
|
There was a problem hiding this comment.
출력을 위한 string을 생성하는 코드로 보여지는데요, 관련하여 toString Override에 대해 찾아보고 적용하면 좋을것 같습니다!
There was a problem hiding this comment.
toString을 활용하니 더 효율적이고 앞으로도 유용하게 쓰일 것 같다고 느꼈습니다.
src/main/java/racingcar/Car.java
Outdated
| } | ||
|
|
||
| Car(String name, int position) { | ||
| this.name = name; |
There was a problem hiding this comment.
더불어 Car(String name) 생성자에서 position에 대한 초기화도 함께 진행하는 것이 어떨까요
해당 생성자는 테스트를 위해 만드신걸로 보이는데, 비록 테스트를 위해 만들어졌다고 하더라도 위 의 생성자에서 하는 is~에 대한 확인은 해야한다고 생각합니다!
There was a problem hiding this comment.
잘 반영했는지 모르겠지만 Car(String name) 생성자에서 position을 초기화 하도록 다른 생성자로 포지션을 0을 넘겨주어 리팩토링하였습니다.
| if (symbol.equals("/")) return CalculatorException.divisionException(firstNumber, secondNumber); | ||
| return CalculatorException.applyCalculationException(); | ||
| } | ||
| } |
There was a problem hiding this comment.
앞으로 남은 미션을 진행하면서 enum이 굉장히 자주 유용하게 사용될것입니다!
따라서 이번 미션을 통해서 enum에 대해 완전히 숙지 하지 못하였더라도 꼭 공부하면 좋을 것 같습니다!!
There was a problem hiding this comment.
다른분이 슬랙에 올려주신 내용을 확인했는데 enum 말고 다른 내용도 이해가 필요할 것 같아서 시간 남을 때마다 모르는 부분을 체크하고 공부하려고 합니다 ㅠ
src/main/java/racingcar/Play.java
Outdated
| } | ||
|
|
||
| public static void moveCar(Cars cars) { | ||
| for (int i = 0, n = cars.getSize(); i < n; i++) { |
There was a problem hiding this comment.
index를 사용한 for loop 보다는 for each에 대해 공부해보면 좋을 것 같습니다!
또한 지금처럼 Cars 클래스에서 움직임을 구현하고 싶다면 updateCarMovement에 index를 넘겨 car.get(i).~ 보다는
위의 구조에서는
for (int i = 0; i < turn; i++) {
cars.moveCars();
}
로 변경하고 Cars 클래스에서 Cars가 가지고 있는 Car List를 순회하며 move 하는 것은 어떨까요?
다만, 해당 클래스의 구조는 제가 위에 드린 예시 코드 적용에 맞지 않을 수 있다고 생각됩니다!
따라서 제가 드린 피드백을 참고하여 고민해보면 좋을 것 같습니다!
There was a problem hiding this comment.
전에 피드백 주신 play 부분을 고치고 나니 여기 부분도 쉽게 수정할 수 있었고 코드가 깔끔해지며 효율성이 더 높아진 효과가 있는 것 같습니다!
| return this.name; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
비교할 로직이 복잡하지 않은 경우, 위와 같이 단순하고 명료한 경우 return this.position == maxPosition 같이 한줄로 구현할 수 있어요!
| Winners winners = new Winners(cars); | ||
| OutputView.printWinners(winners); | ||
| System.exit(0); | ||
| } |
There was a problem hiding this comment.
추후에는 for (int i = 0; i < totalTurns; i++) 이 부분도 Play가 담당하도록 구현해봐도 좋을 것 같아요! 👍
| } | ||
| return max; | ||
| } | ||
|
|
No description provided.