[🚀 사이클2 - 미션 (블랙잭 게임 실행)] 현미밥 미션 제출합니다. by gahyeonnni · Pull Request #1112 · woowacourse/java-blackjack

gahyeonnni · 2026-03-13T09:16:05Z

체크 리스트

미션의 필수 요구사항을 모두 구현했나요?
Gradle test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?
애플리케이션이 정상적으로 실행되나요?

어떤 부분에 집중하여 리뷰해야 할까요?

Hand가 카드를 저장하는 역할뿐만 아니라, canLowerAceScore 메서드처럼 블랙잭의 구체적인 점수 계산 규칙까지 너무 자세히 알고 있다는 느낌을 받았습니다. 데이터를 가진 Hand에서 직접 계산하는 것이 응집도 측면에서는 좋겠지만, 책임이 과도하다고 느껴집니다. 이 로직을 그대로 Hand에 두는 것이 맞을까요, 아니면 별도의 객체(계산기)로 분리하는 것이 더 나은 설계일까요? (7540373 커밋입니다!)

감사합니다 ☺️

pkeugine

안녕하세요 현미밥, 미션 진행하시느라 고생 많으셨습니다.

이번 리뷰는... 조금 냉정하게 리뷰했어요.
개인적으로 현미밥의 사이클 1 구현이 정말 좋았기 때문에
이번에 아쉽다고 느낀 부분들을 좀 강하게 강조해봤습니다.

하지만 기능적 요구사항은 모두 충족한 점은 정말 좋았습니다. 👍
리뷰를 하다보면 버그들을 발견하곤 하는데,
현재 현미밥 애플리케이션에서는 요구사항에 부합하지 않는 버그는 없는 듯 하네요.
(제가 못 발견했을 수도 있는데, 그만큼 잘했다는 거겠죠 ㅋㅋㅋ)

다시 강조한 내용들로 돌아가보자면
제가 이번에 강조하고 싶은 점은 다음과 같습니다 :

코드가 현미밥이 작성한 것인지 조금 의문이 생깁니다. 너무 강하게 말하는건가 싶기도 한데, 본인이 작성했다면 발생하기 힘든 부분들이 몇 가지 보였거든요. 저희가 리뷰했던 내용들도 있고요. 이런 부분들을 잘 챙겨주셔야 앞으로 크루-리뷰어 관계가 아닌 팀원-팀원 간의 리뷰 할 때 현미밥이 더 훌륭한 퍼포먼스를 보여주실 듯 해서 이번에 제 생각을 쿠션어 없이 드러내봤어요. 이 부분, 정말 중요해요. 꼭 잘 챙겨봅시다.
코드 전반에 걸쳐 테스트 커버리지를 높여봤으면 합니다. 현재 요구사항에 부합하지 않는 버그는 없더라도, 예상하지 못한 동작이나 버그에 취약한 구조가 몇 가지 보였거든요. 테스트가 있다면 이를 사전에 방지 할 수 있지 않나 싶어요!

리뷰 확인해보시고, 궁금증이나 대화하고 싶은 내용이 있다면 부담 갖지 말고 DM 또는 코멘트 남겨주세요. 제가 잘못 파악한 내용이 있거나 동의하지 않는 내용이 있다면 현미밥도 제게 강하게 반박하셔도 좋습니다. :)

남은 사이클 같이 화이팅해보죠.
해피해킹하세요!!! 🙂 🙂 🙂

pkeugine · 2026-03-13T15:58:46Z

src/main/java/controller/BlackJackGame.java

@@ -1,24 +1,20 @@
 package controller;


README가 아직 사이클1 체크리스트 중심이라 이번 사이클에서 추가된 배팅 규칙이 잘 안 보이네요.

문서 관리도 꽤나 중요한 일이니, 한 번 업데이트 해볼까요?

pkeugine · 2026-03-13T16:08:35Z

