[2단계 - 블랙잭 베팅] 커찬(이충안) 미션 제출합니다. #11

leegwichan · 2024-03-15T05:52:51Z

부족한 점이나 궁금한 점이 있으시다면 코멘트 부탁드립니다!

설계에서 고려했던 부분

최소한의 변경

이전에 Dealer가 각 플레이어의 승패를 판단하는 기능을 들고 있었습니다. 이것이 승패에 따른 수익을 구하는 것으로 복잡해지면서, 어떻게 복잡성을 해결해야 하는지 고민했습니다. 처음에는 승패를 판단하는 책임을 다른 곳으로 이동할까 하였지만, 굳이 책임을 이동하지 않고 다른 객체를 통해 해결하면 되겠다는 생각을 했습니다. 그래서 "각 플레이어와 승부 여부를 판단한다."는 책임은 유지하되, 다른 객체(HandRank, SingleMatchResult)들을 도입하여 Dealer의 역할 크기를 줄였습니다.

`HandRank` 도입

이번 요구사항에서는 딜러의 카드 상황(블랙잭, 버스트, 그 외)과 플레이어가 카드 상황(블랙잭, 버스트, 그 외)에 따라 승패 여부 판단이 복잡했습니다. 그래서 Hand에서 계산되는 점수와 카드 개수에 따라 HandRank를 생성하는 HandRankFactory를 도입했습니다. 이 때 HandRank의 구현체에 따라 승리 판단을 할 수 있어, 각 경우마다 판단하는 기능들을 나누어 줄 수 있었습니다.

`BetAmount`, `Profit` 도입

'배팅 금액'과 '이익'을 관리하는 객체들을 도입했습니다. 단순히 int 형식으로 관리하는 것보다 타입을 명시함으로써 어떤 상황에서 쓰이는 것인지 명시할 수 있었습니다. 그리고 특정 연산만을 제공하여 원하는 방식과 다르게 사용되지 않도록 할 수 있었습니다.

