[자동차 경주 step1] 윤성원(제페론) 미션 제출합니다. #22

Zepelown · 2024-08-09T11:40:05Z

No description provided.

- add missing carname input feature

- fix validate standard to check only alphabetic

SeongHoonC

안녕하세요 제페론! 수고많으셨습니다ㅎㅎ
전반적으로 함수 분리를 많이 해주셔서 이해하기 편했습니다. 👍
다만, 자동차가 움직이는 것에 대한 테스트가 없는 것이 아쉽습니다.

또한 model 이 입력에 의존하는 경우가 많이 보였어요. 참고하셔서 반영해주세요!

SeongHoonC · 2024-08-11T05:12:06Z

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

+package racingcar.model
+
+class Car(
+    private val _carName : String?


각 자동차에 이름을 부여할 수 있다.

null 이 입력될 수 있지만 그것을 model 이 알아야할까요?

SeongHoonC · 2024-08-11T05:15:00Z

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

+    private val _carName : String?
+) {
+    private var carLocation : Int = 0
+    val carName : String get() = _carName!!


연산자 !! 는 null 이 아닐 경우에 NullPointException 을 발생시킵니다.
현재 nullable 로 만들고 null 이 아님을 주장하는 상황입니다.

https://kotlinlang.org/docs/null-safety.html#the-operator

코드를 이런 식으로 작성한 이유

이렇게 한 이유가 바로 위에 올려주신 리뷰랑 관계가 있습니다. 지금 모델 내부에서 자동차 이름이 조건에 맞는지를 검사하고 있어서 생성자로 ?을 받았고 이후에 ?을 붙이면 코드가 조금 알아보기 쉽지 않아져서 일단 !!을 붙여서 처리를 했었습니다.

궁금한 점

그러면 모델에 들어오기 전에 조건을 검증하는 걸 거쳐서 model을 만들어야할까요??
예시) input -> controller -> validator -> model

null 이 입력될 수 있다는 것은 readline 의 특징입니다. 이는 View 의 책임으로 볼 수 있으며 이는 View 에서 검증 후 null 일 경우 입력을 다시 받는다던지 readline 대신 readln() 을 사용하는 형태로 바꿀 수 있을 것 같네요!

nullable 은 kotlin.io.readline 을 사용함으로써 nullable 입력이 들어오게 되는 것이죠. 다른 View 로 대체된다면 생기지 않습니다.

저는 View 와 Model 의 관심사를 어떻게 분리할지, Model 에서 View 의존성을 어떻게 끊어낼지 고민할 때 항상 View 를 다른 것으로 대체했을 때 model 의 변화가 있을까(의존하고 있는 것은 아닐까) 고민합니다.

View 를 단순히 입출력 함수 실행 로직을 담는 객체라고 보지 않는 것이 좋아보여요.

또한 kotlin in Action 에 따르면 !! 는 NullPointException 을 발생시키기 때문에 필요한 경우가 아니면 사용하지 말라고 "!!" 강조하는 느낌을 줬다고 하네요 :)

오.. 뷰를 너무 단순하게 보고 있었네요. 감사합니다! 다시 수정해볼게요

SeongHoonC · 2024-08-11T05:17:38Z

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

+        carLocation++
+    }
+
+    fun getLocation() = carLocation


getter 를 따로 만들어 주었군요. kotlin 프로퍼티의 getter setter 에 대해 학습해보세요 :)

https://kotlinlang.org/docs/properties.html#getters-and-setters

SeongHoonC · 2024-08-11T05:20:02Z

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

+    private fun validateCarName(): Boolean {
+        if (_carName.isNullOrBlank()) {
+            return false
+        }
+        return isAllAlphabetic() && _carName.length <= 5
+    }


validate 라는 이름과 다르게 검증하고 있지는 않아요. 예외를 직접 터뜨리거나 반환 형식(Boolean)에 맞게 네이밍을 변경해보는건 어떨까요?

SeongHoonC · 2024-08-11T05:22:51Z

src/main/kotlin/racingcar/model/CarRacingGame.kt

+    private val carNames : List<String>?,
+    private val gameRound : String?


마찬가지로 null 이 "입력" 될 수 있다고해서 model 이 이를 알아야할까요? 입력에 의존하고 있는게 아닐까요?

SeongHoonC · 2024-08-11T05:39:56Z

src/main/kotlin/racingcar/util/RandomNumberGenerator.kt

+
+import kotlin.random.Random
+
+object RandomNumberGenerator {


object 와 class 의 차이는 뭘까요? 이 부분에서 왜 object 를 사용했는지 궁금해요!

object는 객체의 생성과 동시에 선언이 되는 클래스로 싱글턴 기법에 적절한 용도이지만 생성자를 사용 못하는 걸로 알고 있습니다. 유틸에 분류하기도 했고 코드의 기능도 작아서 생성자로 인스턴스를 넘기면서 사용하는 것보단 object를 선언하여 빠르게 기능을 구현하고자 하여 object 를 사용하게 되었습니다.

다형성을 이용하면 더 좋아보여요

SeongHoonC · 2024-08-11T05:40:36Z

src/test/kotlin/study/CarNameValidateTest.kt

+
+class CarNameValidateTest {
+
+    @ParameterizedTest()


ParameterizedTest 를 썼군요 👍

SeongHoonC · 2024-08-11T05:43:10Z

src/test/kotlin/study/CarNameValidateTest.kt

+    @ParameterizedTest()
+    @ValueSource(strings = ["abcaaaa","abaaaa","testqweasdf"])
+    fun `자동차 이름을 길이가 5보다 긴 영어 알파벳으로만 지은 경우`(input: String){
+        assertThrows<IllegalArgumentException> {
+            val car = Car(input)
+        }
+    }


테스트 이름은 검증하고자 하는 요구사항이 드러나는게 중요합니다.

자동차 이름을 길이가 5보다 긴 경우 -> 자동차 이름은 길이가 5를 초과할 수 없다.

SeongHoonC · 2024-08-11T05:44:47Z

src/main/kotlin/racingcar/model/CarRacingGame.kt

+        carRacingGameResults.forEach {
+            if (it.round == maxGameRound.carGameMaxRound){
+                return it.getWinners()
+            }
+        }


it 이 많아지면 이해할 수 없는 경우가 많아요.

Suggested change

carRacingGameResults.forEach {

if (it.round == maxGameRound.carGameMaxRound){

return it.getWinners()

}

}

carRacingGameResults.forEach { result ->

if (result.round == maxGameRound.carGameMaxRound){

return result.getWinners()

}

}

SeongHoonC · 2024-08-11T05:50:10Z

src/main/kotlin/racingcar/model/CarRacingGameRoundResult.kt

+        carNames.forEach {
+            val randomNumber = RandomNumberGenerator.generate()
+            if (randomNumber >= 4){
+                it.move()
+            }
+            roundResult[it.carName] = it.getLocation()
+        }


이 자동차 게임은 유연하지 못합니다.

자동차의 움직임 조건이 변경되면 CarRacingGameRoundResult 를 수정해야합니다.

자동차의 움직임 조건이 컴파일 타임에 결정됩니다.

추가로 4라는 숫자를 다른 개발자가 보면 이해할 수 없을거에요. Magic Number 를 없애고 적절한 이름으로 상수화하는건 어떨까요?

companion object { private const val CAR_MOVE_CONDITION = 4 }

리뷰 감사합니다. 잘 생각해보고 다시 제출해보도록 하겠습니다!!

- Create CarRacingManager to separate method

…ructor

…validation failure

…try on validation failure

…ller

SeongHoonC

제페론! 먼저 열심히 미션에 참여해주셔서 감사합니다.
먼저 자동차 경주가 필요 이상으로 복잡해졌습니다.
이런 경우에는 처음부터 짜보는 것이 도움이 됩니다.
TDD 로 해보는 것은 어떤가요? 요청하시면 날 잡고 같이 페어로 해봐요:)

SeongHoonC · 2024-08-24T12:34:15Z

src/test/kotlin/study/CarMovementTest.kt

테스트 코드의 패키지 구조는 프로덕션 코드와 같아야합니다.:)

테스트는 객체 별로, Public 함수를 다 테스트한다고 생각하시면 됩니다.
Car -> CarTest
CarRacingGame -> CarRacingGameTest

SeongHoonC · 2024-08-24T12:38:26Z

src/main/kotlin/racingcar/model/game/CarRacingGameMaxRound.kt