src/main/java/domain/result/GameResult.java

 import java.util.EnumMap;
 import java.util.Map;

 public class GameResult {


이번 사이클의 실제 출력 흐름에서는 GameResult가 더 이상 사용되지 않네요.
사용되지 않는 코드는 제거해보면 어떨까요?

코드베이스가 커지고, 프로젝트 아키텍쳐가 복잡해지면 intelliJ 가 '사용되지 않음' 으로 인식하는 코드가 있더라도 실제로 사용되는 경우가 종종 있어요. 그 상황에서는 코드를 제거했다가는 예상치 못한 사이드 이펙트가 생길 수 있어서 마음 편히 코드를 제거하기가 어려워지는데요,

그렇기 때문에 더더욱 개발하는 과정 안에서 불필요한 코드를 삭제하고,
이렇게 규모가 너무 크지 않아서 코드 사용처를 바로 확인할 수 있을 때
관리해주는 습관을 가져보면 좋아요 :)

리뷰를 하며 전체적으로 사용되지 않는 클래스들, 로직, 그리고 상수까지 다양하게 보여요.
프로젝트 전반적으로 다 확인해보고 불필요한 것들은 제거해주세요.

사용되지 않는 클래스나 로직들을 다시 확인하고 정리하겠습니다!

pkeugine · 2026-03-13T16:18:53Z

src/main/java/domain/result/Outcome.java

+    public Outcome opposite() {
+        if (this == WIN) {
+            return LOSE;
+        }
+        if (this == LOSE) {
+            return WIN;
+        }
+        return DRAW;
+    }


지금은 opposite()를 쓰는 경로가 죽어 있어서 버그가 드러나지 않지만, BLACKJACK_WIN.opposite()가 DRAW로 계산되네요.

위의 코멘트를 반영하면 해당 메서드도 삭제될 듯 한데,
그래도 앞으로 기능 추가 할 때 놓친 부분은 없는지 꼭 확인해주세요.

이런 버그를 방지하기 위해 어떻게 해볼 수 있는지도 한 번 고민하고 같이 얘기해볼까요?
예를 들어 opposite 의 구현 방식을 바꾸는 방법이 있을 것 같아요.
(없어지겠지만 그래도 고민해보아요.)

해당 코멘트와도 연관이 있는 내용이라 대댓글로 남깁니다.

domain 패키지의 테스트 커버리지를 높여보면 어떨까요?
현재 public 으로 열려있는 메서드라도 테스트가 없는 경우가 있는데,
domain 은 특히나 테스트가 중요한 패키지거든요.

pkeugine · 2026-03-13T16:34:26Z

