[🚀 사이클2 - 미션 (블랙잭 게임 실행)] 현미밥 미션 제출합니다.#1112
[🚀 사이클2 - 미션 (블랙잭 게임 실행)] 현미밥 미션 제출합니다.#1112gahyeonnni wants to merge 59 commits intowoowacourse:gahyeonnnifrom
Conversation
pkeugine
left a comment
There was a problem hiding this comment.
안녕하세요 현미밥, 미션 진행하시느라 고생 많으셨습니다.
이번 리뷰는... 조금 냉정하게 리뷰했어요.
개인적으로 현미밥의 사이클 1 구현이 정말 좋았기 때문에
이번에 아쉽다고 느낀 부분들을 좀 강하게 강조해봤습니다.
하지만 기능적 요구사항은 모두 충족한 점은 정말 좋았습니다. 👍
리뷰를 하다보면 버그들을 발견하곤 하는데,
현재 현미밥 애플리케이션에서는 요구사항에 부합하지 않는 버그는 없는 듯 하네요.
(제가 못 발견했을 수도 있는데, 그만큼 잘했다는 거겠죠 ㅋㅋㅋ)
다시 강조한 내용들로 돌아가보자면
제가 이번에 강조하고 싶은 점은 다음과 같습니다 :
- 코드가 현미밥이 작성한 것인지 조금 의문이 생깁니다. 너무 강하게 말하는건가 싶기도 한데, 본인이 작성했다면 발생하기 힘든 부분들이 몇 가지 보였거든요. 저희가 리뷰했던 내용들도 있고요. 이런 부분들을 잘 챙겨주셔야 앞으로 크루-리뷰어 관계가 아닌 팀원-팀원 간의 리뷰 할 때 현미밥이 더 훌륭한 퍼포먼스를 보여주실 듯 해서 이번에 제 생각을 쿠션어 없이 드러내봤어요. 이 부분, 정말 중요해요. 꼭 잘 챙겨봅시다.
- 코드 전반에 걸쳐 테스트 커버리지를 높여봤으면 합니다. 현재 요구사항에 부합하지 않는 버그는 없더라도, 예상하지 못한 동작이나 버그에 취약한 구조가 몇 가지 보였거든요. 테스트가 있다면 이를 사전에 방지 할 수 있지 않나 싶어요!
리뷰 확인해보시고, 궁금증이나 대화하고 싶은 내용이 있다면 부담 갖지 말고 DM 또는 코멘트 남겨주세요. 제가 잘못 파악한 내용이 있거나 동의하지 않는 내용이 있다면 현미밥도 제게 강하게 반박하셔도 좋습니다. :)
남은 사이클 같이 화이팅해보죠.
해피해킹하세요!!! 🙂 🙂 🙂
| @@ -1,24 +1,20 @@ | |||
| package controller; | |||
There was a problem hiding this comment.
README가 아직 사이클1 체크리스트 중심이라 이번 사이클에서 추가된 배팅 규칙이 잘 안 보이네요.
문서 관리도 꽤나 중요한 일이니, 한 번 업데이트 해볼까요?
There was a problem hiding this comment.
이번 사이클의 실제 출력 흐름에서는 GameResult가 더 이상 사용되지 않네요.
사용되지 않는 코드는 제거해보면 어떨까요?
코드베이스가 커지고, 프로젝트 아키텍쳐가 복잡해지면 intelliJ 가 '사용되지 않음' 으로 인식하는 코드가 있더라도 실제로 사용되는 경우가 종종 있어요. 그 상황에서는 코드를 제거했다가는 예상치 못한 사이드 이펙트가 생길 수 있어서 마음 편히 코드를 제거하기가 어려워지는데요,
그렇기 때문에 더더욱 개발하는 과정 안에서 불필요한 코드를 삭제하고,
이렇게 규모가 너무 크지 않아서 코드 사용처를 바로 확인할 수 있을 때
관리해주는 습관을 가져보면 좋아요 :)
There was a problem hiding this comment.
리뷰를 하며 전체적으로 사용되지 않는 클래스들, 로직, 그리고 상수까지 다양하게 보여요.
프로젝트 전반적으로 다 확인해보고 불필요한 것들은 제거해주세요.
There was a problem hiding this comment.
사용되지 않는 클래스나 로직들을 다시 확인하고 정리하겠습니다!
| public Outcome opposite() { | ||
| if (this == WIN) { | ||
| return LOSE; | ||
| } | ||
| if (this == LOSE) { | ||
| return WIN; | ||
| } | ||
| return DRAW; | ||
| } |
There was a problem hiding this comment.
지금은 opposite()를 쓰는 경로가 죽어 있어서 버그가 드러나지 않지만, BLACKJACK_WIN.opposite()가 DRAW로 계산되네요.
위의 코멘트를 반영하면 해당 메서드도 삭제될 듯 한데,
그래도 앞으로 기능 추가 할 때 놓친 부분은 없는지 꼭 확인해주세요.
이런 버그를 방지하기 위해 어떻게 해볼 수 있는지도 한 번 고민하고 같이 얘기해볼까요?
예를 들어 opposite 의 구현 방식을 바꾸는 방법이 있을 것 같아요.
(없어지겠지만 그래도 고민해보아요.)
There was a problem hiding this comment.
해당 코멘트와도 연관이 있는 내용이라 대댓글로 남깁니다.
domain 패키지의 테스트 커버리지를 높여보면 어떨까요?
현재 public 으로 열려있는 메서드라도 테스트가 없는 경우가 있는데,
domain 은 특히나 테스트가 중요한 패키지거든요.
There was a problem hiding this comment.
Dealer와 Player Outcome Enum 간의 관계를 정의하는 방식으로
opposite() 메서드 대신 Opposite이라는 Enum을 별도로 두는 방법을 생각해 보았습니다.
현재 opposite()는 WIN / LOSE만을 if 조건으로 분기하고 있기 때문에
BLACKJACK_WIN과 같은 새로운 Outcome이 추가되면 분기에 포함되지 않아 누락되어 의도하지 않은 값이 반환될 가능성이 있습니다.
실제로 현재 구현에서도 BLACKJACK_WIN.opposite()가 DRAW로 계산되는 문제가 있습니다.
이를 방지하기 위해 Player의 Outcome을 key로 하고 Dealer의 Outcome을 value로 가지는 관계를 Opposite Enum에서 정의하는 방식을 생각해 보았습니다. (ex. <WIN, LOSE>, <LOSE, WIN>, <DRAW, DRAW>)
다만 블랙잭의 경우에는 딜러의 블랙잭 여부에 따라 결과가 달라지기 때문에
(예를 들어 Player가 BLACKJACK인 경우 Dealer도 BLACKJACK이면 DRAW가 되고, Dealer가 BLACKJACK이 아니라면 Player는 BLACKJACK_WIN으로 1.5배의 상금을 받게 되는 경우)
BLACKJACK과 관련된 결과는 Opposite Enum의 관계 정의와는 별도로,
Dealer의 블랙잭 여부를 확인하는 로직을 메서드로 분리하여 처리하면 될 것 같습니다.
이렇게 구현한다면 새로운 Outcome이 추가될 때
관계를 함께 정의하게 되어 누락 가능성을 줄일 수 있을 것 같습니다.
There was a problem hiding this comment.
현미밥이 문제를 바라본 관점은 정말 좋다고 생각해요.
말씀해주신 것처럼 opposite() 방식은 enum 값이 늘어날 때 누락되기 쉬운 구조라, 그 위험을 짚어낸 점이 좋았습니다.
다만 제 생각에는 Opposite enum을 따로 두는 방식은 조금 무겁게 느껴지기도 해요.
특히 BLACKJACK_WIN처럼 단순히 “반대 결과”로 대응시키기 어려운 값이 들어오면, 결국 그 enum 안에서도 다시 예외적인 문맥 처리가 필요해지거든요.
그러면 복잡도가 줄기보다는 다른 위치로 이동하는 느낌이 들 수 있습니다.
그래서 Opposite enum까지 따로 두기보다는, 먼저 "정말 opposite이라는 개념이 필요한가?"를 생각해보면 좋겠습니다. 지금처럼 결과를 계산하는 로직에서는 플레이어와 딜러의 상태를 함께 알고 있으니, 그 자리에서 바로 승/패/무를 결정하는 쪽이 더 자연스러워 보여요. 굳이 enum 하나를 다른 enum으로 일반 변환하는 메서드를 두기 시작하면, BLACKJACK_WIN처럼 단순히 반대값으로 대응되지 않는 케이스에서 다시 예외 처리가 필요해질 수 있거든요.
만약 그래도 이런 변환 메서드를 유지하고 싶다면, WIN/LOSE만 처리하고 나머지를 묵시적으로 DRAW로 보내는 방식보다는, 모든 enum 값을 명시적으로 다루도록 구현하는 편이 더 안전 할 듯 하네요. 그리고 그 경우에는 enum 값이 추가되더라도 누락이 바로 드러날 수 있도록 테스트도 함께 고정해두면 좋겠어요.
그래도 이런 식으로 “왜 이 버그가 생겼는지”, “앞으로 어떤 구조가 덜 위험한지”까지 고민해본 건 정말 좋은 접근이었습니다.
이런 고민들이 쌓이면 리팩터링할 때 훨씬 단단한 판단을 하게 되더라고요 :)
| public final class CardSuitMap { | ||
| private static final Map<Integer, String> SUIT_MAP = Map.of( | ||
| 0, "스페이드", | ||
| 1, "하트", | ||
| 2, "다이아몬드", | ||
| 3, "클로버" | ||
| ); |
There was a problem hiding this comment.
어느 순간 좋았던 부분이 사라지고 리뷰했던 지양해야 할 방식이 적용되었네요 🤔
이전 PR 에서 반영된 부분이긴 한데, 지금 발견했어요.
현미밥이 직접 지우신 것이 아니라면 무언가 코드에 변화를 줄 때 꼭 본인이 다 파악하고 반영해주세요.
There was a problem hiding this comment.
그리고 private 생성자가 필요한 곳이 다른 곳에도 있는지 프로젝트 전체를 한 번 확인해보고, 추가해보죠!
There was a problem hiding this comment.
해당 부분은 제가 직접 삭제한 코드입니다..! 지식이 부족해 발생했습니다..
CardSuitMap이 static 메서드만 가지는 유틸리티 클래스이기 때문에 객체 생성 없이 사용해야 한다는 점을 몰랐습니다. private CardSuitMap() 생성자가 사용되지 않는 코드라고 생각해 리팩토링 과정에서 삭제했습니다.
피드백 이후 직접 객체 생성을 시도해 보며 확인해보니, 생성자가 public 상태에서는 new CardSuitMap()이 가능해 의미 없는 객체가 생성될 수 있다는 점을 알게 되었습니다. 유틸리티 클래스에서는 이러한 객체 생성을 막기 위해 private 생성자를 두어야 한다는 것도 함께 이해했습니다!! 다시 private CardSuitMap()을 추가해 객체 생성을 막도록 수정했습니다. (a4ed4b0 커밋) 좋은 피드백 감사합니다!
There was a problem hiding this comment.
아하 그렇군요~
괜찮아요, 이제 잘 기억하고 있으면 됩니다.
앞으로는 조심해보죠! 👍
| @Override | ||
| public String toString() { | ||
| return String.valueOf(amount); | ||
| } |
There was a problem hiding this comment.
Money 클래스의 toString() 은 사용처가 보이지 않아요.
intelliJ 내에서 사용되지 않는다는 표시가 나타나지는 않지만, 제가 봤을 땐 불필요한 코드네요.
해당 코드를 넣어주신 이유가 궁금합니다.
만약 의도하지 않았다면 제거해주세요.
더 나아가 toString 은 어떤 경우에 override 해서 사용하면 좋을지 고민해보죠.
There was a problem hiding this comment.
삭제했습니다!
toString()을 언제 override해서 사용하는 것이 좋을지도 고민해 보았습니다. System.out.println 등을 통해 객체 내부에 어떤 값이 들어있는지 확인할 수 있도록 디버깅할 때 사용할 수 있다고 생각했습니다!
혹시 toString()을 사용하는 다른 사례가 있다면 알려주시면 더 공부해보겠습니다!
There was a problem hiding this comment.
네, 저도 보통 toString()은 디버깅이나 로깅처럼 객체 상태를 사람이 읽기 쉽게 확인할 때 주로 사용한다고 생각해요.
예를 들면 println으로 빠르게 값을 확인하거나,
로그/예외 메시지/테스트 실패 상황에서 객체 내용을 보기 좋게 드러내고 싶을 때요.
잘 고민해주셨네요 👍
src/main/java/domain/card/Deck.java
Outdated
| public class Deck { | ||
| private final List<Card> cards; | ||
|
|
||
| public record Deck(List<Card> cards) { |
There was a problem hiding this comment.
Deck 도 record로 바꾸셨군요?
여기서도 재밌는 주제가 있는데요,
현재 덱 생성을 위해 필요한 List<Card> cards 는 덱이 만들어진 이후에도 외부에서 값을 조작할 수 있어요.
이를 방지 할 수 있는 방법은 없을까요?
(꼭 도입하지는 않아도 되지만, 도움이 될 주제예요.)
There was a problem hiding this comment.
처음에는 List.copyOf(cards)를 사용하여 불변 리스트로 만들어 방어적 복사를 적용하려고 했습니다. 하지만 draw() 과정에서 removeFirst()로 카드가 제거되며 내부 상태가 변경되기 때문에 불변 리스트를 사용할 수 없었습니다.
그래서 Deck은 record 대신 class로 변경하고, 생성자에서 new ArrayList<>(cards)를 사용하여 방어적 복사를 적용했습니다. 외부에서 전달된 리스트가 이후에 수정되더라도 Deck 내부 상태에는 영향을 주지 않도록 수정했습니다!
| 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--; | ||
| } |
There was a problem hiding this comment.
Q. Hand 의 책임이 과도하다고 느껴지는데, 분리하는 것이 좋을지
저는 Hand가 점수를 계산하는 방향이 꽤 자연스럽다고 느꼈어요.
앞서 재료와 요리에 대한 비유가 여기에도 적용될 듯 한데요,
재료를 가지고 있는 객체가 요리를 다 하는게 맞다고 생각합니다.
util 성 로직이 아니라면 굳이 그 재료를 다른 클래스로 옮길 필요가 없다고 생각해요.
여기서 util 성 로직이란, 라이브러리로 출시해도 될만큼 범용적인 것들이라고 저는 봐요.
예를 들어... 사용자 이름 안에서 ㄱ 의 개수를 세어주는 로직 같은거요 ㅋㅋㅋ
점수가 결국 손패 자체에서만 결정되니까, 계산기를 따로 두기보다 Hand가 자기 상태를 설명하는 쪽이 지금 미션 범위에서는 더 응집도 있게 읽히는 것 같네요. 지금이 딱 좋아요 👍
src/main/java/util/NameParser.java
Outdated
| return Arrays.stream(input.split(",")) | ||
| .map(String::trim) | ||
| .map(Player::new) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
| .collect(Collectors.toList()); | |
| .toList(); |
어떤 차이가 있는지 한 번 알아볼까요?
There was a problem hiding this comment.
.collect(Collectors.toList())는 수정 가능한 List를 반환하고,
.toList()는 수정 불가능한 List를 반환합니다.
이름 목록은 외부에서 변경될 필요가 없기 때문에
불변 List를 반환하는 .toList()로 변경했습니다!
| import java.util.List; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class NameParser { |
There was a problem hiding this comment.
이건 개인적인 궁금증인데, 중복 이름에 대한 제한에 대해 현미밥의 생각은 어떤가요?
같은 이름을 허용하면 좋을지, 아니면 막는게 좋을지 한 번 고민해보죠!
만약 같은 이름의 player 가 있을 수 있다면, 그 둘을 게임상에서 어떻게 구분할 수 있을지도 고민해보면 좋겠어요. 그래야 누가 이기고, 비기고, 졌는지에 따라 (지금은 실제로 돈을 분배하진 않지만) 실제 이득과 손해를 구분할 수 있을테니까요.
가장 쉬운 방법은 그냥 중복 이름을 막는거겠네요 ㅋㅋㅋ
There was a problem hiding this comment.
중복된 이름이 있을 경우 플레이어를 구분하기 어려울 것 같습니다.
따라서 makeNameList() 메서드에서 HashSet을 이용해 중복된 이름이 존재하는지 검증하도록 구현했습니다.
public static List<String> makeNameList(String input) {
List<String> names = Arrays.stream(input.split(","))
.map(String::trim)
.toList();
Set<String> uniqueNames = new HashSet<>(names);
if (names.size() != uniqueNames.size()) {
throw new IllegalArgumentException("중복된 이름은 허용하지 않습니다.");
}
return names;
}| 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; | ||
| } |
There was a problem hiding this comment.
메서드 길이가 10줄이 넘는 메서드들이 조금씩 보이는데,
아무래도 요구사항이 있다보니 한 번 지켜보는건 어떨까요?
(빈줄 포함 10줄은 해당 코멘트가 말하는 '10줄이 넘는 메서드'에 해당하지 않습니다.)
pkeugine
left a comment
There was a problem hiding this comment.
안녕하세요 현미밥~
피드백 잘 적용해주셨네요.
정말 고생 많았어요. 💯 💯
이제 슬슬 merge 가능한 것 같은데,
몇 가지 간단한 리뷰 추가로 남겼으니 한 번 확인해주세요.
필수로 적용하지 않아도 되는 것들도 있으니 가볍게 봐주시면 될 듯 합니다.
다 확인하고 난 뒤, 현미밥이 이제 마무리 해도 되겠다 싶으면 다시 리뷰 요청 주세요.
그 때 merge 하도록 하겠습니다.
궁금한 점이나 더 얘기 나눠보고 싶은 것들이 있다면 언제든 DM 주시고,
해핑해킹하세요~ ⭐️ ⭐️ ⭐️
| public String getFirstName() { | ||
| return getCardNames().getFirst(); | ||
| } |
There was a problem hiding this comment.
보통 getFirstName 이라고 하면 사람의 이름을 나타내는 경우가 많은 것 같아서
해당 메서드명을 첫번째 카드의 이름이라고 구체적으로 나타내보면 어떨까요?
Dealer 쪽에서 한 것처럼 똑같이 바꿔주면 될 듯 해요.
| @DisplayName("이름이 null이거나 비어있으면 예외가 발생한다.") | ||
| @ParameterizedTest | ||
| @NullAndEmptySource | ||
| void throwExceptionWhenNameIsNullOrEmpty(String invalidName) { | ||
| assertThatThrownBy(() -> new Name(invalidName)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessage("이름은 비어 있거나 공백만 있을 수 없습니다."); | ||
| } |
There was a problem hiding this comment.
테스트 잘 만들어주셨네요 👍
더 나아가 " " 같은 케이스를 하나 더 넣어두면 테스트가 코드 의도를 더 정확하게 설명해줄 것 같아요.
| public Money calculateProfit(Outcome outcome) { | ||
| return this.money.multiply(outcome.getRate()); | ||
| } |
There was a problem hiding this comment.
이전에는 controller가 돈 계산을 더 많이 들고 있었는데,
지금은 Player가 자기 수익 계산을 맡도록 바뀐 점 좋네요 👍
| ASK_MORE_CARD("는 한장의 카드를 더 받겠습니까?(예는 y, 아니오는 n)"), | ||
| DEALER_ONE_CARD("딜러는 16이하라 한장의 카드를 더 받았습니다."), | ||
| WINNING_STATISTICS("## 최종 승패"); | ||
| WINNING_STATISTICS("## 최종 승패"), |
There was a problem hiding this comment.
이번 라운드에서 사용되지 않는 코드들을 많이 잘 정리해주셨는데,
WINNING_STATISTICS는 지금 출력 흐름에서는 더 이상 쓰이지 않는 것 같네요.
이것도 지워주죠!
| @@ -1,10 +1,12 @@ | |||
| package message; | |||
|
|
|||
| public enum IOMessage { | |||
There was a problem hiding this comment.
IOMessage 도 우리 컨벤션에 따라 IoMessage 로 바꿔볼 수도 있고,
어색하다면 ViewMessage 또는 TuiMessage, TerminalMessage 같은 이름도 괜찮을 듯 해요.
| List<Card> cards = IntStream.range(0, 52) | ||
| .mapToObj(Card::new) | ||
| .toList(); | ||
|
|
||
| return new Deck(new ArrayList<>(cards)); |
There was a problem hiding this comment.
지금은 Deck 생성자에서 이미 한 번 방어적 복사를 하고 있어서,
여기의 new ArrayList<>(cards)는 한 번 더 복사하는 셈이에요.
동작상 문제는 없지만, "복사의 책임을 어디에 둘지"를 한 곳으로 모아두면 코드를 읽을 때 조금 더 명확해질 수도 있을 것 같아요.
| public Deck(List<Card> cards) { | ||
| this.cards = cards; | ||
| this.cards = new ArrayList<>(cards); | ||
| shuffle(); |
There was a problem hiding this comment.
이번에 반영된 내용은 아니지만, 생각해볼 요소로 남겨보는 리뷰입니다.
생성자에서 바로 shuffle()까지 해주는 구조는 실제 게임 흐름에서는 자연스러운데,
테스트에서는 조금 덜 예측 가능하게 만들기도 하더라구요.
나중에 테스트를 더 촘촘하게 가져가고 싶어지면,
섞기 책임을 분리하면 어떤 점이 좋아질지도 한 번 생각해보면 재밌을 것 같습니다 ㅋㅋㅋ
| public Hand getHand() { | ||
| return new Hand(hand.cards()); | ||
| } |
| @@ -1 +1 @@ | |||
| # java-blackjack | |||
There was a problem hiding this comment.
commit message 컨벤션을 한 번 잘못 적은 건 죄송할 일은 전혀 아니에요 :)
나중에 기회가 되면 git commit --amend, rebase -i, fixup 정도를 같이 익혀보면 commit 메시지 수정이나 commit 정리할 때 꽤 도움이 될 거예요.
참고로 해당 기능들을 활용하면,
지금 당장 해당 PR 의 커밋 메시지들도 수정 가능합니다.
당장 적용해보자는 말은 아니고 git 사용이 익숙해진다면 그 이후 미션부터 한 번 활용해보세요 👍

체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?어떤 부분에 집중하여 리뷰해야 할까요?
Hand가 카드를 저장하는 역할뿐만 아니라, canLowerAceScore 메서드처럼 블랙잭의 구체적인 점수 계산 규칙까지 너무 자세히 알고 있다는 느낌을 받았습니다. 데이터를 가진 Hand에서 직접 계산하는 것이 응집도 측면에서는 좋겠지만, 책임이 과도하다고 느껴집니다. 이 로직을 그대로 Hand에 두는 것이 맞을까요, 아니면 별도의 객체(계산기)로 분리하는 것이 더 나은 설계일까요? (7540373 커밋입니다!)
감사합니다☺️