Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(step2): 블랙잭 #692

Open
wants to merge 10 commits into
base: david-sang
Choose a base branch
from
Open

Conversation

david-sang
Copy link

강의를 듣고 전반적으로 수정해 봤는데 너무 오래 걸렸네요..
잘 부탁드립니다!

- 숫자 카드는 카드의 숫자를 점수로 계산한다
- Ace 카드는 1 또는 11로 점수 계산 가능하다
- King, Queen, Jack은 10으로 계산한다
- 입력한 이름을 기준으로 게임에 플레이어가 정해진다 (플레이어는 유일한 이름을 가진다)
- 플레이어는 최초에 카드 2장을 분배 받는다
- 카드 분배는 최초 한 번 이루어진다
- 보유한 카드의 점수 합이 블랙잭 기준치(21) 미만이면 추가로 카드를 받을 수 있는 상태이다
- 만들 수 있는 최종 점수가 모두 블랙잭 기준치(21)를 넘는다면 버스트이다과
- 버스트인 경우, 최종 점수는 모든 카드를 더해서 만들 수 있는 가장 작은 수이다
- 버스트가 아닌 경우, 최종 점수는 모든 카드를 더해서 블랙잭 기준치(21) 이하에서 만들 수 있는 가장 큰 수이다
- 최종 점수가 블랙잭 기준치(21)이고 가진 카드가 2장이면 블랙잭이다
Copy link

@namjackson namjackson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 건상님!
2단계 고생하셨습니다!
몇가지 코멘트 남겼으니, 확인해주세요!
변경사항 적용후, 리뷰요청해주시면 좋을거같아요!

Comment on lines +27 to +29
JACK(11),
QUEEN(12),
KING(13);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

number는 무엇을 의미하나요?
11,12,13 이라는 숫자는 사용하지 않을거 같아요!

Comment on lines +38 to +52
fun getPossibleScore(rank: Rank) = when (rank) {
Rank.ACE -> listOf(1, 11)
Rank.TWO -> listOf(2)
Rank.THREE -> listOf(3)
Rank.FOUR -> listOf(4)
Rank.FIVE -> listOf(5)
Rank.SIX -> listOf(6)
Rank.SEVEN -> listOf(7)
Rank.EIGHT -> listOf(8)
Rank.NINE -> listOf(9)
Rank.TEN -> listOf(10)
Rank.JACK -> listOf(10)
Rank.QUEEN -> listOf(10)
Rank.KING -> listOf(10)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Rank 별로 관리된다면, Rank에 score 변수를 추가하면 어떨까요?

  • ACE만을 위해, 굳이 list를 사용하는건 불필요하지 않을까요?
    Ace 계산을 위한 상수값을 활용하면 리스트를 활요앟지 않아도 될거같아요!

fun getPossibleScoreSums(score: Score): List<Score> = values.map { score + it }

companion object {
fun getPossibleScore(rank: Rank) = when (rank) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코틀린에서 리턴타입을 생략할수 있지만,
명시적으로 써주는것을 지향하면 좋을거 같아요!
리턴타입을 알기위해 구현부를 살펴봐야하고, 또 구현부의 코드에 따라 의도하지 않아도 리턴타입이 변경되기도해요!

Comment on lines +7 to +9
val game = controller.startGame()
repeat(game.players.values.size) { controller.progressPlayerPhase(game) }
controller.progressEndPhase(game)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

main함수의 코드를 최소화 해보면 어떨까요?
controller의 함수를 통해 리턴된 game을 다시 controller의 함수의 파라미터로 사용하는 등의 동작은
불필요한 책임 아닐까요? 모두 BlackjackController에게 위임해보면 어떨까요?

또한, 게임이 시작 된 이후, 플레이들이 카드를 더받고 등의 로직은 비즈니스로직이 아닐까요?

Comment on lines +13 to +14
OutputView.printGameInitialization(game)
OutputView.printPlayerAndCard(it)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OutputView의 함수를 하나하나 실행하기보다,
OutputView에게 게임 초기값을 그려주라고 메세지를 던저 위임하면 어떨까요?

OutputView가 텍스트가 아닌, UI로 변경된다면 Controller의 로직 또한 변경되야할까요?


fun getCardCount() = cards.size

fun getBestScore(): Score {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

블랙잭의 룰에서는 Ace는 21을 초과하지 않을때는 11, 21이 초과될떄는 1로 계산되고 있어요,
11, 1 모든 케이스를 고려하신 부분은 알겠지만, 불필요하게 로직이 복잡해졌다는 느낌이 드네요,
로직을 간소화해보면 어떨까요

자유롭게 참고해보셔도 좋을거같아요

간단하게 기본적으로 Ace를 1로 계산하고,
카드내에 Ace가 있다면 Ace를 11로 계산했을때 21을 넘지 않는다면 11로 계산하면 좋을거같단 생각이드네요
if (sum + 10 > Score.BLACKJACK) sum else sum + 10


val resultScore: Score by lazy { decideScore() }

fun initialize(deck: Deck) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deck을 파라미터로 받기보다, draw된 cards를 넘겨받는 구조는 어떨까요?
Player는 Card가 관심사지, Deck은 몰라도 될거같단 생각이 들어요!

Comment on lines +30 to +31
score.value < Rule.BLACKJACK_SCORE -> PlayerState.UNDER
score.value > Rule.BLACKJACK_SCORE -> PlayerState.BUST

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

score의 Value에 접근하기보단,
score에게 책임을 위임해보는건 어떨까요?

https://dkswnkk.tistory.com/687 참고해보아요!
ex> score.isBust

Comment on lines +3 to +7
data class Player(val name: String, val hand: Hand = Hand()) {
var state: PlayerState = PlayerState.READY
private set

val resultScore: Score by lazy { decideScore() }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resultScore 는 hand에 따라 상태가 관리되는 함수가 아닌가요?
게임중간에 의도하지 않은곳에서 resultScore가 호출된다면 어떤문제가 생길까요?
변수로 들고 있기보다, 함수로 만드는게 좋을거같아요

val resultScore: Score get() = decideScore()

state 또한 stay를 제외하고는 hand 상태에 따라 상태가 변경되는 구조네요,
hand가 변경될때마다 state가 변경되는 상태라면 불필요하지 않을까요?
오히려 의존적인 상태로 인해 객체가 더 복잡해지는거 같아요,
개인적으로는 state는 Finish되었는지에 대한 부분만 알고있어도 좋을거 같단생각이드네요!

Comment on lines +7 to +10
private val phases: List<Phase>
private var phaseIndex: Int = 0
private val phase
get() = phases[phaseIndex]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phase를 구분하신 이유가 있을까요?
개인적으로는 일반적인 for문을 사용하는 것과 크게 다르진 않을거같단 생각이 드는거 같아요 ㅠㅠ!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants