[2단계 - 블랙잭(베팅)] 오찌(오지훈) 미션 제출합니다. #301

Ohzzi · 2022-03-15T15:16:41Z

안녕하세요 범블비 :)
빠르게 2단계 미션으로 돌아왔습니다. 베팅을 추가하는 것 자체는 어려운 것이 아니었는데 최대한 객체 간의 연관성을 고려하면서 짜다 보니 머리를 싸매고 짜게 됐었네요. 최대한 인스턴스 필드 수를 줄이면서 하자니 순환 참조가 일어났었어서 결국 인스턴스 필드는 3개 이상 만들지 않는다는 규칙을 깨고(사실 Participant의 dto에서 이미 깨지긴 했습니다만) 플레이어에 Betting 클래스를 넣어 주었습니다.

사실 그래서 베팅을 넣고 났더니 프로젝트 구조가 뭔가 맘에 안드는 상황이 생긴 것 같긴 합니다. 😭 한번 보시고 부족한 점이 있다면 리뷰 부탁드려요 :)

추가적인 질문이 있습니다! 제가 추상 클래스 Participant의 필드들을 private으로 막아놨는데요, 이렇게 하니 상속받는 클래스에서 해당 필드를 쓰려면 getter로 꺼내와야 하는데요, 어차피 상속받는 클래스들이 해당 필드를 가지고 있는 개념이라고 생각하는데, 이 경우에 그냥 필드들을 protected로 만들어서 쓰는게 더 낫다는 생각이 들어요! 범블비는 어떻게 생각하세요?

지금 State 클래스에

    BUST(cards -> cards.calculateScore().getValue() > 21),
    BLACKJACK(cards -> cards.calculateScore().getValue() == 21 && cards.getCards().size() == 2),
    NOTHING(cards -> true)

이렇게 되어 있는데요, 객체지향 생활 체조? 를 보면 한 줄에 점(.)은 하나만 찍으라고 되어 있는데, 지금 cards.calculateScore().getValue()로 두 번 꺼내서 쓰고 있더라고요. 우선 cards.getCards().size()는 Cards에 size를 만들면 해결이 될 것 같은데, Cards가 Score 클래스의 value 값이 int라는걸 알지 못하게 하면서 여기서 getter를 한 단계 줄일만한 좋은 방법이 생각이 나지가 않아요 ㅠㅠ 이 경우에는 점(.) 찍는 규칙을 위반하면서 짜는게 나은걸까요?

Ohzzi · 2022-03-15T15:37:43Z

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

 import blackjack.domain.card.Card;
 import blackjack.domain.card.Cards;
 import java.util.List;

 public class Player extends Participant {

+    private Betting betting;
+
    private static final int HIT_STANDARD = 21;

    public Player(Name name, List<Card> cards) {
        super(name, new Cards(cards));
    }


헉 이부분은 안쓰는 생성잔데 닫는 것을 깜빡했네요!
마찬가지로 바로 위에 Betting도 final로 선언하는 것을 깜빡 했네요 😢
수정하도록 하겠습니다!

Ohzzi · 2022-03-15T15:39:41Z

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


        // then
-        assertThat(actual).isEqualTo(card);
+        assertThat(deck.draw()).isInstanceOf(Card.class);
    }


이런 식의 테스트가 의미가 있는 테스트 코드일까요?
어차피 무조건 Card의 인스턴스가 반환된다고 생각이 되는데, 마땅히 테스트 할 방법이 없는 것 같아요

크게 의미는 없어보이는 테스트네요. 그런데 기존 방식에서 이렇게 변경한 이유가 있을까요?

어차피 card를 넣었는데 다시 꺼내서 확인하는 것은 card가 참조 변수니까 actual과 무조건 같은 값을 가리켜서 의미가 없지 않을까 했습니다! 그런데 바꾼 방식도 별 의미가 없네요. 보통 이런 메서드는(카드를 한 장 드로우 해 주는지 확인하는 것 같은) 어떻게 테스트 하시나요?

draw의 어떤 동작을 테스트할지 생각해볼 거 같네요. 아마도 draw하면 가장 첫 번째 카드가 뽑힌다 등으로 테스트해볼 거 같아요.

Ohzzi · 2022-03-15T15:40:39Z

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

+
+    public static List<Card> getPool() {
+        return pool;
+    }


