Step4 - 자동차 경주 #4723
Step4 - 자동차 경주 #4723
Conversation
|
기능구현에 추가하여 step3에서 review해주신 부분 반영했습니다. 지난 리뷰에서
어떤것이 더 유리한 방법인가에 대해서는 아직 잘 모르겠습니다. Car객체에서 Strategy를 포함하여 테스트할 수 있는게 장점일까요? |
네 제대로 이해하신 것 같아요! |
| for (Car car : carList) { | ||
| if (car.getPosition() > maxPosition) { | ||
| maxPosition = car.getPosition(); | ||
| winnerList.clear(); | ||
| winnerList.add(car); | ||
| } else if (car.getPosition() == maxPosition) { | ||
| winnerList.add(car); | ||
| } | ||
| } |
There was a problem hiding this comment.
현재 코드를 보시면 depth가 2단계(for문안에 if 블록)이고 else 키워드를 사용하고 있어요
프로그래밍 요구사항에 맞게 변경해 볼까요?
| public void play() { | ||
| for (Car car : carList) { | ||
| car.move(moveStrategy); | ||
| } | ||
| } |
There was a problem hiding this comment.
intellij를 사용하고 계신가요? 3단계에 있었던 요구사항이었는데요
단축키를 사용하면 쉽게 컨벤션을 맞출 수 있습니다!
| public void play() { | |
| for (Car car : carList) { | |
| car.move(moveStrategy); | |
| } | |
| } | |
| public void play() { | |
| for (Car car : carList) { | |
| car.move(moveStrategy); | |
| } | |
| } |
|
|
||
| public class RacingGame { | ||
|
|
||
| private final List<Car> carList = new ArrayList<>(); |
There was a problem hiding this comment.
| private final List<Car> carList = new ArrayList<>(); | |
| private final List<Car> cars = new ArrayList<>(); |
자료구조에 의존하지 않는 변수명을 사용하는 것을 권장하고 있습니다.
그래서 보통 컬렉션을 담고있는 변수명은 주로 복수형으로 표현하고 있어요
사실 거의 변경이 없겠지만 자료구조가 변경된다는 이유로 변수명이 함께 변경되는 것은 좋지 않은 것 같아요
| private static final String POSITION_MARK = "-"; | ||
| public void printCars(List<Car> cars) { |
There was a problem hiding this comment.
가독성을 위해 변수 필드 부분과 메서드를 개행으로 분리하는 것이 좋습니다.
private static final String POSITION_MARK = "-";
public void printCars(List<Car> cars) {| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class RacingGame { |
There was a problem hiding this comment.
RacingGame도 도메인으로 볼 수 있을 것 같아요
그런데 테스트 코드가 없네요 😭
There was a problem hiding this comment.
패키지 위치도 같이 고려해보시면 좋을 것 같아요
RacingGame과 move 패키지 내 클래스들도 domain의 영역이지 않을까요?
추가로 InputView, ResultView 부분도 view 같은 패키지로 이동시켜보면
구조를 쉽게 파악할 수 있을 것 같습니다.
| for (Car car : carList) { | ||
| if (car.getPosition() > maxPosition) { | ||
| maxPosition = car.getPosition(); | ||
| winnerList.clear(); | ||
| winnerList.add(car); | ||
| } else if (car.getPosition() == maxPosition) { | ||
| winnerList.add(car); | ||
| } | ||
| } |
There was a problem hiding this comment.
로직이 너무 비효율적인 것 같아요
- 자동차 리스트에서 maxPosition을 구한다.
- maxPosition인 Car만 추출해서 반환
이렇게만 작성해도 훨씬 읽기 좋은 코드가 될 것 같습니다 😄
| public boolean isSuccess() { | ||
| final Random random = new Random(); |
There was a problem hiding this comment.
isSuccess()가 호출될 때 마다 Random 객체를 새로 생성할 필요가 있을까요?
재사용할 수 있도록 변경할 수 있을 것 같아요
| private final String name; | ||
|
|
There was a problem hiding this comment.
요구사항 중 자동차 이름은 5자를 초과할 수 없다.가 있었어요
해당 부분도 구현해야 하지 않을까요?
이에 대한 테스트 코드도 있으면 좋겠어요
기능구현 완료하여 pr제출합니다.
감사합니다!