Skip to content
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

Open
wants to merge 27 commits into
base: devfeijoa
Choose a base branch
from

Conversation

devfeijoa
Copy link

@devfeijoa devfeijoa commented Mar 7, 2025

셀프 체크리스트

안녕하세요 두루! 안드로이드 7기 이든입니다.
귀중한 시간내어 리뷰해주셔서 감사합니다.

  • 프로그램이 정상적으로 작동하는가?
  • 모든 테스트가 통과하는가?
  • 이전에 받은 피드백을 모두 반영하였는가?
  • 코딩 스타일 가이드를 준수하였는가?
    • IDE 코드 자동 정렬을 적용하였는가?
    • 린트 검사를 통과하였는가?
  • 프로그래밍 요구 사항을 준수하였는가?
  • README.md에 기능 목록을 정리하고 명확히 기술하였는가?

어떤 부분에 집중하여 리뷰해야 할까요?

이번 미션을 페어와 TDD에 집중하여 만들었습니다.
제가 기초지식이 부족하여 여러 개념들을 헷갈려하는 부분이 많았고 코드작성이 어려운 부분이 많았습니다. 그럴때마다 페어가 제가 이해할때까지 기다려주고 해답을 찾아갈 수 있도록 도와주느라 기다리는 시간이 길어졌습니다.
테스트코드 부분에 있어서는 열심히 작성을 마무리 하였으나, 도메인모델이나 아키텍처 부분에 대해서는 마무리하기에 시간이 부족했습니다.
그래서 리뷰를 해주실 때 테스트코드 부분과 도메인 모델 부분을 중심으로 리뷰를 해주시면 감사하겠습니다 !
이후의 과정에서 아키텍처 부분으로 더 고민하고 공부하여 몽땅 main 함수에 넣어둔 부분에 대해서 수정하도록 하겠습니다.
감사합니다 !

코드 리뷰 커뮤니케이션

📌 GitHub에서 PR에 댓글 남기는 방법

참고 자료

스크린샷

devfeijoa added 27 commits March 4, 2025 19:47
@Gyuil-Hwnag Gyuil-Hwnag self-requested a review March 7, 2025 16:13
Copy link
Member

@Gyuil-Hwnag Gyuil-Hwnag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이든 안녕하세요!
이번 미션을 함께하게 된 두루라고 합니다. 잘부탁드립니다!
해당 미션 구현 고생 많으셨습니다!

고민해보시면 좋을만한 내용들 코멘트를 달았으니 확인해주시면 감사하겠습니다.
이번 블랙잭 미션이 도메인 로직이 어렵다보니, 구현을 하시면서 많은 어려움이 있으셨던 것 같군요.
어떤 방법으로 바꿔보면 좋을지에 대해서 순서를 남겨두었습니다.
해당 내용을 참고하여 구현을 해나간다면, 정리가 될 수 있을 것 같습니다.

궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요! 🙏
화이팅입니다💪

Comment on lines +1 to +3

## 소개
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

README 작성 잘해주셨네요~👍

Comment on lines +13 to +15
fun main() {
println("게임에 참여할 사람의 이름을 입력하세요.(쉼표 기준으로 분리)")
val players: List<Player> = readln().split(",").map { name: String -> Player(name.trim()) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

허걱...🫣
main 에 모든 코드를 올인하셨군요!
해당 내용은 이번 미션을 진행하면서 차근차근 리팩토링을 해봅시다!

Comment on lines +19 to +20
println("${players.joinToString { player -> player.name }}에게 2장씩 나누었습니다.")
println("딜러가 한 장을 오픈했습니다.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. View 와 Controller 의 영역을 분리해주세요!

Comment on lines +22 to +25
println("딜러: ${dealer.cards.joinToString { card -> card.prettyString }}")
players.forEach { player ->
println("${player.name}카드: ${player.cards.joinToString { card -> card.prettyString }}")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. main()에 모여있는 함수들을 하나의 함수가 하나의 기능을 하도록 분리해주세요

Comment on lines +40 to +41
println()
println("딜러 카드: ${dealer.cards.joinToString { card -> card.prettyString }} - 결과: ${dealer.getScore()}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. View 에서 사용되는 하드코딩된 상수들을 상수화 + 포맷을 활용해서 정리를 해주세요

Comment on lines +3 to +4
class Deck {
private val aceCards: List<Card> =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 분리되어 있는 객체들을 통합한다면, Deck 부분의 구현이 어느정도 정리가 될 수 있겠군요!

Comment on lines +57 to +76
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
Copy link
Member

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)로 변경한다면, 해당 구현이 정리되겠군요~

Comment on lines +15 to +23
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")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 내용이 들어간 이유가 무엇일까요?

Comment on lines +28 to +32
fun setResult() {
if (hand.getScore() == null) {
result = Result.LOSE
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

게임에 대한 결과를 담당하는 객체로 분리를 해볼 수 있겠군요!

Comment on lines +13 to +14
fun getScore(): Int? {
val aces: List<Card> = value.filter { card: Card -> card.rank is Ace }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

점수를 구해주는 기능을 담당하는 객체로 분리를 해볼 수 있을 것 같습니다~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants