[유안] 블랙잭 1주차 미션 제출합니다 #39

KimSeongGyu1 · 2020-03-12T08:51:01Z

No description provided.

ResultMaker 는 게임의 결과를 얻는 역할을 맡는다. BlackJackPlayer 는 게임에 참여하는 역할을 맡는다. CardPossessor 는 카드를 소유하는 역할을 맡는다. CardProvider 는 카드를 제공하는 역할을 맡는다. ScoreCalculator 는 점수를 계산해준다.

Card 객체는 캐싱을 위해 정적 팩토리 메서드를 사용했다.

카드를 제공하는 기능 구현

카드 뽑기 기능 구현

- Card 객체의 number(int) 프로퍼티 -> Rank(enum) 변경 Ace, King, Queen, Jack 등을 적절히 표현하기 위해 - Card Symbol -> Suit 이름 변경 보편적인 이름 컨벤션 맞추기 위해 - Ace를 1로 계산하는 메서드 추가 - Ace를 소유하는지 판단하는 메서드 추가 - Ace를 11로 계산 가능하면 업데이트하는 메서드 추가

모든 게이머들의 집합 객체가 딜러와 유저를 분리해서 소유하도록 수정 좀 더 편한 연산을 위해 수정 WinLose enum 생성 BlackJackResult 추상 클래스 생성 UserResult 생성 DealerResult 생성

- 패키지 구조 변경 - 추상 골격화 도입 - 이름 변경

phs1116

안녕하세요, 유안.
리뷰어 화투입니다!
추상화에 대해서 정말 많은 고민을 한 것이 보여요. 👍
몇가지 피드백 추가했는데 참고 부탁드릴게요 :)

phs1116 · 2020-03-15T07:36:32Z

src/main/java/BlackJackApplication.java

+        askPlayersDrawMore(allGamers.getPlayers(), cardDeck);
+        OutputView.printEmptyLine();
+
+        determineDealerMoreDraw(allGamers.getDealer(), cardDeck);
+        OutputView.printEmptyLine();


AllGamers라는 객체를 기왕 작성하신 거, getter를 통해 가져와서 처리하지 말고,
allGamers내에서 처리하고 결과를 출력하는 방향으로 가보면 어떨까요?
현재 구조면 사실 굳이 allGamer가 필요 없을수도 있겠다 싶기도 하네요. :)

저도 현재 구조에서는 굳이 필요 없을 수 있다는 생각을 계속 하고 있었어요.
AllGamers를 삭제했어요.

2주차에서는 AllGamers 같은 객체를 활용할 수 있는 구조로 어떻게 변경할 수 있을까 고민중입니다.

넵. 다형성을 활용해서 하나의 인터페이스로 로직을 수행하는 것도 좋을 것 같아요. :)

phs1116 · 2020-03-15T07:40:01Z

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

+                .filter(card -> card.rank == rank)
+                .filter(card -> card.suit == suit)


두개의 조건을 합친 메서드를 제공할 수도 있지 않았을까요??? :)

말씀하신 대로 하는 것이 더 좋아보여 메서드를 추가하여 사용했습니다.

phs1116 · 2020-03-15T07:48:18Z

src/main/java/domain/card/Rank.java

+        if (this == Rank.K || this == Rank.Q || this == Rank.J) {
+            return ALPHABET_SCORE;
+        }


enum 값 자체에서 SCORE를 들고있고 그 값만 돌려주면 되지 않을까요?

필드를 줄일 수 있으면 줄이려다 보니
명시성도 줄어들고 필요없는 로직이 늘어난 형태가 된 것 같습니다.

또한 SCORE이라는 값 객체를 나중에 만들다 보니 이 부분을 수정하는 것을 놓친 것 같습니다.

말씀하신대로 수정했습니다.

phs1116 · 2020-03-15T07:49:46Z

src/main/java/domain/card/providable/CardDeck.java

+            throw new IllegalStateException("더 이상 카드를 뽑을 수 없습니다.");
+        }
+
+        return cards.remove(FIRST);


stack을 썼어도 괜찮았을 것 같아요. :)
ArrayList에서 첫번째 값을 지웠을 때의 시간복잡도고 고려해본다면요!

