Step 5 - 자동차경주(리팩토링) #5815
Step 5 - 자동차경주(리팩토링) #5815
Conversation
출입력 받는 패키지명을 ui->view로 변경
…ngFleet으로 옮김 racingFleet에 속하는 차들 race하는 raceAll()함수를 만듬
racingFleet에서 getRacingCars로 받고 있어서, RacingFleet로 인자로 받도록 변경
movableStrategy 결정자를 책임자인 carRacing으로 이동
movableStrategy 결정자를 책임자인 carRacing으로 이동
raceAll 테스트 구현
public으로 필드 접근 가능해서 private으로 변경
…apResults 필드를 histories로 변경 wrapResults를 DTO 필드가 아니라 Racinghistory를 나타내는 새로운 클래스를 만들어서 CarRacing에 저장함. 레이싱 결과 기록 관련 책임을 분리하여 응집도를 올림.
RacingFleet를 통해 RacingState를 저장하는 함수에 관한 테스트 위주로 작성
RacingFleet를 통해 RacingState를 저장하는 메서드 구현
안쓰는 import 제거
린트 적용
…분을 main으로 옮김 테스트하기 어려운 RandomMovableStrategy의 의존관계를 Object Graph의 상위인 main으로 옮김
…leStrategy 대신에 ()->true를 주입함 테스트가 특정 movableStrategy에 의존하지 않도록함.
| @@ -12,8 +13,7 @@ public class Main { | |||
| public static void main(String[] args) { | |||
| List<String> carNames = InputView.inputCarNames(); | |||
| int tryNumber = InputView.inputTryNumber(); | |||
|
|
|||
| RacingResultDTO result = RacingCarSimulator.simulate(carNames, tryNumber); | |||
| RacingResultDTO result = RacingCarSimulator.simulate(carNames, tryNumber, RandomMovableStrategy.getInstance()); | |||
| private final RacingHistories histories; | ||
| private final MovableStrategy movableStrategy; |
There was a problem hiding this comment.
histories 객체는 proceedWraps 메서드의 결과인데요.
proceedWraps 메서드를 수행할 때마다 이력이 계속 쌓이게 됩니다.
그렇다보니 DB의 역할을 겸하고 있다고 생각되는데요, 이력을 상태로 가지기 보다 proceedWraps 메서드 내부에서 생성해서 반환하면 어떨까요?
movablesStrategy 변수는 racingFleet.raceAll 메서드에만 필요합니다.
RacingCar 처럼 변수로 선언하기 보다 해당 로직을 수행할 때 메서드의 인자로 받으면 어떨까요?
| @@ -43,19 +37,12 @@ public void proceedWraps(int tryNumber) { | |||
| } | |||
|
|
|||
| private void proceedWrap() { | |||
There was a problem hiding this comment.
자동차들이 1회 이동을 시도한 결과를 반환하면 이력을 분리할 수 있을 것 같아요.
| private void proceedWrap() { | |
| private RacingFleet proceedWrap() { |
| import racingcar.dto.RacingResultDTO; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| public class RacingCarSimulator { | ||
| public static RacingResultDTO simulate(List<String> carNames, int tryNumber) { | ||
| CarRacing carRacing = CarRacing.valueOf(carNames); | ||
| public static RacingResultDTO simulate(List<String> carNames, int tryNumber, MovableStrategy movableStrategy) { |
There was a problem hiding this comment.
RacingCarSimulator 클래스는 상태도 없고 simulate 메서드의 로직은 컨트롤러의 로직이라고 생각되는데요.
로직을 main 메서드로 이동하고 RacingCarSimulator 클래스는 제거하면 어떨까요?
MVC 패턴으로 리팩터링하는 것이 요구사항이니 어느 클래스 컨트롤러의 역할을 하는지 명시해 주시면 좋을 것 같아요
|
|
||
| import java.util.Objects; | ||
|
|
||
| public class RacingCarState { |
There was a problem hiding this comment.
CarRacing 내부에 RacingHistories 필드를 가지다 보니 많은 클래스가 추가되고 전체적인 복잡도가 상승한 것 같아요.
새롭게 추가된 클래스들은 DTO와 다르지 않은 것 같은데요. 기존의 DTO를 활용할 수 없는걸까요?
|
|
||
| public void recordRacingState(RacingFleet racingFleet) { | ||
| RacingCarStates carStates = RacingCarStates.valueOf(racingFleet); | ||
| value.add(RacingHistory.valueOf(findCurrentWrapNo() + 1, carStates)); |
There was a problem hiding this comment.
기존에 lap + 1 로직을 보고 auto increment와 굉장히 유사하다는 느낌을 받았습니다.
그래서 CarRacing 클래스가 RacingHistories 필드가 더 DB 같다는 생각이었는데요,
인상적인 부분이었지만, 요구사항과는 거리가 좀 있는 구현이라 오버 엔지니어링이 아닌지 고민해 보시면 좋을 것 같아요.
마지막 step5 리뷰 요청드립니다. 아래 사항을 리팩토링 했습니다.