src/main/java/domain/card/CardSuitMap.java

 public final class CardSuitMap {
    private static final Map<Integer, String> SUIT_MAP = Map.of(
            0, "스페이드",
            1, "하트",
            2, "다이아몬드",
            3, "클로버"
    );


#933 (comment)

어느 순간 좋았던 부분이 사라지고 리뷰했던 지양해야 할 방식이 적용되었네요 🤔
이전 PR 에서 반영된 부분이긴 한데, 지금 발견했어요.

현미밥이 직접 지우신 것이 아니라면 무언가 코드에 변화를 줄 때 꼭 본인이 다 파악하고 반영해주세요.

그리고 private 생성자가 필요한 곳이 다른 곳에도 있는지 프로젝트 전체를 한 번 확인해보고, 추가해보죠!

해당 부분은 제가 직접 삭제한 코드입니다..! 지식이 부족해 발생했습니다..

CardSuitMap이 static 메서드만 가지는 유틸리티 클래스이기 때문에 객체 생성 없이 사용해야 한다는 점을 몰랐습니다. private CardSuitMap() 생성자가 사용되지 않는 코드라고 생각해 리팩토링 과정에서 삭제했습니다.

피드백 이후 직접 객체 생성을 시도해 보며 확인해보니, 생성자가 public 상태에서는 new CardSuitMap()이 가능해 의미 없는 객체가 생성될 수 있다는 점을 알게 되었습니다. 유틸리티 클래스에서는 이러한 객체 생성을 막기 위해 private 생성자를 두어야 한다는 것도 함께 이해했습니다!! 다시 private CardSuitMap()을 추가해 객체 생성을 막도록 수정했습니다. (a4ed4b0 커밋) 좋은 피드백 감사합니다!

pkeugine · 2026-03-13T16:37:02Z

src/main/java/domain/participant/Money.java

+    @Override
+    public String toString() {
+        return String.valueOf(amount);
+    }


Money 클래스의 toString() 은 사용처가 보이지 않아요.
intelliJ 내에서 사용되지 않는다는 표시가 나타나지는 않지만, 제가 봤을 땐 불필요한 코드네요.

해당 코드를 넣어주신 이유가 궁금합니다.
만약 의도하지 않았다면 제거해주세요.

더 나아가 toString 은 어떤 경우에 override 해서 사용하면 좋을지 고민해보죠.

pkeugine · 2026-03-13T17:05:01Z

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

-public class Deck {
-    private final List<Card> cards;
-
+public record Deck(List<Card> cards) {


Deck 도 record로 바꾸셨군요?

여기서도 재밌는 주제가 있는데요,
현재 덱 생성을 위해 필요한 List<Card> cards 는 덱이 만들어진 이후에도 외부에서 값을 조작할 수 있어요.
이를 방지 할 수 있는 방법은 없을까요?

(꼭 도입하지는 않아도 되지만, 도움이 될 주제예요.)

pkeugine · 2026-03-13T17:11:18Z

src/main/java/domain/participant/Hand.java

    public Score calculateScore() {
-        int totalScore = 0;
-        int aceCount = 0;
-        for (Card card : cards) {
-            totalScore += card.getScore();
-            if (card.isAce()) {
-                aceCount++;
-            }
-        }
-
+        int totalScore = cards.stream()
+                .mapToInt(Card::getScore)
+                .sum();
+        int aceCount = (int) cards.stream()
+                .filter(Card::isAce)
+                .count();
        while (canLowerAceScore(totalScore, aceCount)) {
            totalScore -= 10;
            aceCount--;
        }


Q. Hand 의 책임이 과도하다고 느껴지는데, 분리하는 것이 좋을지

저는 Hand가 점수를 계산하는 방향이 꽤 자연스럽다고 느꼈어요.
앞서 재료와 요리에 대한 비유가 여기에도 적용될 듯 한데요,
재료를 가지고 있는 객체가 요리를 다 하는게 맞다고 생각합니다.

util 성 로직이 아니라면 굳이 그 재료를 다른 클래스로 옮길 필요가 없다고 생각해요.
여기서 util 성 로직이란, 라이브러리로 출시해도 될만큼 범용적인 것들이라고 저는 봐요.
예를 들어... 사용자 이름 안에서 ㄱ 의 개수를 세어주는 로직 같은거요 ㅋㅋㅋ

점수가 결국 손패 자체에서만 결정되니까, 계산기를 따로 두기보다 Hand가 자기 상태를 설명하는 쪽이 지금 미션 범위에서는 더 응집도 있게 읽히는 것 같네요. 지금이 딱 좋아요 👍

pkeugine · 2026-03-13T17:17:06Z

src/main/java/util/NameParser.java

+        return Arrays.stream(input.split(","))
                .map(String::trim)
-                .map(Player::new)
                .collect(Collectors.toList());


Suggested change

.collect(Collectors.toList());

.toList();

어떤 차이가 있는지 한 번 알아볼까요?

pkeugine · 2026-03-13T17:21:54Z

src/main/java/util/NameParser.java

 import java.util.List;
 import java.util.stream.Collectors;

 public class NameParser {


이건 개인적인 궁금증인데, 중복 이름에 대한 제한에 대해 현미밥의 생각은 어떤가요?
같은 이름을 허용하면 좋을지, 아니면 막는게 좋을지 한 번 고민해보죠!

만약 같은 이름의 player 가 있을 수 있다면, 그 둘을 게임상에서 어떻게 구분할 수 있을지도 고민해보면 좋겠어요. 그래야 누가 이기고, 비기고, 졌는지에 따라 (지금은 실제로 돈을 분배하진 않지만) 실제 이득과 손해를 구분할 수 있을테니까요.

가장 쉬운 방법은 그냥 중복 이름을 막는거겠네요 ㅋㅋㅋ

pkeugine · 2026-03-13T17:25:34Z

src/main/java/domain/result/Outcome.java

+    private static Outcome decideByScore(Score playerScore, Score dealerScore) {
+        if (playerScore.isBust()) {
+            return LOSE;
+        }
+        if (dealerScore.isBust()) {
+            return WIN;
+        }
+        if (playerScore.getGameScore() > dealerScore.getGameScore()) {
+            return WIN;
+        }
+        if (playerScore.getGameScore() < dealerScore.getGameScore()) {
+            return LOSE;
+        }
+        return DRAW;
+    }


#933 (comment)

메서드 길이가 10줄이 넘는 메서드들이 조금씩 보이는데,
아무래도 요구사항이 있다보니 한 번 지켜보는건 어떨까요?

(빈줄 포함 10줄은 해당 코멘트가 말하는 '10줄이 넘는 메서드'에 해당하지 않습니다.)

gahyeonnni added 27 commits March 13, 2026 18:04

refactor(domain): Card 클래스를 record로 전환

1da503b

feat(domain): 베팅 금액 관리를 위한 Money 추가

cc058b4

feat(dto): 베팅 금액 관리를 위한 MoneyDTO 추가

b85a897

feat(domain): 덱 생성 책임을 분리

2bd3557

test(domain): 승패 결과와 베팅 금액 검증을 위한 테스트 코드 추가

904efb2

refactor(domain): CardSuitMap 패키지 구조 변경

ff51b03

refactor(domain): Dealer 클래스 패키지 구조 변경

1c569c5

refactor(domain): Hand 클래스 패키지 구조 변경 및 record로 전환

7540373

refactor(domain): Name 클래스 패키지 구조 변경 및 record로 전환

ac7eb2e

feat(domain): Outcome 클래스 패키지 구조 변경 및 블랙잭 승패 판별 및 배당률 로직 추가

d45e1dc

refactor(domain): Participant 클래스 패키지 구조 변경 및 블랙잭 확인, 카드 목록 반환 로직 구현

c181ba7

refactor(dto): Outcome 패키지 이동에 따른 import 경로 수정

268fe7b

refactor(message): 출력 메세지 추가

e0e7145

feat(dto): 도메인 객체 변환을 위한 정적 팩토리 메서드 추가

569ebfc

feat(domain): Player에 배팅 금액 속성 추가 및 블랙잭 시 드로우 방지 조건 추가

9a30ff3

refactor(domain): Players 클래스를 record로 전환하고 이름 출력 로직 추가

21d97c6

refactor(controller): 게임 진행 흐름 개선 및 배팅 로직 추가

d74b589

refactor(domain): Deck 클래스 패키지 구조 변경 및 record로 전환

fa2598d

refactor(domain): GameResult 클래스 패키지 구조 변경

9dd0c9f

refactor(view): 베팅 금액 입력 로직 추가

e2c573f

refactor(view): 베팅 금액 출력 로직 추가

3e9e6c8

refactor(util): NameParser의 반환 타입을 변경

fa98d95

refactor(domain): Score 클래스 패키지 구조 변경 및 record로 전환

a4266b0

test(util): NameParser 반환 타입 변경에 맞춰 테스트 코드 수정

4ff24ce

test(domain): 클래스 패키지 이동에 따른 HandTest import 경로 수정

d7945e4

test(domain): 클래스 패키지 이동에 따른 ScoreTest import 경로 수정

2e9fd10

test(domain): Player 생성자 변경 및 Hand 메서드 수정사항 반영

fcc7d80

pkeugine requested changes Mar 13, 2026

View reviewed changes

refactor(domain): CardSuitMap에 private 생성자 추가

a4ed4b0

Conversation

gahyeonnni commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

체크 리스트

어떤 부분에 집중하여 리뷰해야 할까요?

Uh oh!

pkeugine 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!

pkeugine Mar 13, 2026 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gahyeonnni commented Mar 13, 2026 •

edited

Loading

pkeugine Mar 13, 2026 •

edited

Loading