[레벨1] 자동차 경주 2단계 - 리팩터링

• 저장소: https://github.com/yoondgu/java-racingcar/tree/step2
• 코드리뷰 진행 PR: https://github.com/woowacourse/java-racingcar/pull/568

 

리팩터링 요구사항

자동차 경주 1단계 미션에 이어 진행한 미션이다.

• 핵심 비지니스 로직을 가지는 객체를 domain 패키지, UI 관련한 객체를 view 패키지에 구현한다.
• MVC 패턴 기반으로 리팩터링해 view 패키지의 객체가 domain 패키지 객체에 의존할 수 있지만, domain 패키지의 객체는 view 패키지 객체에 의존하지 않도록 구현한다.
• 테스트 가능한 부분과 테스트하기 힘든 부분을 분리해 테스트 가능한 부분에 대해서만 단위 테스트를 진행한다.

 

구현하며 고민한 내용

1단계에서 이미 MVC 패턴을 생각하며 구현을 해서, 요구사항 관련 수정은 많지 않았다.

대신 1단계 머지 시 받은 추가 피드백을 비롯해 객체 간 책임 분리를 위한 리팩터링에 시간을 투자하였다.

• 리팩터링 시 커밋 단위
  
  • 전면 리팩터링이 필요했고 클래스 간 결합도가 높아 너무 많은 변경사항이 하나의 커밋에 기록됨
  • 이를 피하는 방법이 있을까? 현업에서는 어떻게 할까?

코드리뷰 피드백:
하나의 커밋에 많은 변경사항이 있는 상황은 충분히 있을 수 있다.
대신 커밋에 자세히 변경 내용을 적어주고, 사전에 충분한 소통(문서화나 회의)이 되었다면 문제 없다.

•  각종 책임 분리에 대하여 클래스 설계 검토
  • Car 객체 간 스스로 위치 비교할 수 있도록 변경
  • 경주에 참여하는 자동차들을 관리하는 RacingCars 클래스를 만들어 RacingGame 클래스의 책임 분리
  • RacingCars에서 자동차 개수 및 중복 이름 검증, 1회 경주 실행, 우승자 산출 책임 담당
  • 전달받은 추진력 값(power)에 따라 Car 클래스가 직접 전진 또는 유지할지 결정하도록 변경
    • 이에 따라 Car 클래스의 move() 메소드를 moveOrStayByPower(int power)로 변경하고, 전진 여부를 boolean으로 반환함

코드리뷰 피드백 : 
"전진인지 정지인지 모르는 메서드가 좋지 않다"고 느낀 것은 좋다. 
하지만 boolean 값을 꼭 반환해야 하는지 고민해보면 좋겠다.
자동차가 움직였는지 아닌지를 알려주는 것이 자동차 경주 게임이라는 도메인에서 Car가 가지는 책임인가?

• 테스트 코드 내부 클래스 분리
  • 생성자 테스트/ 기능테스트에 따라 @BeforeEach 로 중복을 제거할 수 있는 테스트와, 그렇지 않은 테스트가 나뉘는 상황에 대하여 내부 클래스로 테스트를 나눈 뒤 필요한 부분에만 어노테이션 적용함
• 예외 테스트에서 메시지 비교를 통한 정확한 원인 파악, 괜찮을까?
  • Exception 객체나 단순 값을 비교 할 수 있는 다른 결과값들과 달리, 예외 메시지는 같은 의미를 가져도 문자열의 정확한 내용은 달라질 수 있다. => 그때마다 테스트 코드를 수정해주어야 한다.

코드리뷰 피드백 : 
테스트가 내부 구현 변경에 취약하면 안된다는 특징을 가지고 있는 것은 맞다.
하지만, 반대로 테스트하고자 하는 바를 정확하게 테스트해야 한다는 특징 또한 중요하다.
무엇이 더 중요한지 스스로 고민해보고 결정을 내리면 좋겠다!
추가로, 정확한 메시지를 비교하는 대신 중요한 키워드가 포함되어있는지를 확인하기 위해 hasMessageContaining() 같은 기능을 사용할 수도 있다.

 

코드리뷰에서 배운 것들

• 자동차 경주 미션에서는 리뷰어님께 "이게 괜찮은 방법일까요?" 질문을 많이 드렸는데,
  "명확한 의도를 가지고 구현했다면 그것이 정답이다"라고 답변해주셨다.
  • 하나의 정답이 있는 것이 아니라, 내가 명확한 의도를 가지고 있고 그 의도를 잘 드러내는 것이 중요하다는 걸 깨달았다.
• 테스트를 위한 생성자 사용에 대한 견해
  • 리뷰어님은 "객체를 생성할 수 있는 다양한 방법을 열어 놓은 것이라고 생각하기 때문"에 생성자는 많아도 된다고 본다고 하셨다. 물론 이에 대해 반대하는 의견도 있을 것이다.
  • 이를 비롯해, 테스트만을 위한 코드에 대한 나의 의견을 정리해볼 필요를 느꼈다.
• 내부 필드의 구현 내용을 그대로 중개해주기만 하는 객체도 도메인으로 볼 수 있을까?
  • 다른 구현 내용 없이 메서드를 호출해서 반환하는 방식으로 전달만 한다면, 도메인 보다는 서비스 계층에 가깝다고 생각된다.
  • 클래스 분리란 무엇인가에 대해 고민하면서 같이 더 생각해봐야겠다.
• Lazy Initialization
  • 필드의 초기화 시점을 그 값이 처음 필요할 때까지 늦추는 기법
  • 생성자 메서드가 호출될 때 초기화 로직을 실행하도록 해보자. (최적화 용도)