자료구조에 따른 시간복잡도에 대해 찾아보았습니다.
스택 자료구조는 써본 적이 없어서 만약 추천해주시지 않았다면
시간복잡도문제를 해결하기위해

ArrayList에서 remove(FIRST) 대신 remove(LAST)을 사용한다.

LinkedList를 사용한다.
를 고려해봤을 것 같습니다.

위 두가지 방법 말고 스택을 추천해주신 이유가 궁금합니다.
혹시 스택을 사용했을 때 시간 복잡도 말고 다른 장점도 있을까요?

넵 말씀하신게 일단 가장 큰 장점이에요. :) 굳이 덧붙이자면 2,3번째 이유도 있을거에요.

시간복잡도가 더 빠르다. (LinkedList, LinkedList로 구현한 Stack과 같음.)

ArrayList처럼 buffer를 가지고 있지 않기 때문에 비해 메모리 사용량이 적다. (LinkedList, LinkedList로 구현한 Stack과 같음.)

remove(0), remove(last)가 아닌 현재 요구상에 맞는 인터페이스를 제공한다. (pop)

phs1116 · 2020-03-15T07:52:40Z

src/main/java/domain/gamer/Gamer.java

+
+    @Override
+    public WinLose determineWinLose(BlackJackGameable counterParts) {
+        if (this.calculateScore().isLargerThan(BLACK_JACK_SCORE)) {


calculateScore()를 통해 매번 계산하지 말고 값을 들고있는게 더 좋을 것 같아요. :)

상태값이 최신화가 되어있다는 보장이 필요하다고 생각했어요.
그러기 위해서는 아래 두가지 방법이 생각났는데,

상태값을 물어볼때마다 계산한다. (상태 값을 들고 있지 않아도 된다.)

상태값이 바뀔 때마다 계산한다. (상태 값을 들고 있어야 한다.)

두가지 방법 중에서 연산 양이 어느쪽이 많을지 예상하기는 쉽지 않다고 생각했어요.
그래서 상태 값을 들고 있지 않아도 되는 1번을 계속 사용하고 싶어요.

제가 고려하지 못한 부분이 있다면 추가적으로 알려주셨으면 좋겠어요.

값의 최신화가 보장이 되야 한다는 것 자체가 해당 객체가 여러 쓰레드에서 접근 가능한 공유 자원이라는 뜻이네요. :)
현재 단계에서는 고민하지 않아도 될 것 같아요!
말씀하신 이슈가 있다면 두번째 if문 호출 시에 this.calculateScore().isLargerThan(BLACK_JACK_SCORE)가 true가 되더라도 이미 넘어가는 등 잘못 동작이 될 케이스가 많아서
이 경우는 동기화 이슈를 어떻게 풀어가야할 지 다른 방법으로 고민해야할 듯 해요. :)
애초에 공유자원으로 활용이 안되는게 가장 좋구요!

리펙토링 과정에서
명시성을 높이기 위해
isBurst()를 만든뒤 활용하는 방향으로 수정해보았습니다.

phs1116 · 2020-03-15T07:53:57Z

src/main/java/domain/gamer/Gamer.java

+        if (counterParts.calculateScore().isLargerThan(BLACK_JACK_SCORE)) {
+            return WinLose.WIN;
+        }
+
+        if (this.calculateScore().isLargerThan(counterParts.calculateScore())) {
+            return WinLose.WIN;
+        }


두 점수를 기반으로 승패를 판단하는 것 자체도 WinLose 내부에서 책임져보면 어떨까요?

두 점수를 기반으로 승패를 판단하는 로직을 추가하려다가
두 게이머를 기반으로 승자를 판단하는 로직을 추가하는 방향으로 진행했어요.

이런 방향에 대해 어떻게 생각하시나요?

phs1116 · 2020-03-15T07:57:24Z

src/main/java/domain/score/BlackJackScoreManager.java

