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

[2단계 - 블랙잭 베팅] 제프리(홍성호) 미션 제출합니다. #922

Open
wants to merge 54 commits into
base: applemint98
Choose a base branch
from

Conversation

AppleMint98
Copy link

체크 리스트

  • 미션의 필수 요구사항을 모두 구현했나요?
  • Gradle test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?
  • 애플리케이션이 정상적으로 실행되나요?
  • 프롤로그에 셀프 체크를 작성했나요?

객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?

1~5점 중에서 선택해주세요.

  • 1 (전혀 충족하지 못함)
  • 2
  • 3 (보통)
  • 4
  • 5 (완벽하게 충족)

선택한 점수의 이유를 적어주세요.

1️⃣ 한 메서드에 오직 한 단계의 들여쓰기(indent)만 한다. ✅
2️⃣ else 예약어를 쓰지 않는다. ✅
3️⃣ 모든 원시값과 문자열을 포장한다. ✅
4️⃣ 한 줄에 점을 하나만 찍는다. ✅
5️⃣ 줄여쓰지 않는다(축약 금지). ✅
6️⃣ 모든 엔티티를 작게 유지한다. ⚠️

  • 딜러의 점수계산 로직이 너무 비대해진 것 같습니다.

7️⃣ 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다. ✅
8️⃣ 일급 콜렉션을 쓴다. ✅
9️⃣ 게터/세터/프로퍼티를 쓰지 않는다. ⚠️

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

안녕하세요, 로키!! : 😄 제프리입니다.
저번 step1 PR을 머지하시면서 남겨주신 미션을 오래 고민했네요..

게임 초기화와 카드 배부를 담은 BlackjackTable 객체를 만들어 게임 진행 로직을 이관했습니다.
덕분에 기존 컨트롤러 역할을 하던 BlakcjackGame 객체가 좀 더 유연하게 설계된 것 같아요.
특히, test fixture 구성 시 알려주신 ... 같이 가변 인자를 사용하는 방법이 정말 좋았습니다! 감사합니다 👍

코드의 몇 가지 문제점이 있습니다. 😢

❓ 1. 딜러의 승패 계산 및 수익 계산 로직이 너무 비대해져, 가독성이 떨어진다고 생각합니다.

현재 딜러는 참가자의 카드들과 비교해, 승 무 패 및 블랙잭 승 을 계산하고, 해당 수익을 계산합니다.
딜러 클래스에 너무 많은 로직이 들어가 있다고 생각해 다른 클래스에 분리를 하려고 시도하였으나,
해당 역할이 모두 딜러의 책임이라고 생각해 모든 로직을 딜러가 담당하도록 한 뒤 메서드분리만 진행하였습니다.

수익 계산 로직까지 포함되어 더욱 가독성이 떨어진 코드가 된 것 같은데, 어떤식으로 리팩터링을 시도할 수 있을까요?

❓ 2. updateBetAmount 등이 사실상 세터이기 때문에, 필드를 가변으로 받고 업데이트를 하는 방식으로 진행되었습니다.

현재 배팅 금액이, 입력받은 그대로 객체의 필드를 변환하는 세터 메서드가 열려있습니다.
승 무 패를 계산하는 로직과 결합시키기 위해 세터를 열어 값을 직접 변경해주고 있는데,
배팅 금액 부분을 어떻게 해야 더욱 안정적으로 코드를 작성할 수 있을까요?

…tionalCard 수정

내부 구현 로직이 직접적으로 드러나는 이름을 수정합니다
countAces()를 한번만 호출하도록 수정합니다.
기존 Bust 기준으로 쓰였던 21을, 의미가 더 잘 드러나도록 최고점수로 수정합니다.
return 값에 모든 로직을 통합했던 기존 코드를, 변수를 추출하여 가독성을 높이도록 수정합니다.
Copy link

@Rok93 Rok93 left a comment

Choose a reason for hiding this comment

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

제프리 2단계도 빠르게 잘 구현해주셨네요. 👍👍
질문 주신 내용에 대한 제 생각과 추가 코멘트 남겨두었는데, 확인해서 반영 부탁드릴게요. 🙏

