-
Notifications
You must be signed in to change notification settings - Fork 82
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단계] 조이 미션 제출합니다. #118
base: gahyunkim
Are you sure you want to change the base?
Conversation
feat: 카드의 속성을 정의하는 enum 클래스 구현
shape와 rank를 담은 Enum 클래스를 구현하면서, 텍스트 정보가 도메인 영역에 포함되어 있는 점에 대해 고민하게 되었습니다. |
Player와 Dealer의 공통 코드와 유사한 기능을 묶어 관리하기 위해 인터페이스와 추상 클래스를 고민하다가 Participant 추상 클래스로 구현해주었습니다. 추상 클래스 내에서는 게임 시작 과정을 담당하는 turn()과 같은 추상 함수를 정의하여, Player와 Dealer가 각자 필요한 방식으로 구현할 수 있도록 했습니다. 공통 기능은 미리 제공하면서도 개별적인 구현이 가능하다는 점에서 추상 클래스를 사용한 접근 방식에 대해 어떻게 생각하시는지 궁금합니다! |
feat: CardsGenerator 클래스 객체 생성 class CardsGenerator {
fun generateCards(): Cards {
val cards: MutableList<Card> = mutableListOf()
Shape.entries.forEach { shape ->
createCardRank(cards, shape)
}
cards.shuffle()
return Cards(cards)
}
private fun createCardRank(
cards: MutableList<Card>,
cardShape: Shape,
) {
CardRank.entries.forEach { cardRank ->
cards.add(Card(cardRank, cardShape))
}
}
}
CardGenerator에서 카드 전체를 생성해주고 shuffle을 통해 전체 카드를 만들어주고 있습니다. 이전 리뷰에서는 1~45의 로또 번호와 같이 객체를 계속 생성할 경우 메모리 문제가 발생할 수 있으니, FlyWeight 패턴이나 factory 메소드 사용에 대해 논의하고 적용해본 경험이 있습니다. 이번 카드의 경우 52장의 카드에 대해 캐싱하는 것이 좋을지 고민해보았지만, 블랙잭 미션 진행 중 시간이 부족해 캐싱을 구현해보지 못했습니다. 이러한 상황에서 캐싱을 적용하는 것이 더 나은 접근 방식인지 궁금합니다! |
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.
안녕하세요 :)
1단계 미션 고생 많으셨어요.
여러 고민해볼만한 곳들에 코멘트 남겨두었으니 깊게 생각해보세요 :)
피드백 반영 후 다시 리뷰 요청 부탁드려요!
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.
게임 규칙에 의해 진행하는 것들, 즉,
딜러가 카드를 몇 번 받고, 플레이어는 처음에 몇 장 먼저 받아야한다는 것들 등등...
이러한 게임을 진행하는 로직은 컨트롤러의 역할일까요?
enum class CardRank(val score: Int, val title: String) { | ||
ACE(11, "A"), |
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.
ACE의 기본 점수는 1이에요.
10점인 경우는 특수 경우이니, 로직으로 표현해보는 건 어떨까요?
|
||
fun isAceCard(): Boolean = cardName.contains(ACE) | ||
|
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.
카드가 ACE인지를 나타내는 다른 객체가 있음에도 문자열로 비교한 이유는 무엇인가요?
|
||
data class Card(private val cardRank: CardRank, private val shape: Shape) { |
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.
카드를 생성할 수 있는 경우는 몇가지나 될까요?
동일한 값을 가지는 중복 인스턴스를 만들지 못하도록 구성해보는 건 어떨까요?
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.
이번 카드의 경우 52장의 카드에 대해 캐싱하는 것이 좋을지 고민해보았지만, 블랙잭 미션 진행 중 시간이 부족해 캐싱을 구현해보지 못했습니다. 이러한 상황에서 캐싱을 적용하는 것이 더 나은 접근 방식인지 궁금합니다!
PR 코멘트에 남겨두신 의견이 있으셨네요.
더 나은 접근 방식이라고 생각한 이유는 무엇인가요?
그렇지 못하다고 생각한 이유는 무엇인가요?
class Cards(allCards: List<Card>) { | ||
val allCards: MutableList<Card> = allCards.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.
아래 코드를 실행시킨다면 어떤 문제가 생길 수 있을까요?
val cards = Cards(listOf(.....))
cards.allCards.clear()
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 만이 가져야 할 관심만을 가지고 있는 것인지 고민해보세요.
카드 뭉치 자체가 다른 것에 관심을 가지고 있진 않은지요.
또, 어떤 곳의 역할이 이 객체로 옮겨지면 적절해보이는 것들이 눈에 보이네요 :)
|
||
class CardsGenerator { | ||
fun generateCards(): Cards { | ||
val cards: MutableList<Card> = mutableListOf() | ||
Shape.entries.forEach { shape -> | ||
createCardRank(cards, shape) | ||
} | ||
cards.shuffle() | ||
return Cards(cards) | ||
} |
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.
이 카드 생성기는 랜덤의 요소때문에 테스트가 불가능한 구조이네요.
테스트 가능하도록 구성해보는 건 어떨까요?
|
||
abstract fun turn(cards: Cards): Boolean |
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.
turn 이라는 메서드명은 다른 개발자들이 보았을 때 어떤 역할을 할지 바로 유추해낼 수 있을까요?
또, 반환 값은 어떤 것을 의미하는지도요.
|
||
class GameResultDecider(private val dealer: Dealer, private val players: Players) { |
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.
딜러와 플레이어를 앞에 두고 어떠한 심판이 게임 결과를 알려줄 수도 있겠어요.
그러나 카지노에서는 점수를 누가 계산해주는지 잠깐 생각해보면, 현실 세계의 것을 객체 지향의 형태로 가져와볼 수 있겠어요.
enum class Shape(val title: String) { | ||
HEART("하트"), | ||
SPADE("스페이드"), | ||
CLUB("클로버"), | ||
DIAMOND("다이아몬드"), | ||
} |
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.
블랙잭 미션 진행 중 카드 이름 처리를 위해 Enum 클래스를 사용하면서, 해당 클래스에 텍스트 관련 정보가 과도하게 포함된 느낌을 받았습니다. 이에 대해, Enum 클래스에 텍스트를 포함시키는 설계 방식에 대한 의견과 함께 ViewMapper 또는 확장 함수를 활용하는 방법에 대해 어떻게 생각하시는지 궁금합니다.
shape와 rank를 담은 Enum 클래스를 구현하면서, 텍스트 정보가 도메인 영역에 포함되어 있는 점에 대해 고민하게 되었습니다.
원래 텍스트는 뷰에서 담당해야 하는 부분인데, 이를 enum 클래스에 넣는 것이 적절한지에 대해 이전 리뷰에서 리뷰를 받은 경험이 있습니다. 이를 개선하기 위해 ViewMapper를 활용하거나, object를 이용해 데이터를 불러오는 방식, 또는 Cards.isDisplayName()과 같은 확장함수를 사용하는 방안을 고려해보았으나, 블랙잭 미션 진행 시 시간 부족으로 적용하지 못했습니다... 이러한 상황에서 현재 enum 클래스에 텍스트가 포함되어 있는 설계에 대해 어떻게 생각하시는지, 그리고 ViewMapper나 확장함수를 사용하는 방식에 대해 어떤 의견을 가지고 계신지 궁금합니다!
이 블랙잭 게임 도메인을 똑 떼어다가 안드로이드에 이식한다면, 이 텍스트 값은 유효한가요?
open class 및 interface와는 어떤 차이점과 장단점이 있었나요? |
안녕하세요 말리빈!
이번 블랙잭 미션을 리뷰 받게 된 조이입니다.
블랙잭 미션을 진행하다보니 시간이 부족해 좀 더 꼼꼼히 확인하지 못한 부분들이 많은 것 같습니다.. 리팩토링을 하면서 조금 더 좋은 코드를 만들기 위해 노력해보겠습니다! 많이 많이 혼내주세요!
셀프 체크리스트
README.md
에 기능 목록을 정리하고 명확히 기술하였는가?어떤 부분에 집중하여 리뷰해야 할까요?
(자세한 내용은 아래 코드와 함께 추가 설명드리겠습니다.)