+public class BlackJackScoreManager implements ScoreManagable {
+    private BlackJackScoreManager() {
+    }
+
+    public static Calculatable calculate(HandCards handCards) {
+        Calculatable defaultSum = handCards.calculateDefaultSum();
+
+        if (handCards.hasAce()) {
+            return updateAceScore(defaultSum);
+        }
+
+        return defaultSum;
+    }
+
+    private static Calculatable updateAceScore(Calculatable score) {
+        Calculatable bigAceScore = score.plus(ACE_ADDITIONAL_SCORE);
+        if (bigAceScore.isLargerThan(BLACK_JACK_SCORE)) {
+            return score;
+        }
+
+        return score.plus(ACE_ADDITIONAL_SCORE);
+    }


static method로 제공한다면 인터페이스로 추상화한 의미가 있을까요? :)
다형성과 인터페이스를 통한 확장 가능한 구조가 인터페이스의 장점인데, 이러한 장점을 전혀 활용할 수 없는 구조로 보여요!

static과 interface에 대해 이해도가 부족했던 것 같아요.
점수를 계산하는 역할에 대한 구조를 변경하고 static을 제거하는 방향으로 수정했어요.
카드들를 가지고 있는 객체가 자신의 카드들의 점수를 계산할 수 있는 형태로 수정했습니다.

phs1116 · 2020-03-15T08:02:19Z

src/main/java/domain/card/Rank.java

+        if (this == Rank.ACE || this == Rank.J || this == Rank.Q || this == Rank.K) {
+            return this.name();
+        }
+
+        return Integer.toString(this.value);


두가지 로직을 분리하기보단 같이 쓸수있는 필드를 두는게 더 좋을 것 같아요!

넵 저도 그렇게 하는 것이 더 좋아보여서 수정했습니다.

phs1116 · 2020-03-15T08:03:35Z

src/main/java/domain/result/AllBlackJackResults.java

+    public List<PlayerResult> extractPlayerResults() {
+        List<PlayerResult> playerResults = blackJackResults.stream()
+                .filter(result -> result instanceof PlayerResult)
+                .map(result -> (PlayerResult) result)
+                .collect(Collectors.toList());
+
+        return Collections.unmodifiableList(playerResults);
+    }
+
+    public DealerResult extractDealerResult() {
+        return blackJackResults.stream()
+                .filter(result -> result instanceof DealerResult)
+                .map(result -> (DealerResult) result)
+                .findAny()
+                .orElseThrow(() -> new IllegalStateException("딜러의 결과가 존재하지 않습니다."));
+    }


어차피 이렇게 뽑아쓸거면 굳이 BlackJackResult로 추상화 할 필요 없이,
두개의 구현체로 들고있는게 더 효율적이지 않을까요?
사용하는 쪽에서도 제네릭으로 설정한 타입 때문에 어차피 정확한 구현체를 알아가야 할 것 같고,
BlackJackResult를 봐도 추상화를 할 수 있는 부분이 전혀 없어 보이네요. :)

넵 말씀하신 부분이 맞다고 생각해서 수정했습니다.

phs1116 · 2020-03-15T08:07:34Z

src/main/java/domain/score/Score.java

+
+import java.util.Objects;
+
+public class Score implements Calculatable {


다른 곳에서 Calculatable를 인터페이스로 받는다는 것은
점수가 아닌 다른 값이 들어올 수 있다는걸 고려했다는 거에요.
블랙잭의 특성 상 그런 케이스는 없을 것 같은데 추상화에 대해 고민하시느라 조금 어렵게 생각하신 것 같아요. :)

넵 Calculatable을 사용하는 것은 약간 이상하다고 느꼈습니다.

그런데 Score가 Comparable을 상속하는 것은 어떻게 생각하시나요?
compareTo()를 Integer.compareTo()를 이용해서 구현하고 잘 활용할 수도 있다고 생각이 들었습니다.

네. 기능상, 비교에 대한 부분이 필요하다면 Comparable를 사용하는건 좋아요. :)
그리고 앞선 피드백에서 Calculatable라는 인터페이스는 Comparable처럼 특정 기능을 표현하기 위한 인터페이스로는 좋지만
비즈니스 도메인의 주된 인터페이스로 풀기에는 너무 느슨한 구조여서 말씀드린거였어요. :)

