[2단계 - 블랙잭(베팅)] 허브(방대의) 미션 제출합니다. #537

greeng00se · 2023-03-11T14:20:10Z

안녕하세요 검프🍫! 잘 지내셨나요?

검프와 같이 협업 한다고 생각하고, 납득이 가는 방향이라면 남겨주신 코멘트를 최대한 반영하려고 노력했습니다.

변경점은 다음과 같습니다!!

베팅 기능 추가
메시지 메서드 구현에 그대로 노출되도록 수정
enum의 상태를 확인하는 메서드 제거
Controller가 BlackjackGame을 필드로 가지고 있도록 수정
카드 리스트에 대한 Fixture 추가
- CardsTest의 경우에는 Input을 자세하게 다루고 싶어서 사용하지 않았습니다.

부족하거나, 더 좋은 방법이 있다면 코멘트 남겨주시면 바로 반영하겠습니다!

greeng00se · 2023-03-11T15:15:19Z

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

+
+    private static void validateBetUnit(final int value) {
+        if (isInvalidUnit(value)) {
+            throw new IllegalArgumentException("베팅은 100 단위로 할 수 있습니다.");


"베팅은 " + BETTING_UNIT + " 단위로 할 수 있습니다."

상수 사용 하겠습니다! 😢

Livenow14

안녕하세요 허브 :) 구현 잘해주셨어요.
전반적인 수정이 필요한 피드백을 남겼는데요. 학습하는 의미로 한번 적용해보셨으면 좋겠어요 :)

진행하시다가 어렵거나 질문사항이 있으면, 언제든 DM 및 커멘트 달아주세요.
그럼 수고 많으셨습니다.

Livenow14 · 2023-03-12T03:38:08Z

README.md

        Hand -- 사용자의 패 --> Cards
        Hand -- 핸드의 상태 --> State
    end
-
+    
    Players --> 플레이어 --> Hand
-
+    
    subgraph 플레이어
        direction BT
        Gambler -.-> Player


인텔리제이에서 사용하는 의존성 다이어 그램을 활용해도 좋겠네요 :)

유료버전에서 제공하는 다이어그램도 좋은 것 같습니다!
IntelliJ에서 markdown mermaid 설정 체크만 해준다면 IntelliJ와 깃허브에서도 둘다 확인이 가능하기 때문에 mermaid도 괜찮다고 생각됩니다 👍

Livenow14 · 2023-03-12T03:39:19Z

src/main/java/blackjack/BlackjackApplication.java


-public class BlackjackApplication {
+public final class BlackjackApplication {


정적 메서드만 가지는 객체에서, final을 사용하는 이유가 있을까요?

네오의 수업을 보고 적용해보았는데요~ 상속을 할 수 없다고 명시적으로 작성하는 것이 좋다고 생각되었습니다!

클래스 final의 경우, 상속으로 인한 사이드 이펙트가 생길 수 있는 클래스 혹은, 추가 확장이 없음이 확실할 때 걸어주시는 게 좋다고 생각하는데요.

intellij 에서 class 생성시 기본적으로 public class로 생성되기 때문에, 협업시 혼란의 여지가 있을 수 있습니다.
(final이 안걸려있으면, 확장을 막 해도 되는건가? final의 기준이 뭘까?)

모든 클래스에 걸어주는것 vs 필요시에 걸어주는 것의 부분에서 고민해보시길 바래요 :)

이부분은 허브 말씀처럼, 선택에 맡길게요~

Livenow14 · 2023-03-12T03:42:16Z

src/main/java/blackjack/controller/BlackjackCommand.java

    private boolean isSameCommand(final String command) {
        return this.command.equalsIgnoreCase(command);
    }


대/소문자를 구분하지 않는 구현의 추상화가 잘되어있네요 :)

Livenow14 · 2023-03-12T03:44:14Z

src/main/java/blackjack/controller/Retry.java

@@ -1,9 +1,8 @@
 package blackjack.controller;

