레디 step1 미션 제출 #6

reddevilmidzy · 2024-03-08T10:03:12Z

리뷰 미리 감사드립니다!!😁

고민했던 점

읽기 좋은 코드란 무엇인가?

이번 블랙잭 미션의 테마는 클린 코드였습니다. 제가 생각하는 클린 코드는 읽기 좋은 코드라고 생각하여 읽기 좋은 코드를 만들기 위해 네이밍에 고민을 많이 하였습니다. (사실 계속 고민하여도 마땅한 이름이 떠오르지 못한 경우도 많았습니다)

네이밍을 하던 도중 “누가 읽었을 때” 읽기 좋은 코드를 짜는 것이 올바를까에 대한 질문이 들었습니다.

이번 미션으로 예를 들면 블랙잭 도메인에 관해 아무것도 모르는 사람이 읽었을 때도 쉽게 이해할 수 있는 네이밍으로 해야할지(도메인 용어를 사용하지 않는 방향), 아니면 애초에 이 코드를 읽는 사람이 해당 도메인에 대한 지식을 가지고 있을 것을 기대하고 도메인 관련된 용어를 사용해도 되는지에 대해 페어 제제와 토론을 하였습니다.

우선 저의 의견은 후자입니다. 읽기 좋은 코드를 만든다는 것은 의미를 잘 전달할 수 있는 네이밍을 하는 것인데, 도메인 용어를 사용한다면 의미를 명확하게 잘 전달할 수 있다고 생각하였습니다.

이런 고민을 가진 클래스는 Hands 입니다. 이 Hands 클래스는 참가자의 가지고 있는 카드들이 있는 일급 컬렉션입니다. 이 클래스는 2장 이상부터 많아봐야 5이하의 카드를 가지고 있어 이런 소수의 카드를 의미하는 도메인 용어로 카드 패킷을 따와 Packet 이라고 지었습니다. 페어와도 토론해보고 데일리 조들과도 이야기해보면서 패킷이라는 용어는 개발자 사이에선 네트워크 패킷과 혼동 될 수 있음을 인지하였습니다.

최종적으로는 적절한 선을 찾아서 참가자가 손에 들고 있는 카드라는 의미로 Hands로 적절하게 지었다고 생각합니다.

만약 수달이라면 어떤 식으로 이름을 지으셨을 거 같나요?

이름을 지을 때 “누구나” 읽기 쉬운 작명을 하시나요 아니면 “도메인 지식이 있는 사람이” 읽기 쉬운 작명을 하시나요?

궁금합니다!🙄

아쉬웠던 점

기능 목록을 섬세하게 작성하지 못했던 점

미션을 진행할 때, 우선 요구 사항을 파악하여 기능 목록을 작성하고, 핵심 로직은 무엇일까 고민하여 각 카드의 합을 더하는 기능이라고 판단하고(지금 다시 생각하니 카드를 더해 승패를 판단하는 게 핵심 로직 같네요😂) TDD로 진행해보았습니다.

하지만 프리코스때 달리 기능 요구 사항이 많이 애매하여 구현하면서 새로 추가한 기능이 생겼습니다. 기존에는 기능 목록대로 커밋 메시지를 작성했는데 이번에는 그러지 못하고 계속 수정사항이 생겼던 것이 아쉬웠습니다.

시간이 부족하여 마음에 드는 구조를 작성하지 못한 점

기능 구현을 할 때 TODO 주석 와다다 바르면서 구현하는 편인데, 이 TODO 발라놓은 코드를 리팩터링할 시간이 부족했던거 같습니다. 현재 Game 이라는 객체는 Dealer 와 Players 를 필드로 가지고 있는데 Controller 에서 Dealer 를 호출하는 코드를 이 Game 객체 안에 있는 구조로 변경해보고 싶었는데 시간이 부족했습니다😥.

그리고 추상 클래스 Participant 가 있고 이를 player와 Dealer 상속받는 형태로 구조를 설계하였는데 과연 적절한 설계였나? 괜찮은 네이밍이였나? 라는 의문은 해결되지 않았습니다.

자세한 기능 사항

이전과 카드의 변화가 있거나 한번도 출력한 적 없으면 출력

요구사항에 명확하게 명시되어 있진 않지만 예제 코드를 보고 유추한 기능입니다.

pobi는 한장의 카드를 더 받겠습니까?(예는 y, 아니오는 n)
y
pobi카드: 2하트, 8스페이드, A클로버
pobi는 한장의 카드를 더 받겠습니까?(예는 y, 아니오는 n)
n
jason는 한장의 카드를 더 받겠습니까?(예는 y, 아니오는 n)
n
jason카드: 7클로버, K스페이드

pobi가 첫번째에 y을 입력했을 때는 카드가 추가되었기에 가지고 있는 카드를 출력해주었지만, 두번째에 n을 입력했을 때는 이전 카드와 변함이 없기에 출력하지 않았습니다.

jason이 첫번째에 n을 입력했을 때, 아직 jason의 카드는 한번도 출력해준 적(보여준 적)이 없기에 출력을 해주었습니다.

딜러의 카드가 17이상이 될때까지 계속 카드를 받는다.

처음 받은 딜러의 카드가 16이하일 때 카드를 한번만 받으면 되는지 아니면 17 이상이 될 때까지 카드를 받는 것인지 애매하였지만 실제 블랙잭 룰을 참고하여 17이상이 될때까지 계속 카드를 받도록 구현하였습니다.

