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
base: naemoo
Are you sure you want to change the base?
[Step3] 블랙잭 (딜러) #485
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개선해보면 좋을것 같은 부분에 코드리뷰를 남겨두었습니다.
확인해보시고 다시 PR요청 부탁드릴게요!
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) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
controller에 이와 같은 중요한 로직들이 세부구현으로 나타나 있는것 같습니다.
turn같은 부분은 테스트를 하지 않아도 된다고 판단하셨을까요?
제가 생각하기에 player가 특정상황에서 카드를 뽑는 다는것은 충분히 테스트할만한 부분이라고 생각이 드는데요.
이를 테스트하려면 controller가 아닌 별도의 객체에서 이러한 책임을 관리해야 할것 같습니다.
There was a problem hiding this comment.
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 로 게임의 흐름을 가져가야해서 일반적인 흐름은 아니라고 생각해서 이렇게 진행하게 됐어요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
블랙잭 게임 요구사항이 client 와 지속적으로 통신을 해야하기 때문에 ui 인터페이스에 의존해야해서 게임의 흐름을 Controller 가 주도하게 된것 같습니다..! 지금 코드에서 출력 요구사항을 지키면서 어떻게 더 분리를 해야할지 많은 고민이 되네요 ㅠㅠ
client와 통신하는 부분을 인터페이스로 의존성 역전을 시도해보면 어떨까요?
그렇게되면 테스트가 가능해질것 같습니다!
val gamePlayers = createPlayers(deck) | ||
val dealer = createDealer(deck) |
There was a problem hiding this comment.
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()) { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상태패턴을 이용하실줄 알았는데 전략패턴을 이용하셨네요!
전략패턴도 좋은것 같습니다 👍
항상 꼼꼼한 리뷰 정말 감사드립니다~! 🙇♂️ 피드백 주신 것을 반영해보려 최대한 노력해보았습니다..! 감사합니다! |
안녕하세요 리뷰어님~! 😀
블랙잭 딜러 개발 후 리뷰 요청 보내드려요~! :)
이번 단계도 리뷰 잘 부탁드리겠습니다 🙇♂️