Conversation
| import java.util.List; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class CarGameStarterTest { |
There was a problem hiding this comment.
이 클래스 내에 인덴트 레벨이 서로 조금 다른 것 같아요!
src/main/java/CarGameStarter.java
Outdated
| System.out.println("경주할 자동차 이름을 입력하세요."); | ||
| Scanner scan = new Scanner(System.in); | ||
| String carNameInput = scan.nextLine(); | ||
| String[] carArr = carNameInput.split(","); |
There was a problem hiding this comment.
| String[] carArr = carNameInput.split(","); | |
| carNames = Arrays.asList(carNameInput.split(",")); |
29, 30 라인에 있는 String[] -> List 변환 작업을 위처럼 한 줄로 해볼 수도 있을 것 같네요.
src/main/java/CarGameStarter.java
Outdated
| getCarNamesFromUser(); | ||
| getNumOfTrialFromUser(); | ||
| List<Car> cars = carNames.stream() | ||
| .map(carName -> new Car(carName)) |
There was a problem hiding this comment.
| .map(carName -> new Car(carName)) | |
| .map(Car::new) |
람다 레퍼런스를 쓰면 조금 더 깔끔해질 수 있습니다!
src/main/java/CarGameStarter.java
Outdated
| return createGame(trialNum, cars); | ||
| } | ||
|
|
||
| CarGame createGame(int trialNum, List<Car> cars) { |
There was a problem hiding this comment.
new CarGame과 createGame이 같은 레이어인 것 같은데, 조금 과한 추상화가 아닌지 생각해보게 됩니다.
src/main/java/CarResult.java
Outdated
| @@ -0,0 +1,27 @@ | |||
| class CarResult { | |||
| private String name; | |||
| private int trialNum; | |||
There was a problem hiding this comment.
사용하지 않는 trialNum을 CarResult가 가지고 있는 것 같습니다.
src/main/java/GameResult.java
Outdated
|
|
||
| class GameResult { | ||
| private List<GameSnapshot> snapshots = new ArrayList<>(); | ||
| private List<Car> winners = new ArrayList<>(); |
There was a problem hiding this comment.
현재는 사용하지 않는 winners인 것 같습니다.
만들 당시에는 어떤 뜻이 있었을 것 같은데.. 무언가 꽃 피우지 못한 변수랄까..
src/main/java/GameResult.java
Outdated
| this.snapshots = snapshots; | ||
| } | ||
|
|
||
| void printResults() { |
There was a problem hiding this comment.
클래스 이름에서 result가 들어가 있어 충분히 많은 정보가 사용하는 측에 전달되는 것 같습니다.
메서드에서 두 번 result라는 단어를 사용하지 않아도 되지 않을까요?
src/main/java/GameResult.java
Outdated
| printWinners(winners); | ||
| } | ||
|
|
||
| void printWinners(List<CarResult> winners) { |
There was a problem hiding this comment.
각 게임의 스냅샷을 객체 내부에 가지고 있다면, winners를 외부에서 받을 필요 있을까요?
winner는 언제 계산 되어지는 것이 좋을까요?
src/main/java/GameResult.java
Outdated
| for (GameSnapshot snapshot : snapshots) { | ||
| snapshot.printOneTrial(); | ||
| } | ||
| List<CarResult> winners = Judge.pickWinners(snapshots.get(snapshots.size() - 1)); |
There was a problem hiding this comment.
현재는 우승자가 결과를 출력할 때 필요하기 때문에 결과를 출력하는 메서드 안에서 우승자를 선별하는 일까지 모두 하고 있는데,
요구사항이 추가되어 우승자를 사용하는 곳이 이곳 저곳 여러 군데가 된다면 어떻게 될까요?
참조할만한 문서: 개방-폐쇄 원칙
src/main/java/Car.java
Outdated
| this.movement = this.movement + "-"; | ||
| CarResult move() { | ||
| trialNum += 1; | ||
| if (RandomGenerator.getNumber() > 3) { |
There was a problem hiding this comment.
이 코드를 처음 보는 ㅡ 요구사항을 모르는 독자 ㅡ 개발자는 이 조건문이 왜 필요한지, 어떤 목적인지 모를 것 같아요.
매직넘버 사용을 피하고, 조건의 의도가 드러나도록 추상화하면 좋을 것 같습니다!
|
코드리뷰 정말 감사합니다 ㅜㅜ |
ㅠㅠ 결국 test 다 못짰어요......
하려다가 못한것들은 TODO로 남겨놓았습니다