[아크] 2단계 블랙잭 제출합니다. #34

re4rk · 2023-03-07T05:45:22Z

안녕하세요 제임스!
아까 조언에서 해주신 것처럼 사용자 입력 처리도 추가하고 머지전에 리뷰달아주신 것들 모두 반영 하였습니다!

re4rk · 2023-03-07T06:09:24Z

안녕하세요 제임스! 한가지 질문이 있습니다!

BLACKJACK_NUMBER = 21
같은 상수가 여러 곳에서 사용중입니다. 만약 해당 값에 변경이 발생한다면 모두 찾아서 변경해줘야 하며 실수로 빠뜨리면 버그가 발생할 수 있습니다. 하나의 상수로 해결 할 수 있는 구조에 대해 고민해보면 어떨까요?

위와 같은 피드백을 반영하기 위해서 해당 상수가 private로 사용되기 위해서 Score에서 isBust()과 isBlackJack()을 작성을 하였지만 bust의 판단은 Score의 역할은 아니라고 또 피드백을 주셔서 User로 옮기게 되었습니다. 그래서 아직도 User하고 Score가 지금 같은 BLACKJACK_NUMBER에 접근하고 있습니다. 이와 관련되서 힌트를 얻을 수 있을까요??
읽어주셔서 감사합니다!

woowajames

아크 2단계 미션 구현 수고하셨습니다!
몇 가지 코멘트 남겼으니 확인 후 다시 리뷰 요청 부탁드려요.

woowajames · 2023-03-07T15:37:24Z

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

+            cardDeck(Cards.all())
+            participants {
+                dealer()
+                getNames().forEach { name -> guest(name, getBettingMoney(name)) }
+            }


해당 함수를 넘겨주지 않아도 컴파일 에러는 없지만 런타임 에러가 발생하게 됩니다. 이 경우를 대비할 수 있는 방법을 고민해보면 어떨까요?

woowajames · 2023-03-07T16:52:32Z

src/main/kotlin/blackjack/domain/participants/User.kt

@@ -2,22 +2,19 @@ package blackjack.domain.participants

 import blackjack.domain.card.Card
 import blackjack.domain.card.Cards
+import blackjack.domain.participants.Score.Companion.BLACKJACK_NUMBER


이러한 결합도는 제거해보면 어떨까요?

woowajames · 2023-03-08T02:24:25Z

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

-            Outcome.LOSE -> "패"
-        }.let { println("${user.name}: $it") }
-    }
+    private fun outputOutcome(result: BlackJackResultUser) { println("${result.name}: ${result.rateOfReturn}") }

    private fun CardMark.name(): String =


이 부분을 뷰에서 구현한 이유가 있을까요? 각 클래스에서 가지고 있으면 만약 다른 뷰에서 사용할 일이 있을 때 중복 코드 없이 사용 가능할 것 같아요.

woowajames · 2023-03-08T02:31:14Z

src/main/kotlin/blackjack/domain/result/Outcome.kt

+    WIN(1.0),
+    DRAW(0.0),
+    LOSE(-1.0),
+    ;


정리하면 좀 더 보기 좋을 것 같아요.

woowajames · 2023-03-08T02:38:08Z

src/main/kotlin/blackjack/domain/participants/User.kt

+    fun score(): Int = cards.result.score()

-    val isBlackJack: Boolean
-        get() = cards.result.score() == BLACKJACK_NUMBER
+    fun isBlackJack(): Boolean = score() == BLACKJACK_NUMBER

-    abstract val isContinue: Boolean
+    fun isBust(): Boolean = score() > BLACKJACK_NUMBER

    fun draw(card: Card) { cards += card }


score(), isBlackJack(), isBust(), draw() 모두 함수로 만드셨네요. 어떤 기준으로 만드신걸까요?
행동은 상태를 변경시킵니다. 상태와 행동을 구분하면 좋겠습니다.

woowajames · 2023-03-08T02:45:28Z

src/main/kotlin/blackjack/domain/card/Card.kt