Comment on lines 26 to 28
public void updateBetAmount(double betAmount) {
this.betAmount = betAmount;
}
Copy link

Choose a reason for hiding this comment

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

질문2) updateBetAmount 등이 사실상 세터이기 때문에, 필드를 가변으로 받고 업데이트를 하는 방식으로 진행되었습니다.
현재 배팅 금액이, 입력받은 그대로 객체의 필드를 변환하는 세터 메서드가 열려있습니다.
승 무 패를 계산하는 로직과 결합시키기 위해 세터를 열어 값을 직접 변경해주고 있는데,
배팅 금액 부분을 어떻게 해야 더욱 안정적으로 코드를 작성할 수 있을까요?

고민하시는 포인트가 정확히 어떤건지가 궁금합니다.
단순히 updateBetAmount를 setter처럼 쓰고있는 점 때문일까요?

이미 알고 계실 수도 있지만 setter 메서드를 쓰지말라는 의미를 한번 되짚어보면 좋을 것 같아요. 😃

betAmount = betAmount; 하는 행위는 Gamer의 입장에서 무엇을 하는걸까요? 🤔
행위적 관점이 드러나는 네이밍으로 변경해보면 어떨까요?

또한 만약 betAmount가 0원 이하이면 어떻게되는걸까요?
0원 이하도 허용이 되는걸까요? 🤔

이런 비즈니스 로직들이 들어가있다면 제프리의 우려(?)가 조금은 해결이될지 궁금합니다.
혹시 이 문제가 아니라면 추가로 코멘트 달아주세요. 😉

import blackjack.view.OutputView;
import java.util.List;

