Step5 (MVC 리팩토링) 리뷰 요청드립니다.

[xlffm3]

안녕하세요.

지난 Step 4 피드백

• 테스트의 가독성 문제

  • 줄바꿈 하기

    • give, when, then 등 주석을 사용하거나 줄바꿈을 했어야했는데 부족했습니다. 최대한 테스트 코드를 짜면서 줄바꿈으로 의도를 분명하게 드러냈습니다.

  • list 관련 문제

      List<String> testTargetCarNames = cars.stream()
              .map(Car::getName)
              .collect(Collectors.toList());;
      List<String> originCarNames = Arrays.asList(carNames);
      List<String> filteredList = testTargetCarNames.stream()
              .filter(target -> originCarNames.stream().noneMatch(Predicate.isEqual(target)))
              .collect(Collectors.toList());
      assertThat(filteredList.size())
              .isEqualTo(0);    
    

  • 스트림 공부하면서 코드를 짜다가 저런 결과가 나왔네요. 배열, list 등을 비교할 때 isEqualTo 등으로 더 간략하게 표현했습니다..

---

1. Domain, View를 구분하면서

   • RacingCar 객체 내부에서 ResultView를 사용하여 결과를 출력하는 기능을 없앰.
   • RacingGameController를 추가함.

2. 코드 수정

   • 불필요하게 this. 키워드를 많이 사용한 듯 싶어서 필요하지 않은 경우 생략함.

   • 예전의 코드는 Car 객체의 MovingStrategy 인스턴스 변수를 활용하여

   void move() {
    if (this.movingStrategy.isMovable())
        position++;
    }
   

   움직일지 판단했었는데요. 현재는

   void move(MovingStrategy movingStrategy) {
    if (movingStrategy.isMovable())
        position++;
    }
   

   와 같이 파라미터로 전략을 전달하여 Car객체가 움직이도록 변경했습니다.

기존 방식은 Random값 때문에 자동차 객체의 position이 정말로 움직였는지 확인하기가 힘들었는데요. (주로 containsAnyOf(0, 1)과 같이 애매한 코드...)

RacingGame이 설정한 규칙을 Car에게 전달해 move 요청을 시키면 테스트 하기도 용이해져서 수정했는데... 이게 혹시 Car 객체의 자율성을 너무 해치는 것일까요?

혹시 어색하거나, 제가 놓친 부분 있으면 피드백 부탁드리겠습니다.
감사합니다:)

[kyucumber]

안녕하세요, 5단계 미션 잘 진행해주셨습니다.

간단한 몇가지 코멘트를 남겼으니 확인 부탁드릴게요!

[kyucumber]

먼저 남겨주신 내용에 대해 먼저 답변 드리고 리뷰를 시작할게요!

기존 방식은 Random값 때문에 자동차 객체의 position이 정말로 움직였는지 확인하기가 힘들었는데요. (주로 containsAnyOf(0, 1)과 같이 애매한 코드...)

RacingGame이 설정한 규칙을 Car에게 전달해 move 요청을 시키면 테스트 하기도 용이해져서 수정했는데... 이게 혹시 Car 객체의 자율성을 너무 해치는 것일까요?

Car가 이동 규칙을 가지는 경우

Car 객체의 position이 명확히 움직이는지를 확인하는 테스트는 전략 패턴을 활용하셨으니 이동 규칙을 갈아끼워 주는 방식으로 테스트를 진행하시면 괜찮지 않을까요?

MovingStrategy는 FunctionalInterface라 람다를 이용해 Car 객체가 가지는 이동 규칙을 쉽게 바꿀 수 있을 것 같습니다.

Car의 단위 테스트에서 생성자로 테스트에서 정의한 새로운 MovingStrategy(무조건 이동한다거나..)를 만들어 테스트에 활용해보시면 어떨까요?

RaingGame이 Car의 이동 규칙을 가지는 경우

추가로 RacingGame의 규칙을 Car에게 전달하는 방식으로 사용한다고 하셔도 MovingStarategy가 RacingGame의 내부에 캡슐화되어 RacingGame 내부의 move 메소드에 대한 테스트는 여전히 힘들것 같습니다.

지금 구조로 진행하신다고 하면 RacingGame의 생성자로 MovingStrategy를 받고 테스트에서 RacingGame 생성 시점에 이동 전략을 교체해주는 방식으로 move까지 통합적인 테스트를 하고, 실제 어떤 자동차가 가장 멀리 이동했는지, 우승자가 누군지를 구하는 방법까지 테스트해주실 수 있을 것 같네요

이게 혹시 Car 객체의 자율성을 너무 해치는 것일까요?

조금 이건 개인적인 견해일 수 있는데요 , Car 각각의 이동 전략을 RacingGame이 통합적으로 컨트롤해주는 것 보다 그 각각의 이동 전략을 Car에 위임하는 방식이 조금 더 자연스럽다고 생각이 듭니다. (각각 Car 마다 이동 전략이 다를수도 있고.. Car 자기 자신이 이동하는데 이동에 대한 규칙을 외부에서 받는게 조금 부자연스러운 생각이 들어서요)

하지만 지금의 요구사항 안에서라면 RacingGame이 이동 규칙을 가지고 각각 자동차 이동 시 마다 규칙을 넘겨주어 이동 여부를 체크하고 각각의 자동차를 이동시키는 방법이 조금 더 효율적인 것 같긴 합니다.

[xlffm3]

자세한 피드백 감사합니다. 그런 방식으로도 테스트가 가능하겠군요!!

[kyucumber]

이런 패키지 분리 등은 개인의 의도와 네이밍이 다 달라서 리뷰를 남기기 조심스러운데, 제가 생각했던 내용을 간단히 코멘트 남길게요 참고만 하시고 변경은 하시지 않아도 될 것 같아요

기존 Controller가 없을 때 Main에서 InputView를 호출해 입력을 받고, domain의 RacingGame을 이용해 결과를 받고, ResultView를 이용해 결과를 출력해주고 있었던걸로 기억합니다.

저는 그래서 이미 Main 자체가 Controller의 역할을 수행하고 있지 않았나? 하는 생각이 조금 들었어요!

[xlffm3]

아 네 맞습니다. Main 자체가 Controller 역할을 하고 있었는데요.
기존 Main 클래스에서 Controller 구조를 추가한 이유는... 특별하기 보다는 연습 차원에서 분리해본것이었습니다.

[xlffm3]

안녕하세요.
Car의 인스턴스 변수로 다시 MovingStrategy를 가지게 했고, 테스트도 수정했습니다.

말씀해주신대로 Car 생성자에서 람다식으로 다른 전략을 가지게 해서 이에 따라 position이 변화하는지 쉽게 확인할 수 있었습니다.
저는 move 메소드의 파라미터에만 집착한 나머지 이 방법을 생각하지못했네요....

리뷰어님 덕분에 평소 어렵게 느껴지던 stream & lambda에 대해 많이 공부할 수 있었습니다. 감사합니다.

[kyucumber]

5단계 리팩토링 미션 진행하시느라 고생 많으셨습니다~ 😄

추가 피드백도 잘 반영해주셨고 요구하는 내용들도 다 충족된 것으로 보입니다.

다음 단계 진행을 위해서 머지하도록 할게요!