[2단계 - 블랙잭(베팅)] 후디(조동현) 미션 제출합니다. #369

devHudi · 2022-03-19T09:29:41Z

안녕하세요, 앨런! 이번 피드백도 잘 부탁드려요!
1단계 코멘트에 대한 답변과 추가 질문은 이전 PR에 남겨두었습니다! (#273)

이전 두 미션보다 이번 미션이 훨씬 오래 걸렸습니다. 도메인 설계를 몇번을 엎은지를 모르겠네요. 🥲
금요일 강의 때 상태 패턴을 사용한 설계에 대해서 배우게 되었는데, 적용해보고 싶었으나 일단 지금 해온 설계를 마무리 해보고싶어서 아쉽게도 상태 패턴을 적용해보지는 못했습니다.

인스턴스 멤버를 2개로 제한하는 요구사항덕분에 도메인 설계에서 참 오래 걸렸던 것 같아요.
추가적인 질문이나 이야기해보고 싶은 내용은 코멘트로 작성할게요 🙇 즐거운 주말 보내시길!

아쉬운 점

최대한 모든 도메인을 불변 객체로 만들고싶었으나, Players, ResultCount 등을 불변 객체로 만들면 더 좋았을텐데, 일정의 문제로 가변 객체로 둔 것이 아쉽습니다.

- HandDto 제거

…서 실행하도록 구조 개선 - BlackjackController의 public 메소드 이름 개선

- Hand.getScore 를 사용하는 코드를 Score.calculateSumFrom 으로 변경 - 테스트코드 수정

- isGreaterThan, isLessThan 과 같은 메소드를 없애고, isBusted, isOverDealerHitThreshold 와 같은 메소드를 별개로 구현함

…atchResult 로 위임 - Dto 였던 DealerMatchDto 를 DealerMatchResult 라는 도메인으로 변경 - Dto 였던 PlayerMatchDto 를 PlayerMatchResult 라는 도메인으로 변경

- 테스트 코드 작성 - Score 클래스가 카드 리스트를 전달받아 합계를 계산할 수 있도록 개선 - Hand 클래스가 첫 두장의 카드를 리스트로 반환하는 기능 구현

- 테스트코드 작성 - Money 도메인 구현 - BetAndProfit 도메인 구현

- Money 도메인에 add, subtract 메소드 구현

- BlackjackController 및 InputView, OutputView 기능 구현 - 승패결과를 나타내는 클래스 제거 - 기타 사용되지 않는 클래스 제거

- PlayerBetResult 에서 ProfitResult 로 이름 변경 - DealerProfitResult 제거

- Player 클래스에 betMoney 필드 추가 - Player 생성자 및 of 메소드에 betMoney 파라미터 추가 - ResultType 에 WIN_WITH_BLACKJACK 상수 추가 - ResultType 에 승부 결과 별 수익 계산 로직 추가

- Players 일급 컬렉션 클래스 구현 - Dealer 와 Players 를 가지고 있는 ParticipantGroup 클래스 구현 - BlackjackGame 이 Dealer 와 List<Player> 대신 ParticipantGroup 을 갖도록 개선 - ParticipantDto 를 추상클래스로 변경 - DealerDto 와 PlayerDto 를 ParticipantDto 를 상속하여 구현 - getCardNames 를 ParticipantDto 에서 Hand 로 이동 - ProfitResult 클래스 제거 - BetAndProfit 클래스 제거 - 변경된 도메인에 맞게 BlackjackController 와 OutputView 로직 수정

- 테스트 코드 수정 - Score 가 다른 Score 에 비해 큰지, 작은지를 검사하는 isGreaterThan, isLessThan 메소드를 구현함

devHudi · 2022-03-19T09:58:54Z

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

 public class Player extends Participant {

    private static final String INVALID_NAME_LENGTH_EXCEPTION_MESSAGE = "이름은 1글자 이상이어야합니다.";

-    private Player(final String name, final Hand hand) {
+    private final Money betMoney;
+
+    private Player(final String name, final Hand hand, final Money betMoney) {


Participant 와 Player 는 별개의 클래스이고, Participant 가 가지고 있는 두개의 필드 (playerName, hand)는 private 으로 Player 측에서 접근하지 못하므로 Player 의 인스턴스 변수는 하나라고 생각했습니다.

devHudi · 2022-03-19T10:02:23Z

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

+        static {
+            // TODO: 2 Depth 인가?...
+            cache = Stream.of(CardSymbol.values())
+                    .flatMap(symbol -> Stream.of(CardRank.values())
+                            .map(rank -> new Card(rank, symbol)))
+                    .collect(Collectors.toSet());
+        }


이 코드를 for 문으로 작성할 경우 깊이가 2가 되어버려, Stream 을 2중으로 사용했는데요, 결국 조삼모사가 아닌가 싶어요! 앨런의 의견이 궁금합니다!

넵, 이 부분은 2중으로 나눠도 괜찮은 것 같아요.

devHudi · 2022-03-19T10:03:30Z

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


+    public List<Card> getInitialCards() {
+        return cards.subList(0, INITIAL_CARDS_COUNT);
+    }
+


이번 미션의 경우 시작과 없음 등을 의미하는 0 이라는 숫자는 별도로 상수로 분리하지 않았습니다. 상수로 분리하지 않은 0 의 경우 해당 코드 자체가 길지 않고, 메소드명으로 충분이 0 이 어떤 역할을 하는지 유추할 수 있을 것 이라고 생각했는데요, 앨런의 의견이 궁금합니다!

hongbin-dev

안녕하세요 후디!
2단계 미션 고생하셨어요~ 구조가 많이 바뀌었군요 😮
몇 가지 코멘트 남겼으니 확인해주세요

hongbin-dev · 2022-03-19T16:22:29Z

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

+        static {
+            // TODO: 2 Depth 인가?...
+            cache = Stream.of(CardSymbol.values())
+                    .flatMap(symbol -> Stream.of(CardRank.values())
+                            .map(rank -> new Card(rank, symbol)))
+                    .collect(Collectors.toSet());
+        }


넵, 이 부분은 2중으로 나눠도 괜찮은 것 같아요.

hongbin-dev · 2022-03-19T16:28:17Z

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

+        return initialScore.equals(Score.valueOf(Score.MAXIMUM_SCORE));
+    }
+
+    protected ResultType compareWith(Participant other) {


Suggested change

protected ResultType compareWith(Participant other) {

protected ResultType compareWith(Dealer other) {

other에 Player가 들어가도 무관한가요?

딜러가 플레이어와 비교할 경우가 있을 것이라고 생각해서 이렇게 구현했는데, 생각해보니 Dealer 로 타입을 제한해도 될 것 같아요. 수정했습니다 :)

hongbin-dev · 2022-03-19T16:28:43Z

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

+        if (other.isBlackjack()) {
+            return ResultType.DRAW;
+        }
+        if (other.isBusted()) {
+            return ResultType.WIN_WITH_BLACKJACK;
+        }
+        return ResultType.WIN_WITH_BLACKJACK;


Suggested change

if (other.isBlackjack()) {

return ResultType.DRAW;

}

if (other.isBusted()) {

return ResultType.WIN_WITH_BLACKJACK;

}

return ResultType.WIN_WITH_BLACKJACK;

if (other.isBlackjack()) {

return ResultType.DRAW;

}

return ResultType.WIN_WITH_BLACKJACK;

이렇게 간소화할 수 있지 않을까요?

앗, 그렇군요 수정했습니다 😓

hongbin-dev · 2022-03-19T16:29:00Z

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

+        if (isBlackjack()) {
+            return getResultTypeIfBlackjack(other);
+        }
+
+        if (isBusted()) {
+            return getResultTypeIfBusted(other);
+        }
+
+        return getResultTypeIfOrdinary(other);


상태별로 나눠놓으니 가독성이 좋군요👍

hongbin-dev · 2022-03-19T16:29:51Z

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

+        if (other.isBlackjack()) {
+            return ResultType.LOSE;
+        }
+        if (other.isBusted()) {
+            return ResultType.DRAW;
+        }
+        return ResultType.LOSE;


Suggested change

if (other.isBlackjack()) {

return ResultType.LOSE;

}

if (other.isBusted()) {

return ResultType.DRAW;

}

return ResultType.LOSE;

return ResultType.LOSE;

플레이어가 버스트면 항상 패배입니다

블랙잭 규칙을 잘 못 이해하고 있었네요 ㅎㅎ; 로직을 변경했습니다!

hongbin-dev · 2022-03-19T16:32:34Z

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

+public class Players {
+
+    private static final String NOT_FOUND_PLAYER_EXCEPTION_MESSAGE = "플레이어를 찾을 수 없습니다.";
+    private final List<Player> value = new ArrayList<>();
+
+    public void add(Player player) {
+        validateNotDuplicateName(player.getName());
+        value.add(player);
+    }


하나씩 추가하는형태(add() 메서드)가 아니라, 한번에 입력받아서 생성할 수는 없을까요?
그래야 불변을 유지하기 좋을 것 같네요

플레이어 이름과 베팅 금액을 한쌍으로 여러개를 전달받아야, 한번에 플레이어 여러명을 생성하고 Players 를 생성할 수 있습니다. 고민을 많이 해봤는데 플레이어를 생성하기 위한 PlayerCreateDto 를 컨트롤러에서 생성해서 BlackjackGame 에 전달하는 형태로 개선했습니다!

hongbin-dev · 2022-03-19T16:35:43Z

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

+        Money totalProfit = Money.from(0);
+        for (Player player : value) {
+            totalProfit = totalProfit.add(player.calculateProfit(dealer));
+        }
+
+        return totalProfit;


이쪽은 스트림으로 분리할 수 있을 것 같아요. 분리하면 totalProfit 같은 가변변수를 줄일 수 있을 것 같네요.

return value.stream() .map(player -> player.calculateProfit(dealer)) .reduce(Money.from(0), Money::add);

위와 같이 수정하였습니다 ! ㅎㅎ

hongbin-dev · 2022-03-19T16:37:15Z

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

+public class Money {
+    private static final double BLACKJACK_PROFIT_RATE = 1.5;
+
+    private final int value;


금액을 사용하는거니 Decimal 타입을 사용해볼까요?

https://tecoble.techcourse.co.kr/post/2021-04-26-BigInteger_BigDecimal/ 이쪽을 참고해보면 좋을 것 같아요

처음 들어본 클래스네요! 금액과 같이 정확한 계산이 필요한 경우 BigDecimal 을 사용해야 하는군요. 지식 공유 감사드립니다 😄

- compareWith 로직을 Participant 에서 Player 로 변경 - 테스트 코드 변경

- Controller 에 잘못 입력된 베팅금액에 대한 재입력 로직 추가 - getValue 를 getIntValue 로 변경

devHudi · 2022-03-20T06:52:22Z

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

+
+    public int getIntValue() {
+        return value.intValue();
+    }
+


value 의 타입을 int 에서 BigDecimal 로 변경하였으므로 명시적으로 메소드명에 'int' 라는 단어를 사용하였습니다.

hongbin-dev

안녕하세요 후디!
2단계 미션 진행하느라 고생많으셨어요! 코드가 많이 깔끔해졌네요.
이만 머지하겠습니다 👏

hongbin-dev · 2022-03-21T11:09:31Z

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

+    public int getIntValue() {
+        return value.intValue();


프로퍼티에 int가 명시되어있는데, 자료형을 메서드이름에 남길 필요가 있을까요?

아 프로퍼티에도 명시되어 있고, 반환타입에도 명시가 되어 있군요!.. 수정해야할 것 같습니다!

hongbin-dev · 2022-03-21T11:11:16Z

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

+    public Player generatePlayer(String playerName, int betMoney) {
+        return Player.of(playerName, generateInitialHand(), Money.from(betMoney));


이 부분도 테스트해볼 수 있겠네요

hongbin-dev · 2022-03-21T11:22:26Z

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

+    public void addPlayer(Player player) {
+        players.add(player);
+    }


사용하지 않는 메서드는 지우는게 좋겠네요 🙃

앗, 미처 확인하지 못했습니다. 지울게요 😓

hongbin-dev · 2022-03-21T11:25:26Z

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

+        BlackjackGame blackjackGame = initializeGame();
+        printInitialHand(blackjackGame);
+        giveExtraCardsToAllPlayer(blackjackGame);
+        giveExtraCardsToDealer(blackjackGame);
+        printFinalHandAndScore(blackjackGame);
+        printBetResult(blackjackGame);


giveExtraCardsToAllPlayer(blackjackGame)

위 같은 메서드를 봤을 때, 코드 사용자 입장에서 파라미터로 넘어간 blackjackGame의 상태가 바뀔거라고 예상하지못하는데요. 이런 부분을 개선해보셔도 좋을 것 같네요.

저도 그 부분이 불편했었는데, 완벽히 blackjackGame 을 불변으로 만들거나 메서드명을 개선해야할 것 같습니다!..

devHudi force-pushed the step2 branch from c22d9e8 to 5db1d34 Compare March 19, 2022 09:34

devHudi added 19 commits March 19, 2022 18:53

refactor(ParticipantDto): cards와 score필드 두가지를 추가로 갖도록 변경

84c025b

- HandDto 제거

refactor(BlackjackController): 순서 의존적인 메소드를 run메소드로 묶고, Application 에…

95f890e

…서 실행하도록 구조 개선 - BlackjackController의 public 메소드 이름 개선

refactor(Score): Hand의 점수 합계 계산 책임을 Score로 위임

9ca745d

- Hand.getScore 를 사용하는 코드를 Score.calculateSumFrom 으로 변경 - 테스트코드 수정

refactor(Participant): 승, 패, 무 결정 로직 가독성 개선

4ff7ed4

refactor(Hand): 카드 패의 자료구조를 Set 에서 List 로 변경

89a7da2

refactor(Score): 특정 기준을 넘긴 것에 대한 메소드 구현

7cc815f

- isGreaterThan, isLessThan 과 같은 메소드를 없애고, isBusted, isOverDealerHitThreshold 와 같은 메소드를 별개로 구현함

refactor(Referee): Referee 를 제거하고, 관련된 책임을 DealerMatchResult, PlayerM…

fc61fb9

…atchResult 로 위임 - Dto 였던 DealerMatchDto 를 DealerMatchResult 라는 도메인으로 변경 - Dto 였던 PlayerMatchDto 를 PlayerMatchResult 라는 도메인으로 변경

feat(Card): 모든 종류의 카드를 미리 캐싱해두도록 개선

fdd27de

docs(README): 2단계 기능 요구 사항 작성

9032293

feat(PlayerBetResult): 플레이어의 베팅금액과 수익을 계산하고 저장하는 도메인 구현

773a6a3

- 테스트코드 작성 - Money 도메인 구현 - BetAndProfit 도메인 구현

feat(DealerProfitResult): 딜러의 수익을 계산하는 도메인 구현

5c41bef

- Money 도메인에 add, subtract 메소드 구현

feat(blackjack): 베팅 금액을 입력받고, 베팅 결과를 출력하는 기능 추가

b1782cb

- BlackjackController 및 InputView, OutputView 기능 구현 - 승패결과를 나타내는 클래스 제거 - 기타 사용되지 않는 클래스 제거

feat(ProfitResult): 딜러의 수익을 계산하는 메소드 구현

2f5be8a

- PlayerBetResult 에서 ProfitResult 로 이름 변경 - DealerProfitResult 제거

docs(README): 기능 요구 사항 반영

fdb94be

fix(Participant): 점수가 같으나 상대는 블랙잭이고, 자신은 블랙잭이 아닌 경우 패배 처리 로직 구현

84ef9bc

refactor(Participant): 승, 패, 무 판단 로직 개선

3b3f496

- 테스트 코드 수정 - Score 가 다른 Score 에 비해 큰지, 작은지를 검사하는 isGreaterThan, isLessThan 메소드를 구현함

devHudi force-pushed the step2 branch from 5db1d34 to 3b3f496 Compare March 19, 2022 09:56

devHudi commented Mar 19, 2022

View reviewed changes

feat(Player): 음수 베팅 금액 입력 시 예외 발생 기능 구현

57de649

hongbin-dev suggested changes Mar 19, 2022

View reviewed changes

devHudi added 3 commits March 20, 2022 02:40

fix(Player): 플레이어가 버스트되었을 때 플레이어가 무조건 패배하도록 로직 변경

79d1dd1

- compareWith 로직을 Participant 에서 Player 로 변경 - 테스트 코드 변경

refactor(Players): 총 이익 계산 로직을 for 문에서 stream 으로 변경

4e4ec13

refactor(Money): value 의 자료형을 int 에서 BigDecimal 로 변경

4563fb6

- Controller 에 잘못 입력된 베팅금액에 대한 재입력 로직 추가 - getValue 를 getIntValue 로 변경

devHudi commented Mar 20, 2022

View reviewed changes

refactor(PlayerCreateDto): 플레이어 생성에 대한 Dto 를 통해 Players 를 생성하도록 로직 변경

433c4c8

hongbin-dev approved these changes Mar 21, 2022

View reviewed changes

hongbin-dev merged commit af0ace7 into woowacourse:devhudi Mar 21, 2022

	protected ResultType compareWith(Participant other) {
	protected ResultType compareWith(Dealer other) {

		public Player generatePlayer(String playerName, int betMoney) {
		return Player.of(playerName, generateInitialHand(), Money.from(betMoney));

[2단계 - 블랙잭(베팅)] 후디(조동현) 미션 제출합니다. #369

[2단계 - 블랙잭(베팅)] 후디(조동현) 미션 제출합니다. #369

Uh oh!

Conversation

devHudi commented Mar 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

아쉬운 점

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!

devHudi Mar 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongbin-dev 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!

hongbin-dev 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!

devHudi Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

Projects

Milestone

Development

Uh oh!

2 participants

devHudi commented Mar 19, 2022 •

edited

Loading

devHudi Mar 19, 2022 •

edited

Loading

devHudi Mar 22, 2022 •

edited

Loading