[2단계 - 블랙잭 베팅] 재즈(함석명) 미션 제출합니다. #12

seokmyungham · 2024-03-15T07:38:16Z

현구막 안녕하세요~! 재즈입니당 😄
1단계에 마지막에 남겨주셨던 코멘트를 반영하면서 2단계를 진행해봤어요.

컨트롤러에서 게임 전반을 담당하는 객체 `BlackjackGame`을 분리

많은 책임이 부여되었던 기존 컨트롤러의 역할을 BlackjackGame으로 분리하면서
코드도 조금 짧아지고, 개선된 것 같은데
블랙잭 게임 입출력이 게임의 진행과 강하게 결합되어 있어서 만족스럽진 않은 것 같아요. 😭
깔끔하게 분리하지는 못했지만 최대한 노력해봤습니다! 🔥

제가 느끼기에도 기존에 너무 많은 역할이 부여되었던 것 같고
앞으로 컨트롤러가 무거워질 때 마다 domain-view 사이 소통, 흐름만 담당할 수 있게하라는
현구막의 피드백을 계속 떠올릴 것 같아요 😎

Step2에서 추가로 구현한 사항

기존에 있던 GameResult를 수정해서 배팅 수익 비율을 설정하고
Money 객체를 추가하여 repository 역할을 하는 GameAccount 클래스로 배팅 금액을 저장, 수익을 계산하였습니다.

그리고 요구사항이 더 복잡해짐에 따라, Hand에게 너무 많은 역할이 부여되는 것 같았고
기존 Hand에서 진행하던 점수 계산 로직을 HandValue라는 클래스로 분리하였습니다.

HandValue는 자신의 value 상태를 던짐으로써
Player가 Delaer의 HandValue를 받아 게임 결과 조건을 판단하고 자신의 승,무,패 여부를 알 수 있도록 구현해보았습니다.

긴 글과 코드 읽어주셔서 감사드리고
리뷰 기다리겠습니다.
감사합니다 😊