무승부

카드의 합이 같고, 가지고 있는 카드의 수가 같고 21이하라면 무승부라고 판단하였습니다.

참여자의 카드의 합이 21 이상이라면 더 이상 카드를 받을 수 없다.

참여자의 카드의 합이 21을 초과한다면 BUST 메시지를 출력하고 턴을 넘기도록 구현하였습니다. 마찬가지로 카드의 합이 21이라면 BLACK JACK!!! 메시지를 출력하고 더 이상 입력받지 않도록 구현하였습니다. (옆에서 다른 크루가 카드가 2장이면서 21이여야만 BLACK JACK라고 설명해주네요😅 메시지를 바꿔야겠습니다)

이렇게 21이상인 경우 턴을 넘기는 것도 실제 블랙잭 룰을 참고하여 구현을 하였는데, 예제를 보면 포비의 카드가 2하트, 8스페이드, A클로버 즉 21일 때 더 받을지 말지를 물어보고 있어 예제 대로 동작하지는 않습니다.

이 부분에 대해 잠깐 고민해보았는데, 프리코스 때에는 “요구 사항에 명시된 출력값 형식을 지키지 않을 경우 0점으로 처리한다.“ 라고 명시되어 있지만 우테코 미션에선 딱히 형식을 지키라고 안내되어 있지 않았습니다.

프리코스에 이런 요구사항이 있는 이유는 아무래도 채점을 위한 도구라고 판단하였습니다. 하지만 우테코 미션에선 출력값으로 채점을 하지는 않기 때문에 출력 형식을 자유롭게 풀었고, 이 점을 이용하여 좀 더 사용자가 보다 쉽게 플레이 할 수 있도록 추가 출력 메시지 기능을 구현하였습니다.

robinjoon

코드 잘 봤습니다!

그리고 추상 클래스 Participant 가 있고 이를 player와 Dealer 상속받는 형태로 구조를 설계하였는데 과연 적절한 설계였나? 괜찮은 네이밍이였나? 라는 의문은 해결되지 않았습니다.

중복을 줄이는 목표라면 구조적으로 적절했다고 생각합니다. 플레이어와 딜러가 공통으로 수행하는 도메인 로직은 상위 클래스에 정의하고, 다른 것들만 하위 클래스에서 정의했으니까요. 다만, �Participant 클래스의 메서드들이 전부 재정의 가능한 메서드인 것은 조금 이상한 것 같아요. 하위 클래스에서 이를 재정의한다면, 중복이 줄어드는 것이 아니라, 오히려 불필요한 코드가 생기는 것 같아요.

robinjoon · 2024-03-09T11:25:57Z

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

