[자동차 경주] 해로(김선호) 미션 제출합니다. #4
Conversation
| package racingcar.model; | ||
|
|
||
| public interface RandomSingleDigitPicker { | ||
|
|
||
| int pick(); | ||
|
|
||
| } |
There was a problem hiding this comment.
RandomSingleDigitPicker의 패키지 위치가 애매해서 고민하다가 우선은 model에 두었는데, 아무래도 도메인 모델은 아니다 보니 영 찜찜하네요. 그렇다고 정적 유틸로 보기에는 애매해서 utils 보다는 더 핵심에 가까운 model에 두었는데 혹시 리뷰어분들이라면 어느 패키지에 두셨을지 궁금합니다 :)
There was a problem hiding this comment.
일단 저도 model(습관적으로 작성해서 패키지명이 model이 아니라 domain인건 미션적 허용으로 넘어가기^^...) 패키지에 같은 역할을 하는 RandomNumberPicker 를 넣어뒀는데요
자동차 경주 속에서 도메인 모델들과 핵심적인 협력을 하고 있어서 model 에 두었는데 다른 분들과도 의견 나눠보죠 내일!
| @ParameterizedTest | ||
| @ValueSource(ints = {-1, -100, -1000}) | ||
| void invalid_car_name(int numOfMovedForward) { |
There was a problem hiding this comment.
해당 부분은 -1 값을 제외한 -100, -1_000 두 값은 테스트에 있어 큰 의미가 없는 듯 한데 혹시 두 값도 테스트하신 이유가 있으신가요?? 경계는 -1이면 충분하다고 생각하는데 해로의 생각이 궁금하네요!
There was a problem hiding this comment.
맞아요 사실 -1 이면 경계값 테스트로는 충분해요.
이렇게 점점 작은 음수를 파라미터로 넣은 것은 음수는 제약에 걸리는구나라는 컨텍스트를 테스트 메서드의 파라미터로 드러내고 싶은 것이 컸습니다.
그런데 가만 생각해보니, 경계값은 이미 -1로도 충분히 검증 가능하고, 컨텍스트를 잘 나타내기 위해 DisplayName도 고심고심하며 지었으므로 굳이 테스트 메서드의 파라미터에서까지 이 컨텍스트를 드러낼 필요가 있을까(혹은 드러나긴 할까)라는 생각이 드네요. 다음 미션부터는 이 부분을 바꿔봐야겠습니다 👍
| AttemptCount(final int count) { | ||
| this.count = count; | ||
| } |
There was a problem hiding this comment.
이 부분은 테스트를 위해 private 이 아닌 default 로 열어두신 건가요?
만약 그렇다면 해로가 작성해주신 테스트들은 from 을 통한 객체 생성으로도 충분히 작성 가능할 듯 한데 이유가 궁금해요!
There was a problem hiding this comment.
맞아요. 보통 테스트를 위한 데이터를 준비할 때(given), raw한 값 그 자체를 사용하고 싶어서 생성자를 사용했어요.
테스트에서 validation이나 코드가 포함된 팩토리 메서드를 이용하면 객체 초기화 시점에서 개발자가 원하는 값을 넣기 어려운 경우가 종종 있을 때 빌더를 이용해서 데이터를 준비하곤 하는데, 빌더 없이 데이터를 준비하다보니 default 접근 제어자를 사용했어요.
그런데 가만 생각해보면 위 AttempCount같은 경우는 위 같은 케이스는 아니기 때문에, 짱구가 제안해주신 것처럼 from을 이용해서 데이터를 준비하는 것도 괜찮아 보이네요. 생성자에 가드(validation)를�옮겨놓고 팩토리 메서드를 없애고 생성자로만 데이터를 준비하는 것도 괜찮아 보이고요 :)
| } | ||
|
|
||
| public void deductOneCount() { | ||
| if (this.count - 1 < 0) { |
There was a problem hiding this comment.
this.count < 1 (시도 횟수가 하나 미만인 경우) 라고 되어 있었다면 더 읽기 쉬웠을 거 같아요!
There was a problem hiding this comment.
횟수 하나를 공제했을 때 음수가 된다면을 드러내고 싶어서 이렇게 작성했는데,
현재 횟수가 0회라면 뺄게 없어서 에러를 던질거에요. 라는 컨텍스트도 괜찮아보이는군요!
| public WinnerCars(List<RacingCar> cars) { | ||
| validateWinnerCarPolicy(cars); | ||
| this.cars = cars; | ||
| } | ||
|
|
||
| private void validateWinnerCarPolicy(final List<RacingCar> cars) { | ||
| final long distinctNumOfMoveForwardCounts = cars.stream() | ||
| .mapToInt(RacingCar::getNumOfMovedForward) | ||
| .distinct() | ||
| .count(); | ||
|
|
||
| if (distinctNumOfMoveForwardCounts > 1) { | ||
| throw new IllegalArgumentException("All number of moved forward of winner cars must be same."); | ||
| } | ||
| } |
There was a problem hiding this comment.
(궁금합니다!)
우승한 자동차들을 나타내는 WinnerCars 객체는 자동차들을 파라미터로 받아서 우승자를 추출하는 것 까지가 WinnerCars 의 역할이라고 저는 생각했는데 해로는 아예 우승자들이 아니면 예외를 처리하도록 해두셨네요!
이렇게 되면 외부에서 우승자들을 정하는 게 되어 버려서 WinnerCars의 존재가 애매해 진다고 생각하는데 이에 대해 해로의 의견이 궁금합니다!
There was a problem hiding this comment.
저는 오히려 우승 자동차를 선정하는 로직은 RacingCars 에 있는게 더 적합하지 않을까 생각이 듭니다.
WinnerCars 객체는 우승한 자동차들을 나타내는 역할을 하고 있고(그냥 그 자체로 우승 자동차들 정보를 담고 있음), 우승 자동차를 선정하는 로직은 레이싱에 참여하는 자동차들을 관리하고 조작하는 역할을 하는 RacingCars 에 있는게 더 적합해보여요..!
There was a problem hiding this comment.
(궁금합니다!) 우승한 자동차들을 나타내는 WinnerCars 객체는 자동차들을 파라미터로 받아서 우승자를 추출하는 것 까지가 WinnerCars 의 역할이라고 저는 생각했는데 해로는 아예 우승자들이 아니면 예외를 처리하도록 해두셨네요!
이렇게 되면 외부에서 우승자들을 정하는 게 되어 버려서 WinnerCars의 존재가 애매해 진다고 생각하는데 이에 대해 해로의 의견이 궁금합니다!
WinnerCars 객체는 이미 생성되는 시점에 우승 자동차가 확정되어야 하므로 우승 자동차를 추출하는 역할은 부여할 수가 없다고 생각했어요. 그래서 우승자가 아니면 객체 생성이 되지 않도록 가드(validation)을 세워두었습니다.
정적 팩토리 메서드에서 파라미터로 받아서 우승자를 추출할 수도 있긴 한데, 이것 역시 정적 메서드니까 객체의 역할/책임으로 보기는 어렵겠더라고요.
결국 우승자를 가져오도록 메시지를 보낼 객체가 자동차의 전진 횟수에 대한 데이터가 연결된 객체로 한정되는데 그러고보니 RacingCars 객체가 메시지를 받는게 제일 괜찮아보이더라고요 :)
외부에서 우승자를 정하는 것
RacingCars한테 메시지를 보내는 것은 오히려 내부에 가깝다고 생각했어요!
There was a problem hiding this comment.
저는 오히려 우승 자동차를 선정하는 로직은 RacingCars 에 있는게 더 적합하지 않을까 생각이 듭니다.
WinnerCars 객체는 우승한 자동차들을 나타내는 역할을 하고 있고(그냥 그 자체로 우승 자동차들 정보를 담고 있음), 우승 자동차를 선정하는 로직은 레이싱에 참여하는 자동차들을 관리하고 조작하는 역할을 하는 RacingCars 에 있는게 더 적합해보여요..!
'그냥 그 자체로 우승 자동차들 정보를 담고 있음' 👈 맞습니다 이것을 의도했습니다! 👍
| package racingcar.model; | ||
|
|
||
| public interface RandomSingleDigitPicker { | ||
|
|
||
| int pick(); | ||
|
|
||
| } |
There was a problem hiding this comment.
일단 저도 model(습관적으로 작성해서 패키지명이 model이 아니라 domain인건 미션적 허용으로 넘어가기^^...) 패키지에 같은 역할을 하는 RandomNumberPicker 를 넣어뒀는데요
자동차 경주 속에서 도메인 모델들과 핵심적인 협력을 하고 있어서 model 에 두었는데 다른 분들과도 의견 나눠보죠 내일!
| public WinnerCars judgeWinnerCars() { | ||
| final List<RacingCar> winnerCars = this.cars.stream() | ||
| .filter(car -> car.matchesNumOfMovedForward(calculateMaxNumOfMovedForward())) | ||
| .toList(); | ||
|
|
||
| return new WinnerCars(winnerCars); | ||
| } |
There was a problem hiding this comment.
저는 필터를 하는 부분도 WinnerCars 객체가 하면 좋지 않을까 생각 하는데 해로의 의견이 궁금하네여!
우승자를 가리는 기준이 변경된다고 하더라도(막말로 시작점에서 가장 가까운 자동차가 우승;;;) RacingCars 객체는 변경과 무관해야 한다고 생각해요!
There was a problem hiding this comment.
(이 부분은 #4 (comment) 이 코멘트와도 관련 있습니다.)
필터를 하는 부분도 WinnerCars 객체가 하면 좋지 않을까
맞아요 저 역시 이 부분을 고민해서 WinnerCars 한테 파라미터를 던져줘서 알아서 뽑히게 하는게 좋을 것 같아서 WinnerCars에 두려고 했습니다.
그런데 생성자에서 이 과정을 하려고 하니까 생성자에서 코드를 집어넣었어야 했고(불-편),
생성자에 코드를 넣더라도 우승자를 추출하는 과정이 메시지를 통해 이루어지는 것이 아니다보니 객체지향적이지 않다고 느꼈어요. 팩토리 메서드에서 하게되면 정적 메소드를 쓰는 시점에서 우승자를 추출하라 라는 메시지를 전달할 수 없게 되고요.
다만 위처럼 구현하는 시점에서 우승자를 가리는 정책이 변경되면 짱구 말대로 RacingCars 코드를 곧바로 수정해야하긴 합니다. 확실히 이 부분은 개선해 볼 필요가 있죠!!
그래서 제가 미션 후에 곰곰히 계속 생각하다가 좋은 아이디어가 떠올랐지 뭡니까?
- 우선
우승차들을 추출해라라는 메시지는 RacingCars가 그대로 받아요. (왜냐면 현재 이 객체 말고는 메시지를 받을 객체가 없으니까요!) - 메시지를 전달 받은 객체가 메시지를 어떻게 수행할 것인지는 객체의 자율성에 따라 RacingCars가 알아서 정할 것이므로, 여기서 WinnerCars의 정적 팩토리 메서드를 호출합니다. 그럼 실제 정책 구현 과정은 팩토리 메서드안에서 일어나겠지만 RacingCars는 본인의 책임을 다하기는 한거니까요!
- 위 처럼 구현을 변경하면, 정책이 바뀌더라도 RacingCars의 코드 수정은 일어나지 않게 될 것 같아요.
위 방법에 대해서 짱구나 은정님은 어떻게 생각하시는지 궁금해지네요 ㅎ
cc. @rueun
| final AttemptCount attemptCount = inputView.inputAttemptCount(); | ||
|
|
||
| resultView.printResultGuide(); | ||
|
|
||
| while (attemptCount.hasCount()) { | ||
| cars.moveForward(singleDigitPicker); | ||
|
|
||
| resultView.printGameStatus(cars); | ||
|
|
||
| attemptCount.deductOneCount(); |
There was a problem hiding this comment.
처음 이 코드를 봤을 땐 AttemptCount 가 단순히 입력 값을 받는 역할을 하는 줄 알았는데 바로 아래를 보니 자동차 경주 흐름에서 꽤 다양한 역할을 하고 있네요!
There was a problem hiding this comment.
AttemptCount가 다양한 역할을 하고 있다고는 생각하지 않습니다!
카운트의 상태를 관리하는 역할 한 가지만을 수행하고 있다고 생각해요. 책임은 아래 두 가지가 부여되어있고요.
책임1. 카운트가 있는지 확인
책임2. 카운트를 감소
Controller가 다양한 역할을 하고 있다고 말씀하신거라면 그것에는 확실히 동의합니다(끄-덕)
| private int numOfMovedForward; | ||
| private final RacingCarName name; |
There was a problem hiding this comment.
자동차 이름을 뜻하는 name 은 RacingCarName 이라는 객체를 만들었는데, 자동차 이동 거리? 는 객체로 만들지 않고 �기본 자료형을 쓴 이유가 따로 있을까요?
There was a problem hiding this comment.
맞아요 사실 이동거리 역시 객체로 만들까 고민했었는데 날카로우십니다 👍
우선 RacingCarName은 명확히 글자 수에 대한 정책이 있어야 하므로 응집도를 높이고 싶어서 클래스로 만들었어요.
numOfMovedForward의 경우 RacingCar가 직접 관리하는게 낫다 싶어서 기본 자료형을 사용해봤습니다 :)
(사실 객체로 분리하는게 관리 측면에서 더 좋은 것 같긴합니다)
| public List<RacingCar> getCars() { | ||
| return List.copyOf(this.cars); | ||
| } |
There was a problem hiding this comment.
List<RacingCar> 를 복사해서 넘기는 이유가 따로 있는건가요? 복사하지 않고 해당 리스트를 넘기게 되면 받은 쪽에서 조작이 가능할 수도 있어서 그걸 방지하려고 하려는 목적인건지, 아니면 다른 목적이 있는건지 궁금합니다!
There was a problem hiding this comment.
해당 리스트를 넘기게 되면 받은 쪽에서 조작이 가능할 수도 있어서 그걸 방지하려고 하려는 목적인건지
맞습니다! 객체 주솟값을 그대로 얻게 되면 그 객체에게 메시지를 보내서 상태가 변경될 수도 있어서 근본적으로 차단하려면 복사해서 넘기는 것이 최선이라고 생각해요!
| public class SystemRandomSingleDigitPicker implements RandomSingleDigitPicker { | ||
|
|
||
| @Override | ||
| public int pick() { | ||
| return Randoms.pickNumberInRange(0, 9); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
RandomSingleDigitPicker 를 인터페이스로 만든 이유가 궁금해요..!
SystemRandomSingleDigitPicker 외에 '다른 방법으로도 랜덤한 숫자를 뽑을 수 있어야 한다.' 는 요구사항이 있을 수도 있어서 그런걸까요?
There was a problem hiding this comment.
우선 RacingCar 객체 입장에서, [0, 9]에서 랜덤으로 추출한 값이 4 이상일 때 전진한다를 테스트로 검증하기 위해서 이렇게 한 것이 커요. (이 부분 관해서는 랜덤값을 테스트 하는 방법에 대해 찾아보시면 좋습니다!)
두 번째는 실수 방지의 목적이 있어요. 만약에 int로 파라미터로 넘겨주는 경우 범위를 넘기는 숫자를 넘겨줄 수 있는 실수할 가능성이 생기니까요. 실수를 못하게 미연에 방지하는 것이 좋은 코드라고 생각해서 요새는 최대한 이렇게 코딩하려고 하고 있어요 :)
| private static String formatSingleCarStatus(final RacingCar car) { | ||
| return new StringBuilder() | ||
| .append(car.fetchName()) | ||
| .append(BLANK) | ||
| .append(SEMI_COLON) | ||
| .append(BLANK) | ||
| .append(String.join("", Collections.nCopies(car.getNumOfMovedForward(), HYPHEN))) | ||
| .toString(); | ||
| } | ||
|
|
||
| public static String formatWinnerCars(final WinnerCars cars) { | ||
| return new StringBuilder() | ||
| .append("최종 우승자") | ||
| .append(BLANK) | ||
| .append(SEMI_COLON) | ||
| .append(BLANK) | ||
| .append(cars.getCars().stream() | ||
| .map(RacingCar::fetchName) | ||
| .collect(Collectors.joining(", "))) | ||
| .toString(); | ||
| } |
There was a problem hiding this comment.
상수를 사용해서 문자열을 관리하는 이유가 있을까요? 개인적으로는 출력하려는 문자열이 한눈에 보이지 않아서 가독성 측면에서는 살짝 떨어지는 것 같은데, 다른 이점이 있어서 사용을 한건지 의견을 들어보고 싶어요!
There was a problem hiding this comment.
맞습니다. 확실히 가독성 측면에서는 손해를 조금 보고 있어요. 그럼에도 상수화 한 것은 실수하기 쉬운 부분이라서 이렇게 만든게 커요. 공백을 넣어야 하는데 실제로는 빈 문자("")를 넣고 있다는 등의 실수를 자주하다 보니 실수 방지 목적으로 부분적으로 상수화를 해봤습니다 :)
리뷰어님께
리뷰 포인트
궁금한 점