public class BlackjackGame {
Copy link

Choose a reason for hiding this comment

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

BlackjackGame은 Controller의 역할을 하는걸까요? 🤔
네이밍을 BlackjackGame 으로 두신 이유가 궁금합니다.
언뜻보기에는 도메인 모델처럼 보이는 네이밍이기 때문에 코드를 보기 전까지는 이게 Controller의 역할을 할거라고 생각하지 못했던 것 같아요. 🤔

Controller를 의도하신거라면 네이밍을 xxxController와 같이 변경해보면 어떨까요?

import blackjack.gametable.gambler.Players;
import java.util.List;

public class BlackjackTable {
Copy link

Choose a reason for hiding this comment

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

...Table이라는 네이밍은 어떤 의미로 네이밍하신건지가 궁금합니다. 😃
언뜻보기에는 DB의 테이블처럼 느껴지는 것 같기도해서요. 🤔

해당 객체가 약간 DB 스러운 느낌이 많이 드는 것 같아요.

저는 deck, dealer, players를 상태로 가진 BlackjackGame 객체에게 게임의 흐름을 제어하는 역할을 위임해볼 수 있을 것 같다고 생각하는데, BlackjackTable을 도메인 모델로 변경해보면 어떨까요? 😃

Comment on lines 25 to 30
public void initializeGame() {
Cards dealerInitialCards = deck.drawInitialCards();
List<Cards> playerInitialCards = deck.drawInitialCards(getPlayersCount());
dealer.initializeHand(dealerInitialCards);
players.initializeHands(playerInitialCards);
}
Copy link

Choose a reason for hiding this comment

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

BlackjackTable 객체의 입장에서는 Game을 initialized 하는거지만, dealer라 player의 입장에서는 초기 패를 뽑는(or 받는다) 행위를 수행하는게 아닐까요? 🤔

네이밍에서 객체의 행위를 드러내주면 좋을 것 같아요. 😉

Comment on lines 25 to 30
public void initializeGame() {
Cards dealerInitialCards = deck.drawInitialCards();
List<Cards> playerInitialCards = deck.drawInitialCards(getPlayersCount());
dealer.initializeHand(dealerInitialCards);
players.initializeHands(playerInitialCards);
}
Copy link

Choose a reason for hiding this comment

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

참가자의 수만큼 카드를 미리 뽑아두기보다는, 매번 deck 객체로부터 초기패를 받도록 구현해도되지 않을까 싶어요. 🤔
개인적인 생각으로는 미리 뽑아두는 방식은 실수할 여지가 보이는 것 같아서, deck 객체에게 초기패를 알아서 뽑아주도록 구현하는게 조금 더 낫지 않을까 싶어요. 😃

Suggested change
public void initializeGame() {
Cards dealerInitialCards = deck.drawInitialCards();
List<Cards> playerInitialCards = deck.drawInitialCards(getPlayersCount());
dealer.initializeHand(dealerInitialCards);
players.initializeHands(playerInitialCards);
}
public void initializeGame() {
dealer.initializeHand(deck.drawInitialCards());
players.initializeHands(deck);
}
// Players Class
private void initializeHands(Deck deck) {
players.forEach(player -> player.initializeHand(deck.drawInitialCards()));
}

.collect(Collectors.toList());
Collections.shuffle(cards);
return new Deck(new Cards(cards));
}

public Cards drawInitialCards() {
return new Cards(new ArrayList<>(List.of(cards.drawOneCard(), cards.drawOneCard())));
return new Cards(cards.drawInitialCards());
Copy link

Choose a reason for hiding this comment

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

cards.drawInitialCards()의 결과 자체가 Cards 객체일 수는 없을까요? 🤔

Cards 객체가 복수장의 카드를 draw한 결과가 Cards 인 것은 자연스럽다고 생각하는데, 제프리는 어떻게 생각하시나요? 😃

return cards.openDealerInitialCards();
}

public void updateBetAmounts(Players players) {
Copy link

Choose a reason for hiding this comment

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

(질문1) 딜러의 승패 계산 및 수익 계산 로직이 너무 비대해져, 가독성이 떨어진다고 생각합니다.
현재 딜러는 참가자의 카드들과 비교해, 승 무 패 및 블랙잭 승 을 계산하고, 해당 수익을 계산합니다.
딜러 클래스에 너무 많은 로직이 들어가 있다고 생각해 다른 클래스에 분리를 하려고 시도하였으나,
해당 역할이 모두 딜러의 책임이라고 생각해 모든 로직을 딜러가 담당하도록 한 뒤 메서드분리만 진행하였습니다.
수익 계산 로직까지 포함되어 더욱 가독성이 떨어진 코드가 된 것 같은데, 어떤식으로 리팩터링을 시도할 수 있을까요?

말씀하신 것 처럼 현재 딜러는 (1)참가자의 카드들과 비교해, 승 무 패 및 블랙잭 승 을 계산하고 (2) 해당 수익을 계산 합니다.
즉 두 가지의 역할을 하고 있는 상황이라고 생각해요. 딜러가 한 가지 역할만하도록 다른 역할 하나를 다른 객체에게 위임해보는 방식을 고려해볼 수 있을 것 같아요. 🤔

저도 가볍게 생각해보고 제안드린 방식이라, 한번 고민해보시고 가능할 것 같다면 리팩터링해보면 좋을 것 같아요. 😉

Comment on lines 51 to 64
if (matchResult == MatchResult.BLACKJACK_WIN) {
double blackjackAmount = betAmount * 1.5;
player.updateBetAmount(blackjackAmount);
return -blackjackAmount;
}
if (matchResult == MatchResult.WIN) {
return -betAmount;
}
if (matchResult == MatchResult.LOSE) {
player.updateBetAmount(-betAmount);
return betAmount;
}
player.updateBetAmount(0);
return 0;
Copy link

Choose a reason for hiding this comment

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

각각의 MatchResult 요소마다 배팅 배당(?)이 다른 것으로 보이는데요.
배팅 배당율을 MatchResult의 상태 값으로 두면 어떨까요?

그렇다면 분기문을 없앨 수도 있지 않을까요?

Suggested change
if (matchResult == MatchResult.BLACKJACK_WIN) {
double blackjackAmount = betAmount * 1.5;
player.updateBetAmount(blackjackAmount);
return -blackjackAmount;
}
if (matchResult == MatchResult.WIN) {
return -betAmount;
}
if (matchResult == MatchResult.LOSE) {
player.updateBetAmount(-betAmount);
return betAmount;
}
player.updateBetAmount(0);
return 0;
double profit = betAmount * 1.5;
player.updateBetAmount(profit);
return profit;

return -blackjackAmount;
}
if (matchResult == MatchResult.WIN) {
return -betAmount;
Copy link

Choose a reason for hiding this comment

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

여기에서는 player.updateBetAmount(betAmount); 가 빠져있는데, MatchResult가 WIN인 경우에 player의 betting 금액에 대한 검증을 해보면 어떨까요?


@ParameterizedTest
@CsvSource(value = {
"NINE, EIGHT, ACE, KING, THREE, FOUR, 5000",
Copy link

Choose a reason for hiding this comment

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

첫 번째 케이스는 dealer는 17점, player1은 21점(블랙잭), player2는 7점이라서 딜러 기준에서 + 20000 - (10_000 * 1.5) = 15,000 원이겠군요?

테스트 코드를 한눈에 파악하기가 힘든 것 같아요. 🥲
@MethodSource와 fixture 함수 등을 이용해서, 조금 더 테스트의 가독성을 높여볼 수는 없을까요?

@AppleMint98
Copy link
Author

안녕하세요, 로키! 😃

코멘트를 확인하고 반영해보았습니다!

말씀드릴 내용은 크게 세 가지입니다.


첫째, 세터에 대한 생각의 변화입니다.

기존에는 객체지향 생활 체조 원칙에서 "세터를 사용하지 말라"고 해서 그냥 쓰면 안 된다고만 생각했습니다.
하지만 이번 기회를 통해 왜 쓰면 안 되는지에 대해 고민해볼 수 있었습니다.
현재 설계에서는 배팅 금액이 변경될 수 있는 것이 자연스럽고, 메서드명을 통해 의도를 명확히 드러내는 것이 더 중요하다고 판단했습니다.


둘째, 실행 로직의 위치에 대한 고민입니다.

초기에 Controller 가 실행 로직을 담당했던 이유는, 사용자의 y/n 입력이 모델 객체에 직접적인 영향을 주었기 때문입니다.

pobi는 한장의 카드를 더 받겠습니까?(예는 y, 아니오는 n)
y
pobi카드: 2하트, 8스페이드, A클로버
pobi는 한장의 카드를 더 받겠습니까?(예는 y, 아니오는 n)
n

y 를 몇 번 입력받을지 미리 결정할 수 없어서, 입출력 예시에 맞게 구현하려면 실행 로직을 가진 객체가 InputViewOutputView 를 포함해야 한다고 판단했습니다.
이에 따라, BlackjackGame 이 실행 로직을 담당하도록 하고, InputViewOutputView 를 주입하는 방식으로 변경했습니다.


셋째, 승패 판별 로직을 BlackjackRule 객체로 위임했습니다.

기존 Dealer 는 승패 계산과 배당 금액 계산을 모두 담당하고 있었습니다.
그러나 실제 카지노에서도 딜러는 배당을 관리하고, 승패 판별은 블랙잭 규칙에 따라 결정되는 것이 더 자연스럽다고 생각했습니다. 기존 딜러의 역할이 과중되었다고도 생각했구요.
따라서 블랙잭의 기본 규칙을 적용하는 BlackjackRule 객체를 만들어, 승패 판별 책임을 위임했습니다.


감사합니다 로키!
그동안 이름을 대충짓는 경향이 있었는데, 행위와 의도, 추상화 단계 등을 고려하는 등 네이밍 실력(?) 이 올라간 것 같아요. 😄
사소해 보이는 것도 중요하게 생각하겠습니다!

@AppleMint98 AppleMint98 requested a review from Rok93 March 19, 2025 17:32
Copy link

@Rok93 Rok93 left a comment

Choose a reason for hiding this comment

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

제프리 피드백 반영 잘해주셨습니다. 👍👍
전체적으로 잘 진행해주셨지만,
마지막으로 조금만 더 짚고넘어가면 좋을 부분이 보여서 추가 코멘트 남겨두었어요. 😃
확인 부탁드릴게요. 💪

@@ -0,0 +1,33 @@
package blackjack;
Copy link

Choose a reason for hiding this comment

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

Controller는 다른 계층과 뎁스를 맞춰서 controller 라는 별도의 패키지에 두면 어떨까요? 🤔

@@ -0,0 +1,95 @@
package blackjack.gametable;
Copy link

Choose a reason for hiding this comment

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

game table 이라는 네이밍은 어떤 의도가 담긴걸까요? 🤔

하위의 패키지들을 보았을 때, domain 같은 네이밍이 조금 더 적절하지 않을까요?

}

outputView.printInitialGameSettings(blackjackGame);
blackjackGame.playGame(inputView, outputView);
Copy link

Choose a reason for hiding this comment

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

y 를 몇 번 입력받을지 미리 결정할 수 없어서, 입출력 예시에 맞게 구현하려면 실행 로직을 가진 객체가 InputView 와 OutputView 를 포함해야 한다고 판단했습니다.
이에 따라, BlackjackGame 이 실행 로직을 담당하도록 하고, InputView 와 OutputView 를 주입하는 방식으로 변경했습니다.

이 부분은 이런 의도로 말씀드렸던거는 아니였어요. 👀
진행하신 방식은 결국 (Domain) Model이 View에 의존성을 가지게되는 상황이라 좋은 방식은 아니라고 생각합니다. 🙄

아무래도 블랙잭 미션 요구사항은 웹 MVC의 방식이 적절한 상황이다보니 고민이 많이되셨을 것 같아요. 😭
차라리 Controller가 이 흐름을 처리하는 구조가 View와 Model이 분리되는 측면에서는 더 낫다고 생각해요. 🤔

Copy link

Choose a reason for hiding this comment

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

완벽하진 않을 수도 있지만, 조금 개선을해보자면...
Conrtoller에서 BlackJackGame 객체에게 게임 진행 여부를 물어보고 현재 턴인 플레이어를 찾도록 메시지를 보내볼 수 있을 것 같아요.

    while(blackJackGame.isPlaying() {
         Player player = blackJackGame.findCurrentTurnPlayer();
         // player에게 패를 더받을지 질의 
        // 패를 주거나 혹은 턴을 종료한다. 
    }

BlackjackController 를 controller패키지 하위로 변경하고,
blackjacktable패키지를 도메인으로 변경합니다.
플레이어 입력 로직을 controller에서 받도록 변경하고,
player의 상태를 조사해 게임을 진행하도록 수정합니다.
@AppleMint98
Copy link
Author

안녕하세요 로키!! 🙋

꼼꼼히 봐주셔서 감사해요! 입력받는 로직이 MVC패턴으로 구현하기 어려워, 어떻게 해야할 지 고민을 많이 했습니다.
제가 의도를 잘못 이해하고 있었네요!! 죄송합니다 🙇

말씀주신 방법대로 바꾸어보았습니다!
플레이어의 상태 ( 턴이 남았는지, 턴이 끝났는지.. ) 를 enum으로 만들었고,
턴이 남은 상태 의 플레이어를 찾아와 게임을 진행시키도록 변경했습니다.
플레이어가 턴을 멈추면, 플레이어의 상태를 턴이 끝난 상태 로 변경하는 방식으로 구현했습니다.

또한, 게임이 진행중인지를 물어보는 isPlaying() 메서드가, 턴이 남아있는 상태의 플레이어가 없는 경우와 동일하다고 생각해서
턴이 남은 플레이어의 수를 계산해서 0일때까지 진행하도록 바꿔보았습니다.

감사합니다 😄

@AppleMint98 AppleMint98 requested a review from Rok93 March 21, 2025 18:03
Copy link

@Rok93 Rok93 left a comment

Choose a reason for hiding this comment

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

제프리 피드백 반영 잘해주셨네요. 👍
마지막으로 BlackjackGame 클래스에 대한 테스트만 보강해주시면 될 것 같아요.
추가적으로 몇몇 코멘트 남겨두었는데, 같이 확인 부탁드릴게요. 🙂

다음 리뷰 요청 때는 블랙잭 미션은 이만 종료하도록하겠습니다. 😉
다음 미션도 같이 병행하시느라 힘드시겠지만 조금만 더 힘내봐요. 💪💪

@@ -0,0 +1,9 @@
package blackjack.constant;
Copy link

Choose a reason for hiding this comment

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

enum은 모두 constant 패키지에 위치한 것 같은데요. 👀
정말로 상수의 역할 정도만 한다면 constant 패키지에 두는 것도 나쁘지 않지만, 대다수는 블랙잭이라는 도메인에 필수적인
도메인 객체로 보이는데요. 😃

MatchResult, TrumpRank, TrumpSuit, GamblerStatus 등은 domain 패키지에 두는게 더 좋지 않을까 생각되는데 제프리는 어떻게 생각하시나요?

Copy link
Author

@AppleMint98 AppleMint98 Mar 24, 2025

Choose a reason for hiding this comment

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

맞습니다! constant 에 있기에는, 도메인 객체에 더 가까운 것 같아요!

TrumpRank, TrunkSuit는 card 도메인에 더 가까운거 같고,
GamblerStatus 는 gambler 도메인에 더 가까운거 같아요 😄

MatchResult를 옮길 패키지가 애매해서, 관련 역할을 찾아보다가
BlackjackRule 이라는 기존의 객체가 evaluate() 메서드를 사용해 MatchResult 를 결정한다고 생각해서,
Rule 대신 Judge 라는 네이밍으로 바꾸고 judgment 라는 새로운 패키지를 만들어 옮겼습니다!

return players.stream()
.filter(player -> player.getPlayerName().equals(playerName))
.findFirst()
.orElseThrow(() -> new IllegalArgumentException("[ERROR] 사용자를 찾을 수 없습니다."));
Copy link

Choose a reason for hiding this comment

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

예외 메시지에 어떤 playerName 이 존재하지 않는지 노출해주면, 해당 유저가 진짜 존재하지 않는건지 혹은 로직에 오류가있는건지 확인하는데 도움이 될 것 같아요. 😃

Comment on lines +8 to +10
public static final int MAX_SCORE = 21;
public static final int SOFT_ACE_DIFFERENCE = 10;
public static final int BLACKJACK_CARD_COUNT = 2;
Copy link

Choose a reason for hiding this comment

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

이 상수들은 모두 Cards 클래스 내부에서만 사용되는 것 같아요. 👀
접근 제한자의 수정이 필요해보이네요.

Comment on lines +46 to +51
String playerName = blackjackGame.findCurrentTurnPlayerName();
while (inputView.readOneMoreCardResponse(playerName).equals(UserAction.HIT)) {
blackjackGame.addCardTo(playerName);
outputView.printPlayerCards(blackjackGame, playerName);
}
blackjackGame.endPlayerTurn(playerName);
Copy link

Choose a reason for hiding this comment

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

여기서 꼭 while로 둘 필요가 없을 것 같아요. 턴을 종료한게 아니라면 findCurrentTurnPlayer()의 결과로 동일한 플레이어가 반환되지 않을까 싶어요.

여기서는 단순히 if 문으로 처리를 해주는게 좋을 것 같아요. 😃

또한 PlayerName을 BlackjackGame 객체에게 전달해줄 필요 없이, 스스로 현재 턴의 유저를 조회해서 처리하도록해도 되지 않을까 싶어요.

Suggested change
String playerName = blackjackGame.findCurrentTurnPlayerName();
while (inputView.readOneMoreCardResponse(playerName).equals(UserAction.HIT)) {
blackjackGame.addCardTo(playerName);
outputView.printPlayerCards(blackjackGame, playerName);
}
blackjackGame.endPlayerTurn(playerName);
Player player = blackjackGame.findCurrentTurnPlayer();
if (inputView.readOneMoreCardResponse(player.getPlayerName()).equals(UserAction.HIT)) {
blackjackGame.drawCurrentTurn(); // 내부에서 findCurrentTurnPlayer() 하여 player.changeStatusToEnd() 진행.
outputView.printPlayerCards(blackjackGame, playerName);
return;
}
blackjackGame.endCurrentPlayerTurn(); // 내부에서 findCurrentTurnPlayer() 하여 player.endPlayerTurn()

Copy link
Author

Choose a reason for hiding this comment

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

오 생각하지 못했습니다!
플레이어가 N을 누를때 까지, 해당 플레이어에게 계속 카드를 더 받을지(Y/N) 를 물어봐야 된다고만 생각해서 while로 구현했는데,
플레이어가 상태를 바꾸기 전까지 findCurrentTurnPlayer() 로 찾아서 더 받을지(Y/N)을 물어보면
while을 안쓰고도 구현이 가능할 것 같아요!!
감사합니다 👍 👍

return cards.openPlayerInitialCards();
}

public void changeStatusToEnd() {
Copy link

Choose a reason for hiding this comment

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

player 객체의 입장에서 행위적 관점으로 네이밍해보면 어떨까요? 😃

턴을 종료하다 와 같은 네이밍이 조금 더 직관적이지 않을까요? 😃

}

private void validatePlayerCount(List<String> playerNames) {
if (playerNames.isEmpty() || playerNames.size() > 6) {
Copy link

Choose a reason for hiding this comment

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

6은 매직넘버로 보여요. 👀

Comment on lines +16 to +22
public BlackjackGame(List<String> playerNames) {
validatePlayerCount(playerNames);
this.deck = Deck.initialize();
this.dealer = new Dealer();
this.players = registerPlayers(playerNames);
distributeStartingHands();
}
Copy link

Choose a reason for hiding this comment

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

지금처럼 BlackjackGame 객체를 생성하게되면, BlackjackGame 객체에 대한 테스트 작성이 어려울 것 같아요. 🤔
또한 객체를 생성하는 시점에 바로 distributeStartingHands() 하기보다는, 생성자에서는 BlackjackGame 객체를 생성만 하고, 이후에 blackjackGame 객체에게 distributeStartingHands() 메시지를 보내면 어떨까요?

추가적으로 processDealerTurn, updateBetAmount, findPlayer, findCurrentTurnPlayerName, addCardTo, isPlaying 등등

BlackjackGmae의 public 메서드에 대한 테스트가 없어보이는데요.
이 부분 테스트 추가 부탁드릴게요.

Copy link
Author

Choose a reason for hiding this comment

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

넵 작성하겠습니다! 😄

컨트롤러는 별도로 테스트코드를 작성하지 않아서, 초기에는 전부 Controller에서 담당하던 로직이라 테스트를 작성하지 않았습니다. MVC 패턴의 이해가 부족한 상태에서, 주변을 따라가기에 급급했던것 같아요.

왜 컨트롤러는 테스트코드를 따로 작성하지 않지? 라는 고민이 부족했던것 같아요. 컨트롤러는 테스트 코드를 작성하지 않아도 된다는 법칙이 있는것도 아닌데, 그냥 페어분들이 컨트롤러는 따로 작성하지 않아서 안하는 것이 맞다고 생각했어요. 🙇
지금 생각나는 바로는, 컨트롤러는 비즈니스 로직을 수행하기 보다는, 입, 출력과 서비스를 연결하는 역할을 하기 때문에, 중요도가 상대적으로 떨어져서 라고 생각합니다!

현재 BlackjackGame 객체같은 경우는 View와 Domain을 연결해주는 Controller의 역할이 아니라, 도메인들간의 비즈니스 로직을 수행하는 서비스 느낌의 도메인이므로 테스트 작성이 필수적인것 같아요!

BlackjackGame 이외에도 누락된 테스트 코드도 보완해서 올리겠습니다!

}

public boolean isPlaying() {
return players.countInProgressPlayers() != 0;
Copy link

Choose a reason for hiding this comment

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

총 몇명의 진행중 플레이어가 존재하지는를 물을 뒤, 카운팅하기보다는
players 객체에게는 진행중인 player가 있는지 여부를 물어봐도 되지 않을까요?

@AppleMint98 AppleMint98 requested a review from Rok93 March 25, 2025 05:49
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