Skip to content

[🚀 사이클2 - 미션 (블랙잭 게임 실행)] 테리 미션 제출합니다.#1128

Open
Yeji-Kim-Erica wants to merge 20 commits intowoowacourse:yeji-kim-ericafrom
Yeji-Kim-Erica:step2
Open

[🚀 사이클2 - 미션 (블랙잭 게임 실행)] 테리 미션 제출합니다.#1128
Yeji-Kim-Erica wants to merge 20 commits intowoowacourse:yeji-kim-ericafrom
Yeji-Kim-Erica:step2

Conversation

@Yeji-Kim-Erica
Copy link

@Yeji-Kim-Erica Yeji-Kim-Erica commented Mar 14, 2026

💌 인사말

안녕하세요, PK! 리뷰이 테리입니다.

우선 지난 PR 때의 상세한 코멘트들에 대해 감사드린다는 말씀을 드리고 싶습니다!

지난 사이클에서 주셨던 피드백과 도전 목표가 많은 도움이 되었습니다.

1. 최대한 적은 범위의 코드 변경을 통해 기능 추가 해보기
2. 테스트 커버리지를 높이기

우선 제가 생각하기에 많이 발전한 부분부터 말씀을 드려볼까 해요.
2번 항목을 어떻게 하면 잘 수행할 수 있을지 고민해봤는데,
README의 기능 목록 문서를 꼼꼼히 작성하는 것이 가장 우선적으로 행해져야 한다는 생각이 들었습니다.
첫번째 PR 당시, 클래스 구조 위주의 기능 목록을 base로 작성을 시작했더니
클래스의 구조가 계속해서 변경되면서 목록 관리가 되지 않았고,
이로 인해 엣지 케이스 커버는 커녕 기능이 제대로 구현되고 있는 건지도 파악하기 힘들었던 경험이 있는데요.
사전에 목록을 상세히 작성하니, 엣지 케이스를 상상해보는 과정을 통해서 서비스에 필요할 것 같은 기능을 구상하기 쉬워졌습니다.
설계 부분에 있어서 가장 깊이 와닿았던 것은,
지난 사이클 때 알려주신 "사용자의 입력 오류 처리와 도메인 규칙의 검증이 구분되어야 한다"는 부분입니다.
기능 목록에 이를 반영해 수정하는 과정에서 각 객체의 책임이 무엇인지를 더욱 명확하게 고민해볼 수 있었던 것 같습니다.
테스트 해봐야 하는 시나리오들을 미리 정리해두고 구현을 시작하니 확실히 TDD의 의의가 무엇인지 느낄 수 있었습니다.
기능을 추가하거나 수정하는 과정에서 아무리 코드가 변경되어도,
해당 도메인의 검증 테스트 덕분에 안심하고 코드를 작성할 수 있었던 경험이 인상적이었습니다.

(+ 실제로 '딜러의 점수가 16점 이하인 상태로 게임이 종료되면 안된다.'는 엣지 케이스를 기능 목록에 추가하고, 테스트 코드와 예외 발생 로직을 작성했는데, 덕분에 나중에 코드가 꼬이면서 딜러가 16점에서 한장 외에는 더 받아오지 않는 컨트롤러 흐름 오류도 잡을 수 있었습니다!! 중요성을 체감할 수 있었던 순간입니다!)

제가 조금 아쉽게 수행했다고 느끼는 부분은 1번 항목인데요.
제시해주셨던 '최대한 적은 범위의 코드 변경'을 제가 잘 수행한 것인지 모르겠습니다.
제가 해당 부분에 대한 이해가 아직 부족한 것 같기도 하네요.
이번 사이클에서 사용자마다 이름을 입력 받은 뒤 배팅 금액을 입력 받는다는 내용이 추가되면서,
Controller에서 검증 흐름을 돌리고 Player, Players가 받아오는 파라미터 인자가 많이 변하게 되었는데
기능 추가로 인해 어쩔 수 없이 변경된 '배팅 금액' 부분을 제외하더라도,
기존에 String으로 받아오던 이름 부분을 객체로 받아오도록 했으니..
해당 도전의 기준에 맞는 적절한 범위에서의 수정이 맞는지 확신이 가질 않습니다. (아마 이 외에도 수정한 사항이 있을 거 같기도 하고요..)
하지만, 이전 사이클의 리팩토링 때만큼의 대격변이 없었다는 점을 고려한다면,
이전에 비해서는 코드 변경이 일어나는 범위와 규모가 굉장히 축소되었다고 느낍니다!

