[Lev1][미션3] 블랙잭 1단계 - 다즐 #5

woo-chang · 2023-03-03T14:23:34Z

시간이 부족해서 많이 부족한 코드가 보일 것 같습니다 .. 잘 부탁드립니다 :)

Co-authored-by: woo-chang <[email protected]>

greeng00se

3~4번정도 쭉 봤는데, 엄청 깔끔하게 작성했는데?
역시 다즐 👍 주말 잘 보내고 월요일날 보자!

greeng00se · 2023-03-03T22:21:18Z

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

+    }
+
+    private void validateRestrictWord(final String value) {
+        if (value.equals(RESTRICT)) {


RESTRICT.equals(value)로 변경한다면 NPE를 방지할 수 있을 것 같당! (null이 들어올리는 없겠지만..?)

NPE 중요한 요소라서 너무 좋은데요?

greeng00se · 2023-03-03T22:24:26Z

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

+
+    public Dealer getDealer() {
+        return participants.stream()
+                .filter(Dealer.class::isInstance)


딜러인지 확인하는 메시지를 던져보는건 어떨까?

instanceOf를 지양하라는 글을 읽고 메시지를 던져야 겠다고 생각이 들었어 👍👍

greeng00se · 2023-03-03T22:31:37Z

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

+    private static Stream<Arguments> generateCardsWithACE() {
+        return Stream.of(
+                Arguments.of(List.of(new Card(ACE, SPADE), new Card(JACK, HEART)), 21),
+                Arguments.of(List.of(new Card(ACE, SPADE), new Card(ACE, HEART), new Card(TEN, CLOVER)), 12),
+                Arguments.of(List.of(new Card(ACE, SPADE), new Card(QUEEN, HEART), new Card(TEN, CLOVER)), 21),
+                Arguments.of(List.of(new Card(ACE, SPADE), new Card(ACE, HEART)), 12)
+        );
+    }
+
+    @ParameterizedTest
+    @MethodSource("generateCardsWithoutACE")
+    void 점수를_계산한다(final List<Card> cardPack, final int totalScore) {
+        final Cards cards = new Cards(cardPack);
+
+        assertThat(cards.calculateTotalScore()).isEqualTo(totalScore);
+    }


테스트가 꼼꼼하네용 👍

greeng00se · 2023-03-03T22:31:54Z

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

+        final Suit suit = Suit.SPADE;
+        final Number number = Number.ACE;
+
+        final Card card = new Card(number, suit);
+
+        assertThat(card.getSuitName()).isEqualTo(suit.getName());


거짓성공이 일어날 수 있을 것 같은데 검증부에는 하드코딩 해보는건 어떨까?

참고자료 -> https://jojoldu.tistory.com/615

그렇게 하려고 했었는데 놓친 부분이 있었네 감사합니다 :)

greeng00se · 2023-03-03T22:32:48Z

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

+        if (value > 21) {
+            if (score.getValue() > 21) return DRAW;


이 부분도 내가 보기에 2depth 같아!

시간 부족으로 인해 급하게 만들어서 수정이 시급합니다 ,,,

greeng00se · 2023-03-03T22:33:54Z

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

+        for (final Suit suit : Suit.values()) {
+            for (final Number number : Number.values()) {
+                pack.add(new Card(number, suit));
+            }
+        }
+        TRUMP = pack;


1depth가 요구사항이라 stream의 flatmap이나 foreach를 사용하는 방법으로 구현해볼 수 도 있을 것 같아! 😄

저도 같은 생각입니다

좋은 의견 너무너무 감사합니다 👍👍

greeng00se · 2023-03-03T22:38:52Z

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

+    public void drawCard(final Deck deck, int count) {
+        while (count-- > 0) {
+            participants.forEach(participant -> participant.drawCard(deck.draw()));
+        }
+    }


count-- > 0에 대해서 어떻게 생각해?

메서드 내부에서만 사용될 변수라 큰 생각안하고 작성했는데 혹시 어떤 문제가 있을까 🤔

drunkenhw

안녕하세요 다즐~ ✋ 리뷰어 박스터 입니다~
코드 읽기가 되게 편했던 것 같아요 코멘트 몇가지 남겨놨는데 확인 부탁드려요

게임에 참여할 사람의 이름을 입력하세요.(쉼표 기준으로 분리)
1,2,3

딜러와 1, 2, 3에게 2장을 나누었습니다.
딜러카드: 9다이아몬드, 3다이아몬드
1카드: 9하트, 10클로버
2카드: K클로버, 4스페이드
3카드: J다이아몬드, 3하트

그리고 딜러의 카드는 한 장만 공개해야하는데 2장이 다 보이는 것 같아요~

drunkenhw · 2023-03-04T03:55:02Z

src/main/java/blackjack/controller/BlackJackController.java

+    private static final int INIT_DRAW_COUNT = 2;
+    private static final int DEALER_LIMIT = 16;


이 부분은 도메인이 알고 있어야 하는 룰이지 않을까요?

동의합니다 ,,, 딜러가 알아야 할 정보인 것 같습니다 '^'

drunkenhw · 2023-03-04T03:56:04Z

src/main/java/blackjack/controller/BlackJackController.java

+                .collect(Collectors.toList());
+    }
+
+    private void cardDraw(final List<Player> players, final Deck deck) {


메소드는 동사로 시작하는게 좋지 않을까요? 그리고 같은 메서드명이 3개나 있네요

급하게 작성한 코드는 다 티가 나는 군요 😂

같은 메서드명은 오버로딩을 사용해서 도메인별로 동작을 보여주려고 했습니다!

drunkenhw · 2023-03-04T03:59:10Z

src/main/java/blackjack/controller/BlackJackController.java

+    private Participants gatherParticipants() {
+        final List<Participant> participants = new ArrayList<>();
+
+        participants.add(new Dealer());


Dealer는 무조건 있어야 게임이 되는 룰이니 participants 클래스 내에서 생성하는 건 어떤가요?

너무 좋은 것 같습니다 👍

생성자에서 생성하면 클래스의 역할이 많아지게 되는 것 같아서, 딜러와 플레이어를 매개변수로 가지는 생성자를 사용하는건 어떻게 생각하시나요?

아주 좋은데요?

drunkenhw · 2023-03-04T04:06:34Z

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

+        for (final Suit suit : Suit.values()) {
+            for (final Number number : Number.values()) {
+                pack.add(new Card(number, suit));
+            }
+        }
+        TRUMP = pack;


저도 같은 생각입니다

drunkenhw · 2023-03-04T04:07:50Z

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

+    }
+
+    public Card draw() {
+        if (cards.empty()) {


이런 예외가 생길 거라 생각을 못해봤는데 좋은 거 같네요! 👍

drunkenhw · 2023-03-04T04:14:41Z

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

+    public void drawCard(final Deck deck, int count) {
+        while (count-- > 0) {
+            participants.forEach(participant -> participant.drawCard(deck.draw()));
+        }
+    }


블랙잭은 카드를 두 장이나 한 장만 나눠주는 게 규칙인데 굳이 count를 파라미터로 받을 이유가 있을까요? 그리고 while 보다는 다른 방법으로 반복하는게 명확할 것 같아요

확장성을 고려해서 여러 장을 나눠주는 상황이 발생했을 때 범용적으로 사용할 수 있는 메서드를 고민해 봤습니다!

이전 리뷰어는 for -> while로 변경할 수 있으면 while 사용을 추천해주셨는데, 이 점은 어떻게 생각하시는지 궁금합니다!

그 분의 의도는 잘 모르겠지만 초보 입장으로는 for가 가독성이 제일 좋은 것 같아요 for loop에 들어가는 인덱스를 사용하지 않아서 for를 지양하는 거라면 intStream도 괜찮지 않을까요? 물론 전 for를 사랑합니다

drunkenhw · 2023-03-04T04:17:55Z

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

+    }
+
+    public Result compare(final Score score) {
+        final int result = this.compareTo(score);


compareTo로 대소를 비교하신 이유가 뭐에요?

Score간 비교를 하고 싶었는데 결국 점수를 비교할 상황이 생기면서 굳이 있어야 하나라는 메서드가 되어버린 것 같습니다 ..

마지막에 급하게 수정한 코드들이라 부족한 부분이 많이 보이네요 😂

drunkenhw · 2023-03-04T04:19:50Z

src/main/java/blackjack/view/dto/DealerStateResponse.java

@@ -0,0 +1,20 @@
+package blackjack.view.dto;
+
+public class DealerStateResponse {


이 클래스는 어떤 역할을 해주죠?

drunkenhw · 2023-03-04T04:22:49Z

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

+    }
+
+    public boolean readMoreDraw(final String name) {
+        System.out.println(format("{0}은(는) 한장의 카드를 더 받겠습니까?(예는 {1}, 아니오는 {2})", name, YES, NO));


이런 포맷은 처음 봤네요. 배우고 갑니다~ 👍

drunkenhw · 2023-03-04T04:26:22Z

src/test/java/blackjack/domain/participant/DealerTest.java

+    void 카드를_받는다() {
+        //given
+        final List<Card> cardPack = new ArrayList<>(List.of(
+                new Card(QUEEN, CLOVER),
+                new Card(SIX, HEART)
+        ));
+        final Cards cards = new Cards(cardPack);
+        final Dealer dealer = new Dealer(cards);
+
+        //when
+        dealer.drawCard(new Card(ACE, DIAMOND));
+
+        //then
+        assertThat(dealer.isDrawable()).isFalse();


여기만 given, when, then 주석이 있네요 이유가 있을까요?

개행만으로 given, when, then이 명확하지 않을 때는 주석으로 명시하도록 했습니다 ⛄️

저는 개행만으로도 완벽히 이해가 될 것 같은데요?

shin-mallang

고생하셨습니다

shin-mallang · 2023-03-08T00:39:49Z

src/main/java/blackjack/controller/BlackJackController.java

+    private final InputView inputView;
+    private final OutputView outputView;


View를 필드로 사용하신 이유가 있으실까요?

static 메서드를 사용하지 않고 생성자 주입을 통해 필드로 가지는 이유는 말하는 걸까요 🤔

제가 이해한 게 맞다면 제가 필드로 사용한 이유는 다음과 같습니다.

우선 static으로 사용해도 좋다고 생각합니다. InputView, OutputView 모두 가변적인 필드를 가지고 있지 않고, 사이드 이펙트가 발생할 가능성이 존재하지 않기에 사용해도 좋은 것 같습니다!

하지만 필드로 사용한 이유는 static으로 사용하게 되면, 어디서든 사용할 수 있기에 뷰가 어디든 침범할 수 있게 되고 알게 된다고 생각합니다. 이를 컨트롤러에서만 알고 관리하기 위해서 필드로 주입받아서 사용하였습니다.

shin-mallang · 2023-03-08T00:40:49Z

src/main/java/blackjack/controller/BlackJackController.java

+
+    public void run() {
+        final Participants participants = new Participants(new Dealer(), gatherPlayers());
+        final Deck deck = Deck.createUsingTrump(1);


저만 이 1이 이해가 잘 안될까요?
다른 분들의 의견도 들어봐야 하겠지만, 처음 보는 사람 입장에서는 조금 불명확하다는 느낌이 들어요

다시 한번 읽어보니 무슨 의미를 가지는지 명확하지 않은 것 같습니다 ..

매직 넘버를 사용하여 의미를 명확하게 드러낼 수 있도록 수정해보겠습니다!

shin-mallang · 2023-03-08T00:42:18Z

src/main/java/blackjack/controller/BlackJackController.java

+    }
+
+    private void dealCards(final Participants participants, final Deck deck) {
+        participants.drawCard(deck, INITIAL_DRAW_COUNT);


INITIAL_DRAW_COUNT 이거를 컨트롤러에서 관리하는 것이 맞을까요?
컨트롤러의 책임이 무엇이라 생각하시나요?

뷰와 도메인 사이에서 중간 매개체 역할을 수행한다고 생각합니다. BlackJackGame을 만들지 않아 해당 역할을 Controller가 수행하도록 했었는데 해당 역할을 BlackJackGame으로 넘겨도 좋은 것 같습니다 👍

shin-mallang · 2023-03-08T00:43:47Z

src/main/java/blackjack/controller/BlackJackController.java

+    }
+
+    private ParticipantResponse getHiddenDealerResponse(final Dealer dealer) {
+        final List<Card> hiddenCards = dealer.getCards().subList(0, INITIAL_DRAW_COUNT - 1);


해당 코드는 Dealer의 자율성을 침해하는 것 같아요~!
좀 더 개선해 볼 수 있을 것 같습니다.

그렇네요! 꼼꼼한 리뷰 감사합니다 👾

shin-mallang · 2023-03-08T00:44:15Z

src/main/java/blackjack/controller/BlackJackController.java

+
+    private ParticipantResponse getHiddenDealerResponse(final Dealer dealer) {
+        final List<Card> hiddenCards = dealer.getCards().subList(0, INITIAL_DRAW_COUNT - 1);
+        final CardsResponse cardsResponse = new CardsResponse(-1, getCardInfos(hiddenCards));


이곳의 -1 역시 처음 보는 사람 입장에서 불명확한 것 같아요
-1이 의미하는 것이 무엇이죠?

이 부분도 위와 동일하게 수정해 보겠습니다!

shin-mallang · 2023-03-08T00:49:38Z

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

+
+public enum Suit {
+
+    SPADE("스페이드"),


위와 동일합니다 👾

shin-mallang · 2023-03-08T00:50:06Z

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

+    public Dealer() {
+        super();
+    }
+
+    public Dealer(final Cards cards) {
+        super(cards);
+    }


생성자가 두개 모두 필요한가요?

처음 설계할 때와 개발할 때 설계가 달라져서 불필요한 생성자가 포함되어 있었네요. 감사합니다 :)

shin-mallang · 2023-03-08T00:50:54Z

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

+    private boolean isDrawableCardCount() {
+        return cards.count() <= MAXIMUM_DRAWABLE_CARD_COUNT;
+    }
+
+    private boolean isDrawableScore() {
+        return cards.calculateTotalScore() <= MAXIMUM_DRAWABLE_SCORE;
+    }


cards로 해당 역할을 위임하는 것은 어떻게 생각하시나요?
cards의 자율성을 해친다고 생각합니다.

cards가 해당 역할을 가지게 되면 딜러가 뽑을 수 있는 카드 개수와 딜러가 뽑을 수 있는 점수를 카드가 알아야 하는데 이 부분에 대해서는 어떻게 생각하시나요?

Cards 가 아는 것은 괜찮지 않을까요?
Cards 말고 조금 더 의미있는 네이밍도 괜찮다고 생각합니다.

shin-mallang · 2023-03-08T00:52:11Z

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

+    }
+
+    @Nested
+    class isAce_메서드는 {


테스트 너무 좋은 것 같아요

shin-mallang · 2023-03-08T00:52:39Z

src/test/java/techcourse/jcf/mission/SimpleList.java

+        return result;
+    }
+
+    static <T> void copy(final SimpleList<? extends T> origin, final SimpleList<? super T> copy) {


kokodak and others added 30 commits February 28, 2023 17:07

docs: 요구사항 명세 작성

93adb74

feat: 이름의 길이 검증 기능 구현

4d669bd

feat: 금지된 이름 검증 기능 구현

ab04caa

docs: 기능 구현 목록 수정

76e478d

Co-authored-by: woo-chang <[email protected]>

feat: 이름 도메인 생성

c5575b7

Co-authored-by: woo-chang <[email protected]>

feat: 문양 도메인 생성

aeb41cc

Co-authored-by: woo-chang <[email protected]>

feat: 숫자 도메인 생성

660568b

Co-authored-by: woo-chang <[email protected]>

feat: 문양 이름 확인 기능 구현

5529812

Co-authored-by: woo-chang <[email protected]>

feat: 숫자 이름 확인 기능 구현

f42b755

Co-authored-by: woo-chang <[email protected]>

feat: 점수 확인 기능 구현

59212a2

Co-authored-by: woo-chang <[email protected]>

feat: 점수 계산 기능 구현

365c189

Co-authored-by: woo-chang <[email protected]>

feat: 에이스인지 확인하는 기능 구현

14cb9da

Co-authored-by: woo-chang <[email protected]>

feat: 점수 합계 계산 기능 구현

d668b8c

Co-authored-by: woo-chang <[email protected]>

feat: 점수 최댓값 초과 여부 확인 기능 구현

07e5c72

Co-authored-by: woo-chang <[email protected]>

feat: 이름 확인 기능 구현

36dacd5

Co-authored-by: woo-chang <[email protected]>

feat: 블랙잭 여부 확인 기능 구현

fe3be1e

Co-authored-by: woo-chang <[email protected]>

fix: 메서드명 수정 및 잘못된 로직 수정

bf4991f

Co-authored-by: woo-chang <[email protected]>

feat: 카드 추가 여부 확인 기능 구현

ed70701

Co-authored-by: woo-chang <[email protected]>

feat: 카드 추가 기능 구현

4c494a0

Co-authored-by: woo-chang <[email protected]>

chore: 테스트 어노테이션 추가

43b6ec5

Co-authored-by: woo-chang <[email protected]>

feat: 카드를 받는 기능 구현

35fd4fc

Co-authored-by: woo-chang <[email protected]>

feat: 점수 확인 기능 구현

d3a6af1

Co-authored-by: woo-chang <[email protected]>

feat: 개수 확인 기능 구현

58f1fae

Co-authored-by: woo-chang <[email protected]>

feat: 카드 추가 여부 확인 기능 구현

81f907a

Co-authored-by: woo-chang <[email protected]>

feat: 점수 확인 기능 구현

96833fd

Co-authored-by: woo-chang <[email protected]>

feat: 카드를 받는 기능 구현

5138dc4

Co-authored-by: woo-chang <[email protected]>

feat: 딜러 도메인 생성

4e58c1d

Co-authored-by: woo-chang <[email protected]>

feat: 참가자 도메인 생성

e3ae0a0

Co-authored-by: woo-chang <[email protected]>

feat: 딜러와 플레이어가 참가자 상속

4cdb38c

Co-authored-by: woo-chang <[email protected]>

feat: 플레이어의 이름 중복 검증 기능 구현

2370e13

Co-authored-by: woo-chang <[email protected]>

kokodak and others added 3 commits March 3, 2023 17:26

fix: 게임 결과 로직 수정

0ad7a72

Co-authored-by: woo-chang <[email protected]>

refactor: TRUMP는 덱이 알도록 수정

6518a49

refactor: 입력 기능 가독성 위해 메서드명 수정

24e9e03

woo-chang requested review from greeng00se and drunkenhw March 3, 2023 14:23

woo-chang self-assigned this Mar 3, 2023

woo-chang changed the title ~~[1단계 - 블랙잭 게임 실행]~~ [Lev1][미션3] 블랙잭 1단계 - 다즐 Mar 3, 2023

greeng00se reviewed Mar 3, 2023

View reviewed changes

drunkenhw reviewed Mar 4, 2023

View reviewed changes

woo-chang added 20 commits March 5, 2023 15:12

refactor: 축약하지 않은 변수명으로 수정

57cb277

refactor: 딜러가 카드를 추가로 뽑을 수 있는 점수는 딜러가 알도록 수정

5f77913

refactor: ACE -> Ace 일관성있는 표현으로 수정

f7eb062

refactor: 테스트 목적을 명확히하도록 수정

90d09d3

refactor: 계층 구조 테스트로 의미를 명확하게 전달하도록 수정

9bddfc3

refactor: 자바 컨벤션에 따라 메서드명 수정

8f294bb

refactor: 카드 뽑을 때 검증하도록 수정

ed5db59

refactor: NPE 방지 및 변수, 메서드명 수정

0223fca

refactor: 참가자에게 딜러 여부에 대한 메시지를 던지도록 수정

7eeedc2

refactor: 거짓 성공을 막기위해 검증부에는 하드코딩 하도록 수정

1a85ff7

refactor: TRUMP 생성 시 flatMap 사용 및 덱 생성 시 섞도록 수정

cf364cf

refactor: 플레이어 이름을 명시하도록 Name -> PlayerName 수정

b15c4c2

refactor: 참여자 목록 생성 책임을 생성자로 수정

6506c31

refactor: Deck 생성 책임은 Deck이 가지도록 수정

60cd11c

refactor: 블랙잭 게임 컨트롤러 수정

2458f44

refactor: 딜러 승패 여부 확인 로직 수정

4daf6f8

refactor: 카드 목록에서 사용되지 않는 메서드 제거

24f34f1

refactor: 딜러가 결과 확인하는 로직 수정

be64ca7

test: 테스트하지 못했던 코드에 대한 테스트 코드 작성

601b98d

feat: 제네릭 미션 구현

44bd964

shin-mallang reviewed Mar 8, 2023

View reviewed changes

		private static final int INIT_DRAW_COUNT = 2;
		private static final int DEALER_LIMIT = 16;

		@@ -0,0 +1,20 @@
		package blackjack.view.dto;

		public class DealerStateResponse {

		private final InputView inputView;
		private final OutputView outputView;

[Lev1][미션3] 블랙잭 1단계 - 다즐 #5

Are you sure you want to change the base?

[Lev1][미션3] 블랙잭 1단계 - 다즐 #5

Conversation

woo-chang commented Mar 3, 2023

greeng00se 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

drunkenhw 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

woo-chang Mar 5, 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

shin-mallang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woo-chang Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woo-chang Mar 8, 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

woo-chang Mar 8, 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

woo-chang Mar 5, 2023 •

edited

Loading

woo-chang Mar 8, 2023 •

edited

Loading

woo-chang Mar 8, 2023 •

edited

Loading

woo-chang Mar 8, 2023 •

edited

Loading