* docs(README): 기능 요구 사항 작성 Co-authored-by: seokmyungham <[email protected]> * feat: 카드 문양과 숫자에 대한 Enum 클래스 생성 Co-authored-by: seokmyungham <[email protected]> * feat(Deck): 모든 카드를 가지는 Deck 클래스 및 테스트 구현 Co-authored-by: seokmyungham <[email protected]> * style(DeckTest): 중복 코드 제거 Co-authored-by: seokmyungham <[email protected]> * feat(BlackJackGamer): 딜러와 플레이어의 공통 추상 클래스 생성 Co-authored-by: seokmyungham <[email protected]> * feat(Dealer): BlackJackGamer를 상속받는 딜러 클래스 생성과 메서드 재정의 Co-authored-by: seokmyungham <[email protected]> * feat(Player): BlackJackGamer를 상속받는 플레이어 클래스 생성과 메서드 재정의 Co-authored-by: seokmyungham <[email protected]> * feat(Name): 문자열 이름을 포장하는 클래스 생성 Co-authored-by: seokmyungham <[email protected]> * feat(Players): Player의 일급 컬렉션 생성 Co-authored-by: seokmyungham <[email protected]> * refactor: 예외 메시지 추가 및 테스트 수정 Co-authored-by: seokmyungham <[email protected]> * test(NameTest): 정상 입력에 대한 테스트 추가 Co-authored-by: seokmyungham <[email protected]> * test(PlayersTest): 정상 입력에 대한 테스트 추가 Co-authored-by: seokmyungham <[email protected]> * feat(Player): 딜러의 점수를 입력받아 플레이어의 승패 여부를 결정하는 기능 추가 Co-authored-by: seokmyungham <[email protected]> * feat(Hand): 참여자의 카드 리스트에 대한 일급 컬렉션 생성 Co-authored-by: seokmyungham <[email protected]> * refactor(BlackjackGamer): Hand를 사용하도록 수정 Co-authored-by: seokmyungham <[email protected]> * refactor(Dealer, Player): 생성자 및 메서드 수정 Co-authored-by: seokmyungham <[email protected]> * feat(GameResult): 승,패를 표현하는 Enum 클래스 생성 Co-authored-by: seokmyungham <[email protected]> * feat: DTO 생성 및 각 도메인에 DTO 생성 로직 추가 Co-authored-by: seokmyungham <[email protected]> * refactor(Hand): 카드 숫자의 합계를 구하는 메서드 세분화 및 상수 추가 Co-authored-by: seokmyungham <[email protected]> * style(Dealer): 상수 추가 및 dto 생성 메서드명 수정 Co-authored-by: seokmyungham <[email protected]> * style(Player): 매직 넘버 상수화 Co-authored-by: seokmyungham <[email protected]> * feat(Players): 전체 플레이어의 패를 초기화 하는 기능 및 dto 생성 기능 추가 Co-authored-by: seokmyungham <[email protected]> * feat(GameResult): 패배인지 확인하는 기능 및 getter 추가 Co-authored-by: seokmyungham <[email protected]> * feat(InputView): 입력을 받아들이는 InputView 클래스 생성 Co-authored-by: seokmyungham <[email protected]> * feat(OutputView): 출력을 담당하는 OutputView 클래스 생성 Co-authored-by: seokmyungham <[email protected]> * feat(BlackjackController): 블랙잭 게임 흐름을 제어하는 컨트롤러 클래스 생성 Co-authored-by: seokmyungham <[email protected]> * style: 출력 요구사항에 맞춰 빈 줄 출력 포맷 수정 Co-authored-by: seokmyungham <[email protected]> * feat(Application): 컨트롤러를 생성하고 호출하여 게임을 시작하는 클래스 생성 Co-authored-by: seokmyungham <[email protected]> * docs(README): 기능 요구 사항 정리 Co-authored-by: seokmyungham <[email protected]> * refactor(BlackjackController): 명령어 상수를 별도의 view Enum으로 분리 및 사용 * refactor(Card): 출력에 의존적인 CardNumber, CardShape을 수정하고 view 패키지에 Enum 추가 * refactor(GameResult): 기존 출력에 의존적이던 필드 문자열을 제거 및 view enum으로 분리 * refactor: 기존 도메인 내에 존재하던 Dto 변환 로직을 Dto 내부로 이동 및 리팩토링 * refactor(Deck): 테스트 목적의 List<Card>를 파라미터로 받는 생성자 개방 * test(CardTest): eqauls&hashCode 테스트 추가 * refactor(PlayersTest): 가독성 향상을 위해 테스트 코드 리팩토링 * test(PlayersTest): 경계 값 테스트를 위해 테스트케이스 수정 * feat(Command): 사용자 입력 명령어를 별도의 Enum 클래스로 분리 * style(Players): getter 메서드의 위치를 맨 밑으로 이동 --------- Co-authored-by: 이상진 <[email protected]>

leegwichan

재즈! 코드 잘봤어! 특히 복잡한 승패 판단 로직을 하나의 클래스에서 가독성 좋게 구현한 것이 인상 깊었어!

추가적으로 언급하고 싶은 것들은,

Players가 있음에도 GameAccount를 따로 사용한다는 아쉬움;;
여러 명의 플레이어들을 관리한다는 면에서 동일한데, Players에서는 Map<Player, GameResult> 형식으로 반환하고, GameAccount는 각 플레이어의 돈을 관리하면서도 계산을 따로 한다는 점이 아쉽다. 이걸 하나로 관리할 수 있으면 더 좋지 않을까라는 생각을 해봤어.
.gitkeep 파일은 지우는 게 좋지 않을까? .gitkeep 파일은 깃허브에 폴더 구조를 올리고 싶을 때 사용하는 파일로 알고 있어. .gitkeep 참고자료

이외의 내용들에는 아래 코멘트 달아놨어!

leegwichan · 2024-03-16T12:04:23Z

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

+    public Deck() {
+        this.cards = new LinkedList<>(CACHE);
+    }
+
+    Deck(LinkedList<Card> cards) {
+        this.cards = cards;
+    }
+
+    public void shuffle() {
+        Collections.shuffle(cards);
+    }


미리 섞여있는 덱을 주지않고, 순서대로 정렬된 덱을 만든 후에, 해당 덱을 받는 쪽에서 섞어 주는 이유가 있을까?
생성자가 사용된 메서드들을 봤는데 지금 구조를 유지하면서도 public으로는 섞여있는 덱을 줄 수 있을 것 같아서.

