[step4] 블랙잭 (베팅) & step3 코드리뷰 반영#812
[step4] 블랙잭 (베팅) & step3 코드리뷰 반영#812SaraHan774 wants to merge 32 commits intonext-step:sarahan774from
Conversation
…트를 PlayerToResultMap에 메시지로 요청하도록 함
…kJackResultManager 의 생성자로 주입하지 않도록 수정.
pci2676
left a comment
pci2676
left a comment
There was a problem hiding this comment.
조금만 더 고민해보면 좋을 부분에 리뷰를 남겨두었으니 확인해주세요~
| private fun getBetMoneyFromGameResult(gameResult: GameResult): BigDecimal { | ||
| return when (gameResult) { | ||
| GameResult.BLACK_JACK -> betMoney.getAmountOnBlackJack() | ||
| GameResult.WIN -> betMoney.getOriginalBetAmount() | ||
| GameResult.PUSH -> betMoney.getOriginalBetAmount() | ||
| GameResult.LOSE -> betMoney.getAmountOnLose() | ||
| GameResult.BUST -> betMoney.getAmountOnBust() | ||
| } | ||
| } |
There was a problem hiding this comment.
이 부분은 GameResult에 따라 betMoney를 어떻게 다루어야하는지에 대한 전략을 다루는것 같습니다.
이 부분을 GameResult에게 위임해보면 어떨까요?
| val isBlackJackInitially: Boolean | ||
|
|
||
| init { | ||
| repeat(initialCardCount) { addCard(drawCard()) } | ||
| repeat(INITIAL_CARD_COUNT) { addCard(drawCard()) } | ||
| isBlackJackInitially = cardsSum == Card.MAX_SUM |
There was a problem hiding this comment.
여러가지 상태를 관리하는것 같습니다.
Cards를 가지고 점수를 계산하고 이러한 상태가 관리되는것 같은데요
디자인 패턴중 상태패턴을 이용한다면 이러한 부분을 개선해볼 수 있을것 같습니다.
반드시 개선을 바라는 부분은 아니니 참고만 해주셔도 좋습니다. 😄
There was a problem hiding this comment.
블랙잭 피드백에 나와있는 내용이군요!
이거 근데 현재 구조에서 적용하려면 코드를 아예 새로 짜야 할 것 같아서
별도로 한번 적용 해보는 연습을 해봐야 할 것 같습니다.
There was a problem hiding this comment.
제가 드리는 리뷰를 반드시 반영할 필요는 없으니 참고만 해주셔도 됩니다~
| fun setProfitMoneyFromGameResult(result: GameResult) { | ||
| val betMoney = getBetMoneyFromGameResult(result) | ||
| profitMoney.set(betMoney) | ||
| } |
There was a problem hiding this comment.
이러한 상태를 변경하는 메서드는 설계하기 굉장히 까다로운 부분인것 같습니다.
가희님은 이 메서드를 의도와 시기에 맞게 잘 호출하여 사용할 수 있지만
다른 개발자는 이 메서드를 적절한 시기에 올바르게 호출하여 사용할 수 있도록 보장할 수 있을까요?
다른 방법으로 result만 구해낸다면 언제 어떻게든 호출할 수 있지 않을까요?
이를 방어할 수 있다면 더욱 견고해질 수 있을것 같습니다.
There was a problem hiding this comment.
안그래도 암시적으로 상태 변경하는 부분이 걸렸는데,
이거 좀 더 고민해보겠습니다
There was a problem hiding this comment.
profitMoney 를 Player private 하게 가져가도록 하고 setter 함수를 제거하는 방향으로 수정했습니다.
| package blackjack.domain | ||
|
|
||
| class PlayerResultCalculator { | ||
| @Deprecated("deprecated - step3 에서만 사용됨") |
| class ProfitMoney { | ||
| private var current: BigDecimal = BigDecimal.ZERO | ||
|
|
||
| fun getCurrentProfit(): BigDecimal { | ||
| return current | ||
| } | ||
|
|
||
| fun set(amount: BigDecimal) { | ||
| current = amount | ||
| } | ||
| } |
There was a problem hiding this comment.
kotlin을 사용하신다면 가능한 불변한 객체를 설계하여 사용하시는것을 권장드립니다~
https://kotlinlang.org/docs/coding-conventions.html#idiomatic-use-of-language-features
| isPlayerBlackJackInitially: Boolean, | ||
| isDealerBlackJackInitially: Boolean, | ||
| dealerCardSum: Int, | ||
| playerCardSum: Int, |
There was a problem hiding this comment.
4개의 파라미터를 다루어야 하는 부분은 부담으로 다가올 수 있을것 같습니다.
4개의 파라미터가 타입도 2개씩 동일하여 순서를 잘못 지켜 주입한다면 의도치 않게 동작할 수도 있을것 같네요!
이러한 값을 추상화하여 표현해볼 수 없을지 한번 고민해보시면 좋을것 같습니다.
| BLACK_JACK -> getAmountOnBlackJack() | ||
| WIN -> getOriginalBetAmount() | ||
| PUSH -> getOriginalBetAmount() | ||
| LOSE -> getAmountOnLose() | ||
| BUST -> getAmountOnBust() |
There was a problem hiding this comment.
이러한 연관관계를 GameResult의 멤버변수로 표현해보는 방향은 어떻게 생각하시나요?
GameResult(val amountStrategy: (BetMoney -> BigDecimal))| val isBlackJackInitially: Boolean | ||
|
|
||
| init { | ||
| repeat(initialCardCount) { addCard(drawCard()) } | ||
| repeat(INITIAL_CARD_COUNT) { addCard(drawCard()) } | ||
| isBlackJackInitially = cardsSum == Card.MAX_SUM |
There was a problem hiding this comment.
제가 드리는 리뷰를 반드시 반영할 필요는 없으니 참고만 해주셔도 됩니다~
2 participants
리뷰 감사드립니다.