우선은 제 선에서 할 수 있는 것들을 최대한 해보았는데,
지금 저의 시야로는 볼 수 없었던 부분들을 또 한번 살펴봐 주시고 짚어주시길 부탁 드립니다.
다시 한번 최선을 다해 개선하고 공부하겠습니다. 🙇‍♂️

애정이 담긴 리뷰 정말 감사드리고, 이번 리뷰도 잘 부탁 드리겠습니다!!

체크 리스트

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

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

고민했던 부분

  • 컨트롤러에 도메인은 어느 정도까지 노출되어도 되는 걸까요?

게임의 초기 설정 부분 흐름이 기능 추가로 인해 많이 변했습니다.
이에 맞춰서 꽤나 많은 도메인이 현재 Controller에 노출되고 있는 상황인데요.
이렇게 산발적으로 분포해있는 형상이 정상적인 현상이라고 보아도 괜찮은지 여쭤보고 싶습니다.

  • 컨트롤러에서 검증 흐름이 결정되고, 검증 메서드가 호출되어도 괜찮은 건가요?

현재 Controller에서 Players 의 validatePlayersCount 정적 메서드를 호출하고 있습니다.
저는 이것이 객체지향적이 아니라 절차지향적인 코드가 작성된 것이라는 느낌이 드는데요.
(블랙잭 자체가 순차적 흐름을 가진 프로그램이고, Controller의 코드이니 어쩔 수 없는 건가, 하는 생각이 들기도 합니다.)
현재의 흐름을 바꿀 수 있는 구조를 다시 한번 고민해봐야 할지, 아니면 이런 현상이 자연스러운 것인지 여쭤보고 싶습니다.
또, 해당 흐름으로 인해 현재 Player 최대 인원수가 Controller에서 한번, Players 객체 생성 시에 한번.
이렇게 두번 검증이 되고 있는데, 이 문제를 해결할 방법이 도저히 떠오르질 않아서.. 좋은 힌트가 있다면 부탁드리고 싶습니다!

  • Game 클래스 에서 현재 인스턴스 변수를 3개 보유하고 있는데, 해당 문제를 해결하기 위해 시도해보면 좋을 방향이 무엇일까요?

처음에는 List<Participant> 로 Dealer와 Players 필드를 통합해보려 했으나,
둘의 성격이 다른 부분이 꽤 존재해서 구분이 필요하다고 느꼈습니다.
현재 Deck, Dealer, Players로 인스턴스 변수를 3개 가진 Game 구조 외에는 떠오르지 않는데,
'3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.' 라는 제약을 지킬 좋은 방법이 무엇일지..
직접적인 정답은 스스로 찾아야 한다면, 방향성에 대한 힌트가 될만한 말씀을 해주실 수 있는지 여쭤보고 싶습니다!

(동일하게 3개의 인스턴스 변수를 가지고 있던 Controller의 경우, CardShuffleStrategy는 상태로 갖지 않고 메서드 파라미터로 주입받도록 개선해 해결을 시도해보았습니다.)

  • 메서드 위임, 이대로 괜찮은 걸까요?

현재 Participant 클래스에서 Cards 클래스의 메서드들을 단순히 호출만 하는 식의 public 메서드들이 꽤 되는데,
이러한 위임 방식이 과도해지면 클래스들 간의 결합도가 너무 높아지는 것이 아닌가 하는 우려가 있습니다.
해당 부분을 좀 더 분리할 수 있는 방안을 고민해봐야 할까요?

  • 불변 객체 사용을 연습해보려고 하는데, 다음 리팩토링에서 해당 내용을 반영해봐도 괜찮을까요?

미션에서 그룹으로 토론하며 설계 규칙을 만들어보는 시간을 가졌는데요,
'모든 객체는 불변 객체로 만든다' 라는 규칙이 사이클 2에서 새롭게 생겼습니다.
사이클 2 진행 과정에서 타협안이 필요하다는 목소리가 나오면서 현재는 다수결로 사장된 규칙입니다만...
우테코의 교육 자료에도 언급되어 있던 내용이라.. 도전을 해보는 것이 좋을지 고민이 되어 여쭤봅니다.