public Deck() { List<Card> cards = new LinkedList<>(CACHE); Collections.shuffle(cards); this.cards = cards; } Deck(LinkedList<Card> cards) { this.cards = cards; }

섞여 있는 덱, 덱을 생성하고 섞는 것 둘 다 나는 괜찮다고 생각해

깊게 생각 안해봤었는데,
블랙잭 게임에서 덱을 섞는다는 행위가 덱 입장에서는 당연하다는 걸 생각해보면
커찬 말대로 섞여 있는 덱을 사용하지 않을 이유가 없긴한 것 같네..

leegwichan · 2024-03-16T12:05:52Z

src/main/java/blackjack/domain/game/BlackjackGame.java

+import blackjack.domain.gamer.Players;
+import java.util.Map;
+
+public class BlackjackGame {


컨트롤러에서의 진행 로직을 최대한 BlackjackGame으로 분리한 것이 좋다. 굿굿!!

leegwichan · 2024-03-16T12:13:24Z

src/main/java/blackjack/domain/game/GameAccount.java

+public class GameAccount {
+
+    private static final Map<Player, Money> store = new LinkedHashMap<>();
+
+    public void betMoney(Player player, Money money) {
+        store.put(player, money);
+    }
+
+    public Money findMoney(Player player) {
+        return store.get(player);
+    }


Player가 배팅한 금액을 들고있으면서, 해당 로직들을 최대한 Player 안으로 넣을 수 있었을 것 같은데, Map으로 관리한 이유가 있을까?
(Map으로 사용한다면, 돈을 외부에서 관리하고 Player는 승패만을 관리한다는 점에서 좋을 것 같은데, 조금 큰 단위의 변경이 필요하다면 Player, GameAccount를 둘 다 손봐야 해서 더 힘들어질수도 있겠다는 생각이 드네)

Map 형식을 차용했다면, store는 private static final 말고 private final로 각 객체마다 store를 하나씩 들고 있는 것이 좋을 것 같아. 이걸 static으로 이용한다면, GameAccount를 여러개 사용할 때 각 고객 데이터가 공유될 수 있을 것 같아.

커찬 말도 맞는 말이라고 생각해.
딜러 입장에서 승패를 결정하는 사람이 있고, 플레이어 입장에서 승패를 결정하는 사람이 있다면
나는 후자의 입장이 좀 더 자연스럽다고 생각해서 일단 플레이어가 승패 로직을 들고 있도록 구현했었어.

그런데 배팅 금액을 플레이어가 가지고 있게 되면, 플레이어의 역할이 너무 많아질 거라 생각해
(커찬이 제시한 방향대로 설계를 했다면, 아마 나는 딜러나 다른 객체에게 승패 결정을 맡겼을 것 같아)

조금 큰 단위의 변경이 필요하다면 Player, GameAccount를 둘 다 손봐야 해서 더 힘들어질수도 있겠다는 생각이 드네

그리고 Repository 개념의 Map을 선택한 이유를 커찬의 생각과 반대로 적용시켜보면
정말 큰 단위의 변경이 일어났을 때 한 곳만을 수정할 수 있을지는 조금 의문이 들기도 하는데
상태 데이터를 한 곳에서 관리한다는 측면에서 서비스 확장성을 생각했을 때 이 방법이 좋은 선택이 될 수 있을 것 같아

앗 2번은 내가 일부러 static final 키워드를 사용한 이유이기도 해
해당 Map을 모든 GameAccount가 공동으로 사용하길 원했고, 프로젝트 실행 동안 "단 하나"만 존재하길 원했어
(블랙잭 게임의 공용 배팅 계좌? 같은 느낌을 원했던 건데 의미가 잘 전달이 안된건 내가 네이밍을 잘 못했던 것 같군..)

그런데 배팅 금액을 플레이어가 가지고 있게 되면, 플레이어의 역할이 너무 많아질 거라 생각해

나도 이 의견에 대해서 매우 공감해! 확실히 많이 무거워 질 것 같네...

해당 Map을 모든 GameAccount가 공동으로 사용하길 원했고, 프로젝트 실행 동안 "단 하나"만 존재하길 원했어

그러면 Map이 단 하나만 존재하는게 아니라, GameAccount가 단 하나만 존재해야 되지 않을까? 지금처럼 new GameAccount()로 생성하는 것을 허용한다면, 모든 GameAccount가 공용 데이터를 쓴다고 생각하지 못할 것 같아. (아마 싱글톤 패턴의 도입을 고려해봐야 하지 않을까?)

leegwichan · 2024-03-16T12:19:22Z

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

+    public GameResult compete(HandValue dealerHandValue) {
+        HandValue playerHandValue = getHandValue();
+        if (isOnlyPlayerBlackjack(playerHandValue, dealerHandValue)) {
+            return GameResult.BLACKJACK_WIN;
+        }
+        if (playerHandValue.isBust() || isLowerThanDealerHandValue(playerHandValue, dealerHandValue)) {
+            return GameResult.LOSE;
+        }
+        if (dealerHandValue.isBust() || isHigherThanDealerHandValue(playerHandValue, dealerHandValue)) {
+            return GameResult.WIN;
+        }
+        return GameResult.DRAW;
+    }
+
+    private boolean isOnlyPlayerBlackjack(HandValue playerHandValue, HandValue dealerHandValue) {
+        return playerHandValue.isBlackjack() && dealerHandValue.isNotBlackjack();
+    }
+
+    private boolean isLowerThanDealerHandValue(HandValue playerHandValue, HandValue dealerHandValue) {
+        return dealerHandValue.isNotBust() && playerHandValue.isLowerThan(dealerHandValue);
+    }
+
+    private boolean isHigherThanDealerHandValue(HandValue playerHandValue, HandValue dealerHandValue) {
+        return playerHandValue.isNotBust() && playerHandValue.isHigherThan(dealerHandValue);
+    }


무승부까지 포함한 승패 판단 로직을 간결하게 잘 작성한 것 같아!
또 isBlackjack, isNotBust 등 여러 메서드를 도입하면서, 가독성이 많이 좋아진 것 같아!

이건 상태 패턴을 사용하지 않아서 나타나는 덕지덕지 조건문으로도 바라볼 수 있을 것 같은데 😭
이미 미션을 구현한 상태에서 상태 패턴을 도입하려니까 다 뜯어고쳐야 하더라고,, 결국 실패했어

leegwichan · 2024-03-16T12:26:41Z

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

+    private void validateCount(int count) {
+        if (count < MIN_PLAYER_COUNT || count > MAX_PLAYER_COUNT) {
+            throw new IllegalArgumentException(
+                    "플레이어는 최소 " + MIN_PLAYER_COUNT + "명에서 최대 " + MAX_PLAYER_COUNT + "명까지 가능합니다"
+            );
+        }
+    }


이건 그냥 내가 리뷰하면서 공부한 거 공유

private void validateCount(int count) { if (count < MIN_PLAYER_COUNT || count > MAX_PLAYER_COUNT) { throw new IllegalArgumentException( "플레이어는 최소 %d명에서 최대 %d명까지 가능합니다".formatted(MIN_PLAYER_COUNT, MAX_PLAYER_COUNT) ); } }

나는 보통 위에 같이 쓰는 편이 더 좋을까라고 생각하는데, 그냥 + 연산 쓰는 것보다 성능이 안나온다 하더라고;;
가독성 면에서도 너가 사용한 코드가 더 좋은것 같기도 해서;; 한 번 내 방법을 제안하려다가 철회;;
참고 자료

오... 나도 에러 메시지를 커스텀할 때 마다 + 연산자를 쓰는거보다 더 좋은 방법이 있지 않을까?.. 고민했는데
(알고 이렇게 사용한건 아니야..)

직접 공부해보고 내용 공유 감사감사 😄 👍 👍

leegwichan · 2024-03-16T12:32:04Z

src/main/java/blackjack/dto/CardDto.java

+    public static CardDto fromCard(Card card) {
+        return new CardDto(card.getCardNumber(), card.getCardShape());
+    }


단순히 from()이라고 쓰는게 더 좋을지도?
참고 자료 : 정적 팩토리 메서드 네이밍 컨밴션

지금은 메소드가 하나라 from도 괜찮을 것 같긴한데
나중에 여러개가 된다면 시그니처만 봤을 때 구분이 안될 것 같아 이렇게 지어봤어
컨벤션이 존재하는지.. 나도 더 찾아봐야겠어

leegwichan · 2024-03-16T12:37:32Z

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

+    @Test
+    @DisplayName("equals & hashCode 재정의 테스트")
+    void equalsTest() {
+        Card card = new Card(CardShape.HEART, CardNumber.KING);
+
+        assertThat(card).isEqualTo(new Card(CardShape.HEART, CardNumber.KING));
+        assertThat(Objects.hash(card)).isEqualTo(Objects.hash(new Card(CardShape.HEART, CardNumber.KING)));
+    }


나의 "equals & hashCode 재정의 테스트" 하는 방법 제안!
(조금 과하게 이것저것 검증 넣었으니까 적당히 필요한 것만 챙기면 될 듯)

Suggested change

@Test

@DisplayName("equals & hashCode 재정의 테스트")

void equalsTest() {

Card card = new Card(CardShape.HEART, CardNumber.KING);

assertThat(card).isEqualTo(new Card(CardShape.HEART, CardNumber.KING));

assertThat(Objects.hash(card)).isEqualTo(Objects.hash(new Card(CardShape.HEART, CardNumber.KING)));

}

@Test

@DisplayName("equals & hashCode 재정의 테스트")

void equalsTest() {

Card card1 = new Card(CardShape.HEART, CardNumber.KING);

Card card2 = new Card(CardShape.HEART, CardNumber.KING);

Card differentCard = new Card(CardShape.HEART, CardNumber.QUEEN);

assertThat(card1).isEqualTo(card1)

.isEqualTo(card2)

.isNotEqualTo(differentCard)

.hasSameHashCodeAs(card2);

}

leegwichan · 2024-03-16T12:40:54Z

src/test/java/blackjack/domain/gamer/PlayerTest.java

+    private Hand score14Hand, score21Hand, bustHand, blackjackHand;
+
+    @BeforeEach
+    void setUP() {
+        score14Hand = new Hand(List.of(
+                new Card(CardShape.DIAMOND, CardNumber.KING), new Card(CardShape.SPADE, CardNumber.FOUR)
+        ));
+
+        score21Hand = new Hand(List.of(
+                new Card(CardShape.CLOVER, CardNumber.KING), new Card(CardShape.SPADE, CardNumber.NINE),
+                new Card(CardShape.SPADE, CardNumber.TWO)
+        ));
+
+        bustHand = new Hand(List.of(
+                new Card(CardShape.HEART, CardNumber.KING), new Card(CardShape.SPADE, CardNumber.JACK),
+                new Card(CardShape.SPADE, CardNumber.TWO)
+        ));
+
+        blackjackHand = new Hand(List.of(
+                new Card(CardShape.SPADE, CardNumber.ACE), new Card(CardShape.SPADE, CardNumber.QUEEN)
+        ));
+    }


score14Hand, score21Hand, bustHand, blackjackHand를 각각 private final로 선언해주어도 될 것 같은데, @BeforeEach를 쓴 특별한 이유가 있을까?

다른 테스트 코드와의 일관성을 생각해서 @BeforeEach를 사용했어!
커찬 말대로 private final로 선언해주어도 아무 문제는 없을 것 같아

leegwichan · 2024-03-16T12:42:23Z

src/test/java/blackjack/domain/gamer/PlayersTest.java

+	@Test
+	@DisplayName("플레이어는 최대 8명까지만 가능하다.")
+	void maximumPlayerCountTest() {
+		assertThatThrownBy(() -> new Players(of("a", "b", "c", "d", "e", "f", "g", "h", "g")))
+			.isInstanceOf(IllegalArgumentException.class)
+			.hasMessage("이름은 중복될 수 없습니다.");
+	}


그냥 놓친 것 하나

Suggested change

@Test

@DisplayName("플레이어는 최대 8명까지만 가능하다.")

void maximumPlayerCountTest() {

assertThatThrownBy(() -> new Players(of("a", "b", "c", "d", "e", "f", "g", "h", "g")))

.isInstanceOf(IllegalArgumentException.class)

.hasMessage("이름은 중복될 수 없습니다.");

}

@Test

@DisplayName("플레이어는 최대 8명까지만 가능하다.")

void maximumPlayerCountTest() {

assertThatThrownBy(() -> new Players(of("a", "b", "c", "d", "e", "f", "g", "h", "i")))

.isInstanceOf(IllegalArgumentException.class)

.hasMessage("플레이어는 최소 2명에서 최대 8명까지 가능합니다");

}

Libienz

하이 재즈 😎
재즈랑 얘기하고 싶은 부분 코멘트 남겨봤어

일이 있어서 늦게 남길 수 밖에 없었네ㅠ 양해 부탁..😓
블랙잭 미션 머지까지 같이 힘내보자잇 💪🏻

Libienz · 2024-03-16T18:40:35Z

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

+
+    private static final List<Card> CACHE = Arrays.stream(CardShape.values())
+            .flatMap(cardShape -> Arrays.stream(CardNumber.values())
+                    .map(number -> new Card(cardShape, number))).toList();
+


덱 캐싱 👍🏻

Libienz · 2024-03-16T18:51:17Z

src/main/java/blackjack/domain/game/GameAccount.java

+    public Money calculateDealerIncome() {
+        int dealerIncome = 0;
+        for (Money money : store.values()) {
+            dealerIncome += money.value();
+        }
+        return new Money(-dealerIncome);
+    }


Suggested change

public Money calculateDealerIncome() {

int dealerIncome = 0;

for (Money money : store.values()) {

dealerIncome += money.value();

}

return new Money(-dealerIncome);

}

public Money calculateDealerIncome() {

int total = calculatePlayersTotalProfit()

return new Money(-total);

}

플레이어 수익의 총합을 계산할 수 있다라는 메서드로 구분할 수 있을 것 같네
책임 분리의 관점에서 고치는게 좋아보이는 것 같아 😀

그리고 이렇게 고치면서 가장 좋은 효과를 볼 수 있는 부분은 따로 있다고 생각하는데 바로 dealerIncome이라는 불변하지 않은 변수를 사용하지 않아도 된다는 것!

상태가 변하게 되는 변수는 (맥락에 따라 다르겠지만) 시스템의 취약성과 직결된다고 생각해
불변하게 사용할 수 있으면 사용하는게 개인적으로 좋다는 생각이야! 👊

추가적으로 만약 calculatePlayersTotalProfit메서드가 Money를 반환하게 하고

Money도메인에 inverse혹은 negate메서드 기능을 추가하면 금액을 마이너스로 바꾸는 책임을 책임에 맞는 도메인에 위임할 수 있을 듯 👍

오오 리비 정말 좋은 의견인 것 같아
예리하군 👍 👍

Libienz · 2024-03-16T18:55:57Z

src/main/java/blackjack/domain/game/Money.java

+public record Money(int value) {
+
+    public Money multipleRatio(double ratio) {
+        return new Money((int) (this.value * ratio));
+    }
+}


레코드는 내가 잘 모르는 부분이라 재즈에게 물어보고 싶군 👊

데이터 전달을 위한 목적으로 레코드가 탄생하게 되었다고 나는 알고 있어

도메인 로직을 포함하는 레코드에 대해서는 어떻게 생각해 ?

맞아 나도 저번 테코톡에서 설명을 듣고, 레코드를 만든 진짜 목적을 알게되었는데
이게 뭔가... 자꾸 인텔리제이에서 변환 추천을 하니까, 다들 이렇게 사용해서 추천을 해주는건가 싶더라구

해외 개발자들은 레코드를 어떻게 사용하고 있는지 조만간 찾아볼 생각이야

Libienz · 2024-03-16T18:57:54Z

src/main/java/blackjack/domain/gamer/HandValue.java

+    /**
+     * ACE가 포함된 경우, 21 이하이면서 가장 가능한 큰 값으로 계산한다.
+     */
+    private int adjustSumWithAce(List<Card> cards, int sum) {


적절한 주석인듯 👍🏻

Libienz · 2024-03-16T19:00:21Z

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

+    public GameResult compete(HandValue dealerHandValue) {
+        HandValue playerHandValue = getHandValue();
+        if (isOnlyPlayerBlackjack(playerHandValue, dealerHandValue)) {
+            return GameResult.BLACKJACK_WIN;
+        }
+        if (playerHandValue.isBust() || isLowerThanDealerHandValue(playerHandValue, dealerHandValue)) {
+            return GameResult.LOSE;
+        }
+        if (dealerHandValue.isBust() || isHigherThanDealerHandValue(playerHandValue, dealerHandValue)) {
+            return GameResult.WIN;
+        }
+        return GameResult.DRAW;
+    }


플레이어와 상대와 겨루는 부분이네 👍
다만 평문적으로 해석하면 플레이어는 HandValue와 겨루게 되네 🤔

딜러와 겨룬다!와 같이 추상화 준위를 맞춰주면 어떨까!
메서드를 호출하는 외부 시점에서 불필요한 동작을 제거할 수 있을 것 같아 😀

Libienz · 2024-03-16T19:01:54Z

src/main/java/blackjack/dto/PlayerHandDto.java

+public record PlayerHandDto(String name, HandDto playerHand) {
+
+    public static PlayerHandDto fromPlayer(Player player) {
+        String playerName = player.getName().value();
+        HandDto playerHand = HandDto.fromHand(player.getHand());
+
+        return new PlayerHandDto(playerName, playerHand);
+    }
+}


데이터 전달 객체와 레코드는 위화감 없이 잘 어울리는 것 같군 👍

Libienz · 2024-03-16T19:04:55Z

src/main/java/blackjack/view/OutputView.java

+    public void printInitialHands(DealerInitialHandDto dealerInitialHandDto, PlayersHandDto playersInitialHandDto) {
+        System.out.printf("\n%s와 %s에게 2장을 나누었습니다.\n", DEALER_DEFAULT_NAME,
+                joinPlayerNames(playersInitialHandDto));
+
+        System.out.printf("%s 카드: %s\n", DEALER_DEFAULT_NAME, formatCardName(dealerInitialHandDto.firstCard()));
+        printAllPlayerHands(playersInitialHandDto);
+    }
+
+    private String joinPlayerNames(PlayersHandDto playersInitialHandDto) {
+        return String.join(", ", getPlayerNames(playersInitialHandDto));
+    }
+
+    private List<String> getPlayerNames(PlayersHandDto playersInitialHandDto) {
+        return playersInitialHandDto.playersHandDto().stream().map(PlayerHandDto::name).toList();
+    }
+
+    private String formatCardName(CardDto cardDto) {
+        return CardNumberOutput.convertNumberToOutput(cardDto.cardNumber()) + CardShapeOutput.convertShapeToOutput(
+                cardDto.cardShape());
+    }
+
+    private void printAllPlayerHands(PlayersHandDto playersInitialHandDto) {
+        playersInitialHandDto.playersHandDto().forEach(this::printPlayerHand);
+        System.out.println();
+    }
+
+    public void printPlayerHand(PlayerHandDto playerHandDto) {
+        System.out.println(getPlayerNameOutputFormat(playerHandDto));
+    }
+
+    public void printDealerHand(HandDto dealerHandDto) {
+        System.out.println(getDealerNameOutputFormat(dealerHandDto));


개인적인 (사실 틀릴 수도 있는) 생각인데 🧐

다른 개발자가 클래스를 열어볼 경우에는 자신이 이용할 수 있는 public 메서드를 찾게 되는 것 같아
public메서드가 위에 위치하는 것이 좋지 않을까 🤔

재즈가 컨벤션 관련 코멘트들 어떤 것들을 받아왔는지 공유해주면 너무 좋을 것 같네 😀
나는 호출되는 순서로 배치하라고도 들어보았고 Public을 위로 올리라는 얘기도 들어봤어

나도 사다리 미션에서 페어와 메서드 순서 배치에 대해 논의했던적이 있었어

페어는 자바 컨벤션에 public -> private 으로 나와있으니 컨벤션을 따르자! 는 입장이었고,
나는 결국 우리 코드를 읽어주는 사람은 리뷰어이니, 구조 파악이 쉽도록 논리적 호출 순서대로 배치하자는 의견이었어(이것도 사실 내 주관적인 생각이긴하지, 전자를 선호하는 사람은 보기 매우 불편할 수도 있을거라 생각해)

사실 위 같은 view 코드는 여러 위치에서 호출되는 메서드가 있는 것 같아서,, 내 의견이 소용이 없는 것 같아

일단 내가 찾아 본 결과 자바 컨벤션으로 정해진 건 없는 것 같아.
그리고 리비 말대로 리뷰어마다 선호하는 방식이 다른 것 같아서, 약간 애매하긴 한데,, 😅

아직까지는 호출 순서로 리뷰를 받아본 적이 없어서,
페어와 컨벤션을 정해서 진행을 하다가 리뷰를 받으면 그 때 대처를 하지 않을까....😂

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

src/main/java/blackjack/view/object/CardShapeOutput.java

+    HEART_OUTPUT(CardShape.HEART, "하트"),
+    CLOVER_OUTPUT(CardShape.CLOVER, "클로버"),
+    SPADE_OUTPUT(CardShape.SPADE, "스페이드"),
+    DIAMOND_OUTPUT(CardShape.DIAMOND, "다이아몬드");


Libienz · 2024-03-16T19:06:58Z

src/test/java/blackjack/domain/gamer/PlayerTest.java

+    @Nested
+    @DisplayName("플레이어가 블랙잭 승리하는 경우의 수 테스트")
+    class PlayerBlackjackWinTest {


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

src/test/java/blackjack/domain/gamer/PlayerTest.java

+            assertThat(player.compete(dealerHandValue21)).isEqualTo(GameResult.BLACKJACK_WIN);
+            assertThat(player.compete(dealerBustHand)).isEqualTo(GameResult.BLACKJACK_WIN);
+        }


나누거나 assertAll로 개선할 수 있을 듯 🙌
하나가 깨지면 다른 하나를 테스트 하지 않기 때문에 단박에 오류 지점을 찾기 힘들 것 같아

seokmyungham and others added 21 commits March 12, 2024 11:10

refactor: 출력에 의존하는 Dealer 내 상수를 제거를 위해 리팩토링

75656d1

refactor(Deck): 카드 52장을 재사용할 수 있도록 캐싱

3dce8a7

fix: dto 클래스를 도메인 패키지 안으로 이동

4c41d0f

docs(README): 구현 기능 목록 재정의

cdf6f80

refactor(Hand): 기존 Hand에서 패 점수를 계산하던 로직을 HandValue 객체로 분리

7b7d33f

feat(Player): 딜러의 HandValue를 파라미터로 받아 승,무,패,블랙잭 승리를 계산하는 로직 추가

e99482b

move: 의존 관계를 고려하여 dto 패키지 이동

872cb10

refactor(Player): 승무패를 계산하는 compete 메서드 리팩토링

cfcf2af

feat(InputView): 플레이어로부터 배팅 금액을 입력받는 메서드 추가

23a17b0

feat(GameAccount): 배팅 금액만큼 금액을 저장하는 저장소 클래스 생성

157d53a

feat(GameAccount): 배팅한 금액에 게임 결과를 적용하는 기능 추가

feaa005

feat(GameAccount): 플레이어의 배팅 금액 결과로 딜러의 수익을 계산하는 기능 추가

6753755

feat: 딜러와 플레이어의 최종 수익 결과를 출력하는 기능 추가

920b6cf

feat(GameAccount): map 저장소를 초기화하는 메서드 추가

af934a5

refactor(BlackjackController): 컨트롤러에서 블랙잭 게임 흐름을 담당하는 로직을 분리

661c788

docs(README): 구현 기능 목록 동기화

0343057

fix: Deck, HandValue 생성자 접근 제한자 default로 수정

19e55ee

fix(HandValue): 블랙잭 요구 카드 개수(2) 조건 상수화

c565111

refactor(HandValue): 가독성 향상을 위해 HandValue 생성 메서드 네이밍 수정

6134b06

refactor(Player): 테스트를 위한 생성자 파라미터 수정

4384505

leegwichan reviewed Mar 16, 2024

View reviewed changes

seokmyungham mentioned this pull request Mar 16, 2024

[2단계 - 블랙잭 베팅] 망쵸(김주환) 미션 제출합니다. #16

Open

Libienz reviewed Mar 16, 2024

View reviewed changes

[2단계 - 블랙잭 베팅] 재즈(함석명) 미션 제출합니다. #12

Are you sure you want to change the base?

[2단계 - 블랙잭 베팅] 재즈(함석명) 미션 제출합니다. #12

Uh oh!

Conversation

seokmyungham commented Mar 15, 2024

컨트롤러에서 게임 전반을 담당하는 객체 BlackjackGame을 분리

Step2에서 추가로 구현한 사항

Uh oh!

leegwichan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Libienz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

컨트롤러에서 게임 전반을 담당하는 객체 `BlackjackGame`을 분리