[RIPL] 문재원 자동차 경주 미션 제출합니다.#202
Conversation
|
PR을 올린 뒤 확인해 보니, 로컬 Git의 작성자 이메일 설정이 되어 있지 않아 커밋 작성자 정보가 제 GitHub 프로필과 연결되지 않은 상태로 올라갔습니다. |
mvg01
left a comment
mvg01
left a comment
There was a problem hiding this comment.
안녕하세요 재원님~! 반갑습니다.
이번 리뷰에선 재원님이 코드를 왜 그렇게 작성했는지, 어떤 의도를 가진 것인지 물어본 부분이 조금 있는데요, 자신의 코드를 설명할 수 있어야 한다고 생각해서입니다.
가장 어려웠던 부분은 각 메서드의 역할을 하나로 제한하고, 들여쓰기 깊이와 메서드 길이 같은 요구 사항을 만족시키는 과정이었습니다.
프로그래밍 요구사항은 잘 지켜주신 것 같습니다. 저도 초반엔 이 프로그래밍 요구사항을 지키는 것이 이해가 잘 가지 않고 어려웠던 것 같습니다. 같은 리뷰어중에 쿠다크루가 관련된 이야기로 테코톡을 진행하셨는데 참고해보셔도 좋을 것 같아요.
참고 링크: https://www.youtube.com/watch?v=-gbqPuHzLj0&t=2s
또한 각 메서드가 한 가지 역할만 하도록 나누려고 하신 시도 좋았습니다. 다만 오히려 과하게 나뉜 부분도 있어서 코멘트 남겨뒀어요. 나눌지 말지 애매할 땐 "혹시 표준 라이브러리에 이미 있는 기능을 직접 구현하고 있는 건 아닌지"도 같이 확인해보시면 좋을 것 같습니다! (이렇게 해결할 수 있는 부분도 몇 군데 보이기에..)
하지만 라운드마다 실행 결과를 출력해야 해서, 실제 애플리케이션에서는 moveCars(int count)를 사용하지 않고 Application에서 반복문으로 moveCars()를 호출한 뒤 출력하고 있습니다. moveCars(int count)는 현재 테스트에서 여러 번 이동시킨 상태를 만들기 위한 용도로만 사용하고 있습니다.
리팩터링 힌트의 Application 예시에서는 racingGame.race()처럼 게임 실행을 한 줄로 표현하고 있어, 현재 Application이 충분히 간결하지 못한 것 같습니다.
예시가 꼭 정답은 아닙니다. 흐름 자체는 controller의 책임이 가까울 것 같네요!
난수에 따라 자동차가 움직이는 기능을 테스트하기 위해, 테스트 코드에서 원하는 값을 전달할 수 있는 메서드를 추가했습니다.
실제 애플리케이션 실행에는 영향을 주지 않지만, 테스트를 위해 생성한 메서드라는 점에서 이러한 방식이 적절한지 궁금합니다.
더 좋은 테스트 가능 구조를 만들기 위해서는 다른 방식으로 구현하는 것이 좋을지 조언 부탁드립니다.
이 부분은 리뷰로 남겨드렸는데요, Car 밖으로 분리하는 방향으로 가보면서 다시 고민해보셔도 될 것 같아요~!
부족하지만 잘 부탁드립니다~
| } | ||
|
|
||
| void moveCar(int number) { | ||
| if (number >= 4) { |
| void move() { | ||
| moveCar(createRandomNumber()); | ||
| } | ||
|
|
||
| void moveCar(int number) { |
There was a problem hiding this comment.
move와 moveCar의 접근제어자가 없는데 의도하신건가요?
- RacingGame 클래스에서도 접근제어자가 없는 메서드가 있습니다.
There was a problem hiding this comment.
각 메서드들이 같은 패키지 내에서만 사용되고 있기에 지금과 같이 두었습니다. Application에 쓰인다면 public으로 설정해야하겠지만, 각 위 메서드들은 Car에서 RacingGame으로 연결되거나 RacingGame에서 Car로 연결되므로 같은 패키지내에서 사용되므로 지금과 같이 하였습니다.
| private void validateNameLength(String carName) { | ||
| if (carName.length() > MAX_NAME_LENGTH) { | ||
| throw new IllegalArgumentException("자동차 이름은 5자 이하여야 합니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
이름 입력에 공백이나 ,, 로 입력하면 어떻게 되는지 확인해보시면 좋을 것 같아요.
| private static class FixedNumberCar extends Car { // 테스트를 위해 예측 가능한 자동차의 움직임을 구현 | ||
| private final int number; | ||
|
|
||
| FixedNumberCar(String name, int number) { | ||
| super(name); | ||
| this.number = number; | ||
| } | ||
|
|
||
| @Override | ||
| void move() { | ||
| moveCar(number); | ||
| } | ||
| } |
There was a problem hiding this comment.
테스트에서 사용하는 fake 객체로 보이는데, 사용하신 의도를 설명해주세요!
|
|
||
| public class Car { | ||
| private static final int MAX_NAME_LENGTH = 5; | ||
| private static final Random RANDOM = new Random(); |
There was a problem hiding this comment.
만약 랜덤에 대한 규칙이 변경되었을 때, Car 클래스를 수정해야할 필요가 있을까요? 책임이 섞여있다고 느끼시는게 이 부분일 수 있다고 생각되네요. Car가 숫자를 어떻게 뽑는지 알아야할지 고민해보신 후 수정해보면 좋을 것 같아요. 그러면 PR 본문에 적어주신 내용을 조금 해소하실 수 있을 것 같아요.
| public static int tryCount() { | ||
| System.out.println("시도할 회수는 몇회인가요?"); |
|
|
||
| public static List<Car> readCars() { | ||
| System.out.println("경주할 자동차 이름을 입력하세요(이름은 쉼표(,)를 기준으로 구분)."); | ||
| String[] carNames = SCANNER.nextLine().split(","); |
| List<Car> cars = InputView.readCars(); | ||
| int tryCount = InputView.tryCount(); | ||
| RacingGame racingGame = new RacingGame(cars); |
There was a problem hiding this comment.
main도 cars 객체를 가지고있고, racingGame이 cars 객체를 가지고있는 이유가 있나요?
| private static String createWinnerNames(List<Car> winners) { | ||
| StringBuilder winnerNames = new StringBuilder(); | ||
| for (Car winner : winners) { | ||
| appendWinnerName(winnerNames, winner); | ||
| } | ||
| return winnerNames.toString(); | ||
| } | ||
|
|
||
| private static void appendWinnerName(StringBuilder winnerNames, Car winner) { | ||
| if (winnerNames.length() > 0) { | ||
| winnerNames.append(", "); | ||
| } | ||
| winnerNames.append(winner.getName()); | ||
| } |
There was a problem hiding this comment.
createdWinnerNames와 appendWinnerNames을 분리하신 이유가 있을까요? 저는 이 부분이 과한 분리라고 생각이 들긴 합니다. 다시한번 메서드명과 의미를 확인해보시면 좋을 것 같아요~
| void moveCars(int count) { | ||
| for (int i = 0; i < count; i++) { | ||
| moveCars(); | ||
| } | ||
| } |
2 participants
안녕하세요 동건님!
이번 초록 스터디에 참여하게 된 문재원입니다.
자바를 학습한 지 약 1년 정도 되었고, 테스트 코드는 강의를 보고 따라 작성해 본 경험이 대부분이라 아직 어려운 부분이 많습니다.
이번 미션에서는 메서드가 매우 간결한 역할만 가지도록 분리하고, 들여쓰기 깊이 등 요구 사항을 지키며 작성해 보았습니다. 이전에 경험하지 못한 규칙 안에서 코드를 작성하다 보니 어려움을 겪었습니다.
체크 리스트
./gradlew test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?베이스 코드 선택 체크
이번 미션에서 어려웠던 부분
가장 어려웠던 부분은 각 메서드의 역할을 하나로 제한하고, 들여쓰기 깊이와 메서드 길이 같은 요구 사항을 만족시키는 과정이었습니다.
기존에는 하나의 메서드 안에서 여러 역할을 처리하는 경우가 많았는데, 이번에는 메서드를 분리하면서 어떤 기준으로 책임을 나눠야 하는지 많이 고민했습니다.
또한 난수를 생성하는 로직을 테스트할 때, 실행에서는 무작위 값이 필요하지만 테스트에서는 결과를 예측할 수 있어야 한다는 점이 어려웠습니다.
리뷰에서 중점적으로 봐주셨으면 하는 부분
1. 테스트를 위한 메서드 구성
난수에 따라 자동차가 움직이는 기능을 테스트하기 위해, 테스트 코드에서 원하는 값을 전달할 수 있는 메서드를 추가했습니다.
실제 애플리케이션 실행에는 영향을 주지 않지만, 테스트를 위해 생성한 메서드라는 점에서 이러한 방식이 적절한지 궁금합니다.
더 좋은 테스트 가능 구조를 만들기 위해서는 다른 방식으로 구현하는 것이 좋을지 조언 부탁드립니다.
2. 메서드 분리 기준
각 메서드가 하나의 역할만 수행하도록 분리하려고 했습니다.
다만 메서드를 어디까지 분리하는 것이 적절한지, 현재 분리가 과하거나 부족한 부분은 없는지 리뷰해주시면 감사하겠습니다.
3. moveCars(int count)와 Application의 책임
현재 RacingGame에는 한 번 자동차를 움직이는 moveCars()와 여러 번 움직이는 moveCars(int count)가 있습니다.
하지만 라운드마다 실행 결과를 출력해야 해서, 실제 애플리케이션에서는 moveCars(int count)를 사용하지 않고 Application에서 반복문으로 moveCars()를 호출한 뒤 출력하고 있습니다. moveCars(int count)는 현재 테스트에서 여러 번 이동시킨 상태를 만들기 위한 용도로만 사용하고 있습니다.
리팩터링 힌트의 Application 예시에서는 racingGame.race()처럼 게임 실행을 한 줄로 표현하고 있어, 현재 Application이 충분히 간결하지 못한 것 같습니다.
이 부분을 해결하지 못하여 라운드별 출력 요구 사항을 유지하면서도 Application을 더 간결하게 만들기 위해 어떤 방식으로 책임을 분리하는 것이 좋을지 조언 부탁드립니다.
앞으로 잘 부탁드리겠습니다!!