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

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

Merged
merged 86 commits into from
Mar 9, 2023

Conversation

tmdgh1592
Copy link
Member

@tmdgh1592 tmdgh1592 commented Mar 3, 2023

안녕하세요 리뷰어님!
이번 미션 리뷰를 맡아주셔서 정말 영광입니다~!

미션을 진행하면서 궁금한 점에 대해 여쭤보고 싶습니다!

  1. DTO로 감싸는 것이 Controller의 역할인가 vs Domain의 역할인가
    우선, 이번 프로젝트를 진행하면서 Domain에서 DTO로 감싸서 전달하도록 하였습니다.
    하지만 테스트 코드를 작성하는 것이 너무 힘들어진다는 문제가 발생하였는데, 이런 구조여도 괜찮은지 궁금합니다!

  2. 에러 메시지와 같은 String도 const val 키워드를 통해 상수화를 해주는 것이 가독성이 높은지, 아니면 문자 그대로 작성하는 것이 높은지 궁금합니다!

tmdgh1592 and others added 30 commits February 28, 2023 14:56
게임에 참여하는 플레이어의 이름을 받는다.
카드를 더 받을지 알려주는 명령어를 받는다.
tmdgh1592 added 28 commits March 8, 2023 14:52
…into step1

# Conflicts:
#	src/main/kotlin/blackjack/domain/BlackJack.kt
- 최소 인원을 1명에서 2명으로 변경
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 +33 to +35
private fun drawInitialCards(blackJack: Blackjack) {
blackJack.readyToStart()
}
Copy link

Choose a reason for hiding this comment

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

함수를 분리하지 않아도? 괜찮아 보여요!
setupCard()와 drawInitialCards()가 어떤 차이인지 헷갈림!

Copy link
Member Author

Choose a reason for hiding this comment

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

앗 그렇군요..!
어차피 한 줄이고 굳이 나누지 않는 것이 가독성이 좋아보입니다!

Comment on lines +6 to +20
fun readyToStart() {
participants.drawFirst(deck)
}

fun start(onDrawn: (Participant) -> Unit): BlackjackResult {
participants.takePlayerTurns(deck, onDrawn)
participants.takeDealerTurns(deck, onDrawn)

return BlackjackResult(
participants.getCardResults(),
participants.getMatchResults(),
)
}

fun getFirstOpenCards(): Map<String, List<Card>> = participants.getFirstOpenCards()
Copy link

Choose a reason for hiding this comment

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

좋습니다.
조금 더 나아가 본다면, start() 함수 하나에서 처음 뽑을 카드와 이어서 뽑는 카드들을 같이 묶을 수 있을 것 같네요!

fun start(onDrawn: (Participant) -> Unit): Result {
   participants.drawFirst(deck, onDrawn)
   participants.draw(deck, onDrawn)

   return ...
}

Comment on lines +4 to +5
val cardResults: List<CardResult>,
val matchResults: List<MatchResult>,
Copy link

Choose a reason for hiding this comment

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

CardResult랑 MatchResult를 나눌 필요가 있을까요?
플레이어의 입장에서도 승, 패, 무를 비교한다면 1승 0무 0패 처럼 나타낼 수 있지 않을까요?

Comment on lines +8 to +10
init {
_items.addAll(cards)
}
Copy link

Choose a reason for hiding this comment

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

_items 프로퍼티를 만들면서 생성자로 받은 cards를 초기화 할 수 있어요


private fun hasAce(): Boolean = _items.any(Card::isAce)

companion object {
private const val BONUS_SCORE = 10
private const val BLACKJACK_SCORE = 21
private const val STAY_SCORE = 17
Copy link

Choose a reason for hiding this comment

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

블랙잭 게임에서 스테이라는 의미는 어떠한 점수를 나타내는 상태가 아닙니다.
카드들의 개념보다는 정확하게는 "딜러"가 카드를 더 뽑기 위한 조건 이라고 보는게 맞을 것 같습니다

21점 미만일 때, 더 뽑는다 -> 히트
21점 미만일 때, 더 뽑지 않는다 -> 스테이
21점 초과일 때 -> 버스트

Copy link
Member Author

@tmdgh1592 tmdgh1592 Mar 10, 2023

Choose a reason for hiding this comment

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

2단계와 피드백을 보면서 깨달았습니다..
아직 Blackjack이라는 도메인 지식이 부족한 것 같습니다.
스테이더 뽑지 않는다 라는 의미이군요.
감사합니다!

fun isStay(): Boolean = cards.isStay()

infix fun judge(other: Participant): GameResult = when {
isBust() && other.isBust() -> GameResult.DRAW
Copy link

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.

그렇군요..! 제가 규칙을 잘못 이해하고 있었던 것 같습니다!
2단계 미션을 진행하면서 수업시간에 배운 State, Template Method 패턴을 함께 적용하면서 리팩터링해보겠습니다.
만약 Dealer의 상태가 블랙잭이고, Player의 상태가 블랙잭이면 Draw를 반환한다. 와 같이 작성해볼 수 있을 것 같습니다!

Comment on lines +20 to +21
getTotalScore() == other.getTotalScore() -> GameResult.DRAW
getTotalScore() > other.getTotalScore() -> GameResult.WIN
Copy link

Choose a reason for hiding this comment

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

이 함수를 끝내기까지 getTotalScore()가 몇번 호출이될까요?
가독성을 위해 위처럼 작성을 할 수 있겠지만, 불필요한 호출은 줄이면 좋을 것 같아요

Comment on lines 17 to 25
fun takePlayerTurns(deck: CardDeck, onDrawn: (Participant) -> Unit) {
getPlayers().forEach { participant ->
drawUntilCanDraw(participant, deck, onDrawn)
}
}

fun getPlayers(): List<Player> = players.toList()
fun takeDealerTurns(deck: CardDeck, onDrawn: (Participant) -> Unit) {
drawUntilCanDraw(getDealer(), deck, onDrawn)
}
Copy link

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.

다시 살펴보니 그럴 필요가 없다는 걸 알게 됐습니다! 🤔
처음에 단순히 participants.forEach 와 같이 하다보니 딜러 -> 플레이어 순으로 카드를 뽑게 되는 문제가 있어 각각 분리하였습니다.
하지만 (getPlayers() + getDealer()).forEach 이와 같은 방식으로 변경하니 정상적으로 프로그램이 동작합니다!

@laco-dev laco-dev merged commit 5e3e342 into woowacourse:tmdgh1592 Mar 9, 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.

3 participants