Hand 객체가 CardDrawable, ScoreCalculable의 두가지 역할을 맡도록 수정 AllGamers 객체 삭제 후 어플리케이션 변경 Result 관련 쓸모없는 추상화 삭제

phs1116

안녕하세요, 유안!
깔끔하게 구현 잘해주셨어요. :)
피드백 반영도 매우 잘해주셨습니다. 👍
질문 주신 사항 및 몇가지 피드백 추가했는데 참고 부탁드릴게요!

phs1116 · 2020-03-16T12:17:28Z

src/main/java/domain/card/providable/CardDeck.java

+            throw new IllegalStateException("더 이상 카드를 뽑을 수 없습니다.");
+        }
+
+        return cards.remove(FIRST);


넵 말씀하신게 일단 가장 큰 장점이에요. :) 굳이 덧붙이자면 2,3번째 이유도 있을거에요.

시간복잡도가 더 빠르다. (LinkedList, LinkedList로 구현한 Stack과 같음.)

ArrayList처럼 buffer를 가지고 있지 않기 때문에 비해 메모리 사용량이 적다. (LinkedList, LinkedList로 구현한 Stack과 같음.)

remove(0), remove(last)가 아닌 현재 요구상에 맞는 인터페이스를 제공한다. (pop)

phs1116 · 2020-03-16T12:19:33Z

src/main/java/domain/score/Score.java

+
+import java.util.Objects;
+
+public class Score implements Calculatable {


네. 기능상, 비교에 대한 부분이 필요하다면 Comparable를 사용하는건 좋아요. :)
그리고 앞선 피드백에서 Calculatable라는 인터페이스는 Comparable처럼 특정 기능을 표현하기 위한 인터페이스로는 좋지만
비즈니스 도메인의 주된 인터페이스로 풀기에는 너무 느슨한 구조여서 말씀드린거였어요. :)

phs1116 · 2020-03-16T12:23:55Z

src/main/java/domain/gamer/Gamer.java

+
+    @Override
+    public WinLose determineWinLose(BlackJackGameable counterParts) {
+        if (this.calculateScore().isLargerThan(BLACK_JACK_SCORE)) {


값의 최신화가 보장이 되야 한다는 것 자체가 해당 객체가 여러 쓰레드에서 접근 가능한 공유 자원이라는 뜻이네요. :)
현재 단계에서는 고민하지 않아도 될 것 같아요!
말씀하신 이슈가 있다면 두번째 if문 호출 시에 this.calculateScore().isLargerThan(BLACK_JACK_SCORE)가 true가 되더라도 이미 넘어가는 등 잘못 동작이 될 케이스가 많아서
이 경우는 동기화 이슈를 어떻게 풀어가야할 지 다른 방법으로 고민해야할 듯 해요. :)
애초에 공유자원으로 활용이 안되는게 가장 좋구요!

phs1116 · 2020-03-16T12:27:06Z

src/main/java/BlackJackApplication.java

+        for (int i = 0; i < INITIAL_CARDS_AMOUNT; i++) {
+            gamer.drawCard(cardDeck);
+        }


두번 호출 하는 것이 비즈니스상 의미가 있다면 별도 메서드로 뺴도 괜찮을 것 같아요.
'아무 카드가 없을 때 2번 뽑는다'는 제약사항도 같이 표현할 수 있구요. :)

맞네요.
openInitialCards()라는 메서드도 있는데 drawInitialCards()가 없을 이유가 없어요.
제약사항을 표현할 수 있는 것은 생각하지 못했는데 짚어주셔서 감사합니다.

phs1116 · 2020-03-16T12:30:04Z

src/main/java/BlackJackApplication.java

+    private static List<WinLose> determinePlayerWinLoses(Dealer dealer, List<Player> players) {
+        return players.stream()
+                .map(player -> player.determineWinLose(dealer))
+                .collect(Collectors.toList());
+    }
+
+    private static Map<WinLose, Integer> determineDealerWinLoses(List<WinLose> winLoses) {
+        return winLoses.stream()
+                .collect(groupingBy(WinLose::reverse, summingInt(x -> 1)));
+    }
+}


결과를 계산해주고 저장하고있는 별도 클래스로 분리해보면 어떨까요?