- ErrorMessage enum을 삭제하고 예외 발생 지점으로 메시지 이동
- 에러 메시지에 [ERROR] 접두사 추가
- InputView의 readHitOrStand 반환 타입을 boolean으로 변경해 컨트롤러 의존성 완화
- 예외 테스트 시의 구체적인 메시지 검증 제거
- Main 코드 가독성 개선
- Controller 내 불필요하게 열려있던 public 메서드를 private으로 변경해 캡슐화 강화
- 각 도메인과 InputView에 산발적으로 존재하던 "[ERROR]" 접두사 문자열을 제거하고, OutputView에서 에러 메시지를 출력할 때 일괄적으로 포맷팅하도록 구조 개선
- 주요 도메인 용어(Blackjack, Bust, Hit/Stand) 정의 추가
- 추가된 배팅 기능을 구현하기 위한 세부 기능 목록 추가
- 사용자 입력값 유효성 검사와 도메인 규칙 검증의 책임을 명시적으로 분리
- 엣지 케이스 처리 정책 명시
- '정수 범위 초과' 예외를 도메인 규칙 검증에서 사용자 입력값 유효성 검사로 이동
- 데이터 타입 변환의 책임은 InputView에, 비즈니스 규칙은 도메인에 있도록 책임을 명확히 분리
- 코드 상에 구현되어 있는 Ace 카드의 유동적 점수 계산(1점 / 11점 판별) 로직을 기능 명세에 구체적으로 반영
- Ace 카드가 여러 장 포함될 경우의 한계(11점으로 계산 가능한 Ace는 최대 1장) 등 엣지 케이스 명시
- 코드와 기능 요구 사항 문서의 동기화를 통해 도메인 규칙의 가시성 확보
- 21점 이상일 때 카드를 추가할 수 없는 도메인 규칙 예외 발생 로직 및 검증 추가
- 경계값(21점 미만, 21점, 21점 초과)에 따른 상태 판정 로직 테스트 작성
- 카드 생성 private 메서드 도입으로 테스트 코드 중복 개선
- 이미 보유하고 있는 동일한 카드를 중복해서 추가하려는 경우의 예외 처리 명시
- 카드를 추가로 받을 때마다 사전 검증되어야 하는 공통 확인 사항을 명시적으로 목록화
- 예외 처리 및 상세 조건 등 하위 항목까지 체크박스를 부여해, 기능 구현 및 테스트 진행도를 더욱 세밀하게 확인할 수 있도록
문서 양식 개선
- 기존 Collections.unmodifiableList 의 'View' 방식에서 'Snapshot' 방식인
List.copyOf로 변경
- 원본 리스트의 변경이 도메인 객체 내부 리스트에 영향을 주지 않도록 원본과의 참조 연결을 완전히 차단
- 배팅 금액을 관리하는 BetMoney 객체 구현
- 0 이하의 금액 입력 시 예외 처리 로직 추가
- 배팅 금액이 공백인 경우 예외 처리
- 숫자가 아닌 문자를 입력한 경우 예외 처리
- 배팅 금액이 정수 범위를 초과하는 경우 예외 처리
- 컨트롤러에서 플레이어별로 배팅 금액을 입력받아 Player 객체를 생성하는 흐름 구현
- Player 생성 시 미리 검증된 Name과 BetMoney를 필수로 받도록 도메인 생성자 변경
- 도메인 시그니처 변경에 따라 Game, Players 및 연관 테스트 코드 전면 수정
- 블랙잭 상황 제외, 일반적인 승/무/패 상황의 배당률(1, 0, -1)을 적용한 플레이어 수익 계산 로직 구현
- 플레이어들의 수익 합산 결과를 바탕으로 딜러의 최종 수익을 계산하는 로직 추가
- 게임 종료 후 딜러와 플레이어의 최종 수익을 화면에 출력하는 로직 추가
처음 받은 2장의 카드가 21점일 경우(블랙잭) 배팅 금액의 1.5배 수익을 반환하는 로직 추가
- Map.copyOf() 사용 시 LinkedHashMap의 순서가 보장되지 않는 문제 해결
- 순서를 유지하면서 불변성을 보장하도록 Collections.unmodifiableMap()으로 변경
- Cards 내부에서 21점 이상일 때 카드를 추가하면 예외를 던져 게임 진행이 막히던 문제 해결
- 불필요해진 21점 초과 예외 검증 로직 제거 및 README 요구사항 동기화
- 게임의 최종 수익 정산 결과를 원시 타입 Long 대신 Profit 래퍼 객체로 포장
- BlackJackController가 필드로 가지던 CardShuffleStrategy를 메서드 파라미터로 분리해 컨트롤러
인스턴스 변수에서 제거
Copy link

