[아크] 1단계 블랙잭 제출합니다. #8
Conversation
src/test/kotlin/Test.kt
Outdated
| @Test | ||
| fun company() { | ||
| val person = introduce { | ||
| name("로피") |
There was a problem hiding this comment.
정말 사소한 것이지만 페어가 아닌 본인 닉네임을 사용하면 좋았을 것 같아요.
src/test/kotlin/Test.kt
Outdated
| } | ||
|
|
||
| fun skills(block: SkillsBuilder.() -> Unit) { | ||
| skills = SkillsBuilder().apply { block() }.build() |
There was a problem hiding this comment.
introduce() 함수에서 사용한 것처럼 이런 식으로도 사용 가능하겠죠? 하나의 방식으로 모두 통일하면 좀 더 보기 좋지 않을까 싶어요.
SkillsBuilder().apply(block).build()
src/test/kotlin/Test.kt
Outdated
| val soft = MutableList(0) { "" } | ||
| val hard = MutableList(0) { "" } |
There was a problem hiding this comment.
private이면 더욱 좋겠네요. 그리고 Skills라는 타입도 이미 있는데 soft, hard 두 개의 프로퍼티를 하나로 줄여볼 수도 있지 않을까요? 🤔
src/test/kotlin/Test.kt
Outdated
| var korean = 0 | ||
| var english = 0 | ||
|
|
||
| fun build(): Languages = Languages(korean, english) |
There was a problem hiding this comment.
Korean, English 외 언어에 관한 입력은 처리가 불가능한 형태입니다. Skills를 참고하여 변경해보면 좋겠네요.
src/test/kotlin/Test.kt
Outdated
| assertThat(person.skills.soft.size).isEqualTo(2) | ||
| assertThat(person.skills.hard.size).isEqualTo(1) |
There was a problem hiding this comment.
hard 스킬에 kotlin을 넣으려고 의도했지만 오류로 결과적으로 java라는 값이 들어갈 수도 있다면 이 테스트 코드가 그걸 검증해 줄 수 있나요? 개인적으로는 심리적 안정감에 도움이 되지 않는 테스트 코드라고 생각합니다.
| class CardDeckTest { | ||
|
|
||
| @Test | ||
| fun `카드덱 잘 있는지 확인`() { |
There was a problem hiding this comment.
잘 있는게 뭔가요? 어떤 상태가 잘 있는거죠? 무엇을 테스트하는 것인지 좀 더 상세하고 명확하게 작성해주세요.(이하 동일)
| import org.assertj.core.api.Assertions.assertThat | ||
| import org.junit.jupiter.api.Test | ||
|
|
||
| class CardValueTest { |
There was a problem hiding this comment.
ParameterizedTest를 통해서 좀 더 간결하게 바꿀 수 있을 것 같아요.
| @@ -0,0 +1,15 @@ | |||
| package blackjack.domain | |||
|
|
|||
| data class BlackJack( | |||
There was a problem hiding this comment.
3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.
위 요구 사항을 지켜보면 어떨까요?
| @@ -0,0 +1,23 @@ | |||
| package blackjack.domain | |||
|
|
|||
| import blackjack.domain.BlackJackGame.Companion.BLACKJACK_NUMBER | |||
There was a problem hiding this comment.
BLACKJACK_NUMBER는 단 하나의 클래스에서만 다룰 수 있는 방향을 고민해보면 좋겠습니다.
| - [X] 게임 초기 세팅을 한다. | ||
| - [X] 중간결과를 반환한다. | ||
| - [X] 최종결과를 반환한다. | ||
|
|
There was a problem hiding this comment.
다음 사항에 관한 구현 여부를 확인해보세요. 기본적인 기능 요구 사항을 만족하지 않고 있습니다.
- 딜러는 처음에 받은 2장의 합계가 16이하이면 반드시 1장의 카드를 추가로 받아야 하고, 17점 이상이면 추가로 받을 수 없다.
- 플레이어는 21을 넘지 않을 경우 원한다면 얼마든지 카드를 계속 뽑을 수 있다. (현재 플레이어 카드가 K, ACE 일 때도 카드를 더 받을 것인지 묻고 있습니다. Ace를 언제 1 또는 11로 계산할지 좀 더 고민이 필요해보여요.)
| @CsvSource(value = ["ACE,1", "KING,10", "QUEEN,10", "JACK,10", "TEN,10", "NINE,9", "EIGHT,8", "SEVEN,7", "SIX,6", "FIVE,5", "FOUR,4", "THREE,3", "TWO,2"]) | ||
| fun `카드의 값들이 나온다`(cardValue: CardValue, value: Int) { |
| val size: Int | ||
| get() = cards.size |
There was a problem hiding this comment.
코틀린 컨벤션에 따라 위치를 조정하면 좋을 것 같아요.
- Property declarations and initializer blocks
- Secondary constructors
- Method declarations
- Companion object
|
|
||
| operator fun plus(card: Card): Cards = Cards(cards.plus(card)) | ||
|
|
||
| fun containsACE() = cards.map { it.value }.contains(CardValue.ACE) |
There was a problem hiding this comment.
isContainsAce()라는 네이밍이 좀 더 어울리지 않을까요?
| @@ -0,0 +1,29 @@ | |||
| package blackjack.domain | |||
|
|
|||
| class Score(val cards: Cards) { | |||
| private val validateAceCondition: Boolean | ||
| get() = minScore + ACE_OTHER_NUMBER_DIFF <= BLACKJACK_NUMBER | ||
|
|
||
| private val isMaxScoreInRange: Boolean | ||
| get() = maxScore <= BLACKJACK_NUMBER |
There was a problem hiding this comment.
보통 Boolean 타입은 is로 시작하죠. 네이밍에 일관성을 맞추면 더 좋겠어요.
|
|
||
| companion object { | ||
| private const val ACE_OTHER_NUMBER_DIFF = 10 | ||
| private const val BLACKJACK_NUMBER = 21 |
There was a problem hiding this comment.
BLACKJACK_NUMBER = 21
같은 상수가 여러 곳에서 사용중입니다. 만약 해당 값에 변경이 발생한다면 모두 찾아서 변경해줘야 하며 실수로 빠뜨리면 버그가 발생할 수 있습니다. 하나의 상수로 해결 할 수 있는 구조에 대해 고민해보면 어떨까요?
| package blackjack.domain | ||
|
|
||
| @JvmInline | ||
| value class Name(private val value: String) { |
| fun User.winTo(other: User): Outcome = | ||
| when { | ||
| other.score > BLACKJACK_NUMBER && this.score > BLACKJACK_NUMBER -> DRAW | ||
| other.score > BLACKJACK_NUMBER -> WIN | ||
| this.score > BLACKJACK_NUMBER -> LOSE | ||
|
|
||
| other.score == this.score -> DRAW | ||
| this.score > other.score -> WIN | ||
| other.score > this.score -> LOSE | ||
|
|
||
| else -> throw IllegalStateException() | ||
| } | ||
| } |
| fun `ACE 카드가 있는지 확인 할 수 있다`() { | ||
| var cards = Cards() | ||
| cards += Card(CLOVER, EIGHT) | ||
| assertThat(cards.containsACE()).isFalse | ||
| cards += Card(CLOVER, ACE) | ||
| assertThat(cards.containsACE()).isTrue | ||
| } |
There was a problem hiding this comment.
두 가지 테스트를 동시에 하기보다는 분리하면 좋습니다.
예를 들면
- ACE 카드가 없으면 false를 반환한다
- ACE 카드가 있으면 true를 반환한다
| assertThat(card.mark).isEqualTo(mark) | ||
| assertThat(card.value).isEqualTo(value) |
안녕하세요 리뷰어님!
코드 구현은 기존에 하던대로 구현하고 추후에 DSL을 적용하여 구현을 하였습니다!