Conversation
joon0447
left a comment
There was a problem hiding this comment.
안녕하세요! 2주차 과제 고생 많으셨습니다~
코드 잘 보았습니다!!!
3주차도 화이팅입니다 !
| fun readInput(): Pair<String, String> { | ||
| println("경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)") | ||
|
|
||
| val names = readLine() | ||
|
|
||
| println("시도할 횟수는 몇 회인가요?") | ||
|
|
||
| val tryCount = readLine() | ||
|
|
||
| return (names to tryCount) | ||
| } |
There was a problem hiding this comment.
하나의 함수에서 이름이랑 시도 횟수를 다 입력받고 있군요!
좋은 것 같습니다.
| fun parseTryCount(inputTryCount: String): Int { | ||
| val tryCount = inputTryCount.toIntOrNull() ?: throw IllegalArgumentException("시도 횟수는 숫자여야 합니다.") | ||
|
|
||
| return tryCount | ||
| } |
There was a problem hiding this comment.
검증 단계를 InputValidator 클래스로 빼는 게 더 좋지 않을까요??
There was a problem hiding this comment.
string -> int로 바꾸는게 파서의 역할이고 그 과정에서 오류가 있다면 Exception을 했던건데 이렇게 봐도 validator로 넘기는게 좋을까요??
| private const val MAX_NAME_LENGTH = 5 | ||
| private const val MIN_TRY_COUNT = 1 |
| name.isEmpty() -> throw IllegalArgumentException("참가자 이름은 비어있을 수 없습니다.") | ||
| name.length > MAX_NAME_LENGTH -> throw IllegalArgumentException("참가자 이름은 ${MAX_NAME_LENGTH}자 이하여야 합니다.") |
There was a problem hiding this comment.
throw 대신 require를 쓰면 어떨까요??
| name.isEmpty() -> throw IllegalArgumentException("참가자 이름은 비어있을 수 없습니다.") | |
| name.length > MAX_NAME_LENGTH -> throw IllegalArgumentException("참가자 이름은 ${MAX_NAME_LENGTH}자 이하여야 합니다.") | |
| require(name.isNotEmpty()) { "참가자 이름은 비어있을 수 없습니다." } | |
| require(name.length <= MAX_NAME_LENGTH) { "참가자 이름은 ${MAX_NAME_LENGTH}자 이하여야 합니다." }${MAX_NAME_LENGTH}자 이하여야 합니다.") |
There was a problem hiding this comment.
조언 감사합니다! 더 코틀린스럽고 좋은 것 같습니다!
| names.forEach() { name -> | ||
| when { | ||
| name.isEmpty() -> throw IllegalArgumentException("참가자 이름은 비어있을 수 없습니다.") | ||
| name.length > MAX_NAME_LENGTH -> throw IllegalArgumentException("참가자 이름은 ${MAX_NAME_LENGTH}자 이하여야 합니다.") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
반복문을 돌리지 않고 아래처럼 any를 활용해 더 코틀린스럽게 표현할 수도 있을 것 같아요!
| names.forEach() { name -> | |
| when { | |
| name.isEmpty() -> throw IllegalArgumentException("참가자 이름은 비어있을 수 없습니다.") | |
| name.length > MAX_NAME_LENGTH -> throw IllegalArgumentException("참가자 이름은 ${MAX_NAME_LENGTH}자 이하여야 합니다.") | |
| } | |
| } | |
| } | |
| if (names.any { it.isEmpty() }) { | |
| throw IllegalArgumentException("참가자 이름은 비어있을 수 없습니다.") | |
| } | |
| if (names.any { it.length > MAX_NAME_LENGTH }) { | |
| throw IllegalArgumentException("참가자 이름은 ${MAX_NAME_LENGTH}자 이하여야 합니다.") | |
| } |
There was a problem hiding this comment.
오 이 방법도 깔끔하고 좋은 것 같네요!! 조언 감사합니다!!
| @@ -0,0 +1,16 @@ | |||
| package racingcar | |||
|
|
|||
| class Car( | |||
There was a problem hiding this comment.
Car를 제외한 나머지 객체들을 모두 object로 정의하신 이유가 궁금합니다!
There was a problem hiding this comment.
Car는 인스턴스를 여러개 생성하지만 나머지는 1개만 생성하고 사용하기 때문에 object로 선엉했습니다!
angryPodo
left a comment
There was a problem hiding this comment.
정찬님, 2주차 미션 정말 고생 많으셨습니다! 전체적으로 꼼꼼하게 구현하신 코드를 보면서 저도 많이 배울 수 있었어요. 😊
특히 객체들의 책임을 어떻게 분리할지, 또 테스트 가능한 코드는 어떻게 만들어야 할지에 대해 함께 고민해볼 수 있어서 정말 의미 있는 시간이었어요. 제가 드린 의견들이 정찬님의 다음 미션에 조금이나마 도움이 되었으면 하는 바람입니다 🙇🏻♂️
저 역시 정찬님 코드를 보며 제가 미처 생각지 못했던 부분을 발견하기도 하고, 당연하게 생각했던 설계에 대해 '왜?'라는 질문을 다시 던져보게 되었어요. 함께 성장하는 즐거운 경험이었어요. 👍
다가오는 3주차 미션도 응원하겠습니다. 화이팅입니다! 💪🏻
| @@ -1 +1,16 @@ | |||
| # kotlin-racingcar-precourse | |||
|
|
|||
There was a problem hiding this comment.
서비스에 대한 소개도 있으면 좋을 것 같아요😊
README 라는 이름 만큼 프로젝트를 파악하는데 가장 처음 보여지는 문서이니 효과적으로 사용하면 더 좋다고 생각해요.
There was a problem hiding this comment.
항상 기능만 적어왔는데 다른 분들을 보면 README.md가 잘 되어있더라구요! 저도 이번 피드백과 @angryPodo 님의 피드백을 듣고 조금 더 상세하게 작성해보려고 합니다!! 조언 감사합니다!
| - 입력 값을 받는 기능 | ||
| - 입력 값을 검증, 잘못된 값이 올 경우 Exception 하는 기능 |
There was a problem hiding this comment.
구현할 기능을 잘 작성해주셨어요! 개인적으로는 To-Do에 그치지 않고 완료했다는걸 더 명시해주면 좋을 것 같아요🤔
| var position = START_DISTANCE | ||
| private set |
There was a problem hiding this comment.
Car의 position을 var, private set으로 관리하고 계시네요! 아마 외부에는 읽기 전용으로 만들기 위해서 사용하셨을 것 같아요. 가변(Mutable)한 설계인데 특별한 이유가 있으실까요? 저는 val로 설계를 했거든요🤔
There was a problem hiding this comment.
position의 값이 move를 할 때마다 변하기 때문에 var로 설정하였고, 외부에서 변경을 막기 위해 외부에서는 읽기만, 내부에서는 수정까지 가능하도록 설계했습니다!
| companion object { | ||
| private const val START_DISTANCE = 0 | ||
| } |
There was a problem hiding this comment.
코틀린 공식 컨벤션 가이드에서 companion object는 클래스 본문의 가장 마지막에 두는 것을 권장하고 있어요!
There was a problem hiding this comment.
가이드에서 이부분을 놓쳤네요 ㅜㅜ 정말 감사합니다!
| fun racingStart(cars: List<Car>, tryRound: Int) { | ||
| repeat(tryRound) { | ||
| roundStart(cars) | ||
| OutputView.printRoundResult(cars) | ||
| } |
There was a problem hiding this comment.
racingStart의 내부를 보면 roundeStart(cars)로 게임로직을 실행한 뒤 바로 OutputView.printRoundResult(cars)를 호출하고 있어요. 이렇게 되면 Racing이라는 핵심 도메인 객체가 OutputView라는 특정 뷰에 대해 너무 많은 것을 알게 된다고 생각이 들어요🤔
만약 나중에 콘솔 출력이 아닌 다른 방식으로 결과를 보여주고 싶다면 그 영향이 Racing 객체까지 영향이 끼질 것 같아요.
racingStart는 게임을 진행하고 변경된 자동차들의 '상태'만 반환하고 출력은 결과를 전달받은 다른 주체가 책임지도록 분리하면 더 유연하고 테스터블한 구조가 될거같은데 정찬님은 어떻게 생각하시나요?😊
There was a problem hiding this comment.
동의합니다! 전체적으로 고퀄의 리뷰 감사합니다 !! 테스트에 대해 더 생각해보는 계기가 된 것 같습니다!
3주차 과제도 화이팅하세요!!
| private fun roundStart(cars: List<Car>) { | ||
| cars.forEach { car -> | ||
| val randomNumber = pickNumberInRange(RANDOM_START_NUMBER, RANDOM_END_NUMBER) | ||
|
|
||
| if (randomNumber >= MIN_MOVE_VALUE) | ||
| car.moveForward() | ||
| } | ||
| } |
There was a problem hiding this comment.
roundStart 함수가 내부에서 직접 Randoms.pickNumberInRange를 호출하고 있어요 이렇게 되면 roundStart를 테스트할때마다 결과가 달라져, '자동차가 전진하는 경우'와 '멈추는 경우'를 명확하게 검증하기가 까다로울 것 같아요🤨
'랜덤 값을 생성하는 로직'을 별도의 인터페이스로 추상화하고 외부에서 주입받는 구조를 적용해보면 어떨까요? 그러면 실제 실행 시에는 랜덤을, 테스트 시에는 항상 4 이상 또는 3 이하의 값을 반환하는 '가짜'를 주입하여, 어떤 상황에서든 예측 가능한 테스트를 작성할 수 있지 않을까 생각이 들어요💪🏻
There was a problem hiding this comment.
저도 이 부분에대해 많이 고민이 있었습니다. 정작 테스트를 하려고 보니 random 값을 제가 제어할 수 없는 상황이더라구요! @angryPodo 님의 말대로 3주차 과제에 적용할 곳이 있다면 꼭 적용해보겠습니다!!
| val (inputNames, inputTryCount) = InputView.readInput() | ||
|
|
||
| val names = InputParser.parseNames(inputNames) | ||
| val tryCount = InputParser.parseTryCount(inputTryCount) | ||
|
|
||
| InputValidator.validateName(names) | ||
| InputValidator.validateTryCount(tryCount) | ||
|
|
||
| val cars = names.map { name -> | ||
| Car(name) | ||
| } | ||
|
|
||
| Racing.racingStart(cars, tryCount) |
There was a problem hiding this comment.
Application.kt의 main 함수가 입력을 받고, 파싱, 검증, 객체 생성, 핵심 로직 호출까지 모든 흐름을 직접 다루고 있어요. 이렇게 main 함수가 모든 것을 알고 지시하는 방식을 선택하신 이유가 궁금해요🤔
만약 main은 전체적인 흐름을 담당하는 GameController와 같은 객체를 생성하고 실행하는 역할만 맡는다면, 각 컴포넌트의 책임이 더 명확해지고 main 함수와 분리된 단위 테스트를 작성하기가 더 쉬워지지 않을까 생각이 들어요!
There was a problem hiding this comment.
main함수가 게임의 흐름을 전체적으로 관리하는 함수라고 생각되어 이렇게 구현하였습니다! 그래서 입력을 받고, 검증을 하고, 객체를 생성 후 게임을 진행하는 로직으로 작성했습니다.
리뷰를 보고 찾아보니 main 함수는 따로 단위 테스트가 안되는군요! 하나 더 배워갑니다! 감사합니다!
| fun parseNames(inputNames: String): List<String> { | ||
| return inputNames.split(DELIMITER) | ||
| } |
There was a problem hiding this comment.
이 부분은 저도 고려하지 못했던 부분인데 "pobi , woni"와 같이 이름 앞뒤에 공백을 넣고 입력하는 경우도 있을 것 같아요. trim()을 적용하는 건 어떻게 생각하시나요?😁
| } | ||
|
|
||
| private fun getWinner(cars: List<Car>): List<Car> { | ||
| val maxPosition = cars.maxOf { it.position } |
There was a problem hiding this comment.
현재 cars.maxOf { it.position }를 사용하셨는데, 이 코드는 cars 리스트가 비어있을 경우 NoSuchElementException을 발생시키게 되는데요, 이 부분에서 설계에 대한 두 가지 관점이 있을 것 같아 정찬님의 생각이 궁금해졌습니다🤔
-
관점 1: getWinner와 같은 함수는 비어있는 리스트가 들어오더라도, 예외를 던지기보다 비어있는우승자 목록을 반환하는 등 스스로의 안정성을 확보
-
관점 2: getWinner에 비어있는 리스트가 전달된 것 자체가 이미 프로그램의 다른 어딘가에 논리적 오류가 있다는 신호이므로, 예외를 발생시켜 문제를 즉시 드러내는 것이 더 낫다
현재 코드는 후자의 설계를 따르신 것으로 보이는데, 혹시 의도하신 것이 맞을까요? 내부 로직의 예상치 못한 상태에 대해, 이처럼 빠르게 실패하고 문제를 드러내는 것이 더 나은 설계라고 생각하시는지, 아니면 제가 생각한 것처럼 함수가 자체적으로 안정성을 확보하는 것이 더 좋다고 생각하시는지, 정찬님의 의견이 궁금해요😊
There was a problem hiding this comment.
저의 의도는 코드상에서 getWinner를 호출하는 시점에 List는 비어있을 수 없다고 생각했습니다. 그래서 따로 가드 코드를 설정하지 않았는데, @angryPodo 님의 말에 대한 의견은 함수 자체적으로 안정성을 확보하는 것이 좋다고 생각합니다! 관점 2보다는 먼저 이런 예외를 생각하고 안정성을 확보하는게 훨씬 좋아보이네요!
| fun moveForward() { | ||
| position++ | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
전체적으로 소스코드에 ⛔ 표시가 되어있는데 혹시 궁금하지 않으셨나요?!
End of file이라는 에러인데 파일 끝에 개행이 없어서 표시되는 에러에요. 이게 필요한 이유는 POSIX(UNIX 표준) 규격을 준수하기 위해서인데요! 실제 프로그램 실행에는 영향이 없으나, 협업과 잠재적 오류 방지를 위해 반드시 마지막 줄에 개행을 포함하는게 좋아요. (실제로 문제 생기는걸 봤어요)
안드로이드 스튜디오 or 인텔리제이에서 lineBreak를 자동으로 추가해주는 기능이 있으니 적용하면 좋을 것 같아요🤗
There was a problem hiding this comment.
이야... 정말 감사합니다. 많은 걸 배우는 리뷰네요!!
No description provided.