[엘리] 자동차 경주 미션 제출합니다. #106
Conversation
경우 해당 숫자 반환 테스트
| } | ||
|
|
||
| public int createRandomValue() { | ||
| return (int) (Math.random() * RANDOM_VALUE_LIMIT); |
There was a problem hiding this comment.
랜덤은 테스트할 수 없는 영역으로 분리가 되고 있습니다. 이런 테스트할 수 없는 영역에서 Cars 가 테스트될 수 있도록, 분리해보시는 것은 어떨까요?
There was a problem hiding this comment.
NumberGenerator 인터페이스를 만들고 그걸 상속받는 RandomNumber와 FixedNumber를 만들었습니다. 그리고 FixedNumber는 테스트코드에서만 사용되게 변경했습니다!
| public static void selectWinners(List<Car> cars) { | ||
| Car maxDistanceCar = cars.stream() | ||
| .max(Car::compareTo) | ||
| .get(); |
There was a problem hiding this comment.
get 은 nullable 상태입니다. Stream 을 사용한다면 null 이 나올 경우를 항상 체크를 해주어 명확한 코드를 작성하는 것을 권장합니다. Optaionl 에서 get 보다는 항상 orElse~ 로 값을 반환받는 것이 안전합니다.
There was a problem hiding this comment.
아 미처 생각지 못했던 부분이네요, 감사합니다!
그런데 max()를 한 결과가 null이 나올 수 있는 경우가 있는지 궁금합니다. 자동차가 0개일 경우, 저 로직이 시작되기 전 예외처리를 하고, 자동차가 1개 이상일 경우에는 무조건 max값이 있기 때문입니다! 이런 경우에도 orElse로 null 체크를 해주는 것이 바람직한지 궁금합니다! :)
| Cars cars = new Cars(names); | ||
| Car firstCar = cars.getCars().get(0); | ||
| Car secondCar = cars.getCars().get(1); | ||
| Car thirdCar = cars.getCars().get(2); |
There was a problem hiding this comment.
테스트가 상호 독립적으로 검증될 수 있도록, 테스트의 조건은 각 테스트 안에 존재하는 것을 권장합니다.
| public class Application { | ||
| public static void main(String[] args) { | ||
| Game game = new Game(); | ||
| game.run(); |
There was a problem hiding this comment.
로직이 있는 함수 run()을 Game 클래스에 두고, 이를 main에서 호출하는 방식과
로직 자체를 main 함수에서 구현하는 방식을 두고 고민이 많았습니다. 개인적으로는 main 함수에서 로직을 숨기고 싶어서 이렇게 구현했는데 혹시 킹뽀대님은 어떻게 생각하시는지 궁금합니다!
| } | ||
| Winners.selectWinners(cars.getCars()); | ||
| OutputView.printWinnerResult(); | ||
| } |
There was a problem hiding this comment.
개인적으로 run()안에 두 로직(게임 준비, 게임 진행) 이 있다고 판단하여 8-11번 라인을 prepareGame()이라는 함수로 묶고, 13-19번 라인을 playGame()이라는 함수로 묶고 싶었습니다. 하지만 그럴 경우 trial과 cars라는 인스턴스 변수를 생성해야 하는 문제가 발생했습니다. 인스턴스 변수 생성을 최소화하라는 피드백이 있어서 우선은 run()함수 안에 전체 로직을 다 구현했는데요, 이렇게 하면 로직이 복잡해졌을 때 run()함수가 너무 길어질 것 같아 걱정됩니다. 혹시 현장에서는 어떻게 구현하시는지 궁금합니다!
There was a problem hiding this comment.
이 부분은 Trade Off 입니다. 극도의 가독을 가져가면서 인스턴스를 생성하는 비용을 지불할 것인지, 현 수준의 가독을 가져가면서 인스턴스 생성 비용을 절약할 것인지. 이것은 그 상황과 경우에 따라 많이 다를 듯 합니다 :)
인스턴스 생성 비용은 요즘 시대에는 거의 신경쓰지 않아도 됩니다. 성능이 많이 좋아졌거든요. 그렇지만 우리에게는 최적화해야할 의무 또한 있습니다. 저 역시 인스턴스 생성에 관대한 편이지만, 위와 같은 코드에서는 사용하지 않고 효율을 취하고 싶네요 :)
모든 것을 극악의 가독으로 만들 순 없습니다. 현 교육에서 제시하는 컨벤션 정도가 그 선이 되면 좋을 것 같습니다 :) 하지만 때로는 극악까지 가독을 끌어올려보는 것 또한 권장합니다. 메서드와 객체를 분리하여 가독을 올렸을 때, 변경에 대한 유연한 변화가 문제가 될 수 있으며, 잘못된 설계 기반에서 극악의 분리를 하였을 때 일부를 변경하지 못하고 모든 것을 변경해야 하는 시행착오 또한 겪을 수 있을 것 입니다.
There was a problem hiding this comment.
그렇군요! 이 부분은 경험치를 더 많이 쌓아야 할 것 같아요.
극악의 가독을 가져가는 방식도 많이 연습해보겠습니다!
자세한 코멘트 감사합니다 :D
| } | ||
| Winners.selectWinners(cars.getCars()); | ||
| OutputView.printWinnerResult(); | ||
| } |
There was a problem hiding this comment.
이 부분은 Trade Off 입니다. 극도의 가독을 가져가면서 인스턴스를 생성하는 비용을 지불할 것인지, 현 수준의 가독을 가져가면서 인스턴스 생성 비용을 절약할 것인지. 이것은 그 상황과 경우에 따라 많이 다를 듯 합니다 :)
인스턴스 생성 비용은 요즘 시대에는 거의 신경쓰지 않아도 됩니다. 성능이 많이 좋아졌거든요. 그렇지만 우리에게는 최적화해야할 의무 또한 있습니다. 저 역시 인스턴스 생성에 관대한 편이지만, 위와 같은 코드에서는 사용하지 않고 효율을 취하고 싶네요 :)
모든 것을 극악의 가독으로 만들 순 없습니다. 현 교육에서 제시하는 컨벤션 정도가 그 선이 되면 좋을 것 같습니다 :) 하지만 때로는 극악까지 가독을 끌어올려보는 것 또한 권장합니다. 메서드와 객체를 분리하여 가독을 올렸을 때, 변경에 대한 유연한 변화가 문제가 될 수 있으며, 잘못된 설계 기반에서 극악의 분리를 하였을 때 일부를 변경하지 못하고 모든 것을 변경해야 하는 시행착오 또한 겪을 수 있을 것 입니다.
| public int movePosition(int moveValue) { | ||
| if (moveValue >= 4) { | ||
| public int movePosition(NumberGenerator numberGenerator) { | ||
| if (numberGenerator.isMovable()) { |
There was a problem hiding this comment.
NumberGenerator 가 움직을 결정하도록 변경되었네요!
NumberGenerator 클래스의 정확한 역할과 책임이 무엇인지 궁금합니다.
There was a problem hiding this comment.
기존의 NumberGenerator는 랜덤한 or 고정 숫자를 생성해서 움직임을 결정하는 역할을 담당했는데, 이를 제거하고 테스트를 위한 movePosition()을
public int movePosition(int fixedNumber) {
return position++;
}
오버로딩하도록 수정했습니다. 지금 생각해보니 저 역할을 모두 포함하도록 NumberGenerator의 네이밍을 수정하면 어땠을까 싶은데요,, 혹시 의도하신 변경사항이 어떤건지 여쭤보고 싶습니다!
| public boolean isMaxPosition(Car maxPositionCar) { | ||
| return this.position >= maxPositionCar.position; | ||
| public boolean isWinnerWith(Car maxPositionCar) { | ||
| return this.position == maxPositionCar.position; |
There was a problem hiding this comment.
이 메서드는 이 차와 같은 위치에 있어? 라는 행위이지 않을까요?
메서드명이 행위와 일치하고 있나요?
There was a problem hiding this comment.
단순히 우승자인지 판단하는 함수라고 생각해서 네이밍했는데, 확장성을 고려하면 행위와 일치하는 이름을 짓는 것이 더 적절하겠네요! isSamePositionWith() 으로 수정했습니다!
| @@ -11,25 +12,25 @@ public Car(Name name) { | |||
| this.position = 0; | |||
| } | |||
|
|
|||
| public int movePosition(int moveValue) { | |||
| if (moveValue >= 4) { | |||
| public int movePosition(NumberGenerator numberGenerator) { | |||
There was a problem hiding this comment.
NumberGenerator 를 외부에서 매번 주입받을 필요가 있을까요?
NumberGenerator는 Car 를 움직임을 결정하는 Car 의 요소가 아닐까 생각해봅니다 :)
There was a problem hiding this comment.
이 부분을 곰곰히 생각해보았는데 움직임을 결정하는 일은 Car가 하는 것이 맞다고 판단했습니다. 그렇게 되면 RandomNumber의 역할이 말 그대로 랜덤숫자를 생성하는 것밖에 없어 불필요한 클래스 생성이라고 보아 Car 클래스 안에
private boolean isMovable() {
int randomNumber = (int) (Math.random() * RANDOM_VALUE_LIMIT);
return randomNumber >= FORWARD_NUMBER;
}
이라는 함수를 구현했습니다. 이 경우에 랜덤 숫자를 생성하는 클래스를 그대로 유지했어야 하는지 리뷰어님의 의견이 궁금합니다!
| @@ -0,0 +1,56 @@ | |||
| package racingcar.domain; | |||
|
|
|||
| public class Car implements Comparable<Car> { | |||
There was a problem hiding this comment.
아이고!
51페이지부터가 도움이 될 것 같습니다 :) (전체화면 해야지만 내용이 나오네요.)
테스트할 수 없는 영역을 테스트하기 위해 전략패턴을 사용하여 테스트 가능하도록 만들 수 수 있습니다.
interface NumberGenerator {
int generateNumber()
}
숫자를 생성해주는 추상체를 만듭니다.
Car 는 내부 속성으로 NumberGenerator 를 필드로 갖고,
void play() {
if (numberGenerator.generateNumber() > FORWARD_NUMBER) {
와 같은 코드를 작성할 수 있습니다.
Car 클래스의 설계 원칙에 랜덤이 적용되지 않도록 합니다. Car 클래스는 NumberGenerator 의 구현체에는 관심없이 특정 숫자 이상이면 움직인다는 설계 원칙을 갖는 것 입니다.
이렇게 될 때 Car class 는
NumberGenerator alwaysTenGenerator = () -> 10
new Car(alwaysTenGenerator)
와 같이 NumberGenerator 를 조작하여 의도한데로 동작하는지 테스트할 수 있습니다.
이와 별게로 NumberGenerator 와 같은 의미없고 추상적인 의미보다는, 해당 클래스가 수행하는 역할을 고려해 Car 와 밀접한 의미를 부여해보는 것도 좋은 연습이 될 듯 합니다.
거의 다 온 것 같아 간단한 수도코드와 문서를 첨부합니다 :)
테스트 가능한 코드를 만들어주세요!
There was a problem hiding this comment.
우와 상세한 코멘트 감사합니다!
제가 잘 이해했는지 모르겠네요 ㅠㅠ NumberGenerator의 이름을 MoveController로 바꿨고, 테스트 코드와 프로덕션 코드에 각각 다른 generateMovingNumber()를 구현했습니다. 혹시 추가 피드백 있으시면 언제든지 말씀해주세요!
| private List<Car> cars; | ||
| private MoveController movingNumber = () -> (int) (Math.random() * RANDOM_NUMBER_MAX); |
There was a problem hiding this comment.
Cars는 Car 를 관리하는 일급 컬렉션의 일에 집중하고, Car 스스로가 움직일 수 있게 하면 어떨까요?
현재 Car 클래스는 단독적으로 게임의 룰을 반영할 수 없는 상황이 되었네요 :) Car 의 역할이 Cars 에게 부여된 상황으로 보이네요.
| private List<Car> cars; | ||
| private MoveController movingNumber = () -> (int) (Math.random() * RANDOM_NUMBER_MAX); |
There was a problem hiding this comment.
람다도 좋지만, 현재는 게임의 룰을 명확히 하기 위해 클래스로 존재하는 것은 어떨까요?
여러가지 새로 알게 된 개념들을 적용해봤는데 적재적소에 활용했는지 잘 모르겠습니다.
static 과 final 키워드를 붙일지 말지는 여전히 헷갈리네요.
궁금한 점들은 코멘트 남기겠습니다! 😄