-public class Retry {
+public final class Retry {

    private static final int DEFAULT_COUNT = 5;


DEFAULT_COUNT를 사용하지 않고,
applicatoin 구동시 넣어줘도 좋을 것 같네요.

더 깔끔한 것 같습니다! 👍 👍 👍

Livenow14 · 2023-03-12T03:50:48Z

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

+    private InputView() {
+        this.scanner = new Scanner(System.in);
+    }
+
+    public static InputView getInstance() {
+        return INSTANCE;
    }


[input | ouput]View가 싱글턴 패턴으로 구현되어야 하는 이유가 있을까요?!

싱글턴 패턴을 사용하신 이유를 알 수 있을까요?

static으로 사용할까, 싱글턴으로 사용할까 고민하다가 싱글턴이 더 괜찮을 것 같다는 생각이 들었습니다.
지금보니 특별히 싱글턴을 도입할 이유가 없는 것 같네요! 생성자를 사용하는 것이 명확할 것 같습니다.

Livenow14 · 2023-03-12T04:10:08Z

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

    public Result play(final Hand other) {
-        if (state.isBlackjack()) {
+        if (state == BLACKJACK) {
            return playWithBlackjack(other);
        }
-        if (state.isBust()) {
+        if (state == BUST) {
            return Result.LOSE;
        }
        return playWithScore(other);
    }


if 문도, 줄일 수 있을 것 같구요 :)

Livenow14 · 2023-03-12T04:11:54Z

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

    public boolean isAce() {
        return this == ACE;
    }


Ace를 가져와서, Cards에서 10을 더해주고 있네요, enum에서

예외로 Ace는 1 또는 11로 계산할 수 있으며,

의 요구사항을 표현할 수 없을까요?

음.. 카드의 상태에 따라 10을 더해 11로 계산할 수 있다고 생각해서 Cards에 있는게 더 좋은 것 같다는 생각이 듭니다!

Livenow14 · 2023-03-12T04:16:09Z

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

+    public Bets() {
+        bets = new LinkedHashMap<>();
+    }


LinkedHashMap 이 필요한 이유가 있을까요?

bets의 순서에 따라 로직이 달라지나요?!

단순히 UI에 순서대로 보여주기 위함이라면, UI를 위한 로직이 Bets에 포함된건 아닐까요?
Dealer와 Gambler를 통해, 출력시, 딜러 -> 플레이어 등으로 표현해볼 수 잇을 것 같아서요.

UI 로직이 반영된 부분 맞습니다!
수정하겠습니다 👍

Livenow14 · 2023-03-12T04:17:03Z

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

+public final class Name {
+    private static final int LENGTH_LOWER_BOUND = 1;
+    private static final int LENGTH_UPPER_BOUND = 5;
+    private static final String RESERVED_DEALER_NAME = "딜러";


RESERVED_DEALER_NAME -> DEALER

깔끔한 것 같습니다 👍 바로 수정하겠습니다!

Livenow14 · 2023-03-12T04:18:03Z

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

+                Arguments.of(CardFixtures.BLACKJACK, BLACKJACK),
+                Arguments.of(CardFixtures.BUST, BUST),


CardsFixutre를 만들어도 좋겠네요 :)

클래스이름은 단수로 설정해주세요 :)

이 부분을 놓쳤네요 😢 명확하게 분리하는 것 너무 좋은 것 같습니다 👍

greeng00se · 2023-03-12T13:27:29Z

검프🍫 안녕하세요!

객체의 다형성을 이용해보았습니다!
cards에 blackjack이나 bust를 확인하는 메서드를 제거하고 싶었는데 state에서 전부 오버라이딩하거나, 제거를 하게된다면 중복된 부분이 많아져서 제거하지 못했습니다. 😢

더 좋은 방법이 있을까요?!

Livenow14

안녕하세요 허브 :) 다형성을 사용해서 객체의 상태들을 잘 관리해 주셨어요.

몇가지 피드백 남겼습니다. 이것까지 반영해보면 좋을 것 같아서, 한번더 변경 요청보낼께요 :)

질문의 대한 답입니다.

cards에 blackjack이나 bust를 확인하는 메서드를 제거하고 싶었는데 state에서 전부 오버라이딩하거나, 제거를 하게된다면 중복된 부분이 많아져서 제거하지 못했습니다. 😢

저는 지금 작성해주신대로 구현되도 나쁘지 않다고 생각이 들어요.
각 상태별로 WIN, LOSE 판별 규칙이 다르니까요.
또한 Cards에서 blackJack이냐 Bust인지 판단하는 책임은 Cards가 가져도 충분히 좋을 책임이라 생각해서요 :)

수고많으셨어요.

Livenow14 · 2023-03-13T02:37:29Z

README.md

+```mermaid
+---
+title: Hand 상태
+---
+flowchart BT
+    AbstractHand -.-> Hand
+    Running --> AbstractHand
+    Finished --> AbstractHand
+    Hit --> Running
+    Blackjack --> Finished
+    Bust --> Finished
+    Stay --> Finished
+
+```
+


uml 다이어 그램 형식을 따르게 작성해도 좋겟네요 :)

간단하게 작성해보겠습니다!

Livenow14 · 2023-03-13T02:43:12Z

src/main/java/blackjack/controller/BlackjackCommand.java

@@ -18,14 +16,12 @@ public static BlackjackCommand from(final String command) {
        return Arrays.stream(values())
                .filter(value -> value.isSameCommand(command))
                .findAny()
-                .orElseThrow(() -> new IllegalArgumentException(INVALID_COMMAND_MESSAGE + command));
+                .orElseThrow(() -> new IllegalArgumentException(
+                        HIT.command + " 또는 " + STAY.command + "을 입력해야 합니다. 입력값:" + command)


var collect = Arrays.stream(values()).map(it -> it.command).collect(Collectors.joining(" 또는 "));

확장성을 고려한다면 위와 같이 상수 반복을 추가해도 괜찮겠네요 :)

오.. 좋은 것 같습니다! 👍 메시지까지 꼼꼼하게 확장을 고려할 수 있겠군요!!

Livenow14 · 2023-03-13T02:45:11Z

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

+    private static final InputView inputView = new InputView();
+    private static final OutputView outputView = new OutputView();


생성자 인자로 받아와도 좋을 것 같네요 :)
IntputView Outputview가 도메인과 밀접한 것 같아서요.

좋습니다! 하지만 필드가 많아지는 것 같아 Retry를 run의 인자로 받아와 3개로 줄여보겠습니다!

Livenow14 · 2023-03-13T02:48:56Z

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

+                retry = retry.decrease();
+            }
+        }
+        throw new IllegalArgumentException(this.retry.getFailMessage());


Retry내에 상수로 관리되도 좋겠네요 :)

예외 자체를 상수로 가지고 있는것으로 이해했습니다! 적용해보겠습니다!

Livenow14 · 2023-03-13T02:51:18Z

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

