[Step4] : 🚀 4단계 - 블랙잭(베팅) by dlwnsgus777 · Pull Request #703 · next-step/kotlin-blackjack

dlwnsgus777 · 2023-12-07T14:58:03Z

안녕하세요 리뷰어님!!
강의 종료 후에도 리뷰해주신다고 하시니... 그저 감사할따름입니다ㅠㅠㅠ 👍

완주를 목표로 달려보겠습니다!!!

이번 단계도 잘부탁드립니다!!

고민한 부분

한참 고민하다가 GameResult에서 하던 결과 계산은 Dealer에게 위임해 결과를 계산하니 훨씬 수월함을 느꼈습니다!
그러나 객체 간의 책임? 역할이 잘 나누어지지 않은 것 같습니다...
하지만 어디서부터 손대나가야할지 감이 서지 않아 리뷰의 힘을 빌려보려 pr드려봅니다ㅎㅎ

wisemuji

안녕하세요 준현님!

구현을 잘 해주셨습니다 💯 깔끔한 구현이 좋았습니다.
테스트 케이스들이 직관적으로 변한 모습이 인상적이네요 👍
고민해보시면 좋을만한 코멘트를 달았으니 확인해주시면 감사하겠습니다.

기한이 지났는데도 미션 진행하시는 모습이 대단하십니다!
궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요! 🙏 💪

wisemuji · 2023-12-09T04:11:16Z

src/main/kotlin/blackjack/GameManager.kt

다음 코멘트에 대해 고민해보세요 🙂
#656 (comment)

wisemuji · 2023-12-09T04:13:21Z

src/main/kotlin/blackjack/GameResult.kt

+        val playerResult = mutableMapOf<Name, BettingAmount>()

