Step4 자동차 경주 리팩토링 by youngsuk-kim · Pull Request #948 · next-step/kotlin-racingcar

youngsuk-kim · 2022-12-07T18:25:48Z

제출이 늦었습니다.. 열심히 해보겠습니다!!

Flamme1004K

안녕하세요!

전체적으로 구현 잘해주셨네요 💯

몇가지 코멘트를 남겼으니 확인 부탁드리겠습니다!

궁금하신 점에 대해서는 DM 부탁드립니다!!

확인 후 다시 리뷰 요청 부탁드립니다!

Flamme1004K · 2022-12-08T11:24:52Z

src/main/kotlin/src/racingcar/RacingCarGame.kt

    fun startGame() {
        val inputValue = InputView.executeInputScreen()
-        CarService().execute(inputValue.tryCount, inputValue.carNumber)
+        val raceCars = CarService(getCarMoveRandomValue()).execute(inputValue.carNames)


처음에 있는 난수가 정해지면, 그 난수를 이용하여 끝까지 진행이 되는군요! 어떻게 해야 자동차들이 각자 움직일 수 있을까요?

Flamme1004K · 2022-12-08T11:25:03Z

src/main/kotlin/src/racingcar/description.md

@@ -0,0 +1,40 @@
+경주할 자동차 이름을 추출 하는 기능.


description.md 파일의 위치는 src가 아닌 다른 디렉터리에 있어야하지 않을까요?

Flamme1004K · 2022-12-08T11:27:45Z

src/main/kotlin/src/racingcar/domain/Car.kt

+    fun getMoveCount(): Int {
+        return moveCount
+    }
+
+    fun getCarName(): String {
+        return name
+    }


코틀린의 프로퍼티는 접근자를 모두 지원하고 있어요. name에 대한 접근 제어자를 public으로 변경하면 되죠.

하지만 moveCount인 경우에는 set도 같이 쓸 수가 있기 때문에 다른 처리가 필요할거에요. 이 부분에 대해서는 한번 찾아보시겠어요~?

Flamme1004K · 2022-12-08T11:29:43Z

src/main/kotlin/src/racingcar/domain/CarService.kt

+import src.racingcar.parseComma
+
+class CarService(
+    private val carMoveRandomValue: Int


CarService는 자동차에 움직임에 대해서 랜덤인지 아닌지 알 수 없다고 생각합니다. carMobeRandomValue 보다는 자동차의 움직임에 대한 carMoveValue가 좋지 않을까요? 🤔

Flamme1004K · 2022-12-08T11:29:57Z

src/main/kotlin/src/racingcar/domain/CarService.kt

+    private val carMoveRandomValue: Int
+) {
+
+    fun execute(carNames: String): MutableList<Car> {


MutableList는 가변리스트에요. 가변 리스트인 경우에는 어떤 단점이 있을까요?

Flamme1004K · 2022-12-08T11:31:52Z

src/main/kotlin/src/racingcar/domain/Race.kt

+class Race(
+    private var cars: MutableList<Car> = mutableListOf()
+) {
+
+    fun create(carNames: List<String>): Race {
+        for (carName in carNames) {
+            cars.add(Car(carName))
+        }
+
+        return this
+    }


정적 팩토리 메서드를 통하여 리팩터링 해볼까요?

[링크]

정적 팩토리 메서드

Flamme1004K · 2022-12-08T11:34:01Z

src/main/kotlin/src/racingcar/domain/Winner.kt

+    private val racingCars: List<Car>
+) {
+    fun findWinners(): List<Car> {
+        return racingCars.filter { car -> car.getMoveCount() >= racingCars.maxOf { it.getMoveCount() } }


Suggested change

return racingCars.filter { car -> car.getMoveCount() >= racingCars.maxOf { it.getMoveCount() } }

val max = racingCars.maxOf { it.getMoveCount() }

return racingCars.filter { car -> car.getMoveCount() >= max }

maxOf 사용 좋습니다 👍
변경 전과 변경 후의 코드의 차이는 무엇일까요?

Flamme1004K · 2022-12-08T11:34:17Z

src/main/kotlin/src/racingcar/utils/Parser.kt

@@ -0,0 +1,5 @@
+package src.racingcar


패키지 구조가 맞지 않네요.

Flamme1004K · 2022-12-08T11:34:22Z

src/test/kotlin/src/racingcar/CarServiceTest.kt

+import org.junit.jupiter.api.Test
+import src.racingcar.domain.CarService
+
+class CarServiceTest {


우승자에 대한 테스트 케이스는 없는걸까요?

Flamme1004K

안녕하세요!

피드백 반영 감사합니다 💯

몇가지 코멘트를 남겼으니 확인 부탁드리겠습니다!

궁금하신 점에 대해서는 DM 부탁드립니다!!

확인 후 다시 리뷰 요청 부탁드립니다!

Flamme1004K · 2022-12-14T12:22:21Z

src/main/kotlin/src/racingcar/domain/Car.kt

@@ -0,0 +1,22 @@
+package src.racingcar.domain
+
+open class Car(


open class를 사용하신 이유가 있을까요?

Flamme1004K · 2022-12-14T12:22:23Z

src/main/kotlin/src/racingcar/domain/Car.kt

+    var moveCount: Int = moveCount
+        protected set


Suggested change

var moveCount: Int = moveCount

protected set

var moveCount: Int = moveCount

private set

kotlin에서 protected와 private의 차이는 무엇일까요~?

Flamme1004K · 2022-12-14T12:22:25Z

src/main/kotlin/src/racingcar/domain/Car.kt

+    open fun move(carMoveNumber: Int = (MOVE_START_POINT..MOVE_END_POINT).random()): Int {
+        if (CAR_MOVE_CONDITION_NUMBER <= carMoveNumber) moveCount++
+
+        return moveCount
+    }


default arguments를 사용하셨군요! 👍

Flamme1004K · 2022-12-14T12:25:22Z

src/main/kotlin/src/racingcar/domain/Race.kt

+    fun start(): List<Car> {
+        cars.forEach { it.move() }
+
+        return cars
+    }


도메인 클래스에 대한 테스트는 모두 해야한다고 생각해요! race에 결과에 대한 테스트는 어떻게 해야 할까요?

참고 문서 입니다!

Flamme1004K · 2022-12-14T12:26:35Z

src/main/kotlin/src/racingcar/domain/Winner.kt

+) {
+    fun findWinners(): List<Car> {
+        val max = racingCars.maxOf { it.moveCount }
+        return racingCars.filter { car -> car.moveCount >= max }


Suggested change

return racingCars.filter { car -> car.moveCount >= max }

return racingCars.filter { car -> car.moveCount == max }

== 비교로도 충분하지 않을까요?

call-me-bread added 3 commits December 8, 2022 03:18

feat: 자동차 경주 (우승자) 완료

78b3f29

feat: 자동차 경주 리팩토링

b1e888a

refactor: 린트 수정

ce29109

Flamme1004K requested changes Dec 8, 2022

View reviewed changes

call-me-bread added 2 commits December 13, 2022 07:15

refactor: carService 제거

fb06d6f

refactor: 코드리뷰 반영

b9cbcee

Flamme1004K requested changes Dec 14, 2022

View reviewed changes

		@@ -0,0 +1,40 @@
		경주할 자동차 이름을 추출 하는 기능.

	return racingCars.filter { car -> car.getMoveCount() >= racingCars.maxOf { it.getMoveCount() } }
	val max = racingCars.maxOf { it.getMoveCount() }
	return racingCars.filter { car -> car.getMoveCount() >= max }

	return racingCars.filter { car -> car.moveCount >= max }
	return racingCars.filter { car -> car.moveCount == max }

Conversation

youngsuk-kim commented Dec 7, 2022

Uh oh!

Flamme1004K left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flamme1004K left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants