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

[Step3] 블랙잭 (딜러) #485

Open
wants to merge 6 commits into
base: naemoo
Choose a base branch
from
Open

[Step3] 블랙잭 (딜러) #485

wants to merge 6 commits into from

Conversation

naemoo
Copy link

@naemoo naemoo commented Jun 25, 2023

안녕하세요 리뷰어님~! 😀
블랙잭 딜러 개발 후 리뷰 요청 보내드려요~! :)
이번 단계도 리뷰 잘 부탁드리겠습니다 🙇‍♂️

Copy link

@pci2676 pci2676 left a comment

Choose a reason for hiding this comment

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

개선해보면 좋을것 같은 부분에 코드리뷰를 남겨두었습니다.

확인해보시고 다시 PR요청 부탁드릴게요!

Comment on lines 58 to 69
private fun turn(deck: CardDeck, player: GamePlayer) {
while (player.isEligibleToHit() && InputView.isHit(player.name)) {
player.draw(deck)
val playerViewModel = GamePlayerConverter.convert(player)
OutputView.printPlayersCard(playerViewModel)
}

val playerViewModel = GamePlayerConverter.convert(player)
if (playerViewModel.cards.size == CardDeck.INIT_DRAW_SIZE) {
OutputView.printPlayersCard(playerViewModel)
}
}
Copy link

Choose a reason for hiding this comment

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

controller에 이와 같은 중요한 로직들이 세부구현으로 나타나 있는것 같습니다.

turn같은 부분은 테스트를 하지 않아도 된다고 판단하셨을까요?

제가 생각하기에 player가 특정상황에서 카드를 뽑는 다는것은 충분히 테스트할만한 부분이라고 생각이 드는데요.

이를 테스트하려면 controller가 아닌 별도의 객체에서 이러한 책임을 관리해야 할것 같습니다.

Copy link
Author

@naemoo naemoo Jun 26, 2023

Choose a reason for hiding this comment

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

리뷰어님이 말씀해주신 부분이 제가 많이 고민하던 부분이어서 깊은 공감이 됩니다! :) 리뷰어님이 말씀하신대로 Controller 에 중요한 비지니스가 섞여져 있는 거 같아요! 블랙잭 게임 요구사항이 client 와 지속적으로 통신을 해야하기 때문에 ui 인터페이스에 의존해야해서 게임의 흐름을 Controller 가 주도하게 된것 같습니다..! 지금 코드에서 출력 요구사항을 지키면서 어떻게 더 분리를 해야할지 많은 고민이 되네요 ㅠㅠ

"제가 생각하기에 player가 특정상황에서 카드를 뽑는 다는것은 충분히 테스트할만한 부분이라고 생각이 드는데요."

라고 말씀하신 부분은 DrawStrategy 의 구현체들의 테스트에서 진행 됐다고 생각이 듭니다!
DrawStrategy 가 있는데 player.isEligibleToHit() 를 굳이한 이유는 BustDrawStrategy 시 draw 를 하게 되면 Exception 을 발생 하게되는데 player.isEligibleToHit() 를 지우면 try-catch 로 게임의 흐름을 가져가야해서 일반적인 흐름은 아니라고 생각해서 이렇게 진행하게 됐어요 ㅎㅎ

Copy link

Choose a reason for hiding this comment

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

블랙잭 게임 요구사항이 client 와 지속적으로 통신을 해야하기 때문에 ui 인터페이스에 의존해야해서 게임의 흐름을 Controller 가 주도하게 된것 같습니다..! 지금 코드에서 출력 요구사항을 지키면서 어떻게 더 분리를 해야할지 많은 고민이 되네요 ㅠㅠ

client와 통신하는 부분을 인터페이스로 의존성 역전을 시도해보면 어떨까요?
그렇게되면 테스트가 가능해질것 같습니다!

Comment on lines 17 to 18
val gamePlayers = createPlayers(deck)
val dealer = createDealer(deck)
Copy link

Choose a reason for hiding this comment

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

GamePlayer와 Dealer를 하나의 일급컬렉션으로 관리하면서 상황에 맞게 적절한 객체를 호출하도록 하면 될것 같습니다.

AbstractPlayer로 추상화를 했으니 도전해보시면 좋을것 같습니다.

@@ -1,23 +1,21 @@
package blackjack.domain.card

class CardDeck(val cards: MutableList<Card>, val initDrawSize: Int = 2) {
class CardDeck(val cards: MutableList<Card> = CARDS.shuffled().toMutableList()) {
Copy link

Choose a reason for hiding this comment

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

cards가 테스트를 위해서만 가시성이 public로 되어있는것 같습니다.

private 로 제한해도 좋을것 같습니다.

import blackjack.domain.card.CardDeck
import blackjack.domain.card.Cards

interface DrawStrategy {
Copy link

Choose a reason for hiding this comment

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

상태패턴을 이용하실줄 알았는데 전략패턴을 이용하셨네요!

전략패턴도 좋은것 같습니다 👍

@naemoo
Copy link
Author

naemoo commented Jun 26, 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

3 participants