-
Notifications
You must be signed in to change notification settings - Fork 92
[강민혁] 자동차 경주 미션 제출 #119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: kangminhyuk1111
Are you sure you want to change the base?
[강민혁] 자동차 경주 미션 제출 #119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
민혁님 미션 잘 구현해주셨네요👍
랜덤한 값에 영향을 받는 로직을 어떻게 통제하고 테스트할 수 있을까에 대해 깊이 고민해주시고 반영해주신게 매우 인상 깊네요! 멋집니다😎
역할에 따른 클래스를 깔끔히 분리해주셨는데, 아직 명확하지 않은 부분들이 있어보여요.
관련해서 코멘트 남겨두었으니, 이부분 고민해보시고 답변 남겨주시면 좋을 것 같아요!
### 자동차 기본 속성 | ||
- 자동차는 고유한 이름을 가진다. | ||
- 이름을 가지는 Car 객체를 통해 관리 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"자동차는 고유한 이름을 가진다."라는 조건이 있는데요.
현재 어플리케이션에서 자동차 이름의 input으로 "a, a, b, c, d"를 입력한다면 어떤 결과가 나올까요?
위와 같이 이름에 공백이 있는 경우 trim 후 이름으로 사용하는것도 고려해보시면 좋을것같아요. (사용성 측면에서요😁)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
중복과 공백을 고려하여 예외 처리가 필요할 것 같습니다.
### 게임 진행 과정 | ||
- 주어진 횟수 동안 게임이 진행됨 | ||
- 매 라운드마다 모든 자동차는 전진하거나 정지함 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위와 같은 맥락으로 횟수로 0, -1이 들어가면 어떤 결과가 나올까요?
미션 요구사항에 명시된 조건 외에도 제공하는 기능에 대한 예외 케이스들을 추가로 고려해보시면 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
명시된 조건 이외에 것은 일부러 생략을 했었는데요, 신경 써야할 부분인 것 같습니다.
README.md
Outdated
# 3단계 - 게임 실행 | ||
## 기능 요구사항 | ||
- 자동차 이름은 쉼표로 구분하며 이름은 5자 이하만 가능함 | ||
- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요것도 마저 작성해보면 어떨까요? 😎
public static void main(String[] args) { | ||
final List<String> carNames = InputView.inputCarNames(); | ||
final int repetition = InputView.inputRacingCount(); | ||
|
||
RacingController racingController = new RacingController(new DefaultRandomNumberGenerator()); | ||
|
||
racingController.playGame(carNames, repetition); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RacingController 밖에서 InputView를 따로 호출한 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
작성 하다보니 이렇게 된 것 이라, 큰 이유는 없습니다. 생각 해보니 controller라는 객체 내부에서 input output 둘 다 처리하는 것이 올바르다고 생각합니다.
private static final int MOVING_STANDARD = 4; | ||
|
||
private final String name; | ||
private int position; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static final
로 상수를 잘 표현해주시고, 네이밍 컨벤셔도 잘 지켜주셨네요!
final
로 불변인 필드와 그렇지 않은 필드를 명확히 구분해주신 점도 너무 좋습니다👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감사합니다👍
public static final String MOVING_STATUS = "-"; | ||
public static final String DELIMITER = " : "; | ||
public static final String WINNER_JOIN_DELIMITER = ", "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수별 이름을 잘 지어주셨네요👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감사합니다.
@FunctionalInterface | ||
public interface RandomNumberGenerator { | ||
int generate(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
함수형 인터페이스의 장점은 어떤거라고 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일단 지금 사용된 관점에서는 제가 생각했을 때, 함수형 인터페이스로 선언하면 람다식을 통해서 간결한 코드 작성이 가능했습니다. 그리고, 테스트 코드에서 람다식을 통해 return 값을 강제 주입이 가능했습니다.
이러한 이유에서 함수형 인터페이스를 사용했습니다. 일반 인터페이스를 사용했을때는 실제로 구현체를 구현해주어야하는 불편함이 존재했습니다.
RandomNumberGenerator generator = new RandomNumberGenerator() {
@Override
public int generate() {
return 4;
}
};
원래 이런식으로 작성해야 되는 코드라면,
RandomNumberGenerator generator = () -> 4;
이렇게 간단하게 표현 가능한게 장점이고, 이를 통해서 가독성이 높아진다고 생각했습니다.
private final Random random; | ||
|
||
public DefaultRandomNumberGenerator() { | ||
this.random = new Random(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생성자에서 Random 객체를 새로 생성하는 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random 객체를 사용하기 위해 생성자에서 선언해줘야 된다고 생각했는데 혹시 어떻게 생성해야되는 건가요?
@Test | ||
@DisplayName("숫자가 4이상 일시 자동차 전진 테스트") | ||
void carMoveTestIfNumberOverFour() { | ||
RandomNumberGenerator randomNumberGenerator = () -> 4; | ||
final Car car = new Car("car"); | ||
car.move(randomNumberGenerator); | ||
|
||
assertThat(car.getPosition()).isEqualTo(1); | ||
} | ||
|
||
@Test | ||
@DisplayName("숫자가 3이하 일시 자동차 정지 테스트") | ||
void carMoveTestIfNumberUnderThree() { | ||
RandomNumberGenerator randomNumberGenerator = () -> 3; | ||
final Car car = new Car("car"); | ||
car.move(randomNumberGenerator); | ||
|
||
assertThat(car.getPosition()).isEqualTo(0); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
더 다양한 케이스를 테스트해보면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다양한 경계값 테스트 케이스가 추가되면 좋을 것 같습니다.
public class TestRandomNumberGenerator implements RandomNumberGenerator { | ||
|
||
private final int[] values; | ||
private int index; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
values와 index는 어떤걸 의미하는걸까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
레이싱 게임을 진행했을 때, 예상 가능한 값이 떨어지도록 만들었습니다.
예를 들어서, 3대의 차량이 경주한다고 가정했을 때,
0 1 2 0 1 2 0 1 2 가 나오도록 만들었습니다.
코드 리뷰 후 수정 과정
Controller가 가져야 할 책임
Controller는 유저의 입력과 출력을 담당하기 위해 설계한 객체이지만, InputView를 Main메서드에 노출되게 하였고 이는 Controller가 가져야 할 책임을 Main 메서드가 가지게 됬습니다. 이는 최초에 설계한 Controller의 책임에 조금 벗어나는 설계였습니다.
그래서 InputView를 Controller 내부로 넣어주면서 Main 메서드에서는 인스턴스 생성과 실행만 가질 수 있도록 변경했습니다.
값 객체를 적극적으로 사용
다른 분의 pr을 보면서 값 객체를 사용한 부분을 보았습니다. 일단 제가 생각한 값 객체의 장점은 다음과 같습니다.
이번에 값 객체를 사용해보니 확실히 코드 분리의 장점이 명확했던 것 같습니다.
일급 컬렉션 사용
game에서 List를 가져 버리니 너무 game객체가 무거워지는 것 같았습니다. 그래서 일급 컬렉션이 List를 가지게 하였고 Cars라는 일급 컬렉션에서 내부 메서드를 통해 보다 코드가 깔끔해졌고, 추가적으로 책임 분리가 명확히 되는 것 같았습니다.
다양한 테스트 케이스 작성
테스트 케이스를 꼼꼼하게 작성하는 것도 중요하다고 다시 생각이 들었습니다. 가장 중요한 부분은 경계값 테스트 라고 생각이 들었습니다. 경계값만 테스트 해주면 그 안쪽의 값은 문제가 없다고 역추적이 가능하기 때문입니다. 그래서 Integer.MAX_VALUE 같은 값으로 보다 더 정확한 경계값 테스트를 할 수 있도록 작성했습니다.
1단계 미션 - 움직이는 자동차
일단 제가 생각했을 때, 이번 미션에서 가장 신경쓰였던 부분은 "랜덤 값 테스트"에 대한 부분이였습니다.
랜덤 값은 항상 다른 숫자를 줄텐데 테스트 코드가 그러면 부분적으로 실패하고 성공하나? 라는 의문이 들었습니다.
그래서 첫번째 했던 생각은, 랜덤값이 항상 0부터 9사이에 값인지 체크하면 될 것 같다는 생각을 했지만 다음과 같은 문제가 있었습니다.
그래서 고민한 다른 방법은 항상 예측 가능한 랜덤값을 주면 될 것 같다고 생각했습니다.
물론 실제 코드가 돌아가는 환경에서는 예측이 불가능한 random이 들어가는게 맞지만, 테스트는 예측 가능한 범주에서 실행되어야 하기 때문에 "항상 예측가능한 값이 나와야 한다" 라고 생각했고 이 문제를 해결하기 위해 함수형 인터페이스를 활용했습니다.
RandomNumberGenerator 인터페이스를 정의하고, 테스트 코드에서는 람다식을 통해 항상 동일한 값을 반환하도록 구현했습니다.
2단계 미션 - 우승 자동차 구하기
두번째 미션에서 가장 주의깊게 본 부분은 게임이라는 하나의 객체 안에서 어떻게 값을 다뤄야 할지에 대한 고민이였습니다.
저는 게임이라는 객체가 자동차들의 리스트를 가지고 그 리스트들 중에서 가장 포지션이 큰 차량이 우승자라고 생각했습니다.
만약 포지션이 겹치는 자동차들이 있다면 이들을 공동우승자로 만들었습니다.
여기서 오는 문제 중 하나는, 어떻게 하면 다수의 자동차들의 전진을 테스트 코드에서 작성할 지에 대한 부분이 어려웠습니다.
결론은 위에서 작성한 RandomNumberGenerator 인터페이스의 구현체인, 테스트에서만 사용하는 가짜객체(TestRandomNumberGenerator)를 하나 생성하고, 이 객체를 통해 특정 패턴의 랜덤 값을 제공함으로써 자동차들의 이동을 예측 가능하게 만들었습니다.
그리고 게임 종료시 findWinners라는 함수를 통해 우승자들을 return 하도록 구현했습니다.
3단계 미션 - 게임실행
세번째 미션에서는 기존의 메서드를 리팩터링 하는 과정이 있었습니다. 기존에 start라는 메서드의 매개변수로 반복 횟수를 받아서 진행되는 과정이였는데, 이 부분을 ui와 controller를 추가하는 과정이 필요한 세번째 과정에서 문제가 있었습니다.
매 라운드 마다 자동차의 진행 상태를 출력해야하는 요구사항이 있었고 반복횟수를 controller에서 받아서 게임의 moveCar메서드(라운드 마다 차량을 이동하는 단건 메서드)를 반복과정 자체를 컨트롤러에 위임하니 매 라운드 마다 차량을 출력하는 것이 가능했습니다.
그렇다고 해서 기존에 있던 start 메서드를 없앨 필요는 없었습니다. 그래서 테스트 코드에서는 반복되는 코드를 줄이기 위해 start를 사용하고 실제 유저와 상호작용이 필요한 부분은 moveCar메서드로 구현하였습니다.
4단계 - 리팩터링
기존에 테스트 코드를 작성하며 구현해서 리팩터링이 필요하지 않다고 생각했습니다.