Conversation
Hyeon9mak
left a comment
Hyeon9mak
left a comment
There was a problem hiding this comment.
안녕하세요 범석님, 여러가지 고민이 있으셨군요.
작성자가 복잡하다고 느끼는 코드라면 동료 개발자에게도 힘든 코드겠네요 😱
가장 간단하고 근원적인 해결 방법은, 범석님도 잘 아시겠지만 추상화를 줄이고 구조 자체를 간단하게 바꾸는 것 같아요.
Blackjack 게임의 상태 관리를 위해 XXXState 클래스들을 사용하여 UI에서 현재 상태를 확인하고 상태에 맞는 함수를 실행하려고 했으나, 이 방식이 BlackjackGame과 State 간의 과도한 상호 의존성을 초래했습니다.
ui 는 사용자와 입출력 소통만을 담당할 뿐, 게임이 어떻게 진행되는지 알아서는 안될 것 같아요.
게임의 상태, 진행, 흐름 모두 domain 에서 담당해야 다른 ui(웹뷰 등) 모듈과 연결했을 때도 큰 코드 수정없이 정상적으로 비즈니스 수행이 가능할 것 같습니다. 그게 멀티 모듈의 가장 큰 장점이기도 하겠구요!
현재 ui 쪽에서 알고 있는 로직을 모두 domain 쪽으로 옮겨줄 필요가 있을 것 같습니다.
저는 BlackjackGame 이 지나치게 많은 책임을 갖고 있어서 구조가 복잡해졌다고 생각하는데요,
BlackjackGame 이 갖고 있는 여러가지 동작들을 여러 사람들이 공통적으로 지닌 지식(상식)을 베이스로 각 객체로 분리하고, 테스트 코드를 추가해봐도 좋을 것 같아요.
추가로 궁금하신점 코멘트, DM 주세요
| while (blackjackGame.state !is GameState.End) { | ||
| processGameState(blackjackGame, inputView, resultView) | ||
| } | ||
| processGameState(blackjackGame, inputView, resultView) | ||
| } | ||
|
|
||
| fun playBlackjack(deck: Deck, players: List<Player>, inputView: InputView, resultView: ResultView) { | ||
| val playingPlayers = resultView.showInitialCards(deck, players).toMutableList() | ||
| println() | ||
|
|
||
| playingPlayers.forEach { player -> | ||
| handlePlayerTurn(deck, player, inputView, resultView) | ||
| private fun processGameState(blackjackGame: BlackjackGame, inputView: InputView, resultView: ResultView) { | ||
| when (val gameState = blackjackGame.state) { | ||
| is GameState.PlayerTurn -> processPlayerTurn(gameState, blackjackGame, inputView, resultView) | ||
| is GameState.DealerTurn -> processDealerTurn(blackjackGame, resultView) | ||
| is GameState.InitialDeal -> processInitialDeal(blackjackGame, resultView) | ||
| is GameState.End -> processGameEnd(blackjackGame, resultView) | ||
| } | ||
| } |
There was a problem hiding this comment.
게임의 상태에 따라 어떤 동작을 결정할지는 화면(ui)이 아닌 비즈니스(domain) 영역 같아요.
현재는 게임의 동작을 변경하고자 할 때 presenter, domain 모듈 2개를 모두 확인해야하겠네요.
domain 모듈만 뚝 떼어내어 다른 ui 모듈과 합체하는 것도 불가능하겠어요.. 😭
There was a problem hiding this comment.
ui 는 이름 그대로 입출력만 담당하고, domain 내에서 게임 상태를 관리할 수 있도록 변경해보면 어떨까요?
| fun calculateResult(): Map<BlackjackParticipant, BlackjackResult> { | ||
| val results = mutableMapOf<BlackjackParticipant, BlackjackResult>() | ||
| results[dealer] = calculateDealerResult() | ||
| players.forEach { results[it] = calculatePlayerResult(it) } | ||
| return results | ||
| } | ||
|
|
||
| fun showPlayerCards(playerName: String): List<Card> { | ||
| val player = state.players.find { it.name == playerName } | ||
| ?: throw IllegalArgumentException("${playerName}이라는 플레이어는 없습니다.") | ||
| return player.cards | ||
| } | ||
|
|
||
| private fun calculateDealerResult(): BlackjackResult { | ||
| val dealerScore = dealer.calculateBestValue() | ||
| var win = 0 | ||
| var loss = 0 | ||
| players.forEach { | ||
| if (dealerScore > 21) loss++ | ||
| else if (it.calculateBestValue() > 21) win++ | ||
| else if (dealerScore > it.calculateBestValue()) win++ | ||
| else if (dealerScore <= it.calculateBestValue()) loss++ | ||
| } | ||
| return BlackjackResult(win, loss) | ||
| } |
There was a problem hiding this comment.
딜러기준 승패를 계산하는 로직을 손보기 위해선 BlackjackGame 코드를 76번 라인까지 확인해야겠어요.
처음 코드를 확인하는 동료 개발자 입장에선 원하는 로직을 확인하기 어려울 것 같은데, 이를 개선할 수 있는 방법이 있을까요?
| private fun calculatePlayerResult(player: Player): BlackjackResult { | ||
| val playerScore = player.calculateBestValue() | ||
| val dealerScore = dealer.calculateBestValue() | ||
| return if (dealerScore > 21) { | ||
| BlackjackResult(1, 0) | ||
| } else if (playerScore > 21) { | ||
| BlackjackResult(0, 1) | ||
| } else if (playerScore >= dealerScore) { | ||
| BlackjackResult(1, 0) | ||
| } else { | ||
| BlackjackResult(0, 1) | ||
| } | ||
| } |
There was a problem hiding this comment.
else 예약어를 쓰지 않는다.
객체지향 생활체조 원칙에 따라 요 코드도 개선해보면 좋겠네요!
https://edu.nextstep.camp/s/lepg4Qrl/ls/QZaMqdGU
| if (isDeal.not()) { | ||
| // 다음 플레이어로 넘어감 | ||
| moveToNextPlayerOrDealerTurn(playerTurnState.currentPlayerIndex) | ||
| } else { | ||
| // 카드 받기 | ||
| check(player.canHit() == BlackJackAction.HIT) { "해당 플레이어는 더 이상 카드를 받을 수 없습니다." } | ||
| val nPlayers = players.map { if (it == player) it.receiveCard(deck.drawCard()) else it } |
There was a problem hiding this comment.
불필요한 주석은 제거해주세요!
만약 주석이 필요하다면 코드가 너무 복잡한게 아닌지 고민해봐야겠어요 🤔
복잡한 코드를 개선하기 위한 방법은 무엇이 있을까요?
| import blackjack.card.CardSuit | ||
|
|
||
| class StandardCardProvider : CardProvider { | ||
| internal class StandardCardProvider : CardProvider { |
There was a problem hiding this comment.
internal 키워드 활용!
internal class 와 class 의 차이는 무엇인가요?
| interface DealerStrategy { | ||
| fun decideAction(hand: Hand, deck: Deck): BlackJackAction | ||
| } |
There was a problem hiding this comment.
현재 DefaultDealerStrategy 만 사용되는데, 굳이 인터페이스로 분리가 필요했을까요? 🤔
2 participants
안녕하세요, 리뷰어님!
스탭의 모든 요구사항을 완벽히 만족시키지는 못했지만, 진행 과정에서 어려움을 겪고 있어 리뷰를 요청하게 되었습니다. 😭😭😭
BlackjackGame클래스가 다양한 상태를 관리하며, 이에 따라 게임이 진행되도록 하는 것이었습니다.BlackjackGame의 상태에 따라 사용자와 상호작용하도록 설계하고자 했습니다.BlackjackGame클래스를 세세하게 살펴보니, 구조가 다소 복잡하고 정돈되지 않은 것 같습니다.문제점을 요약하자면 다음과 같습니다.
XXXState클래스들을 사용하여 UI에서 현재 상태를 확인하고 상태에 맞는 함수를 실행하려고 했으나, 이 방식이BlackjackGame과State간의 과도한 상호 의존성을 초래했습니다.이러한 문제점들에 대해 귀중한 리뷰와 조언을 구합니다. 🥹