+package racingcar.model.game
+
+class CarRacingGameMaxRound(
+    private val carGameMaxRoundInput: String


모델이 뷰를 알아야할까요?

SeongHoonC · 2024-08-24T12:39:03Z

src/main/kotlin/racingcar/model/game/CarRacingGameRoundResult.kt

CarRacingGameRoundResult 라는 이름과 다르게 게임 그 자체로 보이네요ㅜ

SeongHoonC · 2024-08-24T12:41:02Z

src/main/kotlin/racingcar/util/RandomNumberGenerator.kt

+
+import kotlin.random.Random
+
+object RandomNumberGenerator {


다형성을 이용하면 더 좋아보여요

SeongHoonC · 2024-08-24T12:49:34Z

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

+class Car(
+    private val _carName : String
+) {
+    private var _carLocation : Int = 0
+    var carLocation : Int
+        get() = _carLocation
+        private set(newValue){
+            _carLocation = newValue
+        }
+    val carName : String get() = _carName


아래 코드로 충분해 보입니다:)

Suggested change

class Car(

private val _carName : String

) {

private var _carLocation : Int = 0

var carLocation : Int

get() = _carLocation

private set(newValue){

_carLocation = newValue

}

val carName : String get() = _carName

class Car(

val name : String,

) {

var location : Int = 0

private set

감사합니다 한번 다시 처음부터 해봐야겠네요!

Zepelown added 18 commits August 8, 2024 18:07

docs: add feature document

7913b31

docs: edit feature document to fix typo

537bfe0

feat: add carname input feature

0f8defa

fix: fix carname input feature

0180187

- add missing carname input feature

feat: add carname validate feature

dffc74c

feat: add car racing game round input message

320a681

feat: add car game round input feature

85773cb

feat: add car max game round validate feature

3e3347d

fix: fix car name validate feature

6b7e762

- fix validate standard to check only alphabetic

test: add input test

933c4fa

docs: add util feature document

86b1a0f

feat: add random number generator feature

88e77e7

feat: add calculate game result feature

3640a50

feat: add game result display feature

6afca36

refactor: impove print game round result method

25df8d7

feat: add print winners feature

4b0ffe7

refactor: impove calculate game result logic

00ea718

refactor: impove code readability

edb65c7

SeongHoonC requested changes Aug 11, 2024

View reviewed changes

Zepelown added 11 commits August 11, 2024 22:02

refactor: remove car getter method

476a531

refactor: remove nullable argument car object constructor

d2277e7

refactor: separate method for createing car object

00cd8f5

- Create CarRacingManager to separate method

refactor: remove nullable argument CarRacingGameMaxRound object const…

652033f

…ructor

refactor: impove method naming in car name validate test

d121996

refactor: impove method name in car racing max round validate test

6a8e0f8

test : add car movement test

710bf57

refactor: separate car movement condition from round result object

899c151

refactor: refactor getWinner method in CarRacingGame object

bc3a1c6

refactor: enhance CarRacingGameFactory code clarity

b0d6e77

impove: add error handling for name input form to prompt re-entry on …

941e024

…validation failure

Zepelown added 2 commits August 23, 2024 01:02

impove: add error handling for CarMaxRound input form to prompt re-en…

6744d7a

…try on validation failure

refactor : refactor createCarsFromInput method in CarRacingGameContro…

1b72ebf

…ller

SeongHoonC requested changes Aug 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[자동차 경주 step1] 윤성원(제페론) 미션 제출합니다. #22

[자동차 경주 step1] 윤성원(제페론) 미션 제출합니다. #22

Zepelown commented Aug 9, 2024

SeongHoonC left a comment

SeongHoonC Aug 11, 2024

SeongHoonC Aug 11, 2024

Zepelown Aug 11, 2024

SeongHoonC Aug 11, 2024 •

edited

Loading

SeongHoonC Aug 11, 2024

SeongHoonC Aug 11, 2024

Zepelown Aug 11, 2024

SeongHoonC Aug 11, 2024

SeongHoonC Aug 11, 2024

SeongHoonC Aug 11, 2024

SeongHoonC Aug 11, 2024

Zepelown Aug 11, 2024 •

edited

Loading

SeongHoonC Aug 24, 2024

SeongHoonC Aug 11, 2024

SeongHoonC Aug 11, 2024

SeongHoonC Aug 11, 2024

SeongHoonC Aug 11, 2024

SeongHoonC Aug 11, 2024

Zepelown Aug 11, 2024

SeongHoonC left a comment •

edited

Loading

SeongHoonC Aug 24, 2024

SeongHoonC Aug 24, 2024

SeongHoonC Aug 24, 2024

SeongHoonC Aug 24, 2024

SeongHoonC Aug 24, 2024

SeongHoonC Aug 24, 2024

Zepelown Aug 28, 2024

		private val carNames : List<String>?,
		private val gameRound : String?

[자동차 경주 step1] 윤성원(제페론) 미션 제출합니다. #22

Are you sure you want to change the base?

[자동차 경주 step1] 윤성원(제페론) 미션 제출합니다. #22

Conversation

Zepelown commented Aug 9, 2024

SeongHoonC 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

코드를 이런 식으로 작성한 이유

궁금한 점

SeongHoonC Aug 11, 2024 • edited Loading

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

Zepelown Aug 11, 2024 • edited Loading

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

SeongHoonC left a comment • edited Loading

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

SeongHoonC Aug 11, 2024 •

edited

Loading

Zepelown Aug 11, 2024 •

edited

Loading

SeongHoonC left a comment •

edited

Loading