-        val (winCount, loseCount) = when {
-            dealer.isBust -> players.partition { !it.isBust }
-            else -> players.partition { it.resultScore() < dealer.resultScore() }
+        players.forEach {
+            val result = dealer.matchingScore(it)
+            playerResult[it.name] = it.status.calculateBettingAmount(result, it.bettingAmount)
        }


mutableMapOf 사용 없이 players.map { ... } 만으로도 구현할 수 있습니다!

wisemuji · 2023-12-09T04:14:55Z

src/main/kotlin/blackjack/GameResult.kt

-    companion object {
-        private const val WIN: String = "승"
-        private const val LOSE: String = "패"
+        resultMap = mapOf(Name("딜러") to dealer.bettingAmount) + playerResult


딜러의 이름은 출력하는 입장에서 꼭 필요한 요소일까요? 🤔
GameResult를 통해 플레이어와 딜러의 공통된 결과를 조회할 수 있게 되었지만 불필요한 딜러의 Name 객체를 생성하게 되었어요.

개발자들마다 의견이 갈릴 수 있을 것 같은데 준현님은 어떻게 생각하시나요?

wisemuji · 2023-12-09T04:16:08Z

src/main/kotlin/blackjack/participant/BettingAmount.kt

+import kotlin.math.roundToInt
+
+@JvmInline
+value class BettingAmount(


value class 활용 👍
operator overloading도 적용해보면 어떨까요?
https://kotlinlang.org/docs/operator-overloading.html

wisemuji · 2023-12-09T04:18:50Z

src/main/kotlin/blackjack/participant/BlackjackStrategy.kt

+    private fun isBlackjack(): Boolean {
+        return resultScore() == BLACKJACK
+    }


isBust와 isBlackjack()은 각각 프로퍼티, 함수로 선언되었네요.

val isBust get() = resultScore() > BLACKJACK

어떤 상황에서 프로퍼티를 선호하고 함수를 선호해야 할까요?
다음 글이 도움되었으면 좋겠어요.

https://medium.com/smartherd/kotlin-functions-vs-properties-when-to-use-what-6d1f9b71a812

wisemuji · 2023-12-09T04:23:48Z

src/main/kotlin/blackjack/participant/Dealer.kt

-)  {
+) {
    private val blackjackStrategy: BlackjackStrategy = BlackjackStrategy(scoreCalculator)
+    var bettingAmount: BettingAmount = BettingAmount(0)


matchingScore()에서 리턴할 때 BettingAmount 정보를 같이 반환한다면 굳이 variable을 선언하지 않아도 될 것 같아요

모든 엔티티를 작게 유지한다.
3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.

더불어 지금 딜러에서 하고 있는 일이 너무 많습니다.
matchingScore()과 같은 로직이 딜러에 있는게 적절할까요?
승/패 계산 및 배팅 금액 결과를 얻어오기 위해서 딜러에게 요청하는 것이 아닌, 별도의 객체에게 요청하는 형태로 바꿔보면 어떨까요?

wisemuji · 2023-12-09T04:25:31Z

src/main/kotlin/blackjack/participant/BlackjackStrategy.kt

+        changeStatus(cards)
    }

+    private fun changeStatus(cards: List<BlackJackCard>) {
+        when {
+            !isFirstTurn(cards) && !isBust -> status = Stand()
+            !isFirstTurn(cards) && isBust -> status = Bust()
+            isFirstTurn(cards) && isBlackjack() -> status = Blackjack()
+        }
+    }


마지막 강의를 참고하여 이러한 로직도 각각의 상태 객체에서 판단할 수 있도록 바꿔보면 어떨까요? (상태 패턴)
이 코멘트 반영은 선택사항으로 진행하셔도 됩니다 🙂

dlwnsgus777 · 2023-12-11T00:56:14Z

안녕하세요 리뷰어님!! 강의 지난 후에도 리뷰해주셔서 늘 감사한 마음입니다 🙏

피드백 반영하면서 궁금한 점이 생겨 몇가지 질문드려봅니다!!

딜러의 이름은 출력하는 입장에서 꼭 필요한 요소일까요? 🤔
GameResult를 통해 플레이어와 딜러의 공통된 결과를 조회할 수 있게 되었지만 불필요한 딜러의 Name 객체를 생성하게 되었어요.
개발자들마다 의견이 갈릴 수 있을 것 같은데 준현님은 어떻게 생각하시나요?

Name 객체처럼 원시값을 감싼 클래스를 사용하는 이유에 대해서 나름대로의 생각은 다음과 같습니다!

코드 가독성을 위해
이름에 대한 요구사항이 생겼을 때 책임을 Name이 가지고 있기 때문에 유지보수 측면에서 좋다 (Name 클래스만 수정하면 되기 때문)

또한 다른 Player 객체에서도 사용하고 있기도 하고 모든 원시값을 포장하라는 과제의 요구사항이기도 했기 때문에 불필요한 딜러의 Name 객체 를 생성하게 되더라도 괜찮다는 생각입니다!

다만 이와같은 의문은 코드의 모양 mapOf(Name("딜러") to dealer.bettingAmount) 이런 형식 때문에 생겨났다고 생각합니다

좀 더 가독성 있게 처리할 방법을 생각해보겠습니다!

고민한 부분

딜러에 대한 책임이 많아 분리를 하던 도중 GameResultGenerator 와 MatchingScoreCalculator 클래스가 파생되었는데
이 둘의 로직이 비슷한 부분이 많습니다... 단순히 return 값만 다르다는 느낌인데

이럴 때 어떤식으로 리팩터링할지 감이 잡히지 않아 질문드려봅니다!!

잘부탁드립니다!! 😢

wisemuji

안녕하세요 준현님! 리뷰 반영을 잘 해주셨습니다 💯

조금 고민해보시면 좋을 도전적인 내용을 코멘트로 달았으니
편하게 고민해보시고 마음껏 도전해보세요.

아마 다음 리뷰 요청에서는 머지할 수 있을 것 같아요.
마지막까지 화이팅입니다 💪

wisemuji · 2023-12-26T09:49:05Z

src/main/kotlin/blackjack/supoort/MatchingScoreCalculator.kt

+import blackjack.participant.status.Blackjack
+import blackjack.participant.status.Bust
+
+object MatchingScoreCalculator {


딜러에 대한 책임이 많아 분리를 하던 도중 GameResultGenerator 와 MatchingScoreCalculator 클래스가 파생되었는데
이 둘의 로직이 비슷한 부분이 많습니다... 단순히 return 값만 다르다는 느낌인데
이럴 때 어떤식으로 리팩터링할지 감이 잡히지 않아 질문드려봅니다!!

#703 (comment)
이 코멘트를 참고하여 상태 패턴으로 만들어보면 어떨까요? 지금도 Status 객체가 있지만 더 많은 책임을 상태로 위임할 수 있습니다.

어디서부터 패턴을 만들어야 할지 감이 잘 오지 않는다면 상태 구조를 처음부터 구현해보시는 것도 추천드려요 🙂 처음에는 어렵게 느껴지던 블랙잭 룰도 다시 만들어보면 생각보다 쉽게 구현될거예요.

wisemuji · 2023-12-26T09:50:12Z

src/main/kotlin/blackjack/supoort/MatchingScoreCalculator.kt

+        var bettingAmount = BettingAmount(0)
+        players.forEach {


Kotlin Collection의 fold, reduce 등의 API를 활용하면 이 로직도 var 선언 없이 불변으로 만들 수 있습니다.
이 코멘트는 참고만 해주셔도 좋습니다!

wisemuji · 2023-12-26T09:55:20Z

src/main/kotlin/blackjack/supoort/GameResult.kt

+import blackjack.participant.Name
+
+data class GameResult(
+    val resultMap: Map<Name, BettingAmount>


Name 객체처럼 원시값을 감싼 클래스를 사용하는 이유에 대해서 나름대로의 생각은 다음과 같습니다!

코드 가독성을 위해

이름에 대한 요구사항이 생겼을 때 책임을 Name이 가지고 있기 때문에 유지보수 측면에서 좋다 (Name 클래스만 수정하면 되기 때문)
또한 다른 Player 객체에서도 사용하고 있기도 하고 모든 원시값을 포장하라는 과제의 요구사항이기도 했기 때문에 불필요한 딜러의 Name 객체 를 생성하게 되더라도 괜찮다는 생각입니다!
다만 이와같은 의문은 코드의 모양 mapOf(Name("딜러") to dealer.bettingAmount) 이런 형식 때문에 생겨났다고 생각합니다
좀 더 가독성 있게 처리할 방법을 생각해보겠습니다!

준현님이 딜러와 플레이어의 관계를 상속이 아닌, 조합을 통해 구현해주신 만큼
이 둘을 같은 타입으로 구현하지 않을 수도 있지 않을까요?

예를 들어 다음과 같이요 🙂

Suggested change

val resultMap: Map<Name, BettingAmount>

data class GameResult(

val playerResultMap: Map<Name, BettingAmount>

val dealerResult: BettingAmount

또 만약 GameResult가 playerResultMap를 알고 있다면 굳이 dealerResult를 외부에서 계산해줄 필요 없이 GameResult가 자체적으로 판단할 수 있을 것 같아요. 즉, Generator 클래스의 필요 유무도 같이 고민해보시면 좋겠습니다.

wisemuji · 2023-12-26T09:57:43Z

src/main/kotlin/blackjack/GameManager.kt

+//    private fun playBlackJack(): GameResult {
+//        players.forEach {
+//            playerDraw(it)
+//        }
+//        dealerDraw(dealer)
+//
+//        return GameResult(players, dealer)
+//    }


사용하지 않는 함수는 삭제해주세요 🙂 VCS에 모두 남아있으니 코드를 삭제하는 것을 편하게 생각하셔도 됩니다.

wisemuji · 2023-12-26T09:59:17Z

src/main/kotlin/blackjack/GameManager.kt

+        players.forEach {
+            outputManager.printPlayerResultGame(it)
+        }


이러한 구문을 함수 reference로 넘길 수 있습니다.

Suggested change

players.forEach {

outputManager.printPlayerResultGame(it)

}

players.forEach(outputManager::printPlayerResultGame)

lambda로 작성하는 것과 어떤 차이가 있는지 참고할 수 있는 글을 소개해드릴게요 🙂
https://proandroiddev.com/kotlin-lambda-vs-method-reference-fdbd175f6845
참고로 저는 가능한 곳에서는 꼭 함수 reference를 활용하는 것을 지향합니다. lambda로 가져온 데이터를 별도로 조작하지 않고 그대로 함수 시그니처로 넘긴다는 것을 드러낼 수 있기 때문입니다.

dlwnsgus777 and others added 30 commits December 5, 2023 22:44

refactor: 출력 포맷 맞추기

585c554

feat: 점수 계산 버그 수정

60c5695

feat: ktlint

3fb5c38

feat: 메서드 명 수정

8deccc4

refactor: object 로 변경

81a9b87

test: 테스트 코드 추가

dbc4ac9

refactor: default arguments 사용

ba515fe

doc: 요구사항 정리

821edf8

feat: 베팅 금액 클래스 생성

99c2a82

feat: 클래스 이름 오타 수정

7596635

feat: Player가 베팅 금액을 가지도록 개발

f91a9bd

feat: 베팅 금액 입력 기능 추가

09a86f9

feat: 블랙잭 상태 추가

f9ffe93

feat: 히트 상태 추가

8debf0c

refactor: 코드 리팩터링

532524d

feat: 스탠드 상태 추가

f24fb80

feat: 버스트 상태 추가

9f18bcc

refactor: 코드 리팩터링

0742d5c

doc: README.md update

772fafc

feat: 다음 작업을 위한 중간저장...

d631fb5

feat: 점수 계산 로직 추가

dc12e12

feat: 테스트 코드 추가

85a1efb

feat: 딜러 객체가 플레이어 객체의 점수와 자신의 점수를 비교하도록 수정

3464f6a

feat: lint 작업 및 깨진 테스트 정리

243166d

feat: 베팅 금액 계산 로직 추가

ad1a372

feat: 린트 정리

e90f8a6

test: 깨진 테스트 정리

af98d6a

feat: 안쓰는 코드 정리

68ed8cb

feat: isBust를 Status를 사용하는 코드로 변경

46af015

feat: ktlint

353af0a

wisemuji requested changes Dec 9, 2023

View reviewed changes

dlwnsgus777 and others added 13 commits December 9, 2023 21:54

feat: mutableMapOf 사용 제거

cbf31f3

feat: isBust 함수로 변경

6a46496

feat: DSL과 operator 함수 사용

9caaa16

feat: DSL 추가

9eee8bb

feat: game package 생성

b662b36

feat: test 케이스 추가

c772fe8

feat: 게임결과 계산을 위한 GameResultGenerator 오브젝트 추가

a61b188

feat: ktlint

2ca702c

feat: 딜러의 점수 계산 로직을 분리하기 위해 새로운 클래스 추가

3693ab2

feat: 패키지 명 변경 game -> support

4ebdeb7

feat: ktlint

9067668

feat: MatchingScoreCalculator 테스트 케이스 추가 및 역할 분리

70b8d6c

feat: ktlint

cfc087f

wisemuji requested changes Dec 26, 2023

View reviewed changes

refactor: 코드 리뷰 반영

1edce57

Conversation

dlwnsgus777 commented Dec 7, 2023

고민한 부분

Uh oh!

wisemuji 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!

dlwnsgus777 commented Dec 11, 2023

고민한 부분

Uh oh!

wisemuji 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!

2 participants