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

[부나] 2단계 블랙잭 제출합니다. #51

Merged
merged 54 commits into from
Mar 13, 2023

Conversation

tmdgh1592
Copy link
Member

@tmdgh1592 tmdgh1592 commented Mar 11, 2023

라코데브님, 안녕하세요!
너무나도 면밀하게 작성해주신 1단계 피드백 덕분에 정말 많이 성장할 수 있었습니다.
찾아뵙고 인사드리고 싶을 정도로 감동받았습니다..! 🥹

이번 2단계 미션을 진행하면서 카드의 상태에 집중하라는 리뷰어님의 피드백을 생각해보았습니다.
그리고 수업시간에 다룬 State 패턴과 Template method 패턴을 적용해보았습니다.
올바르지 않은 구현일 수 있지만, 위 패턴을 적용하면서 수익을 계산하는 코드가 상태별로 분리되어 있어서 코드를 수정할 때 수월했습니다!

혹시나..... 라코데브님께서 괜찮으시다면 더 채찍질해주셔도 좋을 것 같습니다..!
부족한 부분에 대해 많이 배울 수 있어서 리뷰 하나하나가 소중하게 느껴집니다 😃

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link

@laco-dev laco-dev 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 13 to 15
override fun nextStateCondition(): Boolean = cards.isBust

override fun nextState(): CardState = BustState(cards)

Choose a reason for hiding this comment

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

히트 상태에서는 스테이 혹은 버스트로 넘어갈 수 있지 않을까요?
다음 상태가 무조건 버스트가 되는 것이 맞을까요?

다음 상태라는 추상 함수로 정의하는 것이 맞을까? 를 생각 해보세요

Copy link
Member Author

Choose a reason for hiding this comment

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

제가 잘 이해한지 모르겠습니다만..🥲

히트 상태에서는 스테이 혹은 버스트로 넘어갈 수 있지 않을까요?

우선 히트 상태에서는 스테이와 버스트가 될 수 있습니다.
플레이어라면 n을 입력했을 때, 딜러라면 17점 이상일 때 스테이가 될 수 있습니다.
따라서 각 Participant의 draw()를 호출했을 때 canDraw()가 거짓이라면 Stay되도록 하였습니다.

다음 상태가 무조건 버스트가 되는 것이 맞을까요?

따라서 카드의 점수가 21점을 초과했다면 BustState를 반환하고, 그것이 아니라면 HitState를 반환하도록 수정하였습니다.

다음 상태라는 추상 함수로 정의하는 것이 맞을까? 를 생각 해보세요

다음 상태를 추상 함수로 정의하기보다 draw()에서 카드를 뽑았을 때의 상태를 반환하도록 하는 것이 적합해보입니다.

@tmdgh1592
Copy link
Member Author

저의 만행으로 인해 코드가 많이 변경되었을텐데.. 다시금 리뷰해주셔서 정말 감사드립니다..!! 🥹
작성해주신 피드백을 기반으로 부족한 부분을 보완해보았습니다!
감사합니다 : )

Copy link

@laco-dev laco-dev left a comment

Choose a reason for hiding this comment

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

피드백 반영 잘 해주셨습니다. 👍🏻
코드가 눈에띄게 좋아지는 모습이 좋았습니다.

마지막 단계인 만큼 조금 더 개선해보면 좋을만한 내용을 남겨보았습니다.

Comment on lines 14 to 30
Blackjack(deck = CardDeck(), players = InputView.inputPlayers()).start(object : BlackjackEventListener {
override fun onStartDrawn(participants: Participants) {
OutputView.printFirstDrawnMessage(participants)
}

override fun onFirstDrawn(participant: Participant) {
OutputView.printFirstOpenCards(participant)
}

override fun onDrawnMore(participant: Participant) {
OutputView.printAllCards(participant)
}

override fun onEndGame(gameResults: List<GameResult>) {
OutputView.printBlackjackResult(gameResults)
}
})

Choose a reason for hiding this comment

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

리스너로 잘 묶어 주셨네요.
OutputView에서 Listener 자체를 반환하도록 하면 조금 더 마법이 생겨납니다

Blackjack(deck, InputView::inputPlayers, OutputView::eventListener)

Copy link
Member Author

@tmdgh1592 tmdgh1592 Mar 13, 2023

Choose a reason for hiding this comment

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

오오.. 이런 마법이..!
이 코드를 참고해서 OutputView에서 BlackjackEventListener를 상속하도록하여 OutputView 자체를 넘겨주는 방식으로 해보았습니다!

object OutputView : BlackjackEventListener {
    . 
    .
    .
}

Blackjack(CardDeck(), InputView.inputPlayers(), OutputView).start()

Comment on lines 3 to 4
class Cards(private val _items: MutableList<Card> = mutableListOf()) {
constructor(vararg cards: Card) : this(cards.toMutableList())

Choose a reason for hiding this comment

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

가변 컬렉션 타입을 바로 받는 대신 방어적 복사를 활용하면 좋을 것 같습니다.
여전히 외부에서 items를 변경시킬 수 있습니다.

그런데 add()함수가 불변 객체를 지향하기 때문에, 다른 상태를 변경하는것도 모두 필요없지 않을까? 라는 생각도 듭니다

Copy link
Member Author

Choose a reason for hiding this comment

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

val cardItems = mutableListOf<Card>(Card(), Card() ...)
val cards = Cards(cardItems)
cardItems.add(Card()) // 불변 X

위와 같은 상황이 발생할 수 있겠군요!
제가 올바르지 않은 일급 컬렉션의 예시를 하나 추가해버렸군요..😅

라코데브님 말씀처럼 add() 함수가 불변 객체를 지향하므로 아래와 같이 변경하였습니다!
class Cards(val items: List<Card> = listOf())

Comment on lines 57 to 63
@Test
fun `배팅 금액을 곱했을 때 값이 음수일 수 없다`() {
assertThrows<IllegalArgumentException> {
BetMoney(1000) * -2.0
}
}

Choose a reason for hiding this comment

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

결국 BetMoney 타입이기 때문에 곱한 것과 상관 없이 이렇게 작성하는게 맞겠네요.

fun `배팅 금액은 음수일 수 없다`() { ... }

Copy link
Member Author

Choose a reason for hiding this comment

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

배팅 금액은 결국 음수일 수 없기 때문에 곱하는 것과 무관하게 제안해주신 테스트 코드명이 올바른 것 같습니다!
바로잡아주셔서 감사합니다!


/**
플레이어 손익의 반대이다.
*/
override fun getProfit(other: Participant): Money = -other.getProfit(this)
override fun getProfit(others: List<Participant>): Money {
val players = others.filterIsInstance<Player>()

Choose a reason for hiding this comment

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

하나의 마이너한 팁이라면 딜러가 플레이어 타입을 직접 아는 것 보다는, 반대로
"딜러가 아닌"으로 조건을 바꿔보는 것도 방법입니다.
그럼 다른 객체와의 의존 관계가 사라집니다

others.filterNot { it is Dealer }

Copy link
Member Author

Choose a reason for hiding this comment

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

정말 감동적인 코드입니다.. 🫶
기존 코드는 클래스간의 의존성이 생기는군요
Player 또한 아래와 같이 변경하였습니다!
val dealer = others.first { it !is Player }

override fun getProfit(other: Participant): Money = -other.getProfit(this)
override fun getProfit(others: List<Participant>): Money {
val players = others.filterIsInstance<Player>()
return -Money(players.sumOf { it.getProfit(listOf(this)).getAmount() })

Choose a reason for hiding this comment

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

players를 만드는 메소드에 체이닝을 이용해서 만들어 볼 수 있지 않을까요?

val playerTotalProfit = others.xxx
   .sumOf { ... }
reteurn Money(...)

Copy link
Member Author

Choose a reason for hiding this comment

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

감사합니다!
아래와 같이 메소드를 변경해보았습니다!

override fun getProfit(others: List<Participant>): Money = Money(others
        .filterNot { it is Dealer }
        .sumOf { player -> player.getProfit(listOf(this)).getAmount() }
)

Player도 함께 변경해볼까 했지만
dealer를 변수에 할당해야 가독성이 높아 보여서 Dealer.class만 적용하였습니다.

abstract fun getFirstOpenCards(): List<Card>

abstract fun draw(card: Card): Participant
abstract fun draw(card: Card, justDraw: Boolean = false): Participant

Choose a reason for hiding this comment

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

justDraw()라는 값이 왜 필요해졌는지 잘 모르겠어요.

Copy link
Member Author

Choose a reason for hiding this comment

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

Player의 경우 draw() 메서드 호출시 canDraw()를 호출합니다.
이 때 Player의 canDraw()에는 사용자에게 카드를 더 뽑을 것인지 묻는 needToDraw() 함수를 호출합니다.
따라서 최초에 카드를 2장 뽑을 때에도 의도치 않게 needToDraw() 함수를 호출하는 문제가 발생합니다.
그래서 처음 카드는 무조건 뽑을 수 있는 조건을 추가해주기 위해 justDraw를 추가하였습니다.

- Dealer는 Dealer가 아닌 참여자들을 탐색하도록 변경
- Player는 Player가 아닌 참여자를 한명 탐색하도록 변경
Copy link

@laco-dev laco-dev left a comment

Choose a reason for hiding this comment

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

블랙잭 미션동안 고생 많으셧습니다.

그동안 경험하신 것들이 무조건 맞다고 생각하기 보다는, 자기만의 이유를 만들어서 발전시켜 나가시면 좋겠습니다.

@laco-dev laco-dev merged commit 0290bc2 into woowacourse:tmdgh1592 Mar 13, 2023
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