@@ -1,3 +1,5 @@
 package blackjack.domain.card

-data class Card(val mark: CardMark, val value: CardValue)
+data class Card(val mark: CardMark, val value: CardValue) {
+    fun isACE(): Boolean = value == CardValue.ACE


외부의 요청에 응답하기 위한 함수(=행동)네요. 다만 상태를 변경하는 역할을 하지는 않네요.
그리고 코틀린 코딩 컨벤션에 따른 함수명은 카멜 케이스로 작성하기 입니다.

woowajames · 2023-03-08T02:53:27Z

src/main/kotlin/blackjack/domain/participants/Score.kt


    val maxScore: Int =
-        minScore + if (cards.containsACE() && validateAceCondition) ACE_OTHER_NUMBER_DIFF else 0
+        minScore + if (cards.isContainsACE() && isMaxNumberBust().not()) ACE_OTHER_NUMBER_DIFF else 0


if else 구문은 한 줄 작성보다는 괄호를 추가해주는 게 좋을거 같아요.

woowajames · 2023-03-08T02:57:24Z

src/main/kotlin/blackjack/domain/participants/Score.kt


    companion object {
        private const val ACE_OTHER_NUMBER_DIFF = 10
-        private const val BLACKJACK_NUMBER = 21
+        const val BLACKJACK_NUMBER = 21


여러 곳에서 참조하고 있네요. 그렇다는 것은 여러 곳에서 블랙잭인지 확인 하는 작업을 한다는 의미입니다. 그런 작업을 하는 클래스가 존재하면 해당 상수를 그 클래스에서만 사용하는 것으로 변경 할 수 있습니다.

woowajames · 2023-03-08T03:05:02Z

src/main/kotlin/blackjack/domain/participants/Money.kt

+package blackjack.domain.participants
+
+@JvmInline
+value class Money(private val money: Int) {


val 타입으로 외부에 노출해도 문제가 없을 것 같아요. 그러면 toInt() 함수도 제거할 수 있을 것 같은데 특별히 이렇게 한 이유가 있을까요?

woowajames · 2023-03-08T03:09:26Z

안녕하세요 제임스! 한가지 질문이 있습니다!

BLACKJACK_NUMBER = 21 같은 상수가 여러 곳에서 사용중입니다. 만약 해당 값에 변경이 발생한다면 모두 찾아서 변경해줘야 하며 실수로 빠뜨리면 버그가 발생할 수 있습니다. 하나의 상수로 해결 할 수 있는 구조에 대해 고민해보면 어떨까요?

위와 같은 피드백을 반영하기 위해서 해당 상수가 private로 사용되기 위해서 Score에서 isBust()과 isBlackJack()을 작성을 하였지만 bust의 판단은 Score의 역할은 아니라고 또 피드백을 주셔서 User로 옮기게 되었습니다. 그래서 아직도 User하고 Score가 지금 같은 BLACKJACK_NUMBER에 접근하고 있습니다. 이와 관련되서 힌트를 얻을 수 있을까요?? 읽어주셔서 감사합니다!

해당 값을 참조하고 있는 곳에서 하는 작업은 'BLACKJACK_NUMBER인가?, BLACKJACK_NUMBER보다 큰가?'와 같은 것일텐데 해당 역할을 담당하는 객체가 있다면 BLACKJACK_NUMBER는 그 객체에만 존재해도 되지 않을까요?

re4rk · 2023-03-11T06:57:25Z

안녕하세요 제임스! state를 이용한 구현을 먼저 해보기 위해서 PR 리뷰 요청을 드리며 리뷰로 남겨주신 CardValue와 CardMark 그리고 StringBuilder는 따로 생각을 정리해보겠습니다!! 감사합니다!!

woowajames

아크! 상태 패턴까지 적용하는 도전적인 모습 멋있습니다! 👍
다만 현재 기본적인 기능 동작에 문제가 있어서 코멘트로 남겼습니다.
해당 부분 수정 부탁드리고 코멘트 수는 적지만 충분히 고민할 시간을 가지고 구현해보시면 좋겠습니다!

woowajames · 2023-03-12T10:45:47Z

src/test/kotlin/blackjack/domain/state/inTurn/HitTest.kt

+    fun `히트에서 스테이로 갈 수 있다`() {
+        // given
+        var state: State = FirstTurn().draw(CLOVER_KING).draw(CLOVER_NINE)
+
+        // when
+        state = state.draw(CLOVER_TWO)
+
+        // then
+        assertThat(state).isInstanceOf(Stay::class.java)
+    }


히트에서 블랙잭으로 가는 경우입니다. 예를 들어 16(히트)에서 스테이로 갈 수 있는 상황에 대한 로직 추가 및 테스트도 필요할 것 같아요.

수익금을 생각해서 3장 이상의 경우 21점이면 Stay로 처리하려고 했지만 규칙과 맞지 않게 비지니스로직을 작성한 것같아서 피드백주신 부분을 반영하였습니다! 반영 커밋 : 46a9d45

woowajames · 2023-03-12T10:48:04Z

src/main/kotlin/blackjack/domain/state/endTurn/Stay.kt

+        return when (otherState) {
+            is Bust -> WIN
+            is BlackJack -> compareBlackJack()
+            is Stay -> compareScore(otherState.score)
+            else -> throw IllegalStateException(ERROR_INVALID_STATE)


현재 구조에서 Hit에서도 Stay의 matchWith()를 타고 들어오는데요. 이 경우엔 무조건 else 문으로 처리되어 프로그램이 정상적인 동작을 할 수 없습니다. 간단하게는 Stay와 동일하게 Hit를 처리하면 되겠네요. 혹은 Hit에서 Stay로 가는 로직을 추가하면 되구요.

Hit도 처리할 수있도록 변경하였습니다! 반영커밋 : 46a9d45

woowajames · 2023-03-12T10:52:36Z

src/main/kotlin/blackjack/domain/card/Cards.kt

+    private val hardScore: Score = Score(cards.toList().sumOf { it.value.value })
+
+    private val softScore: Score = hardScore + SOFT_ACE_SCORE


이 프로퍼티를 포함하여 스코어 관련 기능은 스코어 클래스로 이동해보면 어떨까요?

관련 클래스로 이동하는 것으로 반영 하였습니다! 반영 커밋 : 4209ca5

woowajames · 2023-03-12T12:17:52Z

src/main/kotlin/blackjack/domain/state/inTurn/InTurn.kt

+import blackjack.domain.state.Score
+import blackjack.domain.state.State
+
+abstract class InTurn(final override val cards: Cards) : State {


InTurn, EndTurn의 구분 없이 BlackJack, Hit, Stay, Bust, FirstTurn이 State를 상속 받아서 처리하도록 개선해보면 어떨까요?

리뷰를 반영하여 Interface에서 Open Class로 변경하였습니다! 반영 리뷰 : b5d72f5

woowajames

아크! 피드백 반영 수고하셨습니다!
한꺼번에 많이 하기보다 조금씩 변경이 필요해 보여서 관련하여 코멘트 남겼으니 확인 후 다시 리뷰 요청 부탁드립니다. 이 부분도 조금의 변경이 될지 또 많은 일이 될지 약간 걱정스럽긴 하네요. 🤔

woowajames · 2023-03-13T07:09:42Z

src/main/kotlin/blackjack/domain/participants/user/Dealer.kt

+    fun calculateProfit(guests: List<Guest>): Int = guests.sumOf { it.calculateProfit(this) } * -1
+
+    override fun draw(card: Card) {
+        super.draw(card)


초기 2장을 나눠 줄 때 상태가 FirstTurn이 아닌 Hit 상태라 17이상 일 때도 이 시점에 dealer의 state는 Hit 인 상태이기 때문에 카드를 뽑게 됩니다.

woowajames · 2023-03-13T07:29:28Z

src/main/kotlin/blackjack/domain/state/Stay.kt

+            is Bust -> WIN
+            is BlackJack -> compareBlackJack()
+            is Stay -> compareScore(otherState.score)
+            is Hit -> compareScore(otherState.score)


Stay에서 Hit로 가는 경우가 있나요? 제가 이 경우를 말씀드린 것은 당장의 문제 해결을 하려면 단순히 그렇게 처리해도 되긴 할거다라는 거였습니다.. Hit에서 Stay로 변경하는 것이 자연스러운게 아닐까요?

woowajames · 2023-03-13T07:43:32Z

src/test/kotlin/blackjack/domain/state/HitTest.kt

+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.assertDoesNotThrow
+
+class HitTest {


Hit에서 Stay로 갈 수 있다는 테스트가 필요해보여요.

woowajames · 2023-03-13T07:46:15Z

src/test/kotlin/blackjack/domain/state/FirstTurnTest.kt

+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.assertDoesNotThrow
+
+class FirstTurnTest {


dealer의 경우 첫 턴에서 17이상일 경우 Stay로 가야하기 때문에 관련 테스트도 필요합니다.

woowajames

아크! 블랙잭 미션 수고하셨습니다! 👍 💯
제일 먼저 1단계 리뷰 요청을 주신 것도 인상적이었고 시간이 지날수록 깔끔해진 컨트롤러 구현도 기억에 남네요. 그리고 충분히 merge 될 상황에서도 상태 패턴에 도전하는 모습도 멋있었습니다! 앞으로도 남은 미션을 통해서 멋진 개발자로 성장하길 기대할게요! 화이팅 💪

re4rk added 21 commits March 6, 2023 21:21

refactor (Cards): 불변객체에서 getter삭제

dc08094

refactor (Cards): Boolean 반환 타입 is로 시작하도록 통일

84687b6

refactor (CardsTest): 테스트 구조 개선

bcaacd1

refactor (CardTest): assertAll 추가

90f3e9b

docs (README): 구현 목록 작성

a989fb2

feat (Guest): 유저가 배팅금액을 알 수 있도록 수정

9792aa6

feat (OutCome): 이긴 조건에 맞게 배당 비율반환하도록 추가

324ca67

feat (OutputView): 최종 수익을 반환한다.

950fa9c

feat (InputView): 배팅 금액을 입력 받는다.

d12018f

refactor (BlackJack): 프로퍼티에서 함수로 변경

9b20198

refactor (User): 유저 관련에서 테스트 케이스 추가

bdbe91e

refactor (ScoreTest): 점수 관련 테스트 코드 추가

2087f97

refactor (OutCome): 멤버 확장함수 제거

a149344

refactor (Cards): ACE를 가지고 있는지 확인 하는 기능 수정

f42a7ec

refactor (BlackJacBuilderTest): DSL 테스트명 변경

b365143

feat (BlackJackResult): 수익금을 반환할 수 있도록 추가

3de395d

refactor (BlackJackGame): View에서 처리할일 분리

c9c46c9

refactor (InputView): 입력처리는 View에서 처리할 수 있도록 변경

d34773b

refactor (Cards): 카드 뽑고 추가할 때 조건 추가

fd4b304

refactor (Score): BLACKJACK_NUMBER 의존성 변경

b35c4a6

style (BlackJack): 선언 순서 변 및 줄 맞춤

3241e31

re4rk added 2 commits March 7, 2023 17:43

refactor (BlackJack): 필요없는 함수 정리

0049cd0

refactor (BlackJackGame): 초기화 함수 domain으로 추가

addf588

woowajames requested changes Mar 8, 2023

View reviewed changes

re4rk added 4 commits March 8, 2023 15:09

refactor (Score): 상태확인 하는 함수 프로퍼티 게터로 변경

ed9ebdc

refactor (Money): value Class 접근 방법 변경

fa8cf46

refactor (Score): 클래스 상수 캡슐화

de9d0d9

refactor (Score): 상태확인하는 함수 프로퍼티로 변경

5a5c92b

re4rk added 18 commits March 11, 2023 13:18

refactor(BlackJack) : 코드 경로 수정

7790fb6

refactor(State) : 역의존성 제거

edb4ec7

refactor(Outcome) : 상태 객체 이동

9674a8f

refactor(Guest) : 클래스 매개변수 순서 변경

fd7a936

refactor(Guest) : 변수 구조 단순화

a7556bc

refactor(User) : 상태값으로 진행상황 결정하도록 변경

973d1cf

refactor(Dealer) : 상속으로 드로우 하도록 변경

96ca087

refactor(BlackJack) : 불필요한 부생성자 제거

4e254e9

refactor(Participants) : default 값추가

e5fddd8

refactor(BlackJack) : 부생성자 수정

62fcf63

refactor(BlackJackSetting) : 생성함수명 변경

a09743c

refactor(STATE) : 매직넘버 제거

6f93e61

refactor(STATE) : 함수 반환 명시적으로 변경

d9942f4

refactor(State) : 테스트 케이스 간략화

4388c24

refactor(UserTest) : 유저에만 해당하는 테스트 코드로 변경

8cd2df4

refactor(CardsTest) : 테스트 케이스 보완

d708a0d

refactor(DealerTest) : 테스트 케이스명 목적어추가

3441747

refactor(OutputView) : 오타 수정

4721468

re4rk added 2 commits March 11, 2023 16:03

refactor(UserTest) : 테스트 카드 가독성 변경

4b92bc8

refactor(BlackJack) : 네이밍 변경

13a00ce

woowajames requested changes Mar 12, 2023

View reviewed changes

re4rk added 3 commits March 13, 2023 01:16

refactor(BlackJack) : Score 계산 구조 개선

4209ca5

refactor(BlackJack) : State 인터페이스에서 Open Class로 변경

b5d72f5

refactor(BlackJack) : 3장이상의 21점일 경우 Stay에서 Blackjack으로 판단하도록 변경

46a9d45

woowajames requested changes Mar 13, 2023

View reviewed changes

refactor(State) : Stay, BlackJack, Bust에서만 비교할 수 있도록 변경

ea8bf0f

woowajames approved these changes Mar 14, 2023

View reviewed changes

woowajames merged commit 7a23a44 into woowacourse:re4rk Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[아크] 2단계 블랙잭 제출합니다. #34

[아크] 2단계 블랙잭 제출합니다. #34

re4rk commented Mar 7, 2023

re4rk commented Mar 7, 2023

woowajames left a comment

woowajames Mar 7, 2023

woowajames Mar 7, 2023

woowajames Mar 8, 2023

woowajames Mar 8, 2023

woowajames Mar 8, 2023

woowajames Mar 8, 2023

woowajames Mar 8, 2023

woowajames Mar 8, 2023 •

edited

Loading

woowajames Mar 8, 2023

woowajames commented Mar 8, 2023

re4rk commented Mar 11, 2023

woowajames left a comment

woowajames Mar 12, 2023

re4rk Mar 12, 2023

woowajames Mar 12, 2023

re4rk Mar 12, 2023

woowajames Mar 12, 2023

re4rk Mar 12, 2023

woowajames Mar 12, 2023

re4rk Mar 12, 2023

woowajames left a comment

woowajames Mar 13, 2023

woowajames Mar 13, 2023

woowajames Mar 13, 2023

woowajames Mar 13, 2023

woowajames left a comment

		private val hardScore: Score = Score(cards.toList().sumOf { it.value.value })

		private val softScore: Score = hardScore + SOFT_ACE_SCORE

[아크] 2단계 블랙잭 제출합니다. #34

[아크] 2단계 블랙잭 제출합니다. #34

Conversation

re4rk commented Mar 7, 2023

re4rk commented Mar 7, 2023

woowajames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woowajames Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woowajames commented Mar 8, 2023

re4rk commented Mar 11, 2023

woowajames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woowajames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woowajames left a comment

Choose a reason for hiding this comment

woowajames Mar 8, 2023 •

edited

Loading