[Step4] - 자동차 경주(우승자) #909

riyenas0925 · 2022-11-20T11:14:46Z

안녕하세요~ 리뷰어님!

지난번에 리뷰해주신 내용과 step4를 반영했어요. 지난번 리뷰 중 아래의 리뷰들은 조금 더 생각해보고 싶어서 step5에서 반영하려고 해요!

감사합니다~ 😄

riyenas0925 · 2022-11-20T13:44:24Z

src/main/kotlin/racingcar/Car.kt

+    private var _location: Int = DEFAULT_CAR_LOCATION,
+    val name: String,
 ) {
-    fun move(randNumber: Int) {
-        val rule = MovingJudgeRule.judge(randNumber)
-        val strategy = rule.strategy()
+    init {
+        require(isValidLength(name)) { "글자수는 5자를 넘을 수 없어요" }
+    }
+
+    val location
+        get() = _location
+    fun move(number: Int) {
+        val rule = MovingJudgeRule.judge(number)
+        val strategy = rule.strategy
+        _location = strategy.move(_location)
+    }


backing properties를 사용해서 구현했는데 클래스 내부에서 location과 _location이 헷갈리기도 하고 무엇보다 변수 앞에 _ 가 붙는게 익숙치 않다는 느낌을 받았어요. 리뷰어님은 어떤 방식을 선호하시는지 궁금해요~!

저 같은 경우에는 var location 프로퍼티와 같이 상태 값이 기본형이라면 Step2 코멘트 내용 중에 두 번째 방식인 setter만 막는 방법을 사용하는 편입니다. 🙂

저는 backing properties를 잘 사용하지 않는 편이에요. 그 이유는 대부분은 val로 프로퍼티를 선언하여 변경되지 않는 값으로 만드는 편이기 때문인데, 그럼에도 불구하고 _를 쓰는 경우는 MutableList와 같이 가변 Collection 타입이 객체의 프로퍼티로 있는 경우, 이 값은 외부에서 변경이 될 수도 있기 때문에 이를 방지하고자 _로 실제 프로퍼티를 가지고 외부에는 _를 제외한 값으로 ReadOnly용 값을 만들어서 방어적인 코드를 작성하려고하는 편이에요. 😃

Rok93

동민님 4단계도 깔끔하게 구현 잘해주셨네요. 👍
저번 미션에서 드렸던 코멘트 중에서 반영이 안된 부분들이 있는 것 같아요. 🥲
해당 부분 꼭 확인해서 반영해주세요. 🙏
추가적으로 이번 미션은 mock 라이브러리 없이 테스트 코드를 작성해보시면 어떨까요? 😃

이외에도 몇몇 코멘트 추가적으로 남겨두었으니 확인해서 반영해주세요. 🙇‍♂️

Rok93 · 2022-11-22T16:57:43Z

src/main/kotlin/racingcar/Car.kt

+    private var _location: Int = DEFAULT_CAR_LOCATION,
+    val name: String,
 ) {
-    fun move(randNumber: Int) {
-        val rule = MovingJudgeRule.judge(randNumber)
-        val strategy = rule.strategy()
+    init {
+        require(isValidLength(name)) { "글자수는 5자를 넘을 수 없어요" }
+    }
+
+    val location
+        get() = _location
+    fun move(number: Int) {
+        val rule = MovingJudgeRule.judge(number)
+        val strategy = rule.strategy
+        _location = strategy.move(_location)
+    }


저 같은 경우에는 var location 프로퍼티와 같이 상태 값이 기본형이라면 Step2 코멘트 내용 중에 두 번째 방식인 setter만 막는 방법을 사용하는 편입니다. 🙂

저는 backing properties를 잘 사용하지 않는 편이에요. 그 이유는 대부분은 val로 프로퍼티를 선언하여 변경되지 않는 값으로 만드는 편이기 때문인데, 그럼에도 불구하고 _를 쓰는 경우는 MutableList와 같이 가변 Collection 타입이 객체의 프로퍼티로 있는 경우, 이 값은 외부에서 변경이 될 수도 있기 때문에 이를 방지하고자 _로 실제 프로퍼티를 가지고 외부에는 _를 제외한 값으로 ReadOnly용 값을 만들어서 방어적인 코드를 작성하려고하는 편이에요. 😃

Rok93 · 2022-11-22T16:59:31Z

src/main/kotlin/racingcar/Car.kt

+    init {
+        require(isValidLength(name)) { "글자수는 5자를 넘을 수 없어요" }
+    }
+
+    val location
+        get() = _location


Kotlin Coding Convention에 따르면 클래스는 아래의 순서대로 포맷을 지키라고 하고있어요! 😃

Property declarations and initializer blocks

Secondary constructors

Method declarations

Companion object

Suggested change

init {

require(isValidLength(name)) { "글자수는 5자를 넘을 수 없어요" }

}

val location

get() = _location

val location

get() = _location

init {

require(isValidLength(name)) { "글자수는 5자를 넘을 수 없어요" }

}

Rok93 · 2022-11-22T17:01:34Z

src/main/kotlin/racingcar/Car.kt

-        val rule = MovingJudgeRule.judge(randNumber)
-        val strategy = rule.strategy()
+    init {
+        require(isValidLength(name)) { "글자수는 5자를 넘을 수 없어요" }


객체가 생성되는 시점에 유효성을 체크하도록 만드셨군요? 👍👍
개인적으로 지금의 경우에는 Validation 로직이 간단하므로 메서드로 추출할 필요없이 바로 명시하는 것이 더 가독성이 좋은 것 같아요. 🙂

추가적으로 예외 메시지를 조금 더 명확하게 적어보면 어떨까요? 😃

Suggested change

require(isValidLength(name)) { "글자수는 5자를 넘을 수 없어요" }

require(name.length <= VALID_CAR_NAME_LENGTH) { "자동차 이름은 5자를 넘을 수 없어요" }

Rok93 · 2022-11-22T17:07:33Z

src/main/kotlin/racingcar/InputView.kt

+        val list = mutableListOf<Car>()
+        for (name in names) {
+            list.add(
+                Car(name = name)
+            )
        }


아래와 같이 변경해볼 수 있을 것 같아요. 😃

return return names.map { Car(name = it) }

Rok93 · 2022-11-22T17:07:57Z

src/main/kotlin/racingcar/InputView.kt

-        private fun readCars(message: String): List<Car> {
-            println(message)
+object InputView {
+    fun readCars(): List<Car> {


read라는 표현보다는 input이라는 표현이 더 맞지 않을까요? 🤔

Rok93 · 2022-11-22T17:22:19Z

src/main/kotlin/racingcar/RacingGame.kt

@@ -12,4 +17,15 @@ class RacingGame(

        return cars
    }
+
+    fun judge(): List<Car> {


judge라는 메서드 네이밍이 자주 보이는 것 같아요. 🤔
조금 더 의미있는 메서드 네이밍을 지어보면 어떨까요? 😃

좋은 코드를 위한 자바 메서드 네이밍

Rok93 · 2022-11-22T17:36:32Z

src/main/kotlin/racingcar/ResultView.kt

+            print(car.name + " : ")
+            repeat(location) { print("-") }
            println()


아래와 같이 바꿔볼 수 있을 것 같아요. 😃

Suggested change

print(car.name + " : ")

repeat(location) { print("-") }

println()

println("${car.name} : ${"-".repeat(car.location)}")

Rok93 · 2022-11-22T17:38:58Z

src/test/kotlin/racingcar/RacingGameTest.kt

@@ -4,19 +4,33 @@ import io.kotest.core.spec.style.StringSpec
 import io.kotest.matchers.shouldBe
 import io.mockk.every
 import io.mockk.mockk
+import org.junit.jupiter.api.assertThrows
+import java.lang.IllegalArgumentException

 class RacingGameTest : StringSpec({
    "레이싱 게임은 매 횟수마다 자동차의 현재 위치를 보여줘요" {
        val mockNumberGenerator = mockk<NumberGenerator>()


이번 미션에서는 mock 라이브러리 없이 테스트 코드를 작성해보시면 좋을 것 같아요. 😃
gradle에서 mockk 라이브러리 의존성을 제거해주세요. 🙏

Rok93 · 2022-11-22T17:46:18Z

src/main/kotlin/racingcar/MovingJudgeRule.kt

@@ -2,15 +2,15 @@ package racingcar

 enum class MovingJudgeRule(
    private val expression: (Int) -> (Boolean),
-    private val strategy: MovingStrategy
+    val strategy: MovingStrategy


3단계 미션에서 코멘트 드렸었는데 반영이 안된 것 같아요. 🥲

먼저 MovingJudgeRule 객체의 상태 값들의 네이밍이 어떤 역할을 하는지 감이 안잡히는 것 같아요. 🤔
역할을 한눈에 파악할 수 있는 네이밍을 고려해서 변경해보면 좋을 것 같아요. 😉

그리고 MovingJudgeRule와 MovingStrategy는 강한 결합을 가지고 있어서 새로운 MovingJudgeRule이 만들어지면 자연스럽게 새로운 MovingStrategy도 생성되게 될거 같아요. 🤔 이 두 객체는 서로 분리하는 것 보다는 MovingStrategy가 가지는 프로퍼티를 그대로 MovingJudgeRule이 (MovingStrategy 객체 대신에) 가지도록 두는 것이 좋을 것 같아요.

Suggested change

val strategy: MovingStrategy

val strategy: (Int) -> (Int)

Rok93 · 2022-11-22T18:15:14Z

src/main/kotlin/racingcar/Car.kt

+    val location
+        get() = _location
+    fun move(number: Int) {
+        val rule = MovingJudgeRule.judge(number)


이 부분에 대해서도 다시 고민해봤는데 (꼭 강한 결합 때문만이 아니라) MovingJudgeRule 객체가 Car 객체의 상태를 변경하는 것이 맞을까? 🤔 라고 생각했을 때, 객체지향의 세계에서 객체는 자율적인 존재이고 객체는 스스로의 행동에 의해서만 상태가 변경되어야하고 또한 자동차가 움직이는 행위는 온전한 자동차의 역할과 책임이기 때문에 자동차 객체가 스스로 결정하는 것이 더 좋다고 생각하는데, 동민님은 어떻게 생각하시나요? 🤔

참고자료 - 상태 부분

riyenas0925 added 6 commits November 20, 2022 19:44

refactor: step3 코드 리뷰 내용 반영

41e8e25

chore: 패키지 구조를 view와 domain으로 분리

d0bed0a

docs: step4 요구사항 반영

0a9c80e

feat: 자동차는 이름을 가질 수 있어요.

530906b

feat: 레이싱 게임은 자동차의 이름을 쉽표로 구분해요.

57b1c77

refactor: 패키지 위치 원래대로 변경

79f0885

riyenas0925 force-pushed the step4 branch from 01bc7fa to 79f0885 Compare November 20, 2022 11:17

riyenas0925 added 2 commits November 20, 2022 21:39

refactor: 자동차 이름 길이 검증을 Car 클래스로 변경

9ab2acd

feat: 레이싱 게임에서 참여하는 자동차는 1대 이상이어야 합니다.

3dcd8df

riyenas0925 force-pushed the step4 branch from 1cc6607 to 3dcd8df Compare November 20, 2022 13:21

riyenas0925 added 2 commits November 20, 2022 22:25

chore: 자동차 기본 위치를 상수로 변경

82e4a62

feat: 레이싱 게임 우승자 구현

7c6de49

riyenas0925 marked this pull request as ready for review November 20, 2022 13:38

riyenas0925 commented Nov 20, 2022

View reviewed changes

Rok93 requested changes Nov 22, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Step4] - 자동차 경주(우승자) #909

[Step4] - 자동차 경주(우승자) #909

riyenas0925 commented Nov 20, 2022 •

edited

Loading

riyenas0925 Nov 20, 2022 •

edited

Loading

Rok93 Nov 22, 2022

Rok93 left a comment

Rok93 Nov 22, 2022

Rok93 Nov 22, 2022

Rok93 Nov 22, 2022

Rok93 Nov 22, 2022

Rok93 Nov 22, 2022

Rok93 Nov 22, 2022

Rok93 Nov 22, 2022

Rok93 Nov 22, 2022

Rok93 Nov 22, 2022

Rok93 Nov 22, 2022

	require(isValidLength(name)) { "글자수는 5자를 넘을 수 없어요" }
	require(name.length <= VALID_CAR_NAME_LENGTH) { "자동차 이름은 5자를 넘을 수 없어요" }

[Step4] - 자동차 경주(우승자) #909

Are you sure you want to change the base?

[Step4] - 자동차 경주(우승자) #909

Conversation

riyenas0925 commented Nov 20, 2022 • edited Loading

riyenas0925 Nov 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rok93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riyenas0925 commented Nov 20, 2022 •

edited

Loading

riyenas0925 Nov 20, 2022 •

edited

Loading