고민을 조금 했던 부분인데요, pool을 static으로 유지해서 deck에서 가져다 쓰는 것과 getPool 이라는 static 메서드를 열어두는 것 중 어느 것이 더 나은 방법일까요?

lazy한 초기화를 하는 등 특별히 getter가 하는게 없다면 따로 둘 필요는 없어보이긴 하네요.
이런 부분은 컨벤션의 문제인 것 같습니다.

Ohzzi · 2022-03-15T15:41:48Z

src/main/java/blackjack/domain/Score.java

@@ -12,6 +12,15 @@ public int getValue() {
        return value;
    }

+    public boolean isLessThan(int value) {
+        return this.value < value;
+    }


제가 Comparable을 구현하긴 했는데요, 자체적으로 isLessThan을 쓸 일이 있어서 만들었습니다.
그런데 같은 Score 객체와 비교하는 코드가 아니라 value 값만 비교하는 코드로 사용해도 괜찮겠죠?
Score끼리 비교는 일단 compareTo로 된다고 생각해서요!

Comparator를 사용하지 않는데 Comparable을 구현할 필요는 없는 것 같아요.
isLessThan(Score) isLessThan(int) 처럼 여러 타입에 대한 비교를 허용하는 건 좋아보이는데요, 비교하는 방법이 통일되면 좀 더 이해하기 쉬울 거 같아요.

혹시 방법이 통일된다는게 정확히 어떤 말씀을 말하시는건가요?
사실 지금 정확하게 이해가 가지 않네요 😢

지금과 같은 구조를 의도하는 코멘트였습니다. 제대로 표현을 못했네요 🥲

사실 이 방법이나 compareTo나 가독성이 그리 좋지 않습니다. 이 부분은 자바라는 언어가 갖고 있는 한계이기 때문에 그러려니하고 넘어가시면 될 거 같아요. 연산자 오버로딩을 지원하는 다른 언어를 사용하면 해결될 문제입니다.

ddaaac

안녕하세요 오찌!

빠르게 2단계 미션 구현해주셨네요.
기존 클래스들을 크게 건들지 않고 잘 구현해주셔서 변경 폭이 크다고는 느껴지지 않는데요, 구조가 맘에 안 드는 상황이 어떤 상황인지 구체적으로 말씀해주시면 좋을 거 같아요 😄

몇몇 코멘트 남겼으니 확인 부탁드립니다~

ddaaac · 2022-03-16T14:30:38Z

src/main/java/blackjack/domain/Score.java

@@ -12,6 +12,15 @@ public int getValue() {
        return value;
    }

+    public boolean isLessThan(int value) {
+        return this.value < value;
+    }


Comparator를 사용하지 않는데 Comparable을 구현할 필요는 없는 것 같아요.
isLessThan(Score) isLessThan(int) 처럼 여러 타입에 대한 비교를 허용하는 건 좋아보이는데요, 비교하는 방법이 통일되면 좀 더 이해하기 쉬울 거 같아요.

ddaaac · 2022-03-16T14:37:44Z

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

+
+    public static List<Card> getPool() {
+        return pool;
+    }


lazy한 초기화를 하는 등 특별히 getter가 하는게 없다면 따로 둘 필요는 없어보이긴 하네요.
이런 부분은 컨벤션의 문제인 것 같습니다.

ddaaac · 2022-03-16T15:00:43Z

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


        // then
-        assertThat(actual).isEqualTo(card);
+        assertThat(deck.draw()).isInstanceOf(Card.class);
    }


크게 의미는 없어보이는 테스트네요. 그런데 기존 방식에서 이렇게 변경한 이유가 있을까요?

ddaaac · 2022-03-16T15:15:29Z

src/main/java/blackjack/domain/GameResult.java

-        Map<Outcome, Integer> dealerResult = new EnumMap<>(Outcome.class);
-        Map<String, Outcome> playersResult = new LinkedHashMap<>();
+        Map<String, Integer> playersEarnings = new LinkedHashMap<>();

-        initDealerResult(dealerResult);
        for (Player player : players) {
            Outcome playerOutcome = Outcome.judge(player, dealer);
-            playersResult.put(player.getName(), playerOutcome);
-            dealerResult.merge(playerOutcome.getOpposite(), 1, Integer::sum);
+            int playerEarnings = calculateEarnings(player, playerOutcome);
+            int dealerEarnings = getDealerEarnings(playerEarnings);
+            playersEarnings.merge(dealer.getName(), dealerEarnings, Integer::sum);
+            playersEarnings.put(player.getName(), playerEarnings);
        }

-        return new GameResult(dealerResult, playersResult);
+        return new GameResult(playersEarnings);
    }

-    private static void initDealerResult(Map<Outcome, Integer> dealerResult) {
-        Arrays.stream(Outcome.values())
-                .forEach(value -> dealerResult.put(value, 0));
+    private static int getDealerEarnings(int playerEarnings) {
+        return playerEarnings * (-1);
    }

-    public Map<Outcome, Integer> getDealerResult() {
-        return dealerResult;
+    private static int calculateEarnings(Player player, Outcome outcome) {
+        return (int) (player.getBetting().getValue() * outcome.getEarningsRate());


이 부분 관련해서 코멘트 남겼는데요, 점점 더 도메인 로직들이 많아지네요. 한 번 확인부탁드립니다.

확인했습니다 😄 확실히 제가 봐도 로직이 이상하게 복잡해지네요. 최대한 로직을 줄이면서도 동작이 같도록 리팩토링 해볼게요 :)

ddaaac · 2022-03-16T15:16:13Z

src/main/java/blackjack/domain/GameResult.java

-        Map<Outcome, Integer> dealerResult = new EnumMap<>(Outcome.class);
-        Map<String, Outcome> playersResult = new LinkedHashMap<>();
+        Map<String, Integer> playersEarnings = new LinkedHashMap<>();

-        initDealerResult(dealerResult);
        for (Player player : players) {
            Outcome playerOutcome = Outcome.judge(player, dealer);
-            playersResult.put(player.getName(), playerOutcome);
-            dealerResult.merge(playerOutcome.getOpposite(), 1, Integer::sum);
+            int playerEarnings = calculateEarnings(player, playerOutcome);
+            int dealerEarnings = getDealerEarnings(playerEarnings);
+            playersEarnings.merge(dealer.getName(), dealerEarnings, Integer::sum);
+            playersEarnings.put(player.getName(), playerEarnings);
        }

-        return new GameResult(dealerResult, playersResult);
+        return new GameResult(playersEarnings);
    }

-    private static void initDealerResult(Map<Outcome, Integer> dealerResult) {
-        Arrays.stream(Outcome.values())
-                .forEach(value -> dealerResult.put(value, 0));
+    private static int getDealerEarnings(int playerEarnings) {
+        return playerEarnings * (-1);
    }

-    public Map<Outcome, Integer> getDealerResult() {
-        return dealerResult;
+    private static int calculateEarnings(Player player, Outcome outcome) {
+        return (int) (player.getBetting().getValue() * outcome.getEarningsRate());


최대한 인스턴스 필드 수를 줄이면서 하자니 순환 참조가 일어났었어서 결국 인스턴스 필드는 3개 이상 만들지 않는다는 규칙을 깨고(사실 Participant의 dto에서 이미 깨지긴 했습니다만) 플레이어에 Betting 클래스를 넣어 주었습니다.

너 얼마 벌었어?라는 질문에 대답해줄 수 있는 객체는 누구일까요? Player가 이 질문에 대한 답을 해줄 수 있을 거 같지 않나요? 이를 코드로 변환한다면 player.calculateProfit(outcome)처럼 나타낼 수 있을 거 같아요.
조금 더 들어가면 player.calculateProfit(dealer)와 같이도 표현될 수 있는데 지금 구조에서는 위 방법이 조금 더 적당해보이네요.
이렇게 되면 Player가 배팅 금액을 아는 게 자연스러워집니다. 그리고 가이드가 정확히 어떻게 되는지 기억은 안나지만 아마 3개까지 허용하는 걸로 기억해요. 잘 구현해주셨습니다.

생각해보니 이걸 static 메서드로 뺄 필요가 없었네요! :)

ddaaac · 2022-03-16T15:19:48Z

src/main/java/blackjack/domain/Outcome.java

+    WIN("승", 1),
+    DRAW("무", 0),
+    LOSE("패", -1),
+    WIN_BLACKJACK("승", 1.5);


승리를 했을 때는 2배의 금액을 돌려받고 블랙잭일 경우 1.5배를 돌려받습니다.
이율은 0.5배 아닐까요?

처음에 좀 헷갈리긴 했는데 금액으로 생각한다면 본인이 원래 베팅한 금액 + 수익 으로 해서 이겼을 때 2배를 받는거가 생각해보면 맞기는 한데, 여기서 수익으로 계산하는 부분은 원래 베팅한 금액을 배제한 금액이 되는 것 같더라고요!(그래서 LOSE면 베팅 금액을 잃으니까 수익이 -가 되는 상황이 되는거죠)

아하 제가 룰을 오해하고 있었네요 🥲
블랙잭은 운이 좋아서 걸린거니 0.5배의 수익만 가져가는 거라고 이해하고 있었습니다.

ddaaac · 2022-03-16T15:34:37Z

src/main/java/blackjack/domain/Score.java

@@ -1,6 +1,6 @@
 package blackjack.domain;

-public class Score {
+public class Score implements Comparable<Score> {


객체지향 생활 체조? 를 보면 한 줄에 점(.)은 하나만 찍으라고 되어 있는데, 지금 cards.calculateScore().getValue()로 두 번 꺼내서 쓰고 있더라고요. 우선 cards.getCards().size()는 Cards에 size를 만들면 해결이 될 것 같은데, Cards가 Score 클래스의 value 값이 int라는걸 알지 못하게 하면서 여기서 getter를 한 단계 줄일만한 좋은 방법이 생각이 나지가 않아요 ㅠㅠ 이 경우에는 점(.) 찍는 규칙을 위반하면서 짜는게 나은걸까요?

getter가 등장하면 로직을 객체 안으로 넣을 수 있는지 확인해보면 좋습니다. score.isBust() score.canBlackjack()과 같은 메서드로 감쌀 수 있을 거 같아요.

그리고나서 추천드리는 건 State 클래스를 제거해보세요. Cards 클래스의 메서드만으로도 충분히 깔끔하게 표현이 될 거 같네요.
아마 State란 단어를 사용해주신 걸 보면 상태 패턴에 대해서 알고 계신게 아닐까 추측되는데 #225 (comment) 이전에 말씀드렸던 거처럼 이 부분은 머지할 때 피드백을 드리는 게 좋을 거 같아요. 우선 State 클래스를 제거하는 걸 추천드립니다!

사실 작성할 때는 상태 패턴을 알고 작성한건 아니고 어떻게 하면 클래스를 뺄 수 있을까 생각하긴 한거였는데...ㅎ
안그래도 이 부분은 State 클래스가 꼭 필요할까 고민했던 부분이었어요! 그런데 어차피 isBust나 isBlackjack으로 호출을 할테니 별 의미가 없겠네요! 제거하는걸 고려해볼게요!

ddaaac · 2022-03-16T15:35:55Z

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

-    public boolean isBlackJack() {
+    public boolean isBlackjack() {


Participant 관련 질문에 대한 답변은 여기 달게요.

제가 추상 클래스 Participant의 필드들을 private으로 막아놨는데요, 이렇게 하니 상속받는 클래스에서 해당 필드를 쓰려면 getter로 꺼내와야 하는데요, 어차피 상속받는 클래스들이 해당 필드를 가지고 있는 개념이라고 생각하는데, 이 경우에 그냥 필드들을 protected로 만들어서 쓰는게 더 낫다는 생각이 들어요! 범블비는 어떻게 생각하세요?

#225 (comment) 이전 단계의 코드에 코멘트 남겼습니다. 그런데 Participant의 필드들은 모두 외부에서 getter로 사용중인데요 protected로 바꿀 수 있는 것들이 있을까요?

현재 Participant의 getCards()는 세 곳에서 쓰이고 있습니다.
Dealer의 showInitialCards, Player의 showInitialCards, 그리고 dto 생성자에서 getCards요!
이 경우에 Dealer와 Player는 Participant를 상속받으니까 어차피 cards 필드도 상속받기 때문에 그걸 쓰기 위해서 자식 클래스에서 getCards를 호출하는 것은 피드백 남겨주신 것 처럼 저도 부자연스럽다고 느꼈어요!

protected로 바꾸면 getter가 없어지지는 않겠지만(dto 생성자에서 써야 하니까요) 최소한 자식에서 showInitialCards를 사용할 때 cards를 쓰기 위해 getter를 호출하는 일은 없어질 것 같아서 더 좋다고 생각하는데 범블비는 어떻게 생각하세요? 😄

관련해서 1단계에 코멘트를 남겼었습니다. 확인 부탁드려요!

…로 이동

PlayerNames를 만들어 중복 및 인원수 체크 로직을 이동시켜서 Players의 생성자에서 무한루프가 도는 오류를 수정

Ohzzi · 2022-03-21T08:02:00Z

피드백 사항을 반영하면서 상태 패턴까지 적용하려다 보니까 리뷰 요청이 조금 늦었습니다 😢

DM으로 여쭤봤던 부분들 까지 적용하기 위해 노력했는데요, 다만 피드백 사항 중에 여기서 로직은 이름이 중복되면 안된다보다는 플레이어들의 이름이 중복되면 안된다가 적당하지 않을까요? 해당 도메인 로직을 처리하기 위해서는Players 객체가 필요하지 않을까요? 라는 부분을 적용하면서, 검증 로직을 Players를 만들어서 Players에서 가져갔었는데, Players 생성자에서 Betting 입력 때문에 매개변수로 메서드를 넘겨주는 방식을 채용했다보니 무한루프에 빠지는 오류가 발생했습니다. 그래서 일단은 부득이하게 PlayerNames라는 클래스를 다시 만들어서 거기서 이름에 대한 검증 책임을 가지도록 했습니다. (다만 인원 수 검증에 대한 부분은 여기에 있는게 맞는지 조금 의문이 드네요.)

콜백함수 부분은 적용하려고 공부해보려고 했는데, 아직 명확하게 이해가 가지 않아서 일단은 보류해두었습니다. 미션이 끝난 이후에 다시 한번 공부해보고 미션과는 별개로 적용해볼 수 있도록 할게요 😄

ddaaac

안녕하세요 오찌!

상태패턴까지 구현해주셨네요 👍
쉽지 않으셨을 텐데 짧은 시간에 많은 것들을 해내신 거 같아요.

이번 미션은 여기서 머지해도 충분할 거 같아요. 리뷰 남긴 사항들은 참고만 해주시면 될 것 같습니다.

고생하셨고 다음 체스 미션도 화이팅하세요!

ddaaac · 2022-03-21T15:11:06Z

src/main/java/blackjack/domain/Score.java

+    public boolean isGreaterThan(Score target) {
+        return target.isLessThan(value);
+    }


greaterThan은 greaterThan과 체이닝하면 어떨까요?

일단 구현되어있는 메서드를 사용하려고 하다 보니 그렇게 됐는데, 확실히 greaterThan끼리 체이닝 하는게 더 알아보기 쉽겠네요!

ddaaac · 2022-03-21T15:17:59Z

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

@@ -8,27 +8,16 @@
 public class Cards {

    private static final int BUST_STANDARD = 21;
-
-    private static final int INITIAL_CARDS_SIZE = 2;
+    private static final Score BLACKJACK_SCORE = new Score(21);


이 Score 상수는 Score에 있는게 맞지 않을까요?
조금 더 생각해보면 Score의 책임으로 표현할 수도 있을거 같아요
score.canBlackjack()처럼요.

canBlackjack 이라는 개념을 전혀 생각하지 못한 것 같아요.. 블랙잭의 점수 관련된 건 Score 클래스에서 처리하는게 더 합리적인데 블랙잭의 두가지 조건 중 하나인 카드가 2장이어야 한다는 것에 매몰되어서 더 상위 클래스에 책임을 지운 것 같네요! 당시에는 Score 클래스에서 블랙잭 관련된 메서드가 true를 반환하면 무조건 블랙잭이어야 하는게 아닌가? 라고 생각했거든요! 좋은 의견 감사합니다 :)

ddaaac · 2022-03-21T15:27:56Z

src/main/java/blackjack/domain/state/Running.java

+
+import blackjack.domain.card.Cards;
+
+public abstract class Running extends AbstractState {


DealerRunning#hit과 PlayerRunning#hit에는 공통된 로직이 있는데요, 이 부분을 여기에 두는 건 어떨까요?

public abstract class Running extends AbstractState { @Override public State hit(Card card) { // 공통 로직이 들어가는 자리 return hitByParticipant(Card card); } protected abstract State hitByParticipant(Card card);

사실 저는 클래스 자체를 많이 분리하는것도 좋지 않은 설계가 아닌가 하고 생각하고 있어서 메서드가 조금 중복되더라도 클래스를 새로 안만드는 방향으로 하고 싶었거든요! 아마 지금 리뷰 남겨주신게 내부 클래스를 사용한 것 같은데 저는 중첩 클래스에 아직 익숙하지는 않아서요. 이런식으로 설계하면 클래스 파일을 많이 만들지 않으면서도 중복을 제거할 수 있는 걸까요?

중첩 클래스에 관련된 코멘트는 아니었습니다. 지금과 클래스 갯수는 동일하고, hit은 Running에서 오버라이딩하고 참가자별로 동작이 달라지는 부분만 구현체들에서 hitByParticipant로 처리할 수 있겠네요.

// Running.java public abstract class Running extends AbstractState { protected abstract State hitByParticipant(Card card); @Override public State hit(Card card) { // 공통 로직이 들어가는 자리 return hitByParticipant(Card card); } } // DealerRunning.java public final class DealerRunning extends Running { @Override protected State hitByParticipant(Card card) { if (shouldHit(cards)) { return new DealerRunning(cards); } return new Stand(cards); }

ddaaac · 2022-03-21T15:28:54Z

src/main/java/blackjack/domain/state/State.java

+import blackjack.domain.card.Card;
+import java.util.List;
+
+public interface State {


네이밍이 설계 패턴에 맞춰서 지어진 거 같아요 😀
참가자들의 손패를 의미하니 Hand 정도면 어떨까요?

사실 이부분이랑 Cards에서도 고민을 한게 플레이어가 가지고 있는 손패를 의미하니까 Hand라고 하고 싶었는데, 다른 크루들과 얘기해보니 Hand라고 지으면 뭔가 잘 알아듣지 못할 것 같다는 이야기를 하더라고요? Hand 정도면 쉽게 이해할 수 있는(손패라는 의미로) 단어일까요?

Cards도 괜찮아보이네요. 보통 카드게임에서는 손패를 Hand라고 하니 도메인 용어를 사용하는 건 괜찮다고 생각합니다.
하지만 가장 중요한 건 같이 코드를 짜는 팀원들이 쉽게 알아볼 수 있는 단어겠죠 😀

ddaaac · 2022-03-21T15:31:30Z

src/main/java/blackjack/domain/state/DealerRunning.java

+    public static State start(List<Card> initialCards) {
+        Cards cards = new Cards(initialCards);
+        if (cards.isBlackjack()) {
+            return new Blackjack(cards);
+        }
+        if (shouldHit(cards)) {
+            return new DealerRunning(cards);
+        }
+        return new Stand(cards);
+    }
+
+    @Override
+    public State hit(Card card) {
+        Cards cards = this.cards.add(card);
+        if (cards.isBust()) {
+            return new Bust(cards);
+        }
+        if (shouldHit(cards)) {
+            return new DealerRunning(cards);
+        }
+        return new Stand(cards);
+    }


중복되는 로직이 보이네요. start에서 카드 리스트들을 모두 hit해주는건 어떨까요?
cards.isBlackjack()이 카드 사이즈까지 확인해주고 있게 잘 짜여져있어서 이 로직을 hit으로 옮기면 가능해보여요.

블랙잭 확인 다음 부분으로 hit을 사용하는 방법을 고려해보도록 하겠습니다! :)

코멘트가 정확하게 전달되지 않은 거 같아서 코드로 남겨요!
정확하게 문법이 맞는지는 모르겠지만 아래와 같은 코드를 의도했습니다.
hit에 관련된 로직은 한 메서드에서 관리하면 좋을 것 같네요 :)

public static State start(List<Card> initialCards) { State state = new DealerRunning(new Cards(Collections.emptyList())); return initialCards.stream() .reduce(state, State::hit, (s1, s2) -> s2); } @Override public State hit(Card card) { Cards cards = this.cards.add(card); if (cards.isBlackjack()) { return new Blackjack(cards); } if (cards.isBust()) { return new Bust(cards); } if (shouldHit(cards)) { return new DealerRunning(cards); } return new Stand(cards); }

ddaaac · 2022-03-21T15:37:41Z

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

+public final class Stand extends Finished {
+
+    Stand(Cards cards) {
+        super(cards);
+    }
+}


지금은 Finished의 구현체들이 아무런 로직을 갖고 있지 않습니다. enum이 연상되네요.

여기에 수익률을 구하는 로직을 추가해보는 건 어떨까요? Outcome에 있는 로직들이 각 Finished로 옮겨질 수 있을 거 같네요.
아래와 비슷한 모양으로 나올 거 같아요.

public final class Stand extends Finished { @Override public int calculateProfit(Betting betting, Dealer dealer) { if (dealer.isBust()) return betting * 1 ..... } }

ddaaac · 2022-03-21T15:38:35Z

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

+    public void proceed(CardDeck deck, HitRequest hitRequest) {
+        while (!isFinished() && hitRequest == HitRequest.YES) {
+            hit(deck);
+        }


콜백 시도의 흔적이군요 😀

흔적은 남아있지만 콜백을 명확하게 이해하는데는 실패했어요 😢

ddaaac · 2022-03-21T15:49:43Z

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

+import java.util.List;
+import java.util.stream.Collectors;
+
+public class PlayerNames {


Players를 만들어서 Players에서 가져갔었는데, Players 생성자에서 Betting 입력 때문에 매개변수로 메서드를 넘겨주는 방식을 채용했다보니 무한루프에 빠지는 오류가 발생했습니다. 그래서 일단은 부득이하게 PlayerNames라는 클래스를 다시 만들어서 거기서 이름에 대한 검증 책임을 가지도록 했습니다. (다만 인원 수 검증에 대한 부분은 여기에 있는게 맞는지 조금 의문이 드네요.)

지금은 이름과 배팅 금액을 따로 생각해서 이름 검증을 먼저한 다음에 배팅 금액을 입력받아서 Players를 생성하려고 하시는 거 같아요.(아마 콘솔 환경의 영향이 큰 거 같네요)
만약 플레이어 이름과 배팅 금액 두 가지가 플레이어를 만들기 위한 하나의 데이터 덩어리라고 생각하면 플레이어 이름과 배팅 금액을 모두 입력 받은 다음에 이후에 Players를 생성하면서 검증이 일어나도 될 거 같네요.

사실 이 미션 자체가 콘솔에서만 진행되어서 그런건지는 몰라도, 아니면 제가 미션 진행 예시에 집착하고 있는 건지는 몰라도 아직은 이 문제를 해소하기 위한 방법이 잘 떠오르지는 않네요 😢 플레이어 이름과 베팅 금액 모두 플레이러를 만들기 위한 데이터 덩어리라는 말씀은 확실히 공감하는 부분인 만큼, 머지 이후에도 조금 더 고민해보도록 하겠습니다!

Ohzzi added 21 commits March 15, 2022 12:37

refactor: 카드 덱의 구성 방식을 LinkedList로 바꾸고 draw 시 remove 메서드로 카드 반환하도록 변경

d260ec1

style: 접근 제어자 순서 변경

2ebdb05

refactor: 52장의 카드를 가진 덱 생성을 정적 팩토리 메서드로 변경

e5b9592

refactor: HitRequest 생성 시점을 view에서 입력을 받았을 때로 변경

320970f

refactor: 카드 덱의 자료구조를 리스트에서 큐로 변경

0c7d20c

refactor: 빈 덱인지 검증하는 로직 분리

c6ba660

refactor: 참가자가 Score의 value를 가지는 꺼내서 가지지 말고 Score 자체를 사용하도록 변경

281c0bf

fix: 잘못된 메서드 사용 수정

30b9b66

refactor: 딜러 결과를 구하는 메서드 분리

f94176f

docs: 요구 기능 및 객체 별 역할 재정리

d44185a

feat: 베팅 객체 구현

110a51c

chore: Name을 participant 패키지로 이동

4eeb85a

feat: 승패 결과에 블랙잭으로 인한 승리 추가

f13022f

feat: 플레이어에 베팅 상태 추가

30996e3

refactor: 베팅 객체에서 수익률 계산 로직 제거

86765b3

feat: GameResult를 승무패 저장에서 수익 저장으로 변경

95d3c2b

feat: 게임 종료 시 딜러와 플레이어의 수익 출력

4e0f6e2

fix: 메시지 출력 시 개행 조정

490fb54

style: 잘못된 들여쓰기 조정

3538e24

refactor: 딜러 수익을 플레이어 수익의 반대로 하는 로직 분리

b1cab92

refactor: 불필요한 로직과 테스트 제거하고 수익률 테스트 추가

29af8b8

Ohzzi commented Mar 15, 2022

View reviewed changes

refactor: 불필요한 조건문 제거

d76d395

ddaaac requested changes Mar 16, 2022

View reviewed changes

Ohzzi added 6 commits March 17, 2022 15:40

refactor: 수익률 변수 명 조금 더 쉽게 변경

d816dc8

refactor: 수익금 계산 메서드를 플레이어로 이동

e131a0b

refactor: State를 없애고 해당 로직을 Cards로 이동

87e82c3

refactor: 수익 계산 로직 간소화

6c8c717

refactor: 사용하지 않는 생성자 삭제

36855da

refactor: 승패 계산 시 불필요한 조건문 제거

80aead9

Ohzzi added 25 commits March 19, 2022 01:48

feat: Hit 상태 구현

44ca077

feat: Started의 hit에 Bust 로직 추가

ed50441

fix: cards.add 시 반환값을 무시하는 오류 수정

45fbc6f

feat: Bust 상태 구현

453ae04

refactor: 게임 진행을 더 할 수 없는 상태들을 추상 클래스 Finished의 상속 관계로 묶어줌

0cd60cf

refactor: 게임이 진행중인 상태들을 추상 클래스 Running의 상속 관계로 묶어줌

79a8465

refactor: Bust 생성자 public에서 default로 변경

4f6e5a7

fix: 테스트 시 카드 중복 오류 수정

c8d8697

feat: 각 상태에 blackjack, bust, finished 인지 여부를 반환하는 기능 추가

158061e

refactor: Score와 Score 비교에 메서드 체이닝 사용하도록 변경

f773465

refactor: Participant가 State를 가지고 사용하도록 변경

2477149

refactor: 점수 계산을 Cards를 getter로 꺼내지 않고 State 안에서 처리하도록 이동

f40aa15

refactor: Hit, Started, Running을 Running 하나로 통합

21a914d

refactor: State와 Running, Finished 사이에 공통 부분을 AbstractState로 묶어줌

9119d44

refactor: getCards가 Cards가 아닌 리스트 반환하도록 변경

a0dd0eb

refactor: 게임 진행 시 플레이어의 hit 조건 판정 로직을 변경

3641a00

test: 카드 덱의 draw 테스트를 조금 더 의미 있는 테스트로 변경

9be5ffa

refactor: State 객체에서 isFinished를 제외한 상태 판정 메서드를 instnaceOf로 대체

eb9631d

refactor: Running을 딜러와 플레이어 별로 분리

d933b6b

refactor: "플레이어의 이름은 중복될 수 없다"는 규칙 검증 책임을 Name의 컬렉션에서 Player의 일급 컬렉션으…

e9f038d

…로 이동

feat: "플레이어는 2~8인이어야 한다"라는 규칙 추가

3eddd19

fix: 중복 및 인원수 체크 로직 시 무한루프 발생하는 오류 수정

0a96c63

PlayerNames를 만들어 중복 및 인원수 체크 로직을 이동시켜서 Players의 생성자에서 무한루프가 도는 오류를 수정

refactor: 불필요한 메서드와 테스트 삭제

62f21df

docs: 객체 별 역할 재정리

d1ee8a2

refactor: 상태들의 접근 제어자 조정

b750865

Ohzzi added 2 commits March 21, 2022 17:20

refactor: view 클래스들의 생성자 호출을 막기 위해 private으로 변경

f828c08

refactor: 사용하지 않는 메서드 제거

537e3c6

ddaaac approved these changes Mar 21, 2022

View reviewed changes

ddaaac merged commit d524d92 into woowacourse:ohzzi Mar 21, 2022

		public boolean isBlackJack() {
		public boolean isBlackjack() {


		import blackjack.domain.card.Cards;

		public abstract class Running extends AbstractState {

[2단계 - 블랙잭(베팅)] 오찌(오지훈) 미션 제출합니다. #301

[2단계 - 블랙잭(베팅)] 오찌(오지훈) 미션 제출합니다. #301

Conversation

Ohzzi commented Mar 15, 2022 • 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

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

ddaaac 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

Ohzzi Mar 17, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ohzzi Mar 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ohzzi commented Mar 21, 2022

ddaaac 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

Choose a reason for hiding this comment

Ohzzi commented Mar 15, 2022 •

edited

Loading

Ohzzi Mar 17, 2022 •

edited

Loading

Ohzzi Mar 17, 2022 •

edited

Loading