* docs : 기능 명세 및 클래스 설계 작성 * feat (Card) : 카드에 따라 점수를 반환하는 기능 구현 * feat (Deck) : 카드를 한 장 뽑는 기능 구현 * feat (Dealer) : 딜러의 점수 계산 및 카드를 뽑을 수 있는지 확인하는 메서드 구현 * style : 가독성을 위해 클래스 상단에 개행 추가, 클래스 아래 불필요한 개행 제거 * feat (Dealer) : 딜러 턴 진행 구현 * feat (Player) : 점수 계산 기능 구현 * feat (Player) : 딜러의 점수 계산 및 카드를 뽑을 수 있는지 확인하는 메서드 구현 Co-authored-by : zangsu <[email protected]> * feat (Player) : 카드를 한 장 추가하는 기능 구현 Co-authored-by: zangsu <[email protected]> * refactor (Dealer) : 요구 사항을 반영을 위해 턴 진행 기능을 카드를 한 장 추가하는 기능으로 변경 Co-authored-by: zangsu <[email protected]> * refactor : 도메인 구분을 위한 participant 패키지 분리 Co-authored-by: zangsu <[email protected]> * refactor (Participant) : 참가자의 공통 책임을 묶기 위해, 추상 클래스 사용 Co-authored-by: zangsu <[email protected]> * docs : 이름에 대한 요구사항 추가 Co-authored-by: zangsu <[email protected]> * feat (Name) : 요구사항에 따른 이름 검증 기능 구현 Co-authored-by: zangsu <[email protected]> * feat (Player) : 플레이어가 이름을 갖도록 구현 Co-authored-by: zangsu <[email protected]> * refactor : 참가자 관련 코드 개선 - Player 가 Name 을 가지도록 생성자 변경 - 추상 클래스의 생성자 접근제어자 변경 - Dealer 기본 생성자 추가 Co-authored-by: zangsu <[email protected]> * feat (Participant) : 게임 시작 후 카드를 두 장 뽑는 기능 구현 Co-authored-by: zangsu <[email protected]> * feat (Dealer) : 딜러가 플레이어와 승패를 확인하는 기능 구현 Co-authored-by: zangsu <[email protected]> * feat (Player) : 플레이어가 딜러와 승패를 확인하는 기능 구현 Co-authored-by: zangsu <[email protected]> * refactor (Deck) : 사용되지 않는 생성자 제거 Co-authored-by: zangsu <[email protected]> * feat (Players) : 제한된 수의 플레이어만 가지는 Players 생성 Co-authored-by: zangsu <[email protected]> * feat (Players) : 플레이어가 중복되지 않도록 검증 기능 구현 Co-authored-by: zangsu <[email protected]> * feat (Deck) : 카드가 섞인 덱이 생성되도록 구현 Co-authored-by: zangsu <[email protected]> * feat (Players) : 모든 플레이어들이 시작 카드를 뽑는 기능 구현 Co-authored-by: zangsu <[email protected]> * feat (BlackJackGame) : 전반적인 제어 기능 구현 Co-authored-by: zangsu <[email protected]> * feat (InputView) : 한장의 카드를 더 받을지 요청하는 기능 구현 Co-authored-by: zangsu <[email protected]> * feat (OutputView) : 시작 카드를 출력하는 기능 구현 Co-authored-by: zangsu <[email protected]> * feat (OutputView) : 카드 출력 기능 구현 * refactor (Participant) : 카드 출력 개수를 정하는 책임을 View로 넘기기 위해 시작 카드를 건네는 기능 제거 Co-authored-by: zangsu <[email protected]> * feat (Dealer) : 모든 플레이어를 상대로 승패를 계산하는 기능 구현 Co-authored-by: zangsu <[email protected]> * feat (OutputView) : 최종 결과 출력 기능 구현 Co-authored-by: zangsu <[email protected]> * refactor (Participant) : 카트 점수 계산 기능의 인덴트 개선 Co-authored-by: zangsu <[email protected]> * style : 테스트의 DisplayName 변경 Co-authored-by: zangsu <[email protected]> * refactor (Card) : Value, Shape 클래스 분리 및 패키지 변경 Co-authored-by: zangsu <[email protected]> * refactor (BlackJackGame) : 가독성을 위해 메서드 분리 Co-authored-by: zangsu <[email protected]> * refactor (OutputView) : 메서드 순서 조정 * refactor (CardTest) : 다양한 상황의 카드들을 픽스쳐로 추출 Co-authored-by: zangsu <[email protected]> * style : 테스트 이름에서 경계값 제거 Co-authored-by: zangsu <[email protected]> * refactor : 이름에 관한 픽스쳐 NameTest에 통합 Co-authored-by: zangsu <[email protected]> * style : 사용하지 않는 import 문 제거 Co-authored-by: zangsu <[email protected]> * style : 의미 없는 공백 제거 * feat : 생성 단계에서 null을 입력받았을 경우, NullPointerException을 던지도록 변경 * refactor (Participant) : 외부에서 사용하지 않는 변수의 접근 제어자 변경 * docs : README에서 구현에 관련된 내용들 삭제 Co-authored-by: zangsu <[email protected]> * style (OutputView) : 메서드 이름을 추상적으로 변경 Co-authored-by: zangsu <[email protected]> * refactor (PlayerTurn) : 가독성을 위해 `BiConsumer` -> `PlayerTurn` 으로 변경 Co-authored-by: zangsu <[email protected]> * refactor (Value) : 추후 유지보수의 용이성과 enum 인스턴스 자체가 값에 대한 책임을 가지기 위해 `maxValue` 필드 추가 Co-authored-by: zangsu <[email protected]> * refactor (Deck) : 패키지 변경 Co-authored-by: zangsu <[email protected]> * refactor : 테스트를 간결하게 작성하기 위해, Test용 생성자를 사용 Co-authored-by: zangsu <[email protected]> * style : import 문 정리 및 TODO 제거 Co-authored-by: zangsu <[email protected]> * feat (Hand) : 손패 계산의 책임을 분리하기 위해 Hand 구현 Co-authored-by: zangsu <[email protected]> * feat (Hand) : 버스트 상태를 판단하는 기능 구현 Co-authored-by: zangsu <[email protected]> * feat (Hand) : 블랙잭 판단 기능 구현 Co-authored-by: zangsu <[email protected]> * feat (Hand) : 카드를 추가하는 기능 구현 Co-authored-by: zangsu <[email protected]> * feat (Hand) : 빈 손패인지 확인하는 기능 구현 Co-authored-by: zangsu <[email protected]> * refactor (Participant) : 카드와 관련된 책임들을 Hand 로 위임 Co-authored-by: zangsu <[email protected]> * feat (Dealer) : 승패 조건에 블랙잭 추가 Co-authored-by: zangsu <[email protected]> * refactor : 가독성을 위해 테스트 픽스처 분리 Co-authored-by: zangsu <[email protected]> * refactor : 도메인 규칙과 직결되는 경계값을 픽스처에서 제거 Co-authored-by: zangsu <[email protected]> * refactor : 시작 카드가 몇 개 보여지는지의 책임을 뷰에서 도메인으로 이동 Co-authored-by: zangsu <[email protected]> * refactor : DTO 사용을 최소화하기 위해, 원시값을 전달 Co-authored-by: zangsu <[email protected]> * refactor (Hand) : 가독성 개선을 위해 점수 계산 기능 변경 Co-authored-by: zangsu <[email protected]> --------- Co-authored-by: zangsu <[email protected]>

…qualName() 을 사용함

- Controller에서는 getPlayers()를 이용하여 각 플레이어의 턴을 진행

…vate 생성자 추가

robinjoon

코드 잘 봤습니다!

저와는 다르게 상태 패턴을 적용하셨는데, 상태패턴을 적용하면서 불편햇던 점과, 편했던 점이 무엇인지 궁금합니다!

robinjoon · 2024-03-16T10:46:17Z

src/main/java/blackjack/domain/card/Card.java