네 저도 분리하는게 좋아보입니다.

phs1116 · 2020-03-16T12:33:55Z

src/main/java/BlackJackApplication.java

+        askPlayersDrawMore(allGamers.getPlayers(), cardDeck);
+        OutputView.printEmptyLine();
+
+        determineDealerMoreDraw(allGamers.getDealer(), cardDeck);
+        OutputView.printEmptyLine();


넵. 다형성을 활용해서 하나의 인터페이스로 로직을 수행하는 것도 좋을 것 같아요. :)

처음에 두장의 카드를 뽑는 것을 객체의 메서드로 분리 결과를 담는 객체 생성

phs1116

안녕하세요, 유안!
피드백 반영 매우 잘해주셨어요. 👍
이번 단계 머지할게요! 수고하셨어요. :)

KimSeongGyu1 and others added 23 commits March 10, 2020 18:08

코일 유안 블랙잭 시작

b0767bf

기능목록 작성 시작

86344a4

페어 실험용

76f82ff

[docs] 기능목록 작성

1bb1988

[feat] Card 객체 생성

bb2aafc

Card 객체는 캐싱을 위해 정적 팩토리 메서드를 사용했다.

[feat] 카드 덱 객체 구현

4324e1c

카드를 제공하는 기능 구현

[feat] HandCards 클래스 구현

26b6522

카드 뽑기 기능 구현

[feat] User, Dealer 객체 생성

49450b7

[feat] 모든 플레이어가 처음에 카드를 2장씩 뽑기 구현

047c1f7

[feat] 플레이어 카드 드로우 가능 여부 확인 기능 추가

74066cf

[feat] 딜러 카드 드로우 가능 여부 확인 기능 추가

46a68bf

[feat] 승패판단 기능추가

9a65f70

모든 게이머들의 집합 객체가 딜러와 유저를 분리해서 소유하도록 수정 좀 더 편한 연산을 위해 수정 WinLose enum 생성 BlackJackResult 추상 클래스 생성 UserResult 생성 DealerResult 생성

[refact] 도메인 리팩토링

e02441a

- 패키지 구조 변경 - 추상 골격화 도입 - 이름 변경

[refact] 점수 정보 포장 리펙토링

140370c

[feat] 게임 플레이어 이름 입력 기능

4efd73a

[feat] 게이머들 초기카드 출력 구현

f3766db

[feat] 플레이어 더 뽑을지 물어보기 구현

9b60f63

[feat] 딜러 더 뽑을지 구현

fdc8e18

[feat] 스코어 출력 기능 구현

c793ee3

[feat] 결과 출력 구현

a5c5ef4

[refact] 컨벤션 리팩토링

ec3bdb4

phs1116 suggested changes Mar 15, 2020

View reviewed changes

[refactor] 구조 수정 및 피드백 반영

e32c49e

Hand 객체가 CardDrawable, ScoreCalculable의 두가지 역할을 맡도록 수정 AllGamers 객체 삭제 후 어플리케이션 변경 Result 관련 쓸모없는 추상화 삭제

phs1116 suggested changes Mar 16, 2020

View reviewed changes

KimSeongGyu1 added 3 commits March 17, 2020 14:58

[refactor] 피드백 반영

3115c7a

처음에 두장의 카드를 뽑는 것을 객체의 메서드로 분리 결과를 담는 객체 생성

[refactor] 계산하는 역할에 버스트했는지 알아내는 책임 추가

414ebc3

[refactor] 버스트 했는지 알아내는 것 메서드화 시킨 후 활용

38ffc20

phs1116 approved these changes Mar 18, 2020

View reviewed changes

phs1116 merged commit a086a63 into woowacourse:kimseonggyu1 Mar 18, 2020

		.filter(card -> card.rank == rank)
		.filter(card -> card.suit == suit)


		import java.util.Objects;

		public class Score implements Calculatable {

[유안] 블랙잭 1주차 미션 제출합니다 #39

[유안] 블랙잭 1주차 미션 제출합니다 #39

Conversation

KimSeongGyu1 commented Mar 12, 2020

phs1116 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

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

phs1116 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

phs1116 left a comment

Choose a reason for hiding this comment