+
+    public void deal() {
+        while (handsSum() <= MIN_HANDS_SUM) {
+            super.add(cardDeck.pop());


[질문] super 키워드 없어도 될거 같은데, 상위 클래스에서 정의된 메서드라는 것을 표현하기 위해 사용하신건가요?

robinjoon · 2024-03-10T05:04:00Z

src/main/java/controller/BlackJackController.java

+    }
+
+    private boolean shouldShowHands(final boolean handsChanged, final Answer answer) {
+        return (Answer.STAY.equals(answer) && !handsChanged) || Answer.HIT.equals(answer);


[의견] enum 비교는 == 을 이용하는 것이 일반적이라고 알고 있어요!

robinjoon · 2024-03-10T05:11:56Z

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

+    ACE("A", 1),
+    TWO("2", 2),
+    THREE("3", 3),
+    FOUR("4", 4),
+    FIVE("5", 5),
+    SIX("6", 6),
+    SEVEN("7", 7),
+    EIGHT("8", 8),
+    NINE("9", 9),
+    TEN("10", 10),
+    JACK("J", 10),
+    QUEEN("Q", 10),
+    KING("K", 10);


[질문] 지금은 출력에서 A , J, Q, K 로 되어있어서 enum에서 이렇게 해둔 것 같아요. 만약 출력 형식이 바뀐다면 domain 패키지가 변경될거 같은데, 대안이 없을까요?

robinjoon · 2024-03-10T05:12:18Z

src/main/java/domain/Hands.java

+    public boolean equals(final Object target) {
+        if (this == target) {
+            return true;
+        }
+
+        if (!(target instanceof Hands hands)) {
+            return false;
+        }
+
+        return Objects.equals(cards, hands.cards);
+    }


[질문] 동일한 카드를 들고 있다면, 내부의 순서에 상관 없이 동일해야 할 것으로 생각되는데, 맞나요? 맞다면 지금 그렇게 구현되어있는건가요?

내부의 순서 상관없이 동일하게 생각되어야 맞는거 같네요! 그냥 습관적으로 equals 를 구현하는 것이 문제였.. 감사합니다1!

robinjoon · 2024-03-10T05:13:42Z

src/main/java/domain/Hands.java

+    public List<String> getCards() {
+        return cards.stream()
+                .map(Card::toString)
+                .toList();
+    }


[의견] 이 메서드를 호출하는 코드를 작성할 때, 메서드 이름이 getCards 이기 때문에 반환 타입이 List 인 것으로 예상할 것 같아요. 메서드 이름을 바꿔보는건 어떤가요?

동의합니다! 메서드 네이밍 고민 많이하여도 마땅한 이름이 나오지 않는거 같아 일단 다른거 부터 구현하자! 하고 넘어갔다가 다시 와도 좋은 이름이 떠오르진 않는 거 같네요ㅜ

robinjoon · 2024-03-10T05:20:27Z

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

+    public void add(final Card card) {
+        hands.add(card);
+    }
+
+    public boolean isBust() {
+        return hands.isBust();
+    }
+
+    public boolean isBlackJack() {
+        return hands.isBlackJack();
+    }


[의견] 어떤 도메인 로직을 구현할 때에는, 메서드 호출 스택의 깊이가 깊어질수록 더 구체적인 논리를 표현하게 하는 것이 이상적이에요.
그런데, 지금은 Participant의 메서드에서 보여주는 논리와 같은 수준의 논리를 표현한 Hands의 메서드를 호출하네요. 차라리 Participant의 메서드에서 Hands 의 sum 메서드를 호출해 판단하는 것이 좋지 않을까요?

robinjoon · 2024-03-10T05:22:33Z

src/main/java/dto/ParticipantsDto.java

+        this.players = players;
+    }
+
+    public static ParticipantsDto of(final Participant dealer, final Players players) {


[질문] 매개변수의 타입이 Dealer가 아닌 것은 단순 실수인가요?

robinjoon · 2024-03-10T05:52:46Z

src/test/java/domain/DealerTest.java

+    @Test
+    @DisplayName("딜러의 카드의 합이 17이상이 될때까지 카드를 추가한다.")
+    void dealerDeal() {
+        //given
+        final Hands sum10 = new Hands(List.of(new Card(FIVE, SPADE), new Card(FIVE, HEART)));
+        final Dealer dealer = new Dealer(CardDeck.generate(), sum10);
+
+        //when
+        dealer.deal();
+
+        //then
+        Assertions.assertThat(dealer.countAddedHands()).isPositive();
+    }


[질문] deal 수행 이후 딜러가 가진 카드의 합이 17이상인지 확인해야 하는거 아닌가요?

맞아요! 놓친 부분이네요

robinjoon · 2024-03-10T05:55:16Z

src/test/java/domain/HandsTest.java

+                Arguments.of(new Hands(List.of(new Card(ACE, HEART),
+                                new Card(EIGHT, SPADE),
+                                new Card(TWO, CLOVER))),
+                        21),
+                Arguments.of(new Hands(List.of(new Card(ACE, DIAMOND),
+                                new Card(TWO, CLOVER),
+                                new Card(FOUR, CLOVER),
+                                new Card(TWO, CLOVER))),
+                        19)


[의견] Hands.of 가 List를 받는 것이 아니라, 가변 배열을 받도록 한다면, 혹은 둘다 있다면, 코드가 조금 더 단순해질 것 같아요.

저희도 처음에 가변 배열을 사용하여 테스트 코드를 작성했었습니다!

그런데 org.junit.jupiter.api.extension.ParameterResolutionException: Error converting parameter at index 0: No implicit 요런 에러가 발생하더라구요

찾아보니까 Arguments 에서 가변 배열을 사용하려면 다시 배열을 만들어 준다음 넣어야지 되더라구요

오류가 발생하지 않으려면 배열을 넣어줘야 해 가변 배열의 장점을 가지지 못하여 그냥 List로 받는 형태로 구현하였습니다!

static Stream<Arguments> sumParameterProvider1() { return Stream.of( Arguments.of( 20, new Card[]{ new Card(CardNumber.TWO, CardShape.HEART), new Card(CardNumber.EIGHT, CardShape.SPADE), new Card(CardNumber.JACK, CardShape.CLOVER) }), Arguments.of( 12, new Card[]{ new Card(CardNumber.THREE, CardShape.DIAMOND), new Card(CardNumber.NINE, CardShape.CLOVER) }) ); }

robinjoon · 2024-03-10T05:58:26Z

src/main/java/dto/ParticipantDto.java

+
+public record ParticipantDto(String name, List<String> cards, int totalSum) {
+
+    public static ParticipantDto from(final Participant player) {


[질문] 매개변수의 타입이 �Player가 아닌 것은 단순 실수인가요?

상위 클래스를 사용하고 싶었습니다!

seokmyungham

딜러가 덱을 들고있다는 컨셉이 재미있고 신선했던 것 같아요~
전체적으로 네이밍만 조금 더 신경쓰면 읽기 더 수월할 것 같습니다 ㅎㅎ 😊

seokmyungham · 2024-03-10T11:18:17Z

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

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


저도 레디와 같이 Dealer 안에 "딜러" 를 상수로 두고 콘솔 출력에 사용하는 코드를 작성했었는데
리뷰어로부터 리뷰를 받게 되었습니다.

리뷰 내용은

딜러에게 굳이 이름이 필요할까요?

입력과 관련없는 "딜러" 라는 딜러의 이름을 표현하는 책임은, 딜러가 맞을까요?

레디는 이와 관련해서 어떻게 생각하시나요~? 😊

글을 보니 딜러에게 굳이 이름은 필요없어보이네요 ! 후 어렵다 어려워

seokmyungham · 2024-03-10T11:26:19Z

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

+    @Override
+    public String toString() {
+        return rank.getName() + shape.getName();
+    }
+}


현재 toString() 메서드가 카드의 출력을 위해 사용되고 있는 것 같은데
이렇게 되면 출력 형식이 수정되거나 추가될 때 마다 도메인 Card를 계속 수정하게 될 것 같습니다.

저는 toString() 메서드는 보통 디버깅 용으로 사용하는 것이 적절하다 생각하고,
출력을 위해서 getter를 사용해야 한다면 차라리 getter를 사용하는 것이 낫다 생각하는데 레디는 어떻게 생각하시나요~?

그렇다면 Card의 toString()은 어떤 값을 넘겨주는 것이 괜찮을까요?

저의 생각은 Card의 필드에는 rank 와 shape 가 있고 이 필드의 값을 딱히 가공없이 그냥 반환하는 형태로 toString을 사용하여 디버깅으로도 사용할 수 있고 출력시에 사용할 수도 있어 두마리 토끼 잡았다고 생각하여 사용하였습니다!

두마리 토끼 저도 정말 좋아하는데요!! ㅎㅎ
근데 지금은 어찌보면 도메인이 출력형식에 의존하고 있는 것과 다를바 없지 않나~? 생각이 들어요

만약 출력 형식이 5스페이드에서 스페이드5로만 바뀌어도
출력 뷰가 아닌 카드 도메인을 수정해야 할 것 같아 드린 리뷰였습니다!

출력 형식이 변경됐다고 하면 보통 개발자는 도메인 보다 뷰를 먼저 열어보지 않을까용~?
toString()은 디버깅 목적에 따라서 편한대로 사용해도 괜찮다고 생각해요

seokmyungham · 2024-03-10T11:36:46Z

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

+    public void deal(final Player player, final Answer answer) {
+        if (Answer.HIT.equals(answer)) {
+            player.add(cardDeck.pop());
+        }
+    }
+
+    public void deal() {
+        while (handsSum() <= MIN_HANDS_SUM) {


코드를 보면 위의 deal 메서드는 딜러가 플레이어에게 카드를 1장 나누어주는 역할,
아래의 deal 메서드는 딜러 자신이 1장 카드를 받는 역할을 맡고 있는 것 같습니다.

리뷰 도중 BlackJackController 에도 똑같은 deal 메서드가 있는 것을 확인했었는데,
코드를 해석하는 동안 네이밍이 조금 더 명확했으면 어떨까 하는 생각이 계속 들었습니다.

처음 이 코드를 보는 개발자가 dealer.deal(), dealer.deal(player, answer) 또는 컨트롤러의 deal() 네이밍만 봐서는 각각 어떤 역할을 하는지 한 번에 이해하기 힘들거라 생각되는데..! 레디는 어떻게 생각하시나용 😄

seokmyungham · 2024-03-10T11:52:22Z

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

+    public List<Player> getPlayers() {
+        return names;
+    }


일급 컬렉션이 현재 내부의 참조를 반환했는데, 이는 setter 를 허용한 것과 마찬가지라고 생각합니다.
방어적 복사를 활용하는 방법은 어떨까요?

seokmyungham · 2024-03-10T11:59:41Z

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

+    public Player(final String name, final Hands hands) {
+        super(name, hands);


문자열 이름을 포장하지 않은 특별한 이유가 있는지 궁금합니다~ 😄

seokmyungham · 2024-03-10T12:12:26Z

src/main/java/controller/BlackJackController.java

+    private void printHandsIfRequired(final Player player, final boolean handsChanged, final Answer answer) {
+        if (shouldShowHands(handsChanged, answer)) {
+            outputView.printHands(ParticipantDto.from(player));
+        }
+    }


printHandsIfRequired 라는 네이밍을 봤을 때 한 번에 이해하기는 좀 힘든 것 같아요.

직역하면 "필요한 경우 핸드를 출력한다" 라는 의미 같은데,
왜 필요한지 알기 위해 메서드를 열어봤더니 또 if 조건 문 안에
shouldShowHands "보여줘야한다" 라는 의미의 네이밍 조건이 있어서.. 이해하기 조금 힘드네용 😊

메서드 이름에 행위를 드러내더라도 의도가 조금 섞이도록 하는게 좋을 것 같아요

네이밍 어려워요ㅜㅜ
사실 저 메서드명은 채찍피티가 짜줬던 거 같네요
좀 더 고민해보겠습니다!

ㅋㅋㅋㅋㅋㅋㅋ 저는 지쌤이라 부르는데 채찍피티도 재밌네요

seokmyungham · 2024-03-10T12:23:57Z

src/main/java/controller/BlackJackController.java

+    private boolean shouldShowHands(final boolean handsChanged, final Answer answer) {
+        return (Answer.STAY.equals(answer) && !handsChanged) || Answer.HIT.equals(answer);


위 printHandsIfRequired 리뷰의 연장입니다!
이 부분이 컨트롤러에서 중요한 비중을 차지하고 있는 것 같아서 추가로 리뷰를 남겨드려요

shouldShowHands 네이밍이 일단 좀 아쉬운 것 같아용
메서드 네이밍으로 의도를 잘 드러내거나 혹은 내부 조건 로직을 더 가독성 있게 변경하면 어떨까 싶습니다.

Answer 에 메서드를 하나 만들어서 메서드 이름으로 의도를 드러내는 것도 하나의 방법이 될 것 같아요.
(Answer.isStay, Asnwer.isHit 등등)

그리고 부정연산자가 기본적으로 조건문 안에 들어가면 가독성에 안 좋다 생각합니다.
다른 조건과 결합되어 있을 때 더 심한 경향이 있는 것 같아서 따로 분리하는 것도 방법이 될 수 있을 것 같습니다.

hyxrxn

디스크립션대로 전반적으로 메서드 이름에 신경을 많이 쓰신 것 같아요!
몇 가지 질문과 코멘트 남겼습니다~~

hyxrxn · 2024-03-10T07:41:38Z

docs/README.md

+**승**
+
+* (딜러 카드 합 < 참여자 카드 합) && 참여자가 버스트가 아닌 경우
+* (딜러 카드 합 == 참여자 카드 합) && (딜러 카드 수 > 참여자 카드 수) && 참여자가 버스트가 아닌 경우
+* 딜러가 버스트인 경우 && 참여자가 버스트가 아닌 경우
+
+**무**
+
+* (딜러 카드 합 == 참여자 카드 합) && (딜러 카드 수 == 참여자 카드 수) && 참여자가 버스트가 아닌 경우
+
+**패**
+
+* 참여자가 버스트인 경우
+* (딜러 카드 합 > 참여자 카드 합)


개인적으로 승과 무에 참여자가 버스트가 아닌 경우가 맨 앞에 가있거나, 다른 방식으로 언급이 되면 더 좋을 것 같아요~
맨 뒤에 같은 말이 반복되어 가독성이 조금 떨어진다고 느껴집니다.

그리고 딜러 카드 합 == 참여자 카드 합 인데 딜러 카드 수 < 참여자 카드 수인 경우는 어떻게 되나요?
언급이 되어 있지 않아 여쭤봅니다!

딜러 카드 합 == 참여자 카드 합 && 딜러 카드 수 < 참여자 카드 수 라면 "패"조건이네요 작성을 놓쳤네요😅

hyxrxn · 2024-03-10T10:02:56Z

src/main/java/controller/BlackJackController.java

+        this.outputView = outputView;
+    }
+
+    public void run() {


메서드 길이가 10라인을 넘었네요

hyxrxn · 2024-03-10T10:06:28Z

src/main/java/controller/BlackJackController.java

+            Answer answer = Answer.from(inputView.readAnswer(player.getName()));
+            dealer.deal(player, answer);
+
+            printHandsIfRequired(player, handsChanged, answer);


21 이상이면 카드를 받을 수 없는 것은 예제와 다르게 구현하셨는데,
보여준 적 없으면 보여준다는 것은 예제대로 구현하셨네요!

저는 이 부분이 생기면서 로직이 좀 복잡해진 듯 합니다.
혹시 이 기능이 꼭 필요하다고 생각하신 이유가 있나요?

오오 잘 캐치해주셨어요!

어떤건 예제대로 하고 어떤건 또 멋대로 구현하여 혼란을 줄 수도 있을거 같은데 이유를 설명해보자면;;;
21이상 일때 카드를 더 받을 수 없게 한것은 플레이어가 최적의 스코어를 얻을 수 있게하기 위함이었습니다

그리고 변경된 경우에만 출력하는 기능은 예제 보고 규칙을 파악하여 그냥 신나서 구현했던 거 같아요 ㅎㅎㅎ
꼭 필요하다고 생각해서 구현하진 않았습니다😅

hyxrxn · 2024-03-10T10:07:51Z

src/main/java/domain/CardDeck.java

+public class CardDeck {
+    private static final int DECK_SIZE = 6;
+
+    private final Deque<Card> cards;


자료구조 중 deque를 사용하신 이유가 있나요?

요것도 시행착오가 있었는데,,

우선 처음에는 List 였습니다. 하지만 블랙잭 게임에선 계속 위의 값만 꺼내서 사용하기 때문에 상위원소 삭제가 간편한 Stack 을 사용하였다가 동기화 이슈로 LinkedList 를 사용하였다가 꺼내는 로직이 마음에 안들어 Deque를 사용하게 되었습니다!

정리하면 제일 위의 값을 삭제하기 위한 자료구조로 덱이 효율적이라고 판단하여 사용하였습니다!

hyxrxn · 2024-03-10T10:10:19Z

src/main/java/domain/CardDeck.java

+    private static List<Card> generateOneCardDeck() {
+        return Arrays.stream(Shape.values())
+                .flatMap(shape -> matching(shape).stream())
+                .toList();
+    }
+
+    private static List<Card> matching(final Shape shape) {
+        return Arrays.stream(Rank.values())
+                .map(rank -> new Card(rank, shape))
+                .toList();
+    }


저도 고민했던 부분인데...
개인적으로 stream보다는 for문을 그냥 사용하는 것이 가독성 측면에서 더 좋은 것 같아요.
그냥 의견입니다!

맞아요 저도 2중 포문이 더 직관적인거 같아요 ㅎㅎ

hyxrxn · 2024-03-10T10:38:11Z

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

+        this.cardDeck = cardDeck;
+    }
+
+    public void initHands(final Players players) {


제가 리뷰어에게 받은 질문인데요,
메서드명만 봤을 때 처음에 두 장씩 준다는 것은 몰라도 된다고 생각하시나요?

몇 장 주는지 알고 싶다면 INIT_HANDS_SIZE를 보는 형태가 좋을 거 같다고 생각해요!
만약 몇장주는지에 대한 의미를 담아 initTwoCardsHands(...) 이런식(좀 이름을 못지은거 같지만) 짓었다고 가정을 하면,
이때 처음에 두장씩 주는 룰이 3장으로 바뀌었다면 INIT_HANDS_SIZE 상수를 변경하고, 이 메서드명까지 변경해야 한다는 문제점이 발생할 수 있을 거 같아요.

hyxrxn · 2024-03-10T10:44:20Z

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

+    private static final int MIN_SIZE = 2;
+    private static final int MAX_SIZE = 8;
+
+    private final List<Player> names;


Player의 List인데 변수명으로 names를 사용하신 이유가 있나요?

필드명을 players 로 하지 않았냐는 의미이신거죠?!

저도 처음에 클래스이름을 소문자로 변경하여 필드명을 사용하는 식으로 했었는데, 이때 발생할 수 있는 문제점은 외부에서 이 필드값을 사용할 때 발생할 수 있습니다!

getter로 players를 사용해야하는 상황을 가정하면

Players players = new Players(); players.getPlayers();

이런 식으로 getter 를 사용하게 될텐데 뭔가 뭔가여서 names를 사용하였습니다!

참고로 sonarLint에서 배웠어요! 블로그 글 투척

hyxrxn · 2024-03-10T12:43:15Z

src/main/java/dto/DealerHandsDto.java

+        this.displayedCard = displayedCard;
+    }
+
+    public static DealerHandsDto from(final Participant dealer) {


딜러의 카드는 한 장만 보여준다는 것을 알아보기 힘든데,
이 또한 도메인에 대한 이해가 있다고 가정하고 이렇게 작성하신 걸까요?

hyxrxn · 2024-03-10T12:46:50Z

src/main/java/view/InputView.java

+        if (rawNames.endsWith(NAME_SEPARATOR)) {
+            return true;
+        }
+        return rawNames.contains(NAME_SEPARATOR.repeat(2));


startWith과 conatains는 Player의 IsBlank에서 걸러질 것 같은데 명시적으로 두려고 따로 빼놓으신건가요?

엇 질문을 잘 이해 못했어요!

isBlank 에서 걸러지는건 " " 이런 공백이고

startsWith 에서 걸리지는건 ,레디,제제 요런 녀석이고
contains 에서 걸러지는건 레디,,제제 요런 녀석입니다!

hyxrxn · 2024-03-10T12:47:57Z

src/main/java/view/OutputView.java

+
+public class OutputView {
+
+    private static final String FORM = "%s카드: %s";


이름이 약간 모호해보여요!

reddevilmidzy

리뷰 남겨주신분들 감사함니다ㅏ!

reddevilmidzy · 2024-03-10T13:58:10Z

docs/README.md

+**승**
+
+* (딜러 카드 합 < 참여자 카드 합) && 참여자가 버스트가 아닌 경우
+* (딜러 카드 합 == 참여자 카드 합) && (딜러 카드 수 > 참여자 카드 수) && 참여자가 버스트가 아닌 경우
+* 딜러가 버스트인 경우 && 참여자가 버스트가 아닌 경우
+
+**무**
+
+* (딜러 카드 합 == 참여자 카드 합) && (딜러 카드 수 == 참여자 카드 수) && 참여자가 버스트가 아닌 경우
+
+**패**
+
+* 참여자가 버스트인 경우
+* (딜러 카드 합 > 참여자 카드 합)


딜러 카드 합 == 참여자 카드 합 && 딜러 카드 수 < 참여자 카드 수 라면 "패"조건이네요 작성을 놓쳤네요😅

reddevilmidzy · 2024-03-10T14:04:48Z

src/main/java/controller/BlackJackController.java

+            Answer answer = Answer.from(inputView.readAnswer(player.getName()));
+            dealer.deal(player, answer);
+
+            printHandsIfRequired(player, handsChanged, answer);


오오 잘 캐치해주셨어요!

어떤건 예제대로 하고 어떤건 또 멋대로 구현하여 혼란을 줄 수도 있을거 같은데 이유를 설명해보자면;;;
21이상 일때 카드를 더 받을 수 없게 한것은 플레이어가 최적의 스코어를 얻을 수 있게하기 위함이었습니다

그리고 변경된 경우에만 출력하는 기능은 예제 보고 규칙을 파악하여 그냥 신나서 구현했던 거 같아요 ㅎㅎㅎ
꼭 필요하다고 생각해서 구현하진 않았습니다😅

reddevilmidzy · 2024-03-10T14:05:32Z

src/main/java/controller/BlackJackController.java

+    private void printHandsIfRequired(final Player player, final boolean handsChanged, final Answer answer) {
+        if (shouldShowHands(handsChanged, answer)) {
+            outputView.printHands(ParticipantDto.from(player));
+        }
+    }


네이밍 어려워요ㅜㅜ
사실 저 메서드명은 채찍피티가 짜줬던 거 같네요
좀 더 고민해보겠습니다!

reddevilmidzy · 2024-03-10T14:08:50Z

src/main/java/domain/CardDeck.java

+public class CardDeck {
+    private static final int DECK_SIZE = 6;
+
+    private final Deque<Card> cards;


요것도 시행착오가 있었는데,,

우선 처음에는 List 였습니다. 하지만 블랙잭 게임에선 계속 위의 값만 꺼내서 사용하기 때문에 상위원소 삭제가 간편한 Stack 을 사용하였다가 동기화 이슈로 LinkedList 를 사용하였다가 꺼내는 로직이 마음에 안들어 Deque를 사용하게 되었습니다!

정리하면 제일 위의 값을 삭제하기 위한 자료구조로 덱이 효율적이라고 판단하여 사용하였습니다!

reddevilmidzy · 2024-03-10T14:09:17Z

src/main/java/domain/CardDeck.java

+    private static List<Card> generateOneCardDeck() {
+        return Arrays.stream(Shape.values())
+                .flatMap(shape -> matching(shape).stream())
+                .toList();
+    }
+
+    private static List<Card> matching(final Shape shape) {
+        return Arrays.stream(Rank.values())
+                .map(rank -> new Card(rank, shape))
+                .toList();
+    }


맞아요 저도 2중 포문이 더 직관적인거 같아요 ㅎㅎ

reddevilmidzy · 2024-03-10T14:44:33Z

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

+    private static final int MIN_SIZE = 2;
+    private static final int MAX_SIZE = 8;
+
+    private final List<Player> names;


필드명을 players 로 하지 않았냐는 의미이신거죠?!

저도 처음에 클래스이름을 소문자로 변경하여 필드명을 사용하는 식으로 했었는데, 이때 발생할 수 있는 문제점은 외부에서 이 필드값을 사용할 때 발생할 수 있습니다!

getter로 players를 사용해야하는 상황을 가정하면

Players players = new Players(); players.getPlayers();

이런 식으로 getter 를 사용하게 될텐데 뭔가 뭔가여서 names를 사용하였습니다!

참고로 sonarLint에서 배웠어요! 블로그 글 투척

reddevilmidzy · 2024-03-10T14:45:36Z

src/main/java/dto/ParticipantDto.java

+
+public record ParticipantDto(String name, List<String> cards, int totalSum) {
+
+    public static ParticipantDto from(final Participant player) {


상위 클래스를 사용하고 싶었습니다!

reddevilmidzy · 2024-03-10T14:49:51Z

src/main/java/view/InputView.java

+        if (rawNames.endsWith(NAME_SEPARATOR)) {
+            return true;
+        }
+        return rawNames.contains(NAME_SEPARATOR.repeat(2));


엇 질문을 잘 이해 못했어요!

isBlank 에서 걸러지는건 " " 이런 공백이고

startsWith 에서 걸리지는건 ,레디,제제 요런 녀석이고
contains 에서 걸러지는건 레디,,제제 요런 녀석입니다!

reddevilmidzy · 2024-03-10T14:50:22Z

src/test/java/domain/DealerTest.java

+    @Test
+    @DisplayName("딜러의 카드의 합이 17이상이 될때까지 카드를 추가한다.")
+    void dealerDeal() {
+        //given
+        final Hands sum10 = new Hands(List.of(new Card(FIVE, SPADE), new Card(FIVE, HEART)));
+        final Dealer dealer = new Dealer(CardDeck.generate(), sum10);
+
+        //when
+        dealer.deal();
+
+        //then
+        Assertions.assertThat(dealer.countAddedHands()).isPositive();
+    }


맞아요! 놓친 부분이네요

reddevilmidzy · 2024-03-10T14:55:52Z

src/test/java/domain/HandsTest.java

+                Arguments.of(new Hands(List.of(new Card(ACE, HEART),
+                                new Card(EIGHT, SPADE),
+                                new Card(TWO, CLOVER))),
+                        21),
+                Arguments.of(new Hands(List.of(new Card(ACE, DIAMOND),
+                                new Card(TWO, CLOVER),
+                                new Card(FOUR, CLOVER),
+                                new Card(TWO, CLOVER))),
+                        19)


저희도 처음에 가변 배열을 사용하여 테스트 코드를 작성했었습니다!

그런데 org.junit.jupiter.api.extension.ParameterResolutionException: Error converting parameter at index 0: No implicit 요런 에러가 발생하더라구요

찾아보니까 Arguments 에서 가변 배열을 사용하려면 다시 배열을 만들어 준다음 넣어야지 되더라구요

오류가 발생하지 않으려면 배열을 넣어줘야 해 가변 배열의 장점을 가지지 못하여 그냥 List로 받는 형태로 구현하였습니다!

static Stream<Arguments> sumParameterProvider1() { return Stream.of( Arguments.of( 20, new Card[]{ new Card(CardNumber.TWO, CardShape.HEART), new Card(CardNumber.EIGHT, CardShape.SPADE), new Card(CardNumber.JACK, CardShape.CLOVER) }), Arguments.of( 12, new Card[]{ new Card(CardNumber.THREE, CardShape.DIAMOND), new Card(CardNumber.NINE, CardShape.CLOVER) }) ); }

reddevilmidzy and others added 30 commits March 5, 2024 14:00

docs: 기능 요구 사항 작성

e504ca0

test: 카드 합 구하기

3ed10c0

feat: 카드 합 구하기

42a28a9

chore: 패키지 변경

8972ef5

feat: 참여자 생성 기능 구현

3a7d4d4

feat: null 이나 공백인 경우 예외가 발생 기능 구현

d719d08

feat: 참여자 이름이 중복시 예외 발생 기능 구현

a83c06d

feat: 총 참여자의 수는 2이상 8이하가 아닐시 예외 발생 기능 구현

3a1cd48

feat: 카드를 추가하는 기능 구현

acf6680

feat: Player가 Packet을 필드로 갖도록 구현

68fa548

feat: CardDeck 생성 기능 구현

fbf514f

test: 플레이어에게 카드 2장을 나눠주는 기능 구현

6017614

refactor: 리스트에서 스택으로 변경

ac2aa22

feat: 참여자에게 카드를 2장씩 분배하는 기능 구현

aa6549c

feat: 딜러가 카드 2장을 분해하다.

9450f3b

refactor: 스택에서 덱으로 변경

b533c94

fix: CardNumber ONE 제거, TEN으로 추가

b458bdf

feat: 카드 내의 ACE 가 포함된 경우 합이 21이하이면 에이스는 11로 계산 기능 구현

63ec5fb

feat: 카드 합 계산시 ACE 고려하는 로직 추가

50e902d

refactor: Packet 클래스 이름을 Hands로 변경

52dcb84

feat: 참여자의 대답이 y, n 가 아닐시 예외가 발생한다.

ba0dd83

feat: 참여자 이름 입력받는 기능 구현

56eaa06

feat: 플레이어의 시작 카드 출력 기능 구현

ee77777

feat: 딜러 카드 출력하는 기능 구현

a5255e3

docs: 완료한 기능 체크

9575be9

feat: answer에 따라 카드를 추가 기능 구현

e4d4369

refactor: Dealer 필드에서 Players 제거

51f5aea

feat: 추가된 카드 출력하는 기능 구현

3a0ddb4

style: 사용하지 않는 메서드 삭제

9b1e7a4

feat: 참여자의 대답이 y일시 한번 더 deal 기능 구현

546c4e9

robinjoon reviewed Mar 10, 2024

View reviewed changes

seokmyungham reviewed Mar 10, 2024

View reviewed changes

hyxrxn reviewed Mar 10, 2024

View reviewed changes

reddevilmidzy commented Mar 10, 2024

View reviewed changes

reddevilmidzy and others added 26 commits March 11, 2024 13:52

docs: 리팩터링 목록 추가

2d822e9

refactor: createEmptyPacket 메서드명 createEmptyHands로 수정

405ac6b

feat: 카드덱이 비었을때 꺼낸 경우 예외 발생

dfaae33

refactor: Name 객체 포장

aaf03ab

refactor: 승패무 판단을 Result의 책임으로 이동 (재미있게 BiPredicate 활용)

017e6a0

refactor: Game 클래스 삭제 및 책임 분리

36094c8

refactor: TestFixture 생성

ad35da3

style: import 문 순서 변경

5e142b0

refactor: 테스트코드 변수명 변경 및 테스트 추가

cc6a91f

fix: LinkedHashMap으로 변경하여 참여자 순서 유지

d5564b4

refactor: System.out.println 대신 %n과 System.lineSeparator 사용

2b02cdb

docs: 리팩터링 목록 수정

b23548c

feat: 참가자의 이름에 딜러가 사용될 시 예외 발생 기능 구현

8e95a70

refactor: isHit 메서드 생성

2a8dfdc

docs: 리팩터링 목록 수정

3de3d0b

style: 메서드 위치 변경

21ec566

refactor: controller 메서드 추출

f2d25ec

chore: 패키지 정리

96cdc3b

docs: 리팩터링 목록 추가

fec95ee

feat: 예외 시 재입력 기능 구현

41b1df7

feat: 커스텀 예외 구현

4347c8c

refactor: Participant 추상 메서드 사용

1713750

refactor: InvalidPlayerName 예외 클래스 이름 변경

167957a

refactor: 카드 생성 책임을 Card로 변경 및 카드를 캐시하여 가져오는 기능 구현

e9374bc

docs: 리팩터링 목록 수정

19b71c6

refactor: cardDeck 생성 책임 위치 변경

80e13e4


		public record ParticipantDto(String name, List<String> cards, int totalSum) {

		public static ParticipantDto from(final Participant player) {


		public class Dealer extends Participant {

		private static final String NAME = "딜러";

		public Player(final String name, final Hands hands) {
		super(name, hands);

		private boolean shouldShowHands(final boolean handsChanged, final Answer answer) {
		return (Answer.STAY.equals(answer) && !handsChanged) \|\| Answer.HIT.equals(answer);


		public class OutputView {

		private static final String FORM = "%s카드: %s";

레디 step1 미션 제출 #6

Are you sure you want to change the base?

레디 step1 미션 제출 #6

Conversation

reddevilmidzy commented Mar 8, 2024 • edited Loading

고민했던 점

읽기 좋은 코드란 무엇인가?

아쉬웠던 점

기능 목록을 섬세하게 작성하지 못했던 점

시간이 부족하여 마음에 드는 구조를 작성하지 못한 점

자세한 기능 사항

이전과 카드의 변화가 있거나 한번도 출력한 적 없으면 출력

딜러의 카드가 17이상이 될때까지 계속 카드를 받는다.

무승부

참여자의 카드의 합이 21 이상이라면 더 이상 카드를 받을 수 없다.

robinjoon 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

seokmyungham left a comment • 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

hyxrxn 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

reddevilmidzy 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

reddevilmidzy commented Mar 8, 2024 •

edited

Loading

seokmyungham left a comment •

edited

Loading