2단계 - 블랙잭 by Pablo7V · Pull Request #780 · next-step/kotlin-blackjack

Pablo7V · 2024-12-07T22:10:24Z

경록님 안녕하세요!

개인 사정으로 인해 리뷰 요청이 너무 늦어졌네요 😭

이번 블랙잭 미션에서 제가 블랙잭 게임에 대해 잘 몰랐던 부분이 있어 알아본 아래 룰이 확실하지 않을 수 있는데요ㅠ

블랙잭 게임은 숫자 21이 넘는 순간 즉시 패배로 더이상 게임을 진행하지 않는다

미션에 명시된 "21을 넘지 않을 경우 원한다면 얼마든지 카드를 계속 뽑을 수 있다."와 의미가 일맥 상통한게 아닐까 싶어

이번 미션에서 합이 21을 넘을 경우 플레이어가 즉시 패배하도록 구현했습니다

추가로 1단계에서 리뷰 주셨던 부분들 확인 후 반영해보았습니다, 꼼꼼한 리뷰 감사합니다 🙇

사용하지 않는 테스트 로직 제거
불필요한 테스트 라이브러리 제거
sealed class 활용 등

그럼 남은 기간 빠르게 미션 완수할 수 있도록 노력해보겠습니다, 잘 부탁드립니다!

Rok93

창민님 2단계 리뷰 요청주신 코드가 컴파일 에러로인해 실행이 안되는 것으로 보입니다. 😂
해당 내용 확인해서 수정한 뒤 다시 리뷰요청 부탁드릴게요. 🙏

Rok93 · 2024-12-08T05:06:33Z

src/main/kotlin/blackjack/domain/BlackJack.kt

