[Step4] 4단계 - 블랙잭(베팅) #697

bong6981 · 2023-12-03T15:48:33Z

안녕하세요! 블랙잭 베팅을 구현했습니다.

21점이면 더이상 카드를 받지 못하는 것으로 수정했습니다.
베팅금액을 어디에 둘지 고민했는데, 경기와 ( GameTable ) 과 별도로 움직여 이를 분리했습니다!
잘 부탁드립니다. 감사합니다.

jinyoungchoi95

안녕하세요 보경님 😄

4단계 잘 구현해주셨네요. 몇가지 피드백 코멘트로 남겨두었으니 확인부탁드립니다.

궁금하거나 고민이 되는 부분이 있으시다면 언제든 pr 코멘트 또는 dm으로 요청 부탁드립니다.
감사합니다 🙇‍♂️

jinyoungchoi95 · 2023-12-04T08:05:26Z

README.md

@@ -9,45 +9,87 @@
 - git의 commit 단위는 앞 단계에서 README.md 파일에 정리한 기능 목록 단위로 추가한다.

 ### 기능 목록 


문서화 👍

jinyoungchoi95 · 2023-12-04T08:09:39Z

src/main/kotlin/blackjack/domain/batting/Amount.kt

+    val value: BigDecimal,
+) : Comparable<Amount> {
+
+    operator fun plus(other: Amount): Amount = value.plus(other.value).let(::Amount)


Suggested change

operator fun plus(other: Amount): Amount = value.plus(other.value).let(::Amount)

operator fun plus(other: Amount): Amount = Amount(value.plus(other.value))

let으로 변환하는 방법도 있겠지만 단순히 Amount에 대한 생성이 목표라면 생성자를 붙여주는것이 가독성이 더 높을 것 같아요.

결국 Amount에 대한 생성을 목표로하기 때문에 생성자를 사용하지 않을 이유도 없고, let(stream) 처리가 추가됨으로써 가독성이 오히려 떨어질 수도 있거든요.

오른쪽으로 쭉 읽으며 의미가 이해 된다고 생각했는데, Amount 생성자를 붙여주는 것이 더 가독성이 좋군요!

a4db3db 에서 반영했습니다!

jinyoungchoi95 · 2023-12-04T08:12:03Z

src/main/kotlin/blackjack/domain/batting/Amount.kt

+
+    companion object {
+        val ZERO = Amount(BigDecimal.ZERO)
+        fun betAmount(amount: Int): Amount {


Int로 들어온 value를 단순히 BigDecimal로 변환하고 있기 때문에 betAmount 보다는 Amount를 생성한다는 의미가 더 어울리지 않을까요?

정적팩토리메소드를 사용하고 있다면 생성자를 private하게 해주어도 되지 않을까요?

혹은 정적팩토리메소드를 사용하지 않고 부생성자에서 처리하는건 어떨까요? 이정도의 타입을 변화하는 로직만 존재한다면 부생성자를 추가해도 충분할 것 같아보여서요.

Amount 는 단순 금액의 의미를 나타냅니다. Amount 자체는 0이 될 수 있습니다 ( 플레이어의 수령 금액 등 )

betAmount 는 Amount 중 베팅 금액에 대한 의미를 나타내기 때문에 팩토리 메서드로 분리해 검증했습니다
해서 베팅금액에 대한 정적 팩토리 메서드를 따로 정의했습니다.!

BetAmount(val value: Amount) 로 분리하지 않은 이유는 단순 생성시에서 값 체크만 BetAmount 로서의 기능을 한다고 생각해서 정적 팩토리 메서드로 검증을 하도록 했습니다!

말씀하신대로의 Amount가 0이 될 수 있다면 음수를 검증하는 validation은 Amount에서 필요해보입니다.
(반영하지 않으셔도 됩니다.) Amount가 단순 금액을 의미하고, "배팅 금액"은 금액에서 배팅이라는 비즈니스로직이 들어간다면 오히려 분리하는 것이 어떨까하네요. 개인적으로는 betAmount으로 호출의 의도하여도, 그 파라미터나 필드가 Amount 생성자를 통해서 생성하여 사용하게 된다면을 가정해보면 원하는 객체에 대한 생성 안전성이 떨어져보입니다.

BetAmount 로 이를 분리했습니다!

jinyoungchoi95 · 2023-12-04T08:14:03Z

src/main/kotlin/blackjack/domain/card/HandScore.kt

        private const val BUST_THRESHOLD = 21
+        private const val BLACK_JACK_SCORE = 21


BUST_THRESHOLD와 BLACK_JACK_SCORE가 두개가 동시에 존재해야하는 이유가 있을까요?

게임 수치상 두 점수는 같지만 BUST_THRESHOLD 는 더 이상 카드를 받을 수 없는 한계점, BLACK_JACK_SCORE 승패 결정시 해당 점수 확인 이라는 역할이 다르다고 생각해서 분리했습니다. !

생각해 보니 제가 도메인(블랙잭)에 대한 이해가 부족해서 나온 개념일수도 있겠네요

블랙잭 스코어를 넘으면 버스트다 라고 이해를 하고 BLACK_JACK_SCORE 로 통일했습니다

f96543d

jinyoungchoi95 · 2023-12-04T08:29:25Z

src/main/kotlin/blackjack/domain/player/Player.kt

+    fun judgeVictory(dealer: Dealer): VictoryStatus =
+        when {
+            score.isBust -> VictoryStatus.LOSS
+            score isGreaterGameScoreThan dealer.score -> VictoryStatus.WIN


현재 judgeVictory 함수만 보았을 때 when절의 조건이 이해가 안갈 수 있을 것 같습니다.

BUST되는 경우 score가 0이 되어서 현재 로직이 구성되고 있는데요. 이 로직을 알아야 버스트가 되더라도 isGreaterGameScoreThan에서 dealer.score가 0으로 반환되어 WIN이 된다는 사실을 알 수 있습니다. score라는 프로퍼티에 BUST인 경우 0이다라는 로직이 들어가기 때문인데요. 프로퍼티의 getter, setter에는 특별한 비즈니스 로직이 없는 것이 다른 개발자가 보고 유지보수를 진행하는데 도움이 됩니다.

score가 단순히 점수만을 반환하고, BUST 유무에 대한 케이스는 이 함수에서 체크해야할 것 같은데 어떻게 생각하세요?

말씀 주신 부분이 조금 이해되지 않습니다. ! 😢

버스트라면 딜러의 점수와 상관 없이 LOSS 때문에 위와 같이 구성했습니다!

score 가 어떤 상태인지 (버스트라 0점인지 여부 등은) judgeVictory()가 있는 Player는 는 몰라도 된다고 생각했습니다. score 비교 여부는 game score 로 비교하기 때문에 game score 의 대소로만 승패를 겨루고, 단, 버스트일 때는 무조건 패야. 이렇게 구성한 로직입니다!

리뷰어님이 말씀하신 것이 버스트의 유무 자체가 HandScore 가 아니라 judgeVictory() 의 책임이라고 말씀주신 것이 맞으실까요?

그렇게 이해했을 때 역시 맞는 의견 같아서(HandScore에서는 가진 카드의 점수만 책임지도록)

BlackJackJudge 로 해당 책임을 옮겼습니다. !

블랙잭 여부도 이에서 판단하도록 옮겼습니다!

옮기고 보니 훨씬 책임이 명확해진 것 같습니다. 👍 👍

혹시 잘못이해했다면 말씀주세요! 00f52f0 에서 반영했습니다!

프로퍼티의 getter, setter에는 특별한 비즈니스 로직이 없는 것이 다른 개발자가 보고 유지보수를 진행하는데 도움이 됩니다.

👍 👍 👍 간단하게만 쓰려고 했었는데 많이 배워갑니다!

네 변경해주신 내용이 의도한 바가 맞습니다.

(반영하지 않으셔도 됩니다.) 다만 BUST_SCORE가 0이 된다라는 부분은 개인적으로는 지양하는 로직인데요. 사용하면 안되는 케이스는 아니지만 "0"이라는 숫자의 값이 담기는 의미는 빈 값이기도 하고 BUST_SCORE라는 점수가 실제에서 나타냈을 때 점수로 표현하지 못하는 값이에요. 따라서 이런 실제로 표현하지 못하는 값을 임의로 0 혹은 -1 등과 같이 처리하여 계산할 수도 있지만 이게 왜 0 혹은 -1이지를 한번 더 검토해야하는 문제가 발생할 수 있습니다.

그래서 개인적으로는 없는 값 혹은 특정 비즈니스에 대한 값으로 0, -1 과 같은 처리는 최대한 지양하려는 편이고 현재 케이스였다면 dealer가 버스트인 when 케이스를 추가하게끔할 것 같아요.

실제 점수가 아니기 때문에 BUST_SCORE 사용을 지양하는 피드백을 주신 거였군요
BlackJackJudge 내부에서만 사용되는 로직이라 판단용 점수라고 생각했는데 오히려 이것이 다른 사람이 읽기에는 어렵다는 것을 알 수 있었습니다
주신 피드백 대로 수정했더니 이해가 쉬어진 것 같습니다! 👍

jinyoungchoi95 · 2023-12-04T08:33:24Z

src/main/kotlin/blackjack/domain/result/game/PlayerGameResult.kt

+import blackjack.domain.player.PlayerName
+
+data class PlayerGameResult(
+    val player: Player,


Suggested change

val player: Player,

private val player: Player,

막아두어도 좋을 것 같네요 :)

780067a 에서 반영했습니다 : )

jinyoungchoi95 · 2023-12-04T08:34:50Z

src/main/kotlin/blackjack/view/dto/DealerCardsResultDto.kt

+
+import blackjack.domain.card.Card
+
+data class DealerCardsResultDto(


만들어주신 클래스의 종류에는 Result와 Dto가 있는데요. 보경님이 생각하시는 Result와 Dto의 역할은 각각 무엇인가요?

Result 는 각 로직이 실행 된 결과를 반환합니다.

발생한 변화를 반영한 이벤트라고 생각했습니다.

그리서 사실 ResultHandler 를 EventHandler 로 생각했습니다

해당 이벤트는 지금은 View 만 받고 있지만 다른 도메인 로직에서도 이를 받아 처리할 수 있을 것 같습니다.

Dto 는 계층간 데이터를 전달하는 목적으로 사용했습니다.

(Card 가 들어가긴 하지만 😭 ) 최소한의 정보만 view 에서 볼 수 있도록 사용했습니다!

jinyoungchoi95 · 2023-12-04T08:37:15Z

src/test/kotlin/blackjack/domain/betting/AmountTest.kt

+import java.math.BigDecimal
+
+class AmountTest : StringSpec({
+    "0보다 큰 베팅 금액이 생성된다" {


require(amount > 0) { "베팅 금액은 0보다 커야 합니다" }

"0보다 큰 베팅 금액이 생성된다"를 만족시키려면 해당 validation은 팩토리메소드보다는 생성자에 위치하는게 더 좋겠네요 :)

Amount 는 베팅 금액이 아니라 돈에 대한 포괄적인 개념으로 사용됩니다. 금액 자체는 0 이 될 수 있지만 betAmout 는 0이 될 수 없어 팩토리 메서드로 작성했습니다!

jinyoungchoi95 · 2023-12-04T08:42:29Z

src/test/kotlin/blackjack/domain/betting/BetBoardTest.kt

+import java.math.BigDecimal
+
+class BetBoardTest : DescribeSpec({
+    describe("BettingBoard") {


DCI 패턴에 대한 가독성(다른 nested 케이스에 대해서도 비슷할 것 같아요)에 대해서 이야기하자면, D, C, I 각각을 통해서 무엇을 전달하느냐도 중요하지만 모두 이어놓고 보았을 때의 가독성도 해당 패턴을 사용하는데 도움이 되어요.

테스트를 모두 진행하게되었을 때 nested 테스트는 위에서부터 D, C, I가 디스플레이되는데요. 이 결과를 통해 이어보면

플레이어 이름으로 베팅 금액 등록 (kim: 3_000원, lee: 4_000원) 플레이어 kim 의 베팅 금액은 3_000

이 됩니다. 말로 표현하기에는 조금 어색한 면이 있어 이를 이어보았을 때 테스트도 하나의 문서처럼 보이게 하려면

context("플레이어 이름으로 베팅 금액 등록 (kim: 3_000원, lee: 4_000원)되면 ") { it("플레이어 kim 의 베팅 금액은 3_000이 된다.") { }

플레이어 이름으로 베팅 금액 등록 (kim: 3_000원, lee: 4_000원)되면 플레이어 kim 의 베팅 금액은 3_000이 된다.

로 결과물에서 테스트 자체만으로도 문서가 되는 효과를 만들어낼 수 있습니다.

이번 미션에서 테스트 이름에 대해 많이 배워가는 것 같습니다.
적용해보니 훨씬 명확해지네요! 감사합니다

jinyoungchoi95 · 2023-12-04T08:43:42Z

src/main/kotlin/blackjack/domain/batting/PlayerBet.kt

+import blackjack.domain.player.PlayerName
+import blackjack.domain.result.game.Profit
+
+sealed interface PlayerBet {


sealed 👍

보경님이 생각하실 때 sealed는 언제 사용하는게 좋은 것 같나요?

저는 주로 상속을 제한하고 싶을 때 사용하고 있습니다

PlayerBet 에서와 같이 같은 도메인이지만 상태에 따라 조금씩 필드가 달라져 상태별로 다른 클래스로 묶어야 할 때 사용하고 있는데요

다른 곳에서 PlayerBet 의 상속을 막기 위해서 사용했습니다.

bong6981 · 2023-12-06T15:40:37Z

리뷰어님! 해당 리뷰 반영했습니다!

jinyoungchoi95

안녕하세요 보경님 😄

피드백 잘 반영해주셨네요. 관련해서 몇가지 코멘트 남겨두었으니 확인부탁드려요.

이만 머지하고자하는데 궁금하신 사항이 있으실까 하여 rc 한번 드립니다. 그리고 블랙잭 미션의 블랙잭 피드백을 확인해보시면 BlackJack hand에 따른 상태패턴을 사용하는 방법에 대한 피드백이 적혀져있는데요. 읽어보시고 괜찮으시다면 해당 부분도 진행해보면 좋을 것 같습니다.

궁금하거나 고민이 되는 부분이 있으시다면 언제든 pr 코멘트 또는 dm으로 요청 부탁드립니다.
감사합니다 🙇‍♂️

jinyoungchoi95 · 2023-12-14T15:16:46Z

src/main/kotlin/blackjack/controller/ViewInputProcessor.kt

        return toPlayerAction(action)
    }

+    private fun Player.nameDto(): PlayerNameDto = this.let(PlayerNameDto::from)


let에 생성자 제가 조금 설명을 러프하게 드렸었는데요. 생성자를 사용해도 되는 것에 대해서 굳이 let을 사용하지 않는게 좋을 것 같다라는 것이 제 의견이었습니다.

this.let(PlayerNameDto::from) PlayerNameDto.from(this)

에서 읽히는 과정을 본다면 let은 this를 let으로 처리하는데 이때 from을 사용하여 생성한다가 될 것이고, 아래는 this를 from을 사용하여 생성한다로 읽히게 되어요. 결국 let은 추가적인 재처리를 위한 용도로 사용되는데 생성자를 재처리하는 것이 로직적인 문제는 없으나 의미가 없는 tailing 구문이 발생하는 것과 다름이 없으므로 큰 복잡한 로직 없이 생성을 한다면 let을 제거하는 것이 좋다라는 생각이에요.

음.. 제가 아직 명확하게 이해하지 못한 것 같습니다.

말씀주신 예시에서는 let 을 쓰지 않는 것이 더 가독성도 좋아 보입니다.

만약, 아래 예시에서는

InputParser.intoPlayerNames(readInput())

readInput().let(InputParser::intoPlayerNames)

아래 구문이 개인적으로는 가독성이 더 좋아보입니다. 이 역시 불필요한 tailing 구문으로 볼 수 있긴 하겠지만, 왼쪽에서 오른쪽으로 읽을 경우 input 을 읽어와서 파싱한다로 좀 더 자연스럽지 않나 라는 생각이 드는데요.

혹시 이런 경우에도 let 사용을 지양하시는 것일까요?

return when { ranks.hasAce().not() -> score score + ACE_BONUS_SCORE > MAX_SCORE -> score else -> score + ACE_BONUS_SCORE }.let(::HandScore)

이런 케이스에서도 HandScore 을 생성하다는 로직 보다는 어떻게 점수 계산이 되는지 when { } 안의 로직이 먼저 읽혀야 한다고 생각해서 여기는 let 사용이 낫지 않나 라는 생각이 듭니다!

그러나 위 2 케이스 모두 리뷰어님이 말씀주신 큰 복잡한 로직 없이 생성하는 로직인 것 같습니다. 이에 대해서도 같은 생각이신지 궁금합니다!

jinyoungchoi95 · 2023-12-14T15:18:33Z

src/main/kotlin/blackjack/controller/ViewResultProcessor.kt

+        val dealerDto =
+            DealerCardsResultDto(result.dealerHand.cards, result.dealerScore.value)
+        val playersDto = result.playerResults.map {
+            PlayerCardsResultDto(it.name.value, it.hand.cards, it.score.value)
+        }


Suggested change

val dealerDto =

DealerCardsResultDto(result.dealerHand.cards, result.dealerScore.value)

val playersDto = result.playerResults.map {

PlayerCardsResultDto(it.name.value, it.hand.cards, it.score.value)

}

val dealerDto = DealerCardsResultDto(result.dealerHand.cards, result.dealerScore.value)

val playersDto = result.playerResults

.map { PlayerCardsResultDto(it.name.value, it.hand.cards, it.score.value) }

사소한 의견이지만, 개인적으로 이렇게 처리하는 것이 가독성이 더 좋을 것 같네요.

이를 반영했습니다!

jinyoungchoi95 · 2023-12-14T15:23:12Z

src/main/kotlin/blackjack/domain/BlackJackJudge.kt

+object BlackJackJudge {
+    private const val BUST_SCORE = 0
+    private const val BLACK_JACK_CARD_COUNT = 2
+    private const val BLACK_JACK_SCORE = 21


21이라는 비즈니스 상수값이 BlackJackJudge와 HandScore에 존재하고 있어요. 21을 이렇게 분산되어서 관리하면 하나의 클래스가 변경하여야할 때 다른쪽 클래스도 변경하는 것에 대해서 의식하지 못할 수도 있을 것 같은데 한군데서 관리할 수 있도록 하는 방법은 없을까요?

제가 도메인에 대한 지식이 부족해서 발생한 코드 같습니다

21점 이상을 가질 수 없다 / 블랙잭이라면 점수 21점을 얻는다. 이렇게 아예 다르게 인식했고, 그에 따라 각기 다른 클래스에 두게 되었습니다

BlackJackJudge 가 HansScore 의 MAX_SCORE 을 참조할 수도 있겠지만 이는 다른 성격이라 생각이 드는데용
어떻게 생각하시는지 궁금합니다!

bong6981 · 2023-12-31T08:36:10Z

안녕하세요 리뷰어님
피드백 반영이 늦었습니다 🙏
주신 피드백 반영했으며 궁금한 점 코맨크 남겨두었습니다.
상태 패턴 적용은 시도해보았지만 점점 커져 현재 프로젝트 구조를 크게 바꾸게 되어 그냥 두었습니다.
감사합니다!

bong6981 added 9 commits December 2, 2023 17:01

[step4] docs: 기능 목록 정리

ed22ad5

[step4] feat: 21점이면 더이상 카드를 받지 못하도록 변경

ea2d901

[step3] chore: getDesiredAction -> desiredAction 네이밍 변경

d095226

[step4] feat: 플레이어 이름을 식별자로 사용

b1ebc8a

[step4] feat: 플레이어별 베팅 금액 저장

158af44

[step4] feat: 베팅 금액에 따른 수익 계산 및 출력

c8eca31

[step3] refactor: View를 보여줄지 말지는 ViewResultProcessor에서 처리

3d30c05

[step3] refactor: requireNotNul()을 활용

5181cef

[step3] refactor: step3 피드백 반영

fc1d9ae

jinyoungchoi95 requested changes Dec 4, 2023

View reviewed changes

bong6981 added 5 commits December 6, 2023 22:25

[step4] chore: 생성 로직을 앞단으로 이동

a4db3db

[step4] feat: PlayerGameResult 의 player 접근제어자 -> private

780067a

[step4] test: DCI 패턴을 고려한 테스트 이름 수정

d95162a

[step4] feat: 게임 승패는 BlackJackJudge에서 판단하도록 변경

00f52f0

[step4] feat: 블랙잭 여부는 BlackJackJudge에서 판단하도록 변경

8bbefe0

bong6981 requested a review from jinyoungchoi95 December 6, 2023 15:40

[step4] chore: BUST_THRESHOLD 대신 BLACK_JACK_SCORE 사사용

f96543d

jinyoungchoi95 requested changes Dec 14, 2023

View reviewed changes

bong6981 added 4 commits December 31, 2023 13:33

[step4] feat: BetAmount 분리

ebae7ee

[step4] chore: 가독성을 위한 코드 줄바꿈 변경

c66f8bf

[step4] chore: 불필요한 let 사용 삭제

02e8ce0

[step4] refactor: 의미 없는 BUST_SCORE 사용 삭제

25c8735

		@@ -9,45 +9,87 @@
		- git의 commit 단위는 앞 단계에서 README.md 파일에 정리한 기능 목록 단위로 추가한다.

		### 기능 목록

	operator fun plus(other: Amount): Amount = value.plus(other.value).let(::Amount)
	operator fun plus(other: Amount): Amount = Amount(value.plus(other.value))

		private const val BUST_THRESHOLD = 21
		private const val BLACK_JACK_SCORE = 21


		import blackjack.domain.card.Card

		data class DealerCardsResultDto(

[Step4] 4단계 - 블랙잭(베팅) #697

Are you sure you want to change the base?

[Step4] 4단계 - 블랙잭(베팅) #697

Conversation

bong6981 commented Dec 3, 2023

jinyoungchoi95 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

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

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

bong6981 commented Dec 6, 2023

jinyoungchoi95 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

bong6981 Dec 31, 2023 • edited

Choose a reason for hiding this comment

bong6981 commented Dec 31, 2023

bong6981 Dec 31, 2023 •

edited