-
Notifications
You must be signed in to change notification settings - Fork 411
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
[Step4] 자동차 경주 #1529
base: be-student
Are you sure you want to change the base?
[Step4] 자동차 경주 #1529
Conversation
b083f4e
to
b42bacd
Compare
b42bacd
to
31f5a03
Compare
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.
누누 안녕하세요. 😊
4단계 구현하시느라 수고 많으셨습니다!
몇 가지 코멘트 남겼는데 확인 부탁 드릴게요.
@@ -7,25 +7,34 @@ import org.junit.jupiter.params.ParameterizedTest | |||
import org.junit.jupiter.params.provider.ValueSource | |||
|
|||
internal class CarTest { | |||
@Test | |||
fun `자동차의 이름은 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.
결과적으로 예외 처리를 하네요. 그러면 정확한 동작을 기술하면'자동차 이름이 공백("")일 경우 예외가 발생한다.' 정도로 하면 더 명확하게 테스트 내용을 이해하는데 도움이 될 것 같다는 생각이 드네요.
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.
아하 글자수보다는 훨씬 더 직관적이겠네요 감사합니다! 👍
fun `자동차들은 1대 이상 있어야 한다`() { | ||
assertThrows<IllegalArgumentException> { Cars.initialize(0, listOf("a", "b", "c"), FixedPowerGenerator(5)) } | ||
} | ||
|
||
@Test | ||
fun `자동차들은 특정 숫자 이상이 들어오면 움직인다`() { |
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.
특정 숫자라고 표현하면 이 테스트를 처음 보는 사람이 명확히 인지하기 어렵습니다. 4라는 숫자로 테스트 했을 때 움직인는 것이 맞는건지 이 테스트가 의도한 것이 맞는지 또 한 번 커뮤니케이션이 발생하겠죠.
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.
CarTest를 보니 또 명확히 적어주셨네요! 🤔
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.
file changed 만 보다보니 저번에 말씀해주신 부분을 놓쳤네요 😢
반영하겠습니다!
fun inputNameOfCars(): List<String> { | ||
println("자동차 이름을 입력하세요. (이름은 쉼표(,) 기준으로 구분)") | ||
return readln().split(",") | ||
} |
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.
View에서 바로 입력 값에 대한 가공을 하셨네요. 이건 참 사람마다 의견이 나뉠 수 있는 부분이라고 생각해요. 그런데 이런 처리까지 View에서 한다면 입력 받고 split 하면서 바로 이름에 대한 사이즈 검증도 하면 되지 않나? 라는 생각이 들기도 하네요. 어떤 부분은 View에서 직접 처리하고 어떤 부분은 별도 객체에서 처리할 지 결정한 기준이 있으신가요?
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.
"자동차 n 대가 있으면 이름이 n 개 있어야 해" 라는 로직은 비즈니스 로직쪽에 더 가까울 것 같아서, 제거했고, split 같이 단순하게 처리하기 편하게 하기 위한 것들은 그냥 view 에서 처리하는 방향으로 하려고 했어요
입력을 처리하기 위한 편의성의 여부에 따라 달라질 것 같아요
src/main/kotlin/racingcar/Cars.kt
Outdated
fun findWinner(): List<Car> { | ||
val maxPosition = cars.maxBy { it.position }.position | ||
return cars.filter { it.position == maxPosition } | ||
} |
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.
우승자를 찾는 것은 Cars 객체가 반드시 해야 할 역할은 아닐 수도 있다는 생각이 들기도 해요. 우승자 선정 정책이 바뀐다거나 여러가지 정책을 적용해야 하는 경우 등이 생길 경우를 고려한다면 더 좋은 구조가 나올 수 있지 않을까요?
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.
누누 안녕하세요.
요구 사항과 다르게 구현 부분이 존재하여 코멘트 남겼습니다.
해당 부분 수정 후 다시 리뷰 요청 드립니다.
} | ||
|
||
private fun initializeCars(): Cars { | ||
val numberOfCars = InputView.inputNumberOfCars() |
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.
이번 단계에서는 자동차 대수에 대한 입력은 받지 않습니다. 따라서 현재 전체적으로 프로그램이 제대로 동작하지 않습니다. 수정 후 다시 리뷰 요청 드립니다.
안녕하세요 제임스 리뷰요청이 늦어졌네요 잘 부탁드립니다! 🙇