+
+class BlackJack(val players: Players, val cardDeck: CardDeck) {
+    init {
+        repeat(INIT_CARD_DRAW_REPEAT) { players.players.forEach { it.addCard(cardDeck.drawCard()) } }


player 객체에게 메시지를 보내보면 어떨까요?

이 부분도 같이 확인 부탁드릴게요. 😃

Rok93

창민님 2단계 빠르게 잘 구현해주셨습니다. 👍💯
몇몇 코멘트 남겨두었는데, 확인 부탁드릴게요. 🙏

미션 진행하시면서 잘 이해가 안되거나 어려운 부분 있으시면 언제든지 DM 주세요. 😉

Rok93 · 2024-12-09T11:31:23Z

src/main/kotlin/blackjack/domain/DeckGenerator.kt

@@ -0,0 +1,17 @@
+package blackjack.domain
+
+fun defaultDeckGenerator(): MutableList<Card> {


반환 타입이 가변리스트군요? 👀
가급적 가변 리스트보다는 불변 리스트를 사용해보면 어떨까요? 😃

생성한 Card들을 자주 사용되게 될 것 같은데요.
모든 카드에대해 캐싱해서 재사용하면어떨까요? 😃

또한 이런 관점에서 별도의 DeckGenerator를 만들기보다는 Card의 상수로 두면 어떨까요?

Rok93 · 2024-12-09T11:37:12Z

src/main/kotlin/blackjack/domain/BlackJack.kt

+
+class BlackJack(val players: Players, val cardDeck: CardDeck) {
+    init {
+        repeat(INIT_CARD_DRAW_REPEAT) { players.players.forEach { it.addCard(cardDeck.drawCard()) } }


이 부분도 같이 확인 부탁드릴게요. 😃

Rok93 · 2024-12-09T11:45:58Z

src/main/kotlin/blackjack/domain/CardDeck.kt

@@ -0,0 +1,13 @@
+package blackjack.domain
+
+data class CardDeck(val deck: MutableList<Card>) {


CardDeck의 내부 값을 외부에서 변경할 수 없도록해보면 어떨까요? 🤔

Rok93 · 2024-12-09T11:47:47Z

src/main/kotlin/blackjack/domain/CardDeck.kt

+
+    fun drawCard(): Card {
+        require(deck.isNotEmpty()) { "더이상 카드가 남아있지 않습니다" }
+        return deck.removeAt(deck.lastIndex)


removeLast()를 사용하도록 변경해볼 수 있을 것 같아요. 😃

Suggested change

return deck.removeAt(deck.lastIndex)

return deck.removeLast()

Rok93 · 2024-12-09T11:51:43Z

src/main/kotlin/blackjack/domain/Players.kt

+
+data class Players(val players: List<Player>) {
+    init {
+        require(players.size == PLAYER_COUNT) { "블랙잭 게임에서 플레이어는 2명이어야 합니다" }


아래와 같이 에러 메시지에도 상수를 사용하면, 추후 값이 변경되더라도 에러 메시지를 추가로 수정하는 공수가 없을 것 같네요. 😃

Suggested change

require(players.size == PLAYER_COUNT) { "블랙잭 게임에서 플레이어는 2명이어야 합니다" }

require(players.size == PLAYER_COUNT) { "블랙잭 게임에서 플레이어는 ${PLAYER_COUNT}명이어야 합니다" }

추가적으로 PLAYER_COUNT는 Players 클래스 내부에서만 사용되는 것 같은데, 접근 제한자를 private로 제한해주시면 좋을 것 같아요. 😁

Rok93 · 2024-12-09T12:01:36Z

src/main/kotlin/blackjack/domain/Rank.kt

+    KING(10, "K"),
+    ;
+
+    fun getNumber(): List<Int> {


getNumber의 결과는 사실상 value를 가져오는거니 getValue가 조금 더 적절한 네이밍인 것 같아요. 🤔
value는 의미가 조금 애매해지는 것 같아서 점수와 같은 의미를 가지는 네이밍을 고려해보시면 조금 더 네이밍으로 의도를 드러낼 수 있지 않을까 싶어요. 😁

Rok93 · 2024-12-09T12:03:08Z

src/main/kotlin/blackjack/domain/Player.kt

+package blackjack.domain
+
+data class Player(val playerName: PlayerName, val mutableCards: MutableCards) {
+    fun addCard(card: Card) {


현실에서 참가자가 카드를 한장 가져가는 것을 어떻게 표현하면 조금 더 역할이 드러날 수 있을까요? 👀
현실 세계에 빗대어서 네이밍을 고려해보시면 어떨까요? 😁

Rok93 · 2024-12-09T12:06:58Z

src/main/kotlin/blackjack/domain/Players.kt

+    fun toPlayerNamesString(): String {
+        return players.joinToString(", ") { it.playerName.name }
+    }


Players 객체가 출력을 위한 행위를하고있네요? 🤔
바로 아래로직에서 한 것처럼 InitBlackJackView에서 출력을 위해 프로퍼티로부터 직접 name을 가져와서 출력을 위한 형태로 변경해보면 어떨까요?

Rok93 · 2024-12-09T12:08:01Z

src/main/kotlin/blackjack/view/BlackJackController.kt

@@ -0,0 +1,38 @@
+package blackjack.view


Controller클래스도 view 패키지에 위치하고있네요. 👀
view랑은 별도의 패키지에 두는게 조금 좋을 것 같아요. 😃

Rok93 · 2024-12-09T12:17:00Z

src/main/kotlin/blackjack/domain/Player.kt

+        require(sumCardValues() <= MutableCards.MAX_SUM_CARD_VALUES) {
+            playerName.name + " Bust! / " + mutableCards.cardsToString()
+        }


현재 21점을 넘어서 Bust가 되는 경우에는 바로 블랙잭 게임이 종료되는대요.
플레이어의 점수가 Bust되더라도 블랙잭 게임이 바로 종료되면 안될 것 같아요. 🙄

김창민 and others added 20 commits November 26, 2024 23:03

dsl 실습 -> 귀가 후 집에서

3bfd849

Skills 추가 구현 및 테스트, Person 관련 코드 테스트 코드와 분리

e6317a6

Languages 관련 기능 추가 및 테스트 구현

bc7c509

초기 요구사항 명세 정리

8e5e26e

모든 카드는 중복이 불가능 하도록 미리 생성하여 관리하는 기능 구현 및 테스트

e70f2a0

Ace(숫자1)는 1 또는 11로 계산하는 기능 구현 및 테스트

139c060

Suit, Rank enum 활용에 따른 Card 객체 수정

a92a28b

블랙잭 게임 속 카드를 관리할 CardDeck 객체 구현 및 테스트

cccb374

플레이어가 가질 수 있는 카드 더미를 나타낼 MutableCards 객체 구현 및 테스트

d0c2d3f

블랙잭 플레이어 객체 구현 및 테스트

1e4e656

README 수정

0d98f91

블랙잭 게임에 참여하는 플레이어들을 관리할 Players 객체 구현 및 테스트

ebb3859

블랙잭 게임 초기 로직 구현 및 테스트

04af441

테스트 코드 작성을 위한 util 함수 CreatePlayers 구현

9c8df00

불필요한 gradle 라이브러리 설치 원복

48995ce

view 로직 구현

384ef32

불필요한 테스트 코드 제거

81d98d2

formating 수정

c591305

sealed class 활용

2f63e01

Merge remote-tracking branch 'upstream/pablo730' into feature/blackjack

993e49b

Rok93 requested changes Dec 8, 2024

View reviewed changes

Pablo7V added 2 commits December 9, 2024 00:45

불필요한 라이브러리 제거

ce3f865

sealed class 활용

5130f04

Rok93 requested changes Dec 9, 2024

View reviewed changes

		@@ -0,0 +1,17 @@
		package blackjack.domain

		fun defaultDeckGenerator(): MutableList<Card> {

		@@ -0,0 +1,13 @@
		package blackjack.domain

		data class CardDeck(val deck: MutableList<Card>) {

	return deck.removeAt(deck.lastIndex)
	return deck.removeLast()

	require(players.size == PLAYER_COUNT) { "블랙잭 게임에서 플레이어는 2명이어야 합니다" }
	require(players.size == PLAYER_COUNT) { "블랙잭 게임에서 플레이어는 ${PLAYER_COUNT}명이어야 합니다" }

Conversation

Pablo7V commented Dec 7, 2024

Uh oh!

Rok93 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rok93 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants