[2단계 - 블랙잭 베팅] 구름(김민수) 미션 제출합니다. #675

alstn113 · 2024-03-11T08:23:04Z

안녕하세요 파랑! 리뷰이 구름 ☁️ 입니다.
우선 1차때 시간에 맞춰 제출하지 못한점 죄송합니다.

제가 궁금한 점은 다음과 같습니다.

⭐️ 딜러와 플레이어가 추상클래스(참여자)를 상속받아서 만든 이유

딜러와 플레이어의 중복 코드를 제거할 수 있는 방법은 총 4가지로 생각했습니다.

플레이어를 상속 받은 딜러
행동에 관련된 인터페이스 생성
행동에 대한 클래스를 만들고 딜러와 플레이어에서 조합해서 사용
추상클래스를 상속 받은 플레이어와 딜러

1번의 경우 간단한 구조를 가지지만 딜러와 플레이어가 다르게 동작해야하는 경우 적절하지 않다고 생각했습니다.
2번의 경우 관계가 유연해지지만 딜러와 플레이어 모두 구현해야하기에 코드가 복잡해진다고 생각했습니다.

3번과 4번에 대해서 고민을 했습니다.

상속을 하면 부모 클래스가 변할 경우 자식 클래스에도 영향이 간다는 단점이 있었습니다.
하지만 "카드 받기"와 "카드 정보 가져오기"등 공통된 역할을 업캐스팅을 통해 편하게 처리할 수 있어서 4번을 선택했습니다.

파랑은 어떤 것이 더 적합하다고 생각하시나요??

⭐️ CardShape이나 CardRank의 view 정보를 분리해야할까?

도메인에 뷰 정보가 들어 있으면 다음과 같은 단점이 있습니다.

도메인에 뷰 정보가 있으면 뷰가 변경될 경우 도메인이 변경된다.
뷰가 여러 개인 경우 다르게 처리할 수 없다.

이 문제를 해결할 수 있는 방법은 다음과 같습니다.

view에서 CardShape과 CardRank에 대한 enum을 더 만들고 매핑하는 것입니다.

하지만 현재 상태에서 그것을 처리하기에는 복잡도가 너무 많이 올라가서 처리하지 않았습니다.
필요한 상황이 됐을 때 view에 enum으로 처리하는 것이 어떨까요??

⭐️ Controller의 게임 로직들을 분리해야할까?

게임과 관련된 로직들을 BlackjackGame이라는 도메인 혹은 서비스를 만들고 Deck, Bettings를 분리할 수 있다고 생각합니다.
또한 컨트롤러가 아닌 도메인, 서비스 층에서 테스트할 수 있다고 생각합니다.

컨트롤러에서의 테스트는 복잡도가 높아 유지보수의 비용이 크다고 알고 있습니다.
하지만 현재 컨트롤러에서 사용하는 각각의 도메인들이 충분히 테스트되어서 분리할 필요성을 느끼지 못했습니다.

이 부분에 대해서 어떻게 생각하시나요??

⭐️ 승패 판단을 어디서 해야할 지 모르겠다.

첫번째 승패 판단이 들어 있던 곳은 PlayerGameResult Enum입니다.
승, 무, 패의 값이 있어서 여기가 처리하기 좋겠다 싶었지만 복잡한 로직이 들어가서 enum의 책임은 아니라고 생각했습니다.
두번째 승패 판단이 들어 있던 곳은 Dealer 클래스입니다.
플레이어의 승패를 판단하기 위해서는 딜러의 상태를 알아야하므로 딜러가 판단하는 것이 맞다고 생각했습니다.
이 부분에서 PlayerGameResult를 Dealer 위치로 그대로 옮겼습니다.
하지만 승패 판단 로직이 딜러 코드의 절반에 해당해서 굉장히 복잡하다고 생각했습니다.
현재는 승패 판단 코드를 Judge라는 클래스로 분리해서 사용하고 있습니다.
Dealer는 Judge라는 클래스를 가지고 플레이어 승패를 판단해주는 메서드가 있습니다.

승패를 판단하기 위해서는 점수가 필요하고, 또 블랙잭 상태가 필요합니다.
또한 플레이어, 딜러에 따라 결과가 다르니 Player, Dealer가 꼭 필요한 것 같습니다.
그래서 자꾸 승패 판단이 다른 객체와 결합도가 높아지네요

이 부분에 대해서 어떻게 생각하시나요??

그 외