+public class Card {
+


record로 사용한다면 코드가 더 깔끔해질 것 같아요.

robinjoon · 2024-03-16T10:50:40Z

src/main/java/blackjack/domain/handrank/HankRank.java

@@ -0,0 +1,12 @@
+package blackjack.domain.handrank;
+
+public interface HankRank {


Hank가 무슨 의미인가요?

HandRank는 내가 가진 Hand에 따라 거기에 등급을 부여한다고 생각하면 됩니다.

승패 판단에서 필요한 정보는 "해당 카드가 블랙잭인가?", "버스트인가?", "그 외 경우라면 점수가 몇점인가?" 라는 정보였고 이에 따라서 필요한 승패 판단 로직이 달라지는 것이었습니다. 그래서 Hand의 정보에 따라 HandRank라는 적절한 객체를 만들어주고, 두 HandRank에 따라 승패 판단을 하도록 했습니다.

HandRank는 내가 가진 Hand에 따라 거기에 등급을 부여한다고 생각하면 됩니다.

승패 판단에서 필요한 정보는 "해당 카드가 블랙잭인가?", "버스트인가?", "그 외 경우라면 점수가 몇점인가?" 라는 정보였고 이에 따라서 필요한 승패 판단 로직이 달라지는 것이었습니다. 그래서 Hand의 정보에 따라 HandRank라는 적절한 객체를 만들어주고, 두 HandRank에 따라 승패 판단을 하도록 했습니다.

클래스 이름에 오타가 있는 것 같다는 의도였습니다!

robinjoon · 2024-03-16T10:53:44Z

src/main/java/blackjack/domain/handrank/HankRank.java

+
+public interface HankRank {
+
+    SingleMatchResult matchAtDealer(HankRank playerHandRank);


매개변수로 Player의 HankRank가 들어오는 것을 가정한 코드인 것 같습니다. 만약, 딜러의 HankRank가 매개변수로 들어온다면 어떻게 되나요?

뭐 망하는 거죠 허허...
이걸 컴파일 타임에서 방지하는 방법을 생각 중인데 잘 모르겠네요 ㅠ_ㅠ

3Juhwan

커찬~ 안녕하세요 😀
훌륭한 코드 감사해요. 리뷰하는 제가 인사이트를 많이 얻었어요.
몇가지 코멘트 남겼는데, 확인 부탁드려요~

그리고 상태 패턴을 사용하셨는데 사용하면서 느낀 점, 장점과 단점이 궁금해요!

3Juhwan · 2024-03-16T11:20:27Z

src/main/java/blackjack/BlackJackGame.java

+    private void printResult(Dealer dealer, Players players) {
+        outputView.printEndingStatus(dealer, players);
+
+        Profit dealerProfit = dealer.calculateDealerProfit(players);
+        Map<Player, Profit> playersProfit = dealer.calculatePlayersProfit(players);
+        outputView.printMatchResult(dealerProfit, playersProfit);
+    }
+}


이 부분에만 개행이 있네요! 당연히 의도한 바라고 생각이 드는데요~ 어떤 의도인지 궁금해요 💭
만약 개행을 사이로 컨텍스트가 다르다는 근거라면, 메서드를 분리하는 것은 어떨까요? 이에 대한 커찬의 의견이 궁금해요 💭

솔직히 말하면, 그냥 생각없이 해버렸다는 게 맞을 것 같아요.

굳이 무의식을 분석해보자면,

run() 부분에서 결과를 출력하는 부분을 printResult()로 구분함

이 로직이 printEndingStatus와, printMatchResult를 호출하는 부분으로 나눌 수 있는데, 이걸 나누게 된다면 printEndingStatus 부분이 한 줄 인데도 하나의 메서드로 나눠야 함 (굳이??)
이런 느낌으로 생각했던 것 같아요~ 그래서 그냥 한 줄 띄어서 컨텍스트를 구분했던 것 같습니다~

3Juhwan · 2024-03-16T11:34:58Z

src/main/java/blackjack/domain/participant/Player.java

+
+    public static Player from(String name, int money) {
+        return new Player(Collections.emptyList(), new Name(name), new BetAmount(money));
+    }
+


정적 팩토리 메서드 네이밍은 다음을 따르는 것으로 알고 있어요~

from: 매개변수가 1개인 경우

of: 매개변수가 2개 이상인 경우

위 관례를 따른다면 of가 적절할 것 같네요!

그렇네요 ㅎㅎ (이걸 왜 놓쳤지?)
처음에는 from(String name)이었는데, 같은 역할이니까 메서드 오버로딩 해야겠다고 생각해 버렸네요;;
수정해서 반영해보도록 할께요!

3Juhwan · 2024-03-16T11:35:43Z

src/main/java/blackjack/BlackJackGame.java

+
+    public void run() {
+        Deck deck = Deck.createShuffledDeck();
+        Dealer dealer = new Dealer();
+        Players players = createPlayers();
+
+        drawStartCards(dealer, players, deck);
+        play(players, dealer, deck);
+        printResult(dealer, players);
+    }
+


실화인가요? 왜 이렇게 깔끔하죠 👍👍👍

페어였던 짱수에게 이 영광을 돌립니다 👍👍👍

3Juhwan · 2024-03-16T11:41:07Z

src/main/java/blackjack/domain/participant/Participant.java

+
+    public final List<Card> getStartCards() {
+        return getCards().subList(0, getStartCardSize());
+    }
+


깔끔한 처리 좋네요 👍

3Juhwan · 2024-03-16T12:03:20Z

src/main/java/blackjack/domain/handrank/HandRankFactory.java

+package blackjack.domain.handrank;
+
+import blackjack.domain.card.Hand;
+
+public final class HandRankFactory {
+
+    private static final HankRank BLACKJACK = new Blackjack();
+
+    private HandRankFactory() {
+    }
+
+    public static HankRank from(Hand hand) {
+        if (hand.isBlackjack()) {
+            return BLACKJACK;
+        }
+        if (hand.isBusted()) {
+            return new Bust(hand.calculateScore());
+        }
+        return new Stand(hand.calculateScore());
+    }
+}


저는 팩토리 클래스는 안티 패턴이라고 생각해요! 클래스는 이미 객체를 생산할 수 있는 훌륭한 역할을 하지 않나요~?
커찬이 사용한 의도가 궁금해요 💭
제가 커찬 코드를 수정할 수 있다면, from 메서드를 HandRank 내부로 넣을 것 같아요. 로컬에서 해봤는데 테스트는 모두 통과하네요!

이 HandRankFactory의 기본적인 의도는 "굳이 Hand가 HandRank의 정보를 알아야 할까?"에서 시작했어. 그리고 Hand의 상태에 따라 다른 객체를 생성해 줄 마땅한 장소가 없어 팩토리 클래스를 도입했어.
여기에서 "HandRank 내부"로 넣는다는 생각은 정말 좋은 것 같아. 이와 관련해서 수정해보고 리뷰어한테 피드백을 받아봐야 겠다.

c,f, 리뷰어는 Hand에서 반환하는게 좀 더 자연스럽지 않냐는 피드백을 남겨주었어. 망쵸는 어떻게 생각해요?
리뷰어 리뷰 내용

3Juhwan · 2024-03-16T12:07:04Z

src/main/java/blackjack/domain/participant/Players.java

+
+    private void validateSize(List<Player> players) {
+        Objects.requireNonNull(players);
+        if (players.isEmpty()) {
+            throw new IllegalArgumentException("최소 한 명의 플레이어가 있어야 합니다.");


저번에 물어보는 것을 깜박했는데, 널체크하는 의도가 궁금해요 💭
저는 널체크를 커찬만큼 하지 않는 편인데, 한다면 빠른 실패를 할 수 있다는 점에서 좋을 것 같네요!

생성자에서 null이 들어올 수 있는 환경이라면, 왠만하면 null체크를 하는 편이에요.
지금 상황에서는 검증 단계에서 players.isEmpty()와 같은 메서드를 사용하다보니 굳이 필요한 것 같지 않은 것 같기도??
그리고 만약에 어느 상황에서든 null을 인자로 넘기지 않는다는 것이 팀 컨벤션으로 갖추어져 있다면, 굳이 null체크를 하지 않을 것 같에요. (실제로 다들 충분히 공감할 수 있을 것 같구요)

3Juhwan · 2024-03-16T12:16:46Z

src/main/java/blackjack/domain/handrank/Stand.java

+
+    public Stand(int score) {
+        validate(score);
+        this.score = score;
+    }
+


public으로 둔 의도가 있을까요?
package-private으로 패키지 내부에서만 접근하도록 캡슐화하는 것은 어떨까요? 💭

지금 구조에서는 그렇게 해도 좋을 것 같아요! 외부에서 의도치 않게 생성되는 것을 방지해줄 수 있겠에요!

Libienz

하이 커찬 👊
일이 있어서 늦게 코멘트 남겼네 양해부탁.. 😓

객체 지향적인 특성을 잘 살린 구현과 촘촘한 테스트 코드가 인상적이네 🔥
많이 배웠으 ㅎㅎ

2단계 머지까지 같이 힘내자구 💪🏻

Libienz · 2024-03-16T19:13:34Z

src/main/java/blackjack/BlackJackGame.java

+    private void playTurn(Player player, Deck deck) {
+        while (player.isDrawable() && inputView.isPlayerWantDraw(player.getName())) {
+            player.add(deck.draw());
+            outputView.printPlayerCards(player);
+        }
+    }
+


저번에 얘기해줬던 리뷰어의 코멘트 반영했나 보네 👍

나는 개인적으로 provider느낌으로 아이디어가 번뜩인다고 생각하긴 했는데
외부에서 주입받는 동작에 의존하는 것이 좋지 않긴 한가봐 🤔

1단계 때 커찬이 리뷰내용 공유해줘서 나도 많은 생각해볼 수 있었으 👍

나와 페어도 번뜩이는 아이디어라고 생각해서 도입했는데,
언제든지 외부에서 Player의 행동을 막 시킬 수 있는 느낌이라, 리뷰어는 복잡도가 올라가는 만큼의 장점은 안느껴졌던 것 같아. 이 말에 어느정도 납득이 가서, 그냥 getPlayers() 사용하도록 바꿨어

Libienz · 2024-03-16T19:15:29Z

src/main/java/blackjack/domain/card/Hand.java

+    public Hand add(Card card) {
+        List<Card> newCards = new ArrayList<>(cards);
+        newCards.add(card);
+
+        return new Hand(newCards);
+    }


핸드에 카드를 더하는 메서드에 리턴 타입이 필요하다고 생각했던 것 같아 🤔
의도가 있어 보이는데 공유해줄 수 있을깜 😃

처음에는 Hand라는 얘를 가변객체로 사용하고 있었는데, 갈아엎기 전에 이게 가변객체이다보니 불편한 점이 많더라고. 그래서 카드를 한 장 추가할 때 카드를 한장 추가한 불변 객체로 만들었어
이것도 다시 롤백시킬 수 있었지만, 지금 성능에 문제는 안 생길 것 같고 오히려 유지보수하기 편하다는 생각이 들어서 불변객체로 놔두었어.

Libienz · 2024-03-16T19:28:12Z

src/main/java/blackjack/domain/participant/Dealer.java

+        for (Player player : players.getPlayers()) {
+            Profit dealerProfit = player.calculateProfit(this).reverse();
+            profit = profit.add(dealerProfit);
+        }


미세한 차이지만 플레이어들 수익의 총합을 구하고 다음에 reverse하면 불필요한 연산 줄일 수 있지 않을까 👀

또한 플레이어들 총 수익을 계산하는 과정을 메서드로 추상화할 수 있을 것 같아

아래에 Map<Player, Profit>의 형태로 값을 반환하고 있는데, 이걸 재사용해도 되겠네~ 좋은 아이디어인 것 같아.
이 로직을 작성할 때 별 생각이 없었는데, 조금 떠올려보자면 "각 플레이어의 대결 결과를 뒤집은 것이 딜러 수익이고, 이것의 총 합이 총 딜러 수익이다"라고 생각했던 것 같아. 리비 방법이 성능상에 이점이 있을 것 같은데, 가독성은 비교해봐야겠네~ 좋은 아이디어 줘서 고마워 리비!

Libienz · 2024-03-16T19:30:28Z

src/main/java/blackjack/domain/participant/Player.java

+        super(cards);
+        this.name = Objects.requireNonNull(name);
+        this.betAmount = Objects.requireNonNull(betAmount);


Null 검증 👍🏻

Libienz · 2024-03-16T19:34:49Z

src/main/java/blackjack/domain/participant/Players.java

+        }
+        if (players.size() > MAX_PLAYERS_SIZE) {
+            throw new IllegalArgumentException("최대 4명의 플레이어만 참여 가능합니다.");
+        }


우선 코멘트에 앞서 나도 커찬과 비슷한 구현을 했음을 밝혀야겠네
그런데 이 부분은 한 번 같이 이야기해보고 싶어

최대 4명의 플레이어만 참여가능하다는 것은 Players 도메인의 규칙일까 BlackJackGame의 로직일까 🤔
나는 블랙잭 게임의 규칙이라고 최근에 생각하게 되었어

그런 의미에서 Players에는 몇명인지 알 수 있는 메서드를 구현해놓고 블랙잭 게임 생성시에 이를 검증하도록 하면 어떨까?
누구의 규칙인지 명확히 하면 각각의 도메인에서 명세가 자세히 드러날 수도 있을 것 같아 🙌

이전 코멘트에서 이야기 했었는데, "플레이어가 4명이어야 한다"라는 검증 절차를 둘 때, Players가 모든 플레이어를 관리하는 객체이다 보니 Players가 적합한 객체라고 생각했어.

"객체지향을 이용해서 프로그램을 만드는 것"은 요구사항들을 적절한 객체들에서 흩뿌려 놓는 것이라고 생각해. 그래서 최대한 연관된 객체에게 연관된 요구사항을 놓아야 된다고 생각해. 물론 최대 플레이어가 4명이라는 것이 블랙잭 게임의 규칙이라는 것에는 나도 공감해. 그렇다면 블랙잭 게임에서 "한장의 카드를 추가로 받을 수 있다"라는 것도 블랙잭 게임의 규칙 중 하나가 아닐까?

내가 조금 리비가 전달하고자 하는 바를 이해하지 못한 것 같아. 이와 관련한 이야기는 나도 자세히 듣고 싶어 ^^. 추가적인 내용은 나한테 찾아와서 이야기해주거나, 코멘트 달아줘~

Libienz · 2024-03-16T19:35:29Z

src/main/java/blackjack/view/InputView.java

+    private static final String WANT_DRAW_INPUT = "y";
+    private static final String WANT_NOT_DRAW_INPUT = "n";


Libienz · 2024-03-16T19:36:36Z

src/test/java/blackjack/domain/card/HandTest.java

+    static Stream<Arguments> cardsAndScore() {
+        return Stream.of(Arguments.of(CardsFixture.BLACKJACK, 21),
+                Arguments.of(CardsFixture.TWO_ACE, 12),
+                Arguments.of(CardsFixture.CARDS_SCORE_16, 16));
+    }


이게 fixture군 👍
미리 생성되어 있는 테스트 데이터를 잘 활용하고 있는 듯 😀

Libienz · 2024-03-16T19:41:05Z

src/main/java/blackjack/domain/card/Card.java

+    @Override
+    public String toString() {
+        return new StringJoiner(", ", Card.class.getSimpleName() + "[", "]")
+                .add("value=" + value)
+                .add("shape=" + shape)
+                .toString();
+    }


디버깅용인가 🤔

toString은 디버깅 용으로 많이 사용한다고 하는데 어떤 부분에서 디버깅이 필요했는지, 테스트 코드만으로 방어할 수 없었는지 커찬의 생각 궁금하네 💭

사실 toString()을 붙였던 시기는 2단계를 구현하다가 한 번 갈아엎기 전에 가시성이 떨어져서 붙였어
최대한 테스트코드로 구분하려고 했는데, @ParameterizedTest가 쓰인 부분이 많아서 어떤 케이스가 실패했는지 구분하기 어려워서 toString을 붙였어.
그리고는 만든 toString()을 지울 이유는 없었어서 그냥 놔두었어

Libienz · 2024-03-16T19:42:08Z

src/main/java/blackjack/domain/card/Hand.java

+        int differenceScore = cards.stream()
+                .filter(Card::isAce)
+                .mapToInt(this::calculateDifferenceScore)
+                .findAny()
+                .orElse(0);


에이스에 대한 처리를 가중치로써 진행한다면 파생되는 코드를 줄일 수 있을 것 같다는 개인적인 생각 🧐

나는 오히려 반대.
여기에서 사용하는 Card의 기본 목적은 "ACE가 1 또는 11을 갖는다"는 정보를 최대한 Card 안쪽으로 숨긴다는 목적이 컸어. Ace일 때 10의 가중치를 줄수도 있지만, "10의 가중치"라는 정보다 Card가 아닌 Hand에서 관리하는게 캡슐화에 저해된다고 생각했어. 비록 코드가 더 복잡해지긴 했지만, 추후 유지보수에는 유리할거라고 생각해

Libienz · 2024-03-16T19:44:36Z

src/main/java/blackjack/domain/handrank/SingleMatchResult.java

+    public Profit calculatePlayerProfit(BetAmount betAmount) {
+        return Profit.of(betAmount, playerMultiplier);
+    }


오호 .. 이 부분 멋지다

수익을 계산하는 것이 게임 결과의 책임이라고 생각했구나 👍
레버리지를 가지고 있는 도메인이니 커찬의 의도가 합리적인 것 같아

나는 의도하지 않아도 자꾸 게터로 꺼내와서 동작을 추상화하려고 하게 되는 것 같아 🫠
커찬은 도메인들과의 협력을 통한 객체지향을 잘 활용하고 있는 것 같네 💪🏻

나는 각 객체에 역할이나 책임을 부여할 때, "이 역할을 할 때 필요한 정보를 누가 가장 많이 들고 있는가?"를 생각하는 편이야. 수익을 계산하는 부분은 게임 결과에 따라 달라질 수 있는 부분이라 SingleMatchResult에 배치했어.

leegwichan

로빈, 리비, 망쵸! 모두 달아준 리뷰 잘 봤어~ 달아준 리뷰 관련해서 내가 그 코드를 작성할 때 들었던 생각, 리뷰에 대한 생각들을 코멘트 달아놨어. 궁금한 점이 있다면 나한테 찾아와서 얘기해줘~ (MBTI는 I로 시작해도, 토론은 좋아한답니다~)

추가적으로 로빈과 망쵸가 말한, 상태 패턴을 적용하면서 느낀 장단점을 정리해봤어.

설계 및 의도

플레이어가 Blackjack, Bust, Stand일 때, 딜러가 Blackjack, Bust, Stand일 때, 서로 결과가 다르니까 if-else문이 길게 쓰여져 있을 것 같은 느낌이 들었어. 그래서 각 상황을 class로 만들어서 각 경우일 때를 한정시켜 줄 수 있어서 좋았어.

장점

각 상황일 때마다 로직을 작성해줄 수 있으니까, Blackjack, Bust, Stand 하나 당 판단하는 로직의 무게가 많이 줄어들었어.
만약에 다른 상황이 추가된다면, if-else 등 복잡한 구문을 건드리지 않고, 새로운 상태를 하나 도입하면 되니까, 상대적으로 "확장에 대해 열려 있어야 하고, 수정에 대해서는 닫혀있는 코드"가 아닐까?

단점

Hand의 상태마다 다른 class를 만들어주어야 하는데, 그걸 어디서 해야될지 잘 모르겠어 ㅠ_ㅠ
- Hand에서 해주자니, Hand가 Blackjack, Bust, Stand를 다 알고 있어야 되니까...
- 그래서 팩토리를 도입했더니, 충분히 다른 곳에서 해줄 수 있을 것 같기도 하고 (망쵸의 의견대로 HandRank에서 해야될까 싶기도...)
지금 코드의 한정에서는 Dealer와 Player가 모두 Hand를 들고 있고 HandRank를 공용으로 사용하고 있어서 matchAtDealer()를 의도하지 않은 대로 사용할 수 있다는 점?

leegwichan · 2024-03-17T00:36:43Z

src/main/java/blackjack/BlackJackGame.java

+
+    public void run() {
+        Deck deck = Deck.createShuffledDeck();
+        Dealer dealer = new Dealer();
+        Players players = createPlayers();
+
+        drawStartCards(dealer, players, deck);
+        play(players, dealer, deck);
+        printResult(dealer, players);
+    }
+


페어였던 짱수에게 이 영광을 돌립니다 👍👍👍

leegwichan · 2024-03-17T00:39:33Z

src/main/java/blackjack/BlackJackGame.java

+    private void playTurn(Player player, Deck deck) {
+        while (player.isDrawable() && inputView.isPlayerWantDraw(player.getName())) {
+            player.add(deck.draw());
+            outputView.printPlayerCards(player);
+        }
+    }
+


나와 페어도 번뜩이는 아이디어라고 생각해서 도입했는데,
언제든지 외부에서 Player의 행동을 막 시킬 수 있는 느낌이라, 리뷰어는 복잡도가 올라가는 만큼의 장점은 안느껴졌던 것 같아. 이 말에 어느정도 납득이 가서, 그냥 getPlayers() 사용하도록 바꿨어

leegwichan · 2024-03-17T00:43:46Z

src/main/java/blackjack/BlackJackGame.java

+    private void printResult(Dealer dealer, Players players) {
+        outputView.printEndingStatus(dealer, players);
+
+        Profit dealerProfit = dealer.calculateDealerProfit(players);
+        Map<Player, Profit> playersProfit = dealer.calculatePlayersProfit(players);
+        outputView.printMatchResult(dealerProfit, playersProfit);
+    }
+}


솔직히 말하면, 그냥 생각없이 해버렸다는 게 맞을 것 같아요.

굳이 무의식을 분석해보자면,

run() 부분에서 결과를 출력하는 부분을 printResult()로 구분함

이 로직이 printEndingStatus와, printMatchResult를 호출하는 부분으로 나눌 수 있는데, 이걸 나누게 된다면 printEndingStatus 부분이 한 줄 인데도 하나의 메서드로 나눠야 함 (굳이??)
이런 느낌으로 생각했던 것 같아요~ 그래서 그냥 한 줄 띄어서 컨텍스트를 구분했던 것 같습니다~

leegwichan · 2024-03-17T00:45:46Z

src/main/java/blackjack/domain/card/Card.java

+    @Override
+    public String toString() {
+        return new StringJoiner(", ", Card.class.getSimpleName() + "[", "]")
+                .add("value=" + value)
+                .add("shape=" + shape)
+                .toString();
+    }


사실 toString()을 붙였던 시기는 2단계를 구현하다가 한 번 갈아엎기 전에 가시성이 떨어져서 붙였어
최대한 테스트코드로 구분하려고 했는데, @ParameterizedTest가 쓰인 부분이 많아서 어떤 케이스가 실패했는지 구분하기 어려워서 toString을 붙였어.
그리고는 만든 toString()을 지울 이유는 없었어서 그냥 놔두었어

leegwichan · 2024-03-17T00:49:11Z

src/main/java/blackjack/domain/card/Hand.java

+    public Hand add(Card card) {
+        List<Card> newCards = new ArrayList<>(cards);
+        newCards.add(card);
+
+        return new Hand(newCards);
+    }


처음에는 Hand라는 얘를 가변객체로 사용하고 있었는데, 갈아엎기 전에 이게 가변객체이다보니 불편한 점이 많더라고. 그래서 카드를 한 장 추가할 때 카드를 한장 추가한 불변 객체로 만들었어
이것도 다시 롤백시킬 수 있었지만, 지금 성능에 문제는 안 생길 것 같고 오히려 유지보수하기 편하다는 생각이 들어서 불변객체로 놔두었어.

leegwichan · 2024-03-17T01:10:19Z

src/main/java/blackjack/domain/handrank/Stand.java

+
+    public Stand(int score) {
+        validate(score);
+        this.score = score;
+    }
+


지금 구조에서는 그렇게 해도 좋을 것 같아요! 외부에서 의도치 않게 생성되는 것을 방지해줄 수 있겠에요!

leegwichan · 2024-03-17T01:14:54Z

src/main/java/blackjack/domain/participant/Dealer.java

+        for (Player player : players.getPlayers()) {
+            Profit dealerProfit = player.calculateProfit(this).reverse();
+            profit = profit.add(dealerProfit);
+        }


아래에 Map<Player, Profit>의 형태로 값을 반환하고 있는데, 이걸 재사용해도 되겠네~ 좋은 아이디어인 것 같아.
이 로직을 작성할 때 별 생각이 없었는데, 조금 떠올려보자면 "각 플레이어의 대결 결과를 뒤집은 것이 딜러 수익이고, 이것의 총 합이 총 딜러 수익이다"라고 생각했던 것 같아. 리비 방법이 성능상에 이점이 있을 것 같은데, 가독성은 비교해봐야겠네~ 좋은 아이디어 줘서 고마워 리비!

leegwichan · 2024-03-17T01:16:36Z

src/main/java/blackjack/domain/participant/Player.java

+
+    public static Player from(String name, int money) {
+        return new Player(Collections.emptyList(), new Name(name), new BetAmount(money));
+    }
+


그렇네요 ㅎㅎ (이걸 왜 놓쳤지?)
처음에는 from(String name)이었는데, 같은 역할이니까 메서드 오버로딩 해야겠다고 생각해 버렸네요;;
수정해서 반영해보도록 할께요!

leegwichan · 2024-03-17T01:20:05Z

src/main/java/blackjack/domain/participant/Players.java

+
+    private void validateSize(List<Player> players) {
+        Objects.requireNonNull(players);
+        if (players.isEmpty()) {
+            throw new IllegalArgumentException("최소 한 명의 플레이어가 있어야 합니다.");


생성자에서 null이 들어올 수 있는 환경이라면, 왠만하면 null체크를 하는 편이에요.
지금 상황에서는 검증 단계에서 players.isEmpty()와 같은 메서드를 사용하다보니 굳이 필요한 것 같지 않은 것 같기도??
그리고 만약에 어느 상황에서든 null을 인자로 넘기지 않는다는 것이 팀 컨벤션으로 갖추어져 있다면, 굳이 null체크를 하지 않을 것 같에요. (실제로 다들 충분히 공감할 수 있을 것 같구요)

leegwichan · 2024-03-17T01:27:15Z

src/main/java/blackjack/domain/participant/Players.java

+        }
+        if (players.size() > MAX_PLAYERS_SIZE) {
+            throw new IllegalArgumentException("최대 4명의 플레이어만 참여 가능합니다.");
+        }


이전 코멘트에서 이야기 했었는데, "플레이어가 4명이어야 한다"라는 검증 절차를 둘 때, Players가 모든 플레이어를 관리하는 객체이다 보니 Players가 적합한 객체라고 생각했어.

"객체지향을 이용해서 프로그램을 만드는 것"은 요구사항들을 적절한 객체들에서 흩뿌려 놓는 것이라고 생각해. 그래서 최대한 연관된 객체에게 연관된 요구사항을 놓아야 된다고 생각해. 물론 최대 플레이어가 4명이라는 것이 블랙잭 게임의 규칙이라는 것에는 나도 공감해. 그렇다면 블랙잭 게임에서 "한장의 카드를 추가로 받을 수 있다"라는 것도 블랙잭 게임의 규칙 중 하나가 아닐까?

내가 조금 리비가 전달하고자 하는 바를 이해하지 못한 것 같아. 이와 관련한 이야기는 나도 자세히 듣고 싶어 ^^. 추가적인 내용은 나한테 찾아와서 이야기해주거나, 코멘트 달아줘~

leegwichan and others added 30 commits March 13, 2024 01:16

refactor (Player) : equals() 의미에 오해를 불러일으킬 수 있어, equals()를 제거하고 isE…

a2adac3

…qualName() 을 사용함

refactor (Players) : 복잡도를 증가시키는 play() 함수 제거

8315134

- Controller에서는 getPlayers()를 이용하여 각 플레이어의 턴을 진행

docs (README) : 2단계에서 추가된 요구 사항 정리

9ff2eb7

refactor : 더 이상 사용하지 않는 인터페이스 제거

489c685

feat (Money) : 돈은 항상 양수만 가지도록 검증하는 기능 구현

150cc56

feat (Monet) : 해당 배수만큼 곱하는 기능 구현

a121d1a

feat (Money) : 금액끼리 서로 더하는 기능 구현

106c036

feat (Blackjack) : 블랙잭 일 때만 해당 객체를 생성할 수 있도록 검증하는 기능 구현

0578c39

refactor (Hand) : Hand를 용이하게 사용하기 위해 불변 객체로 변경

786608a

feat (Blackjack) : 블랙잭에서 카드를 추가할 경우, 버스트가 되는 기능 구현

dac6b93

refactor (Card) : 카드의 원활한 트래킹을 위해, toString() 구현

6146caf

feat (HandRankFactory) : 현재 손패에 따라 다른 HandRank가 생성되는 기능 구현

5f7b0be

refactor (Money) : 컨벤션에 따라 메서드 순서 변경

f2708af

feat (Blackjack) : 해당 랭크가 블랙잭, 버스트인지 확인하는 기능 구현

979de37

feat (Blackjack) : 딜러가 블랙잭일 때, 승패 판단 기능 구현

7ed3ca0

feat (Bust) : 해당 랭크가 블랙잭, 버스트인지 확인하는 기능 구현

124faa7

feat (Blackjack) : 딜러가 버스트일 때, 승패 판단 기능 구현

ad9ae1e

feat (HandRank) : 일반 케이스에서 두 점수를 비교하기 위해, HandRank에서 Score를 반환하는 기능 구현

35d8986

test (NormalRank) : 해당 랭크가 블랙잭, 버스트인지 확인하는 기능 구현

a4e86a6

style (Stand) : NormalRank 네이밍이 추상적이어서, 도메인 용어 중 하나인 Stand로 변경

4cbebeb

feat (Stand) : 딜러가 Stand일 때, 승패 판단 기능 구현

c74a451

style (BetAomunt) : 단순히 애매한 돈 이라는 표현보다, 도메인 용어인 배팅 금액으로 클래스 이름 변경

14c2d9b

feat (Profit) : 배팅 금액과 승패에 따라 이익을 계산하는 기능 구현

cf300b7

feat (Profit) : 두 이익을 더하여 계산할 수 있다.

2ddd8de

feat (Profit) : 얻은 만큼 잃은 이익을, 잃은 만큼 얻은 이익을 구하는 기능 구현

58e9925

feat (SingleMatchResult) : 매치 결과에 따라 플레이어, 딜러의 이익을 계산하는 기능 구현

51e5232

feat (Dealer) : 플레이어들의 이익 결과를 출력하는 기능 구현

c707523

feat (Dealer) : 플레이어들과 대결 후, 딜러의 이익을 계산하는 기능 구현

1ff93c7

feat (InputView) : 배팅 금액을 입력받는 기능 구현

25408c2

leegwichan added 11 commits March 14, 2024 13:36

feat (OutputView) : 딜러 및 플레이어들의 최종 결과 출력 기능 구현

8e74520

feat (BlackjackGame) : 추가된 요구 사항을 반영하여, 전반적인 제어 로직 수정

8e13c47

refactor (OutputView) : 사용하지 않는 메서드 제거 및 일부 메서드 분리 실시

69045b8

feat (Players) : 사용하지 않는 메서드 제거 및 중복 확인 로직 리팩터링 실시

9d1e979

refactor (Participant) : 사용하지 않는 메서드 및 테스트 제거

ddad41a

refactor : SingleMatchResult, BetAmount에서 사용하지 않는 메서드 제거

4a1481b

refactor (Profit) : 정적 팩토리 메서드가 편의성이 떨어져, 사용하기 용이한 하나의 메서드롤 통합

1d59566

style (Test) : 불필요한 필드 제거 및 불필요한 접근 제어자 제거

329a6e8

refactor (HandRankFactory) : static method만 있는 클래스를 객체로 생성하지 못하도록 pri…

795a5d3

…vate 생성자 추가

refactor (Players) : 입력받는 List가 null일 경우 예외를 던지도록 변경

4542815

refactor(HandRank) : 각 핸드 결과마다 가능한 점수 검증 기능 추가

243e8f8

robinjoon reviewed Mar 16, 2024

View reviewed changes

3Juhwan reviewed Mar 16, 2024

View reviewed changes

Libienz reviewed Mar 16, 2024

View reviewed changes

leegwichan commented Mar 17, 2024

View reviewed changes

Libienz mentioned this pull request Mar 17, 2024

[2단계 - 블랙잭 베팅] 리비(이근희) 미션 제출합니다. woowacourse/java-blackjack#709

Merged

		@@ -0,0 +1,12 @@
		package blackjack.domain.handrank;

		public interface HankRank {


		public interface HankRank {

		SingleMatchResult matchAtDealer(HankRank playerHandRank);

		private static final String WANT_DRAW_INPUT = "y";
		private static final String WANT_NOT_DRAW_INPUT = "n";

[2단계 - 블랙잭 베팅] 커찬(이충안) 미션 제출합니다. #11

Are you sure you want to change the base?

[2단계 - 블랙잭 베팅] 커찬(이충안) 미션 제출합니다. #11

Conversation

leegwichan commented Mar 15, 2024

설계에서 고려했던 부분

최소한의 변경

HandRank 도입

BetAmount, Profit 도입

robinjoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3Juhwan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leegwichan Mar 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Libienz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leegwichan left a comment

Choose a reason for hiding this comment

설계 및 의도

장점

단점

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`HandRank` 도입

`BetAmount`, `Profit` 도입

leegwichan Mar 17, 2024 •

edited

Loading