Conversation
사용자에게서 입력받는 자동차의 이름과 이동의 횟수 부여 기능을 구현할 예정임을 작성했다
[Feature] 사용자에게서 입력을 받는다
…가 IllegalArgumentException을 발생시키는지 확인하는 코드 작성
[Feature] 차 이름이 조건에 맞지 않을 경우 예외 발생
…dation [Feature] 입력받은 움직인 횟수를 확인하는 로직 작성
[Feature] 주어진 이름들을 `,`로 분리한다
[Feature] 랜덤 숫자 생성
…mberGenerator의 규칙을 지키도록 수정
[Test] 난수 결과 테스트
[Feature] 우승자 선별 구현
- 자동차의 이름이 빈값이 아니라면 IllegalArgumentException 발생하는 코드 고침 - 자동차의 이름이 5자 이하로 주어지면 IllegalArgumentException 발생 코드 고침 - 이동횟수가 빈칸으로 주어지지 않으면 IllegalArgumentException 발생 코드 고침
본래 RandomNumberGenerator이라는 구체 클래스에 의존했으나 NumberGenerator이라는 인터페이스에 의존하도록 변경
racing 기능 구현
| try { | ||
| totalMovement.toInt() | ||
| } catch (e: NumberFormatException) { | ||
| throw IllegalArgumentException("정수를 입력하셔야 합니다") | ||
| } |
There was a problem hiding this comment.
toInt() 대신 toIntOrNull()을 사용하면 require로 통일할 수 있을 것 같아요!
| try { | |
| totalMovement.toInt() | |
| } catch (e: NumberFormatException) { | |
| throw IllegalArgumentException("정수를 입력하셔야 합니다") | |
| } | |
| require(totalMovement.toIntOrNull() != null) { "정수를 입력하셔야 합니다" } |
|
|
||
| import camp.nextstep.edu.missionutils.Console | ||
|
|
||
| class InputView { |
There was a problem hiding this comment.
object 키워드는 싱글톤 객체를 생성하는 것으로 알고 있습니다! 물론 object가 생성까지 같이해서 좋지만, class로 선언하는 게 확장성적인 측면에서 좀 더 좋다고 생각했거든요..ㅎㅎ 나중에 인터페이스를 사용하게 되거나 사용자에게서 콘솔로 받는 것이 아닌 등등의 상황을 생각했었습니다.
물론 지금 같은 상황에선 싱글톤 객체로 생성하는 것도 좋은 방법인 것 같습니다! 사용자에게서 입력을 한 곳에서 받아오니 싱글톤디 좋은 것 같기도 해요. 사실 구현할 땐 생각하지도 못했는데 허를 찔린 기분이네요,,ㅎㅎ
lepitaaar
left a comment
lepitaaar
left a comment
There was a problem hiding this comment.
코틀린코드 정말 여러번봐도 재밌네요 ㅋㅋㅋ
컨트롤러에 꽤많은 역할이 부여된거같은데 분리하지않은 이유도있는지 궁금하네요 잘봤습니다!
|
|
||
| import camp.nextstep.edu.missionutils.Console | ||
|
|
||
| class InputView { |
| fun printCurrentLocation(name: String, distance: Int) { | ||
| print("$name : ") | ||
| repeat(distance) { | ||
| print("-") | ||
| } | ||
| println() | ||
| } |
There was a problem hiding this comment.
코틀린이면 한스트링에 변수를 삽입 가능했을텐데 나누신 이유가 뭘까요?? 가독성 부분에서 나누신걸까요?
| val fakeInput = "sam, ,toby" | ||
| val inputStream = ByteArrayInputStream(fakeInput.toByteArray()) | ||
|
|
||
| val inputView = InputView() | ||
| val outputView = OutputView() | ||
| val judge = Judge() | ||
|
|
||
| System.setIn(inputStream) | ||
|
|
||
| // when & then | ||
| assertThrows<IllegalArgumentException> { RacingController(inputView, outputView, judge).run() } |
There was a problem hiding this comment.
우테코에서 제공하는 assertSimpleTest 사용하지않고 이렇게 스트림 으로 넣어준 이유가있을까요?
| package racingcar.domain | ||
|
|
||
| class Judge { | ||
| fun judgeWinner(gameResult: List<Car>): List<String> { |
| fun moveForward() { | ||
| val randomNumber = numberGenerator.generateNumber() | ||
| if (randomNumber >= MOVE_FORWARD_CONDITION_NUMBER) { | ||
| distance++ | ||
| } | ||
| } |
angryPodo
left a comment
angryPodo
left a comment
There was a problem hiding this comment.
성우님, 2주차 미션 정말 고생 많으셨어요!
README에 구현 기능을 정리하고 MVC 패턴을 적용하려 노력하신 모습이 인상적이었어요. 특히 NumberGenerator 인터페이스로 추상화하신 시도와, ParameterizedTest까지 활용해 테스트를 작성하신 점이 정말 좋았습니다! 😊
제가 드린 리뷰 포인트들이 성우님의 다음 미션에 조금이나마 도움이 되었으면 좋겠어요. 특히 "함수가 한 가지 일만 하도록"과 "테스트에서 인터페이스 활용하기"는 다음 주차에 꼭 적용해보시면 큰 성장이 있을 거예요! 저도 성우님의 코드를 통해 검증 로직의 위치와 책임 분리에 대해 다시 생각해볼 수 있었습니다. 👍
3주차 미션도 같이 힘내봐요! 화이팅입니다! 💪
| require(!name.contains(" ")) { "자동차 이름엔 공백이 포함되면 안됩니다" } | ||
| } | ||
|
|
||
| fun moveForward() { |
There was a problem hiding this comment.
Car에 대한 테스트 코드가 없는 것 같아요!
"JUnit 5와 AssertJ를 이용하여 정리한 기능 목록이 정상적으로 작동하는지 테스트 코드로 확인한다"
moveForward()같은 핵심 로직은 테스트를 해야 한다고 생각합니다. 혹시 제가 놓친거라면 죄송해요🥲
| var distance: Int = INITIAL_CAR_DISTANCE | ||
|
|
||
| init { | ||
| require(name.length < MAX_NAME_LENGTH) { "자동차 이름의 길이는 5자 이하여야 합니다" } |
There was a problem hiding this comment.
요구사항은 "이름은 5자 이하만 가능"으로 되어있어요.
<연산자로 인해서 4자까지만 허용되는것 같아요😊
| require(carName.isNotBlank()) { "자동차의 이름은 빈칸일 수 없습니다" } | ||
| require(carName.length <= MAX_CAR_NAME_LENGTH) { "자동차의 이름은 5글자 이하여야 합니다" } |
There was a problem hiding this comment.
위의 부분은 Car.kt와 중복되는 검증 로직 같아요.
// Car.kt - init
require(name.length < MAX_NAME_LENGTH) { "자동차 이름의 길이는 5자 이하여야 합니다" }
require(!name.contains(" ")) { "자동차 이름엔 공백이 포함되면 안됩니다" }그리고 조건 또한 <와 <=로 차이가 존재해요 🤔
한곳에서만 검증을 책임지는게 명확한 책임분리라고 생각이 드는데 성우님은 어떻게 생각하시나요?
| interface NumberGenerator { | ||
| fun generateNumber(): Int | ||
| } No newline at end of file |
There was a problem hiding this comment.
이 인터페이스를 만들어 주시고 테스트에서는 활용해주시지 않은 것 같아요.
저라면 아래처럼 테스트용 구현체를 만들어서 테스트를 구현 하는편인데요!
class FixedNumberGenerator(private val value: Int) : NumberGenerator {
override fun generateNumber(): Int = value
}성우님은 val car = Car("test", RandomNumberGenerator()) 처럼 테스트를 실제 랜덤을 사용하신 이유가 궁금해요 😊
| fun race(carNames: List<String>, repeatTime: Int): List<Car> { | ||
| val cars: List<Car> = carNames.map { carName -> | ||
| Car( | ||
| name = carName, | ||
| RandomNumberGenerator() | ||
| ) | ||
| } | ||
|
|
||
| repeat(repeatTime) { | ||
| moveCar(cars) | ||
| printCurrentCarState(cars) | ||
| } | ||
| return cars | ||
| } |
There was a problem hiding this comment.
"함수(또는 메서드)가 한 가지 일만 하도록 최대한 작게 만들어라"
라는 요구사항이 있었는데요! 제가 생각했을때는 지금 race() 함수가 여러가지 일을 하고 있어요.
자동차 생성과 게임을 진행,출력,반환 을 담당하고 있는데, 최소한 자동차 생성과 게임 진행은 분리되어야 한다고 생각이 들어요! 어떻게 생각하시나요?🤔
There was a problem hiding this comment.
코드 패키징을 분리하신 걸 보니, InputValidator와 Separator가 루트 패키지에 있네요!
두 코드는 입력 처리 관련이니 view패키지에 있는 게 자연스럽다고 생각해요! 성우님이 이렇게 배치하신 이유가 궁금해요😊
| fun handleCarNames(): List<String> { | ||
| outputView.printCarNameInputGuide() | ||
| val inputFromUser = inputView.getCarNameFromUser() | ||
| val carNames = Separator().separateName(inputFromUser) | ||
| InputValidator().validateCarName(carNames) | ||
| return carNames | ||
| } | ||
|
|
||
| fun handleRepeatTime(): String { | ||
| outputView.printMovementTimeInputGuide() | ||
| val repeatTime = inputView.getMovementTimeFromUser() | ||
| InputValidator().validateTotalMovement(repeatTime) | ||
| return repeatTime | ||
| } |
There was a problem hiding this comment.
// fun handleCarNames
val carNames = Separator().separateName(inputFromUser)
InputValidator().validateCarName(carNames)
// fun handleRepeatTime
InputValidator().validateTotalMovement(repeatTime)위의 함수에서 해당하는 부분을 보면 매번 함수 호출시마다 새로운 객체를 생성하고 있어요. 호출 횟수만큼 힙 메모리에 새 객체가 생성되면서 프로그램 실행 중 최소 2번 객체가 생성되는것 같아요.
상태가 있다면 유의미하지만 상태가 없는 클래스인데도 매선 새 객체를 만드는게 타당한가?라는 의문이 들어요🤔
또한 다른 개발자가 해석하면서 새 인스턴스를 매번 만드는 코드를 보면
"혹시 상태를 가지나?"
라는 의문을 줄 수 있다고 보이는데요😊 싱글톤이나 companion object를 활용하는 방법은 고려 해보셨나요? 저는 싱글톤으로 같은 인스턴스를 사용하는게 좋다고 생각해요!
| import racingcar.view.InputView | ||
| import racingcar.view.OutputView | ||
|
|
||
| class RacingController(val inputView: InputView, val outputView: OutputView, val judge: Judge) { |
There was a problem hiding this comment.
추가로 RacingController의 생성자 파라미터가 public 프로퍼티로 노출되고 있어요. 혹시 의도하신 걸까요? 🧐
제가 생각하기에는 외부에서 접근하지 않아도 되는 파라미터라고 생각해요. 때문에 private val을 사용하는게 좋다고 생각이 들어요 😊
| val outputView = OutputView() | ||
| val judge = Judge() | ||
|
|
||
| System.setIn(inputStream) |
There was a problem hiding this comment.
System.setIn을 사용해서 테스트에서 전역으로 생태를 변경하는 것은 좋지 않은 테스트 방식이라고 저는 생각해요.
테스트간 독립성이 보장되지 않아서 '의도한' 테스트를 검증하지 못하게 된다고 봐요!
InputView를 Mock으로 만들어서 주입하는 방법은 어떠신가요?
| @Test | ||
| fun `자동차의 이름이 없다면 IllegalArgumentException을 발생시킨다`() { | ||
| // given | ||
| val carName = mutableListOf<String>() | ||
|
|
||
| // when & then | ||
| assertThrows<IllegalArgumentException> { InputValidator().validateCarName(carName) } | ||
| } |
There was a problem hiding this comment.
이 테스트에서 변경하지 않는 리스트를 mutableListOf로 만들 필요는 없다고 생각해요! emptyList() 혹은 listOf()를 사용하는 게 의도를 더 명확히 전달 할 것 같아요😁
4 participants
No description provided.