+    private void addPlayers() {
+        Retry retry = this.retry;
+        while (retry.isRepeatable()) {


Retry retry 를 파라미터로 받으면,

Retry retry = this.retry;

를 줄일 수 있겠네요

왜 이런 생각을 못했을까요 😢

Livenow14 · 2023-03-13T02:52:56Z

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

+        for (final Name player : blackjackGame.getGamblerNames()) {
+            addBet(player);
+        }


blackjackGame.getGamblerNames() .forEach(this::addBet);

로도 사용할 수 있을 것 같은데,
과연 메서드로 분리해야 했을까요?!

addPlayers(retry); addBets(retry); initialDraw(); draw(retry); play();

run 메서드가 위와 같이 추상화 되어있기 때문에 분리하는 것도 괜찮을 것 같다는 생각이 듭니다!

Livenow14 · 2023-03-13T02:53:36Z

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

+public abstract class AbstractHand implements Hand {
+    private final Cards cards;
+
+    AbstractHand(final Cards cards) {


해당 생성자는 default 접근 지정자를 가지는 걸까요?

추상클래스와 구현클래스의 생성자의 접근지정자는 어떻게 가져가는게 좋을까요?

protected가 더 적절할 것 같습니다. 다른 패키지에서도 상속받아서 사용할 수도 있으니까요!

Livenow14 · 2023-03-13T03:02:26Z

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

+import java.util.Map;
+
+public final class Bets {
+    private final Map<Name, Money> bets;


map의 key를Name이 아닌, Player등으로 설정해도 어색하지 않겠네요 :)

반영했습니다! 테스트가 조금 더 복잡해져서 Name으로 변경을 했었는데, 의미상으로는 Player가 Key로 있는데 더 깔끔한 것 같습니다!

Livenow14 · 2023-03-13T03:05:20Z

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

+        draw();
+        play();


draw하는 과정은 play 과정이 아닐까요?!

play에 draw를 넣었을 때 initialDraw도 넣어야할 것 같고, 둘 다 넣으면 베팅하는 부분도 넣어야할 것 같은 느낌이 들어서 지금도 괜찮을 것 같다는 생각이 드는데 검프는 어떻게 생각하시나요? 🤔

베팅, initialDraw 등을 initialize 로 빼고, play에 draw를 포함하는 방법도 있겠네요 :)

오 좋은 방법인 것 같습니다! 2개로 나눠지겠군요 😄

greeng00se · 2023-03-13T06:13:32Z

피드백 남겨주신 부분 반영해보았습니다!
검프랑 페어프로그래밍 하는 느낌이어서 너무 재밌었습니다! 😄
더 반영하거나 추가해야될 부분이 있을까요?

항상 꼼꼼하게 코멘트 달아주셔서 감사합니다 검프!

Livenow14

안녕하세요 허브 ! 구현 잘해주셨어요 :)
꽤나 복잡한 리뷰였을 텐데도, 잘 적용해주셨어요...!

여기까지 진행해도 다음레벨을 진행하는데 무리 없을거라 판단하여, 이번 미션은 여기서 머지하겠습니다
수고많으셨어요 :)

간단한 리뷰를 남긴다면

다음 레벨의 미션에는 학습의 취지로 테스트에 given when then 패턴을 한번 적용해보시면 좋을 것 같아요 :)
테스트의 "일관성"을 지킬 수 있거든요.
물론 테스트가 요구사항을 잘 검증하는지가 1순위이긴 하지만요.

Livenow14 · 2023-03-13T15:51:09Z

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

-        throw new IllegalArgumentException(retry.getFailMessage());
+        throw retry.getException();


헉 retry.getFailMessage()를 상수화 하자는 의미였어요..!
명확한 피드백을 못드렸었네요 ㅠㅠㅠ 요건 다음 레벨에서 적용해주세요 :)

메시지 자체를 상수화하는 거였군요! 😄

블랙잭 미션을 진행하면서 다양한 방법을 제시해주셔서 재밌었습니다!
항상 꼼꼼하게 리뷰 남겨주셔서 감사합니다!

greeng00se added 30 commits March 8, 2023 15:21

docs: 요구사항 정리

ccc3548

feat: 결과에 블랙잭 승리 추가

60d25ad

feat: 금액을 표현하는 클래스 추가

8cea82b

feat: 결과 계산에 필요한 Money 객체와 메서드 추가

ca753db

feat: 베팅에 대한 클래스 추가

861c0b4

refactor: 블랙잭으로 승리하는 경우 결과 값 변경

176d856

feat: 플레이어 베팅 기능 추가

247f8a1

refactor: Name 클래스 리팩터링

69973f1

refactor: 플레이어 추상 클래스 및 구현체 리팩터링

5422c7e

refactor: Players 플레이어를 추가할 수 있게 수정

3b33ae6

refactor: 블랙잭 게임에서 플레이어를 추가할 수 있게 수정

057e2bc

refactor: InputView 상수 제거

a0fc83b

refactor: State 확인하는 메서드 제거

d359e1b

refactor: 점수 관련된 메서드명 간소화

b32f0b6

refactor: 카드 Letter -> Symbol로 변경

3d2d047

refactor: 클래스에 final 적용

5035a6e

refactor: Bets 기본 생성자 추가

13a327f

refactor: BlackjackGame 필드로 변경

b023498

docs: 요구사항 정리

9a48ad4

refactor: InputView, OutputView singleton으로 변경

0789e0b

feat: 겜블러의 Name을 반환하는 메서드 추가

3159966

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

a0c108f

refactor: bets의 Key를 Name으로 사용하도록 수정

519581f

refactor: blackjackCommand 클래스 필요없는 메서드 제거

a9db97b

refactor: draw 가능한지 확인하는 기능을 블랙잭 내부로 이동

37f27ff

refactor: 베팅시 Money를 도메인 내부에서 생성하도록 수정

37f8538

test: 제네릭 미션 테스트명 수정

837392b

test: 카드 여러장에 대한 Fixture 추가

e427b50

refactor: 100 단위로 베팅할 수 있게 수정

0a56284

refactor: AbstractPlayer 필드 private으로 변경

1d1742e

greeng00se commented Mar 11, 2023

View reviewed changes

Livenow14 requested changes Mar 12, 2023

View reviewed changes

greeng00se added 15 commits March 12, 2023 14:57

refactor: 의미없는 싱글턴 제거

10da7b7

refactor: 애플리케이션에서 Retry 횟수 입력받도록 수정

e4b0bbc

refactor: 상수 사용하도록 개선

9b94ee9

refactor: 클래스명 단수로 사용

e72af7e

refactor: LinkedHashMap -> HashMap으로 변경

ad75aa7

refactor: 딜러 이름에 관한 상수명 변경

651c645

test: Card, Cards Fixture 분리

afd1fdd

refactor: Player 인터페이스 제거

43d3c18

refactor: auto arrange 된 부분 수정

433adea

refactor: 기존에 작성된 메서드 사용하도록 수정

846bc8b

refactor: 상태 패턴 적용

024ffbf

refactor: 카드를 뽑을 수 있는지 확인하는 메서드명 변경

48e24e0

refactor: Result enum 패키지 변경

39c14f8

refactor: retry 불변으로 변경

371de1b

refactor: 재입력 횟수 초과시 메시지 출력 후 게임 종료

b1175c9

docs: 다이어그램 추가

506f696

Livenow14 requested changes Mar 13, 2023

View reviewed changes

greeng00se added 6 commits March 13, 2023 12:40

refactor: ResultTest 패키지 변경

6b2fce6

docs: Hand관련 다이어그램 수정

cef4521

refactor: Command 확장성 고려해서 상수 반복 추가

30d25f2

refactor: InputView, OutputView 생성자로 받도록 수정

2e87df7

refactor: 상태 관련 클래스 생성자 protected로 변경

b2be325

refactor: Bets의 key를 Player로 변경

80e27df

refactor: players 중복이름 확인하는 메서드 수정

f2ead9c

Livenow14 approved these changes Mar 13, 2023

View reviewed changes

Livenow14 merged commit 2949f85 into woowacourse:greeng00se Mar 13, 2023


		public class BlackjackApplication {
		public final class BlackjackApplication {

		Arguments.of(CardFixtures.BLACKJACK, BLACKJACK),
		Arguments.of(CardFixtures.BUST, BUST),

		private static final InputView inputView = new InputView();
		private static final OutputView outputView = new OutputView();

		throw new IllegalArgumentException(retry.getFailMessage());
		throw retry.getException();

[2단계 - 블랙잭(베팅)] 허브(방대의) 미션 제출합니다. #537

[2단계 - 블랙잭(베팅)] 허브(방대의) 미션 제출합니다. #537

Conversation

greeng00se commented Mar 11, 2023

Choose a reason for hiding this comment

Livenow14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greeng00se Mar 12, 2023 • 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

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

greeng00se Mar 12, 2023 • edited Loading

Choose a reason for hiding this comment

greeng00se commented Mar 12, 2023

Livenow14 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

greeng00se Mar 13, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greeng00se commented Mar 13, 2023

Livenow14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greeng00se Mar 12, 2023 •

edited

Loading

greeng00se Mar 12, 2023 •

edited

Loading

greeng00se Mar 13, 2023 •

edited

Loading