[둘리] 2단계 자동차 경주 제출합니다. #68
Conversation
| outputView.outputErrorMessage(it.message ?: "에러가 발생했습니다.") | ||
| }.getOrNull() | ||
| } | ||
| private fun executeInputTryNumber(): Int = inputView.inputTryNumber().toInt() |
There was a problem hiding this comment.
함수명만 봤을 때에는 "InputTryNumber를 실행한다"라는 의미로 보여요.
하지만, executeInputTryNumber함수보다는 inputTryNumber라는 함수가 더 잘 이해가 돼요.
executeInputTryNumber함수를 사용하는 것을 "간접 호출"이라고 합니다.
"간접 호출"은 유용한 경우도 많지만, 자칫 잘못하게 되면 과하게 함수를 호출하게 되는 상황을 만들기도 해요.
불필요한 간접 호출을 줄여보면 어떨까요? (이하 관련 내용 동일)
src/main/kotlin/model/Car.kt
Outdated
| @@ -2,16 +2,19 @@ package model | |||
|
|
|||
| import util.Validator | |||
|
|
|||
| class Car(private val name: String, private var position: Int = 0) { | |||
| class Car(private val _name: String, private var _position: Int = 0) { | |||
There was a problem hiding this comment.
코틀린에서 변수에 언더바(_)를 붙이는 것은, backing property를 사용할 때 붙여요.
생성자에 언더바를 붙이지 않고 backing property를 사용하려면 아래와 같이 하시면 됩니다. (이하 관련 내용 동일)
class Car(val name: String, position: Int) {
private var _position: Int = position
val position: Int get() = _position
}ref. https://kotlinlang.org/docs/coding-conventions.html#names-for-backing-properties
src/main/kotlin/model/Cars.kt
Outdated
| fun move(index: Int) { | ||
| cars[index].move(RandomGenerator().getRandomNumber()) | ||
| _cars[index].move(RandomGenerator().getRandomNumber()) | ||
| } |
There was a problem hiding this comment.
move 함수를 잘 구현해주셨네요. 👍
하지만 랜덤값을 생성하는 로직이 함수 내부에 존재하게 되면서, 이 함수가 동작하는 것을 제대로 테스트하기가 어려워보여요.
함수가 의도하는 기능을 테스트할 수 있도록 코드를 개선해보면 어떨까요?
src/main/kotlin/util/CarsHelper.kt
Outdated
| val equalCars = carInfos.groupBy({ it.position }, { it.name }) | ||
| return equalCars[equalCars.keys.max()]?.toList() ?: listOf() | ||
| } | ||
| object CarsHelper { |
There was a problem hiding this comment.
객체 이름만 봤을 때는 "자동차들의 도우미"로 해석이 되는데요.
여기서 Helper라는 단어는 포괄적인 의미로 사용돼요.
클래스에 정의된 함수의 역할에 맞게 클래스명을 작성해보면 어떨까요?
src/main/kotlin/util/CarsHelper.kt
Outdated
| object CarsHelper { | ||
| fun findWinners(cars: Cars): List<String> { | ||
| val equalCars = cars.cars.groupBy({ it.position }, { it.name }) | ||
| return equalCars[equalCars.keys.max()]?.toList() ?: listOf() |
There was a problem hiding this comment.
방어적 복사 활용 👍
추가로 빈 리스트를 리턴하고 싶으시다면, listOf()보다는 emptyList()가 좀 더 가독성이 좋습니다.
src/main/kotlin/util/Validator.kt
Outdated
| @@ -1,5 +1,7 @@ | |||
| package util | |||
|
|
|||
| import view.OutputView | |||
There was a problem hiding this comment.
| import view.OutputView |
domain과 view에 둘 다 접근이 가능한 것은 Controller입니다.
Controller의 역할에 대해서 다시 한번 학습해보시고, Validator는 Controller가 아니므로 OutputView의 의존성을 제거해보면 어떨까요?
src/main/kotlin/view/InputView.kt
Outdated
| }.getOrNull() | ||
| return input ?: inputCarNames() |
There was a problem hiding this comment.
input이 null일 경우 자신의 함수를 다시 호출해주고 있네요.
InputView의 책임은 입력된 값을 받아서 전달해주는 역할입니다.
해당 함수를 반복할지에 대한 여부는 Controller에서 처리해보면 어떨까요? (이하 관련 내용 동일)
src/test/kotlin/CarTest.kt
Outdated
| @@ -6,10 +6,10 @@ import org.junit.jupiter.api.assertThrows | |||
| class CarTest { | |||
|
|
|||
| @Test | |||
| fun getInfoTest() { | |||
| fun initTest() { | |||
There was a problem hiding this comment.
지금과 같은 초기화 테스트보다는, 내가 원하는 자동차 이름을 넣었을 때 해당 이름으로 객체가 생성되는지를 확인한다는 의미를 담아보면 어떨까요?
| private fun inputCarNames(): String { | ||
| var input = inputView.inputCarNames() | ||
| while (input.isNullOrBlank() || input.contains("[ERROR]")) { | ||
| outputView.outputErrorMessage(input ?: Constants.INPUT_IS_NULL) | ||
| input = inputView.inputCarNames() | ||
| } | ||
| return input | ||
| } |
There was a problem hiding this comment.
컨트롤러에서 유효한 입력값을 받을때까지 계속해서 호출하도록 잘 개선해주셨네요. 👍
참고로, 프로그래밍 요구사항에는 유효한 입력값을 받을때까지 계속해서 호출하는 항목은 없습니다. 😅
| }.onFailure { | ||
| outputView.outputErrorMessage(it.message ?: "에러가 발생했습니다.") | ||
| }.getOrNull() | ||
| private fun inputTryNumber(): String { |
There was a problem hiding this comment.
함수는 행위 혹은 행동을 의미하기 때문에, 동사구로 작성하곤 합니다.
하지만 이 함수에는 동사가 2개가 존재합니다. (input, try)
함수에는 하나의 동사만 포함시켜보면 어떨까요? (이하 관련 내용 동일)
src/main/kotlin/model/Cars.kt
Outdated
| class Cars(cars: List<Car>) { | ||
| private val _cars: List<Car> = cars | ||
| val cars: List<Car> get() = _cars |
There was a problem hiding this comment.
| class Cars(cars: List<Car>) { | |
| private val _cars: List<Car> = cars | |
| val cars: List<Car> get() = _cars | |
| class Cars(val cars: List<Car>) { |
Cars 클래스에는 add를 하는 로직이 없습니다.
그렇다면 굳이 backing property 구조로 작성할 필요가 없어 보이네요.
피드백 주신 부분 반영해서 수정했습니다. 감사합니다!