-
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단계] 이든 미션 제출합니다. #121
base: devfeijoa
Are you sure you want to change the base?
Conversation
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.
이든 안녕하세요!
이번 미션을 함께하게 된 두루라고 합니다. 잘부탁드립니다!
해당 미션 구현 고생 많으셨습니다!
고민해보시면 좋을만한 내용들 코멘트를 달았으니 확인해주시면 감사하겠습니다.
이번 블랙잭 미션이 도메인 로직이 어렵다보니, 구현을 하시면서 많은 어려움이 있으셨던 것 같군요.
어떤 방법으로 바꿔보면 좋을지에 대해서 순서를 남겨두었습니다.
해당 내용을 참고하여 구현을 해나간다면, 정리가 될 수 있을 것 같습니다.
궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요! 🙏
화이팅입니다💪
|
||
## 소개 |
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.
README 작성 잘해주셨네요~👍
fun main() { | ||
println("게임에 참여할 사람의 이름을 입력하세요.(쉼표 기준으로 분리)") | ||
val players: List<Player> = readln().split(",").map { name: String -> Player(name.trim()) } |
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.
허걱...🫣
main 에 모든 코드를 올인하셨군요!
해당 내용은 이번 미션을 진행하면서 차근차근 리팩토링을 해봅시다!
println("${players.joinToString { player -> player.name }}에게 2장씩 나누었습니다.") | ||
println("딜러가 한 장을 오픈했습니다.") |
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.
- View 와 Controller 의 영역을 분리해주세요!
println("딜러: ${dealer.cards.joinToString { card -> card.prettyString }}") | ||
players.forEach { player -> | ||
println("${player.name}카드: ${player.cards.joinToString { card -> card.prettyString }}") | ||
} |
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.
- main()에 모여있는 함수들을 하나의 함수가 하나의 기능을 하도록 분리해주세요
println() | ||
println("딜러 카드: ${dealer.cards.joinToString { card -> card.prettyString }} - 결과: ${dealer.getScore()}") |
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.
- View 에서 사용되는 하드코딩된 상수들을 상수화 + 포맷을 활용해서 정리를 해주세요
class Deck { | ||
private val aceCards: List<Card> = |
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.
현재 분리되어 있는 객체들을 통합한다면, Deck 부분의 구현이 어느정도 정리가 될 수 있겠군요!
private val numberCards: List<Card> = spadeNumberCards + heartNumberCards + diamondNumberCards + cloverNumberCards | ||
|
||
private val spadeCharacterCards: List<Card> = | ||
listOf(Card(Character.JACK, Suit.SPADE), Card(Character.QUEEN, Suit.SPADE), Card(Character.KING, Suit.SPADE)) | ||
|
||
private val heartCharacterCards: List<Card> = | ||
listOf(Card(Character.JACK, Suit.HEART), Card(Character.QUEEN, Suit.HEART), Card(Character.KING, Suit.HEART)) | ||
|
||
private val diamondCharacterCards: List<Card> = | ||
listOf( | ||
Card(Character.JACK, Suit.DIAMOND), | ||
Card(Character.QUEEN, Suit.DIAMOND), | ||
Card(Character.KING, Suit.DIAMOND), | ||
) | ||
|
||
private val cloverCharacterCards: List<Card> = | ||
listOf(Card(Character.JACK, Suit.CLOVER), Card(Character.QUEEN, Suit.CLOVER), Card(Character.KING, Suit.CLOVER)) | ||
|
||
private val characterCards = | ||
spadeCharacterCards + heartCharacterCards + diamondCharacterCards + cloverCharacterCards |
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.
각 Shape별로 따로 있을 이유가 있을까요?
Card(val shape, val number)
로 변경한다면, 해당 구현이 정리되겠군요~
val acesSums: List<Int> = | ||
when (aces.size) { | ||
0 -> listOf(0) | ||
1 -> listOf(1, 11) | ||
2 -> listOf(2, 12, 22) | ||
3 -> listOf(3, 13, 23, 33) | ||
4 -> listOf(4, 14, 24, 34, 44) | ||
else -> throw IllegalArgumentException("Cards can't contain more than 4 aces") | ||
} |
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.
해당 내용이 들어간 이유가 무엇일까요?
fun setResult() { | ||
if (hand.getScore() == null) { | ||
result = Result.LOSE | ||
} | ||
} |
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.
게임에 대한 결과를 담당하는 객체로 분리를 해볼 수 있겠군요!
fun getScore(): Int? { | ||
val aces: List<Card> = value.filter { card: Card -> card.rank is 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.
점수를 구해주는 기능을 담당하는 객체로 분리를 해볼 수 있을 것 같습니다~
셀프 체크리스트
안녕하세요 두루! 안드로이드 7기 이든입니다.
귀중한 시간내어 리뷰해주셔서 감사합니다.
README.md
에 기능 목록을 정리하고 명확히 기술하였는가?어떤 부분에 집중하여 리뷰해야 할까요?
이번 미션을 페어와 TDD에 집중하여 만들었습니다.
제가 기초지식이 부족하여 여러 개념들을 헷갈려하는 부분이 많았고 코드작성이 어려운 부분이 많았습니다. 그럴때마다 페어가 제가 이해할때까지 기다려주고 해답을 찾아갈 수 있도록 도와주느라 기다리는 시간이 길어졌습니다.
테스트코드 부분에 있어서는 열심히 작성을 마무리 하였으나, 도메인모델이나 아키텍처 부분에 대해서는 마무리하기에 시간이 부족했습니다.
그래서 리뷰를 해주실 때 테스트코드 부분과 도메인 모델 부분을 중심으로 리뷰를 해주시면 감사하겠습니다 !
이후의 과정에서 아키텍처 부분으로 더 고민하고 공부하여 몽땅 main 함수에 넣어둔 부분에 대해서 수정하도록 하겠습니다.
감사합니다 !
코드 리뷰 커뮤니케이션
📌 GitHub에서 PR에 댓글 남기는 방법
참고 자료
스크린샷