@pkeugine pkeugine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 테리~
미션 정말 잘 진행해주셨고,
코드도 정말 읽기 편하네요 👍
README 꼼꼼하게 챙기고 테스트도 탄탄하게 작성해주셔서,
사실 지금 바로 머지해도 될 듯 합니다.

이번 리뷰에서 제가 중점으로 본 것들은 앞서 제가 사이클 1 마무리 하면서 제안한 두가지 부분인데요,
잘 해주셨습니다.

특히 불변을 바로 도입하지 않고, 일단 최소한의 범위로 변경해주신 뒤 의견을 물어보는 것까지 좋네요.
사실 리뷰어 입장에선 불변을 도입해서 완전 다른 프로젝트가 되어버리면,
오히려 이전 사이클과 비교해서 보기가 힘들어져서 리뷰하기 어렵거든요.
그만큼 리뷰 퀄리티도 떨어지고요.

과연 이번 미션에서 불변을 써보면 좋을지는
저는 안 써봐도 된다고 생각하는 편인데,
다른 사람들은 다 하는데 나만 안하면 불안하잖아요? ㅎㅎ
해보세요.
감안해서 리뷰해보도록 하겠습니다.

사실상 지금 머지해도 이상하지 않을 상황이니, 맘 편하게 하시고 언제든 리뷰 요청 주세요.
궁금한거 있거나 얘기할 내용이 있다면 언제든 DM 주시구요!
해피해킹하세요~~ 🏄‍♂️ 🏄‍♂️ 🏄‍♂️


콘솔 기반으로 즐길 수 있는 블랙잭 게임입니다.

## 💡 주요 용어

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기능 목록을 README에 세분화해두신 점 정말 좋네요 👍

사이클 2에서 "테스트 커버리지를 높여보자"는 목표를 실제 작업 방식으로 연결하신 느낌이 들어요.
리뷰어 입장에서도 어떤 케이스를 의식하며 구현하셨는지 바로 따라갈 수 있었습니다.

사례를 알려주셔서 그것도 재밌게 읽었습니다 ㅋㅋㅋ

Comment on lines 7 to 9
private final Deck totalDeck;
private final Dealer dealer;
private final Players players;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q. Game 클래스 에서 현재 인스턴스 변수를 3개 보유하고 있습니다.

이 제약을 그냥 넘기지 않고 실제로 지켜보려고 고민해보신 점 좋네요 👍

제 생각을 말해보자면
3개 이하 제약은 절대적인 목표라기보다, 객체를 더 작게 나누고 객체지향적으로 설계해보도록 유도하는 장치에 가깝다고 생각해요.

그래서 필드 수를 억지로 줄이려다 오히려 책임이 더 어색해진다면,
그때는 제약 자체보다 현재 구조가 왜 더 자연스러운지 설명할 수 있는 쪽이 더 중요한 것 같습니다.

즉, 숫자 자체를 맞추는 것보다
이 구조가 정말 가장 자연스러운지 한 번 더 점검해보는 정도로 접근해보셔도 좋을 듯 하네요.
자연스럽다고 생각이 든다면 유지하셔도 됩니다.

물론 요구사항을 지키는 것이 무엇보다 중요하지만,
이 주제는 테리가 충분히 많이 고민해보셨으니 지금 상태로 둬도 좋다고 봐요.

