4단계 - 블랙잭(베팅) #551

ohsung · 2023-07-07T08:59:11Z

안녕하세요
반선호님 리뷰 요청 드립니다.

감사합니다.

README 정리

1. BlackJackController 플레이어 배팅 입력 구현 2. Dealer result 함수 수익으로 변경 3. GameResult Blackjack 추가 및 배당률 추가 4. Money class 추가 5. Player Money 추가, result, ofGameState 함수 변경 6. Cards 카드가 블랙잭과 21인 경우 분리 7. 승패가 수익으로 변경으로 인한 Test case 수정

1. print 함수 제거

vsh123

안녕하세요!
step4 구현 잘해주셨네요 :)

몇가지 코멘트를 남겨놨으니 확인부탁드려요~

vsh123 · 2023-07-08T05:40:07Z

src/main/kotlin/baclkjack/domain/BlackJackGame.kt


-class BlackJackGame(playersName: List<String>, private val deck: Deck = Deck.createDeck()) {
+class BlackJackGame(playersName: List<String>, betting: List<Int>, private val deck: Deck = Deck.createDeck()) {


playersName과 betting을 index로 매핑하는 방식이 괜찮을까요?? 현재는 playerName과 betting의 사이즈가 동일할거라는걸 전제하에 만들어진 코드로 보여서요~

Player를 만들고 이후에 배팅금액을 업데이트하는 방식으로 해보는건 어떨까요?

vsh123 · 2023-07-08T05:40:31Z

src/main/kotlin/baclkjack/domain/BlackJackGame.kt


-    val players = playersName.map { Player(it) }
+    val players = playersName.mapIndexed { index, s ->
+        Player(name = s, money = Money(betting[index]))


s보다는 name과 같이 명확하게 네이밍해주면 좋을 것 같아요!

vsh123 · 2023-07-08T05:43:38Z

src/main/kotlin/baclkjack/domain/play/Money.kt

+
+
+@JvmInline
+value class Money(val value: Int)


0보다 커야한다는 등의 간단한 validation을 추가해주면 더 좋을것 같네요!

딜러가 Money 를 0으로 가지고 있어서 validation 안했었는데요.
수정했습니다.

vsh123 · 2023-07-08T05:44:31Z

src/test/kotlin/blackjack/PlayerTest.kt

@@ -46,9 +34,29 @@ class PlayerTest : StringSpec({
        player.score() shouldBe 30
    }

-    "딜러가 17 내카드가 18 인 result " {
+    "딜러가 17 내 카드가 21 인경우 blackjack 수익은 1500" {


제가 놓치고 있던 부분인데요..!!
프로덕션 코드와 테스트코드의 패키지 위치가 동일하지 않을 것 같아요!
동일하게 맞춰주면 어떨까요?

좋은 리뷰 감사합니다.

vsh123 · 2023-07-08T05:45:04Z

src/main/kotlin/baclkjack/domain/BlackJackGame.kt

-import baclkjack.domain.play.Dealer
-import baclkjack.domain.play.Player
-import baclkjack.domain.play.User
+import baclkjack.domain.play.*


린트가 실패했을 것 같은데요! 한번 확인부탁드려요~

vsh123 · 2023-07-08T05:47:03Z

src/main/kotlin/baclkjack/controller/BlackJackController.kt

@@ -46,11 +49,11 @@ class BlackJackController(

        resultView.showFinal()
        backJackGame.dealer.also {
-            resultView.showWinner(it.name, it.result(backJackGame.players).toResult())
+            resultView.showWinner(it.name, it.result(backJackGame.players).toString())


int를 toString해주신 이유가 궁금합니다!

showWinner(name: String, result: String)가 다음과 같이 받고 있어서 입니다.

showWinner(name: String, result: Int)로 바꿀수는 없는걸까요? 충분히 가능해보여서요~!

1. Players 추가 2. User Interface 수정 3. Money validation 추가 4. 테스트 코드 package 수정

lint 수정

vsh123

안녕하세요!
피드백 빠르게 반영잘해주셨군요!! 👍

몇가지 고민할 포인트를 추가해놨으니 확인부탁드릴게요~

vsh123 · 2023-07-08T13:13:29Z

src/main/kotlin/baclkjack/controller/BlackJackController.kt


 class BlackJackController(
    private val inputView: InputView = InputView,
    private val resultView: ResultView = ResultView
 ) {

    fun play() {
-        val players = inputView.inputPlayer()
+        val playerNames = inputView.inputPlayer()


함수(또는 메서드)의 길이가 15라인을 넘어가지 않도록 구현한다.

play()라는 함수는 위의 제약조건을 위배하고 있는 것 같은데요! 고민 후 함수를 작게 나눠보는 건 어떨까요?

vsh123 · 2023-07-08T13:20:24Z

src/main/kotlin/baclkjack/domain/play/Player.kt

+    override val name: String,
+    override val cards: Cards = Cards(),
+    val money: Money
+) : User {

    var cardDrawListener: CardDrawListener? = null


지연 초기화에 대해 알아보고 적용해보면 어떨까요?어떤 이점이있을까요?

https://kotlinlang.org/docs/properties.html#late-initialized-properties-and-variables

vsh123 · 2023-07-08T13:21:13Z

src/main/kotlin/baclkjack/domain/play/Player.kt

    override fun isDraw(): Boolean = cardDrawListener?.isDraw(name) == true

-    fun result(user: User): GameState = ofGameState(user)
+    fun result(user: User): Int = (ofGameState(user).earningsRate * money.value).toInt()


GameResult.earningRate와 money.value를 get하지 않고 값을 도출해보면 어떨까요?

vsh123 · 2023-07-08T13:22:28Z

src/main/kotlin/baclkjack/domain/play/Money.kt

@@ -0,0 +1,15 @@
+package baclkjack.domain.play
+
+


lint가 적용되지 않을 것 같아요!

코드가 추가되었다면 테스트를 작성해보면 어떨까요?

vsh123 · 2023-07-08T13:24:07Z

src/test/kotlin/blackjack/play/PlayerTest.kt

+        val dealer = Dealer()
+        val player = Player(name = "a", money = Money(1_000))
+        val deck = Deck(
+            cards = mutableListOf(


어떤 테스트는 apply를 활용하여 add를 해주고 있고, 어떤테스트는 생성과 함께 초기화를해주는데요! 하나로 통일해보면 어떨까요?

vsh123 · 2023-07-08T13:25:51Z

src/main/kotlin/baclkjack/domain/play/Players.kt

+
+class Players(private val players: List<Player>) : List<Player> by players
+
+fun List<Player>.toPlayers() = Players(this)


저는 리턴이 있는 모든 함수에는 리턴값을 명시해주는 편인데요! 성오님의 만의 규칙이 있을까요?

ohsung added 3 commits July 7, 2023 15:18

feat: 4단계 - 블랙잭(베팅)

6b77413

README 정리

feat: 4단계 - 블랙잭(베팅)

d541db5

1. print 함수 제거

vsh123 requested changes Jul 8, 2023

View reviewed changes

ohsung added 2 commits July 8, 2023 16:15

refactor: 4단계 - 블랙잭(베팅)

4c66898

1. Players 추가 2. User Interface 수정 3. Money validation 추가 4. 테스트 코드 package 수정

refactor: 4단계 - 블랙잭(베팅)

d251e01

lint 수정

vsh123 requested changes Jul 8, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4단계 - 블랙잭(베팅) #551

4단계 - 블랙잭(베팅) #551

ohsung commented Jul 7, 2023

vsh123 left a comment

vsh123 Jul 8, 2023

vsh123 Jul 8, 2023

vsh123 Jul 8, 2023

ohsung Jul 8, 2023

vsh123 Jul 8, 2023

ohsung Jul 8, 2023

vsh123 Jul 8, 2023

vsh123 Jul 8, 2023

ohsung Jul 8, 2023

vsh123 Jul 8, 2023

vsh123 left a comment

vsh123 Jul 8, 2023

vsh123 Jul 8, 2023

vsh123 Jul 8, 2023

vsh123 Jul 8, 2023

vsh123 Jul 8, 2023

vsh123 Jul 8, 2023


		class BlackJackGame(playersName: List<String>, private val deck: Deck = Deck.createDeck()) {
		class BlackJackGame(playersName: List<String>, betting: List<Int>, private val deck: Deck = Deck.createDeck()) {


		class Players(private val players: List<Player>) : List<Player> by players

		fun List<Player>.toPlayers() = Players(this)

4단계 - 블랙잭(베팅) #551

Are you sure you want to change the base?

4단계 - 블랙잭(베팅) #551

Conversation

ohsung commented Jul 7, 2023

vsh123 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

vsh123 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