마지막으로 캐싱과 상태 패턴을 이용해서 한다는 방법도 생각해봤습니다.
캐싱은 한 번의 게임이고, 덱을 하나만 사용하기 때문에 불필요하다고 생각했습니다.
상태 패턴으로 처음에 만들어 볼려했는데 생각보다 상태를 안가져도 복잡도가 높지 않기에 사용하지 않았습니다.

다시 한 번 1차 제출에 대해서 사과드립니다.

미리 리뷰 감사합니다. 파랑 🔵 ~

summerlunaa

구름 안녕하세요~

2단계 빠르게 구현해주셨군요ㅎㅎ 리뷰 남겨드렸으니 확인부탁드립니다 🙇‍♀️

summerlunaa · 2024-03-11T11:38:17Z

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

    public boolean equals(Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
+        if (this == o) {


equals & hashCode 오버라이딩 👍

summerlunaa · 2024-03-11T12:06:25Z

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

@@ -38,17 +41,11 @@ private int calculateHardHandScore() {
    }


.mapToInt(Card::getScore) .sum();

으로도 수정할 수 있겠네요~!

처음에 mapToInt를 사용했다가 reduce가 더 성능이 좋을 것 같았습니다.
확인해보니 reduce가 내부적으로 Integer -> int, int -> Integer를 해서 더 성능이 느리군요!!!
의외네요. 수정하겠습니다.

네 사실 reduce가 더 빠르다 하더라도 그렇게 가독성이 좋은 것 같지는 않습니다 😅 성능의 차이가 크지 않다면 가독성을 챙기는 것이 더 이득일 수도 있어요.

summerlunaa · 2024-03-11T12:08:24Z

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


-class Name {
+public class Name {


값객체 👍 동등성이 필요한 객체로 보여서 equals&hashCode를 오버라이딩 해도 좋겠네요

summerlunaa · 2024-03-11T12:20:36Z

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


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

 public abstract class Participant {


딜러와 플레이어가 추상클래스(참여자)를 상속받아서 만든 이유
파랑은 어떤 것이 더 적합하다고 생각하시나요??

여러 케이스를 생각해보고 합리적인 판단을 내리신 것 같습니다.
플레이어와 딜러 모두 참가자라는 점(is-a 관계), 공통된 역할이 필요하다는 점 등을 고려해보았을 때 저도 4번을 선택할 것 같네용

summerlunaa · 2024-03-11T12:22:56Z

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

@@ -8,6 +8,6 @@ public Player(String name) {

    @Override
    public boolean isPlayable() {
-        return !(hand.isBust() || hand.isBlackJack());
+        return !isBust() && !isBlackJack();


훨씬 읽기 좋네요 👍

summerlunaa · 2024-03-11T13:07:17Z

src/test/java/blackjack/domain/JudgeTest.java

+        @Nested
+        @DisplayName("플레이어의 점수가 21점 이하이고, ")
+        class PlayerScoreLessThanOrEqual21 {
+            @DisplayName("플레이어의 점수가 딜러의 점수보다 높으면 플레이어가 이긴다.")


한 줄 띄우면 더 가독성이 좋아질 것 같아요

summerlunaa · 2024-03-11T13:07:56Z

src/test/java/blackjack/domain/JudgeTest.java

+import org.junit.jupiter.api.Test;
+
+class JudgeTest {
+    @Nested


Nested를 이용해서 테스트 코드 읽기가 좋네요 👍

summerlunaa · 2024-03-11T13:15:28Z

src/main/java/blackjack/controller/BlackjackController.java

+    public void run() {
+        try {
+            Participants participants = createParticipants();
+            Deck deck = Deck.createShuffledDeck();
+            Bettings bettings = new Bettings();
+
+            initialDeal(participants, deck);
+            placeBetsByPlayers(participants, bettings);
+            playersTurn(participants.getPlayers(), deck);
+            dealerTurn(participants.getDealer(), deck);
+            printResult(participants, bettings);
+        } catch (IllegalArgumentException e) {
+            outputView.printErrorMessage(e.getMessage());
+        }
+    }


함수(또는 메서드)의 길이가 10라인을 넘어가지 않도록 구현한다.

함수(또는 메소드)가 한 가지 일만 하도록 최대한 작게 만들어라.

가능하면 지켜볼까요?

수정했습니다.

summerlunaa · 2024-03-11T13:16:30Z

src/main/java/blackjack/controller/BlackjackController.java

+        for (Participant participant : participants.getParticipants()) {
+            participant.hit(deck.draw());
+            participant.hit(deck.draw());
+        }


Participant 내부로 넣을 수 있는 로직으로 보여요. participant.hit(deck.draw());의 중복도 줄여볼까요?

혹시 Participant에서 public void hit(Deck deck)이 되게 하라는 말씀이실까요?
이 코드의 문제점은 테스트할 때 Card가 아닌 Deck을 만들어야해서 복잡도가 올라가는 것 같습니다.

아니면 Participants에서 2장씩 뽑는 것을 의미하신걸까요?
제 생각에는 Participants의 책임과 거리가 먼 것 같아서 하지 않았습니다...

hit의 파라미터로 count를 넣을까 했는데 뭔가 메서드도 늘어나고 count가 1인 경우 오버로드도 해야해서 뭔가 이상한 느낌이더라구요;;

participant.hit(deck.draw());을 뽑아야 하는 횟수만큼 코드로 반복하는 것이 좋은 방향은 아니라고 봤어요~
각각의 방법들에 장단점이 있으니 고민해보시면 좋을 것 같습니다.

Particiapnts에 위치하게 수정했습니다.

summerlunaa · 2024-03-11T13:25:33Z

src/main/java/blackjack/controller/BlackjackController.java

+import java.util.List;
+import java.util.Map;
+
+public class BlackjackController {


Controller의 게임 로직들을 분리해야할까?

view까지 domain 로직으로 넘어가지 않는 선에서 controller를 줄일 수 있다면 줄이는 게 좋겠죠~ controller 보다는 domain이 풍부한 게 좋으니까요. 현재 로직에서는 view와 섞인 코드가 많아 분리가 쉽진 않겠네요ㅎㅎ.

alstn113 · 2024-03-12T04:58:44Z

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

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


Hand의 add 메서드가 Hand를 리턴하게 해서 Hand를 불변 객체로 수정했습니다.

alstn113 · 2024-03-12T13:04:01Z

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

+public class Card {
+    private static final Map<String, Card> CARD_POOL = new HashMap<>();
+
+    private final CardRank cardRank;
+    private final CardShape cardShape;
+
+    static {
+        for (CardRank cardRank : CardRank.values()) {
+            for (CardShape cardShape : CardShape.values()) {
+                CARD_POOL.put(toKey(cardRank, cardShape), new Card(cardRank, cardShape));
+            }
+        }
+    }
+
+    private Card(CardRank cardRank, CardShape cardShape) {
+        this.cardRank = cardRank;
+        this.cardShape = cardShape;
+    }
+
+    public static Card of(CardRank cardRank, CardShape cardShape) {
+        return CARD_POOL.get(toKey(cardRank, cardShape));
+    }


카드 캐싱 기능 추가해봤습니다~

summerlunaa

구름 안녕하세요~

제 리뷰 속도가 구름의 반영 속도를 따라가지 못하네요 ...ㅎㅎ ㅠㅠ 빠른 반영과 추가 리팩토링까지~ 수고하셨습니다.

지금도 요구사항을 충분히 만족했지만 아쉬우실까봐 한번 더 RC 드려봅니다~ 파이팅 💪

summerlunaa · 2024-03-12T12:43:25Z

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

+
+    private static final int MIN_MONEY = 1;
+
+    private final int value;


클래스명과 겹치는 네이밍보다는 value가 좋네요 👍

summerlunaa · 2024-03-12T12:47:19Z

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

        List<Participant> participants = new ArrayList<>();

        participants.add(dealer);
        participants.addAll(players);

-        return participants;
+        return Collections.unmodifiableList(participants);


방어적 복사 👍

summerlunaa · 2024-03-12T12:48:32Z

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

        String message = String.format("%s는 한장의 카드를 더 받겠습니까?(예는 y, 아니오는 n)", name);
        String input = readLine(message);

-        if ("y".equals(input)) {
+        if (YES.equalsIgnoreCase(input)) {


YES.equalsIgnoreCase(input)와 input.equalsIgnoreCase(YES)의 차이를 아시나요 👀

input. 으로 하면 NPE가 발생할 수 있습니다!

summerlunaa · 2024-03-12T13:12:28Z

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

+
+public class Dealer extends Participant {
+
+    private static final String DEALER_NAME = "딜러";


다시 보니 반드시 딜러의 이름이 "딜러"여야 하는지 의문이 드네요. 콘솔뷰 한정으로 "딜러"라는 이름을 사용한다면 view의 영역으로도 볼 수 있겠네요~!

Dealer에서 Name은 기본 이름으로 쓸 수 있다고 생각해서 남겨놨습니다.
뷰에서는 딜러 이름을 처리했습니다.

summerlunaa · 2024-03-12T13:17:59Z

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

+        for (CardRank cardRank : CardRank.values()) {
+            for (CardShape cardShape : CardShape.values()) {
+                CARD_POOL.put(toKey(cardRank, cardShape), new Card(cardRank, cardShape));


indent(인덴트, 들여쓰기) depth를 2를 넘지 않도록 구현한다. 1까지만 허용한다.

예를 들어 while문 안에 if문이 있으면 들여쓰기는 2이다.

힌트: indent(인덴트, 들여쓰기) depth를 줄이는 좋은 방법은 함수(또는 메서드)를 분리하면 된다.

솔직히 이중 for문이 더 이해가 쉬운 것 같지만 수정했습니다!

static { for (CardRank cardRank : CardRank.values()) { generateRankCards(cardRank); } } private static void generateRankCards(CardRank cardRank) { for (CardShape cardShape : CardShape.values()) { CARD_POOL.put(toKey(cardRank, cardShape), new Card(cardRank, cardShape)); } }

(공감합니다 ㅋㅎㅎ)

summerlunaa · 2024-03-12T13:20:22Z

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

+        List<Card> cards = Arrays.stream(CardRank.values())
+                .map(Deck::createRankCards)
+                .flatMap(List::stream)
+                .collect(Collectors.toList());


리뷰 드리지도 않았는데 캐싱까지 👏 넘 좋네요. 캐싱한 카드들을 여기서도 사용할 수 있을 것 같은데 어떠신가요?

Card 클래스에서 캐싱된 전체 카드를 가져오게 한 후 Deck에서 사용하게 했습니다.

// Card.java public static List<Card> getCards() { return new ArrayList<>(CARD_POOL.values()); }

// Deck.java public static Deck createShuffledDeck() { List<Card> cards = Card.getCards(); Collections.shuffle(cards); return new Deck(cards); }

summerlunaa

구름 안녕하세요~

요구사항도 모두 충족하셨고 깔끔하게 잘 완성해주셨습니다 👏 이번 미션은 여기서 머지하겠습니다.

블랙잭 미션도 고생하셨습니다~ 👍 👍

alstn113 added 30 commits March 10, 2024 17:31

refactor: controller 이름 변경 및 패키지화

082a70f

fix: 플레이어 턴 재귀 에러 수정

a6ec8a1

refactor: 예외 발생 시 스택 트레이스 출력하지 않음

ab1d370

refactor: 게임 승패 규칙 변경

5bb7d8c

refactor: 딜러의 승패 판단 여부 개선

2b3534e

refactor: 덱 생성 분리

b47e473

refactor: 중복된 메서드 제거

f4c72e5

refactor: Hand 접근 제어자 변경

e33ccd2

refactor: 딜러의 보여지는 카드 부분 도메인 책임 이동

cd15abf

refactor: containsExactly를 통해 테스트 개선

a149cc7

refactor: 블랙잭 승무패 테스트 추가

5bb22af

refactor: Hand 생성 테스트 삭제

03b505a

refactor: 테스트 코드 개선

4d1c91d

fix: 블랙잭 승패 조건 수정

1c9c93b

refactor: 메서드 순서 변경

7459846

refactor: 블랙잭 상태 테스트 추가

ad1dc35

refactor: 딜러 보여지는 카드 테스트

e740ec0

docs: 도메인 용어 수정

2c09d22

docs: 2단계 블랙잭 배팅에 대한 요구 사항 작성

675b109

feat: Money 구현

6b357b0

feat: 베팅 정보를 저장 구현

cfd5d05

feat: 플레이어 게임 결과 구현

ad41d33

feat: 베팅 금액 입력 구현

c1e1739

refactor: 딜러가 플레이어의 승패 결과를 판단

9140b1c

feat: 최종 수익을 출력

edbb55b

refactor: 배팅 금액 범위 테스트 개선

a02b713

refactor: 플레이어와 딜러의 수익 계산

15fbd9d

refactor: 관심사별 패키지화

d5d7d18

fix: 덱 접근 제어자 변경

7125361

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

cd6b44a

alstn113 added 3 commits March 11, 2024 16:35

refactor: 딜러의 승패 판단을 분리

646b0f0

refactor: 테스트 패키지화

d2cdade

refactor: 코드 정리

aecab3a

summerlunaa requested changes Mar 11, 2024

View reviewed changes

alstn113 added 8 commits March 11, 2024 22:34

refactor: reduce -> mapToInt로 변경

514b190

feat: Name 클래스 동등성 비교 추가

de48d41

refactor: 출력 코드 개선

708c4e3

refactor: 동등성 비교 코드 개선

bf75a14

refactor: 테스트 가독성 개선

dcf5560

refactor: 컨트롤러 코드 개선

ac924ca

refactor: 원시값, 일급 컬렉션 value로 필드 메서드 수정

6da2d47

refactor: 딜러에서 승패 판단 분리

7d2eb04

alstn113 force-pushed the step2 branch from 0e458e1 to 7d2eb04 Compare March 11, 2024 15:19

alstn113 added 3 commits March 12, 2024 00:44

refactor: 승패 판단 로직 이동

cb9cb2a

refactor: 메서드 명 수정

d941fe9

refactor: hand를 불변 객체로 개선

9b8a0c8

alstn113 commented Mar 12, 2024

View reviewed changes

alstn113 added 3 commits March 12, 2024 14:00

refactor: 생성자 수정

b990235

refactor: 초기 카드 뽑기 코드 개선

8a89796

refactor: 카드 캐싱 기능 추가

8ca7443

alstn113 commented Mar 12, 2024

View reviewed changes

summerlunaa requested changes Mar 12, 2024

View reviewed changes

alstn113 added 3 commits March 12, 2024 22:33

refactor: 캐싱 코드 개선

4e02f04

refactor: 뷰에서 딜러 이름 처리 수정

6ccd452

fix: 딜러 출력 수정

8a515e2

summerlunaa approved these changes Mar 13, 2024

View reviewed changes

summerlunaa merged commit 14cb67f into woowacourse:alstn113 Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 블랙잭 베팅] 구름(김민수) 미션 제출합니다. #675

[2단계 - 블랙잭 베팅] 구름(김민수) 미션 제출합니다. #675

alstn113 commented Mar 11, 2024 •

edited

Loading

summerlunaa left a comment

summerlunaa Mar 11, 2024

summerlunaa Mar 11, 2024

alstn113 Mar 11, 2024

summerlunaa Mar 11, 2024

summerlunaa Mar 11, 2024

summerlunaa Mar 11, 2024

summerlunaa Mar 11, 2024

summerlunaa Mar 11, 2024

summerlunaa Mar 11, 2024

summerlunaa Mar 11, 2024

alstn113 Mar 11, 2024

summerlunaa Mar 11, 2024

alstn113 Mar 11, 2024

summerlunaa Mar 12, 2024

alstn113 Mar 12, 2024

summerlunaa Mar 11, 2024

alstn113 Mar 12, 2024

alstn113 Mar 12, 2024

summerlunaa left a comment

summerlunaa Mar 12, 2024

summerlunaa Mar 12, 2024

summerlunaa Mar 12, 2024

alstn113 Mar 12, 2024

summerlunaa Mar 12, 2024

alstn113 Mar 12, 2024

summerlunaa Mar 12, 2024

alstn113 Mar 12, 2024

summerlunaa Mar 12, 2024

summerlunaa Mar 12, 2024

alstn113 Mar 12, 2024

summerlunaa left a comment


		private static final int MIN_MONEY = 1;

		private final int value;


		public class Dealer extends Participant {

		private static final String DEALER_NAME = "딜러";

[2단계 - 블랙잭 베팅] 구름(김민수) 미션 제출합니다. #675

[2단계 - 블랙잭 베팅] 구름(김민수) 미션 제출합니다. #675

Conversation

alstn113 commented Mar 11, 2024 • edited Loading

⭐️ 딜러와 플레이어가 추상클래스(참여자)를 상속받아서 만든 이유

⭐️ CardShape이나 CardRank의 view 정보를 분리해야할까?

⭐️ Controller의 게임 로직들을 분리해야할까?

⭐️ 승패 판단을 어디서 해야할 지 모르겠다.

그 외

summerlunaa 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

summerlunaa 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

summerlunaa left a comment

Choose a reason for hiding this comment

alstn113 commented Mar 11, 2024 •

edited

Loading