Comment on lines +35 to 40
private void validateIsNumeric(String input) {
if (input.matches("-?\\d+")) {
return;
}
throw new IllegalArgumentException("숫자가 아닌 문자를 입력할 수 없습니다.");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서 "공백 체크 → 숫자 여부 → int 범위 파싱" 순서로 나눠주신 점 읽기 좋았어요 👍

입력 형식 오류와 타입 변환 오류를 섞지 않으니까,
에러 메시지의 의미도 훨씬 또렷해졌네요.

더 나아가 regex 부분을 상수로 추출해서 이름을 지어주면 어떨까 합니다.
아무래도 많은 사람들, 그리고 미래의 테리가 이 프로젝트를 유지보수하다 보면,
해당 regex 가 무엇을 의미하는지 이해가 잘 안될 수도 있거든요.

Comment on lines -14 to -18
private static void validateIsNotBlank(String name) {
if (name == null || name.isBlank()) {
throw new IllegalArgumentException(ErrorMessage.NOT_ALLOW_EMPTY_INPUT.getMessage());
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이전 사이클과 비교하면 Name 에서 blank / null 방어가 빠졌네요.

그래서 지금은 new Name("") 이면 "공백"이 아니라 "이름은 영어 또는 한국어만 가능합니다." 로 가고,
new Name(null)IllegalArgumentException 도 아니라 NullPointerException 이 날 수 있어요.

해당 로직을 빼기로한 이유가 궁금합니다.

Comment on lines +22 to +26
private void validateAddable(Card card) {
if (cards.contains(card)) {
throw new IllegalArgumentException("이미 보유하고 있는 카드입니다. 중복 추가할 수 없습니다.");
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

중복 카드 추가 방어를 Cards 에 넣어주셨는데,
이 규칙을 "손패 자체의 책임"으로 보는지, 아니면 "전체 덱 분배 과정의 책임"으로 보는지 궁금해요.

현재 구조에서는 둘 다 그럴듯하지만,
여러 덱을 사용한다는 생각을 해보면 Hands 보다는 Deck의 특성에 더 가까운 것 같기도 해요.
그럴 땐 덱이 중복 카드를 가지고 있지 않지만 사용자가 받을 때는 같은 카드를 받을 수 있으니까요.

테리는 어떻게 생각하시나요?

Comment on lines 88 to 99
@@ -93,24 +98,17 @@ public void printWinTieLossResult(GameResultDto gameResultDto) {
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서는 이미 "최종 수익"을 출력하고 있는데,
메서드 이름은 아직 printWinTieLossResult, consistDealerWinTieLossResult 로 남아 있네요.

동작은 맞지만 이름만 예전 도메인에 머물러 있어서,
다음에 읽는 사람은 "이 값이 결과인가, 수익인가?"를 한번 더 해석하게 될 것 같아요.

Comment on lines +10 to +11
public record GameResultDto(Map<String, Long> playerWinTieLossResults,
long dealerWinTieLossResult) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DTO 도 같은 느낌이네요.

실제 필드는 Map<String, Long> / long 으로 수익을 들고 있는데,
이름은 playerWinTieLossResults, dealerWinTieLossResult 라서 의미가 어긋나 보입니다.
profit 기준으로 이름을 맞춰주면 View 쪽도 자연스럽게 정리될 듯 해요.

}

@Test
@DisplayName("딜러는 카드의 합이 16이면 카드를 더 뽑을 수 있다.")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

16 미만 / 16 / 16 초과를 나눠서 테스트한 점 좋아요 👍

딜러 규칙은 경계값이 핵심이라,
이렇게 쪼개두면 나중에 읽는 사람도 규칙을 훨씬 빨리 이해할 수 있을 것 같아요.

이런 경계값 테스트는 지금처럼 나눠서 적어도 좋고,
중복을 조금 더 줄이고 싶다면 parameterized test로 정리하는 방법도 있겠네요. :)
(바꾸자는 말은 아니고, 굳이 ParameterizedTest를 사용하지 않아도 좋다는 의미입니다.)

// when & then
assertTrue(testDealer.isDrawable());
// when & then
assertTrue(testDealer.isDrawable());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트에서 assertThat(...)assertTrue(...)/assertFalse(...) 가 조금 섞여 있네요.

물론 기능적으로 문제는 없지만, 이런 부분은 한 가지 스타일로 통일해두면 테스트 코드가 덜 흔들려 보여요.
가능하면 프로젝트 안에서 기준을 하나 정해 맞춰보셔도 좋을 것 같아요.

import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;

class PlayerResultTest {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

플레이어만 블랙잭인 경우는 GameResultTest 에서 이미 한 번 확인해두신 것 같아요.

다만 지금 테스트들에는 딜러만 블랙잭 이나 딜러는 블랙잭이고 플레이어는 3장 21인 경우 처럼 블랙잭 규칙이 일반 21점과 갈리는 경계 케이스는 아직 없는 것 같네요.

이번 PR에서 BLACKJACK 규칙을 따로 분리하신 만큼, 이런 케이스도 하나쯤 있으면 훨씬 더 안전해 보일 것 같습니다 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants