Skip to content

[🚀 사이클1 - 미션 (블랙잭 게임 실행)] 도우너 미션 제출합니다.#997

Merged
include42 merged 29 commits intowoowacourse:soojin6943from
Soojin6943:step1
Mar 11, 2026
Merged

[🚀 사이클1 - 미션 (블랙잭 게임 실행)] 도우너 미션 제출합니다.#997
include42 merged 29 commits intowoowacourse:soojin6943from
Soojin6943:step1

Conversation

@Soojin6943
Copy link

체크 리스트

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

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

1. 카드 생성 방식에 대한 설계

카드를 구현할 때 두 가지 방식을 고민했습니다.

  • 모든 카드를 미리 생성하여 저장한 뒤, 그 중에서 카드를 배분하는 방식
  • 모양과 랭크를 랜덤으로 생성하여 조합하는 방식

전자의 경우 카드 객체를 모두 보관해야 하므로 메모리 사용이 증가하고,
후자의 경우 매번 조합을 생성해야 하므로 연산 비용이 발생한다는 점에서 고민이 있었습니다.


일반적으로 이러한 상황에서 공간 사용과 연산 비용 중 어떤 요소를 더 우선적으로 고려하는지 궁금합니다.

2. TDD로 개발할 때의 구현 순서

TDD 방식으로 개발할 때 어떤 기준으로 구현 순서를 잡는 것이 좋을지 궁금합니다.

  • 서비스의 실제 동작 흐름을 기준으로 구현하는 방식
  • 도메인 또는 기능 단위를 기준으로 구현하는 방식

두 방식 중 어떤 접근이 더 일반적인지, 혹은 상황에 따라 어떤 기준으로 판단하는지 궁금합니다.

3. TDD 초기 테스트의 불완전성

TDD를 진행하면서 테스트가 충분히 촘촘하지 못하고 빈 부분이 생기는 느낌이 있었습니다.
이런 경우 어떤 방식으로 접근하는 것이 좋은지 궁금합니다.

4. 예외 메시지의 구체성

예외 메시지를 작성할 때 구체적인 메시지포괄적인 메시지 중 어떤 방식이 더 적절한지 궁금합니다.


예시 )

  • 구체적인 방식
    • “입력 가능한 범위를 초과했습니다.”
    • “빈 값이나 공백은 유효하지 않은 형식입니다.”
  • 포괄적인 방식
    • “유효하지 않은 입력 형식입니다.”

보통 실무나 프로젝트에서는 어느 수준의 구체성을 유지하는지 궁금합니다.

5. ACE(1 / 11) 점수 처리 방식

처음 설계에서는 카드의 Rank를 enum으로 정의하고,
ACE는 1과 11 두 가지 값을 모두 가지도록 설계했습니다.
비록 구현 과정에서 enum은 설계처럼 구현하였지만, 이 구조를 그대로 활용하지 못하고,
 기본 점수(1)에 상황에 따라 10을 더하는 방식으로 처리했습니다.)

지금 구조에서는
다른 랭크 카드들은 동일한 점수를 가진 필드를 두 개씩 가지게 됩니다.
이처럼 특정 케이스(ACE)를 위해 다른 값들이 중복 저장되는 구조가 설계적으로 적절한지 의견을 듣고 싶습니다.

Uechann added 17 commits March 4, 2026 17:14
1. 페어프로그래밍 관력 규칙과 요구사항 분석을 진행하였습니다.
2. 기능목록(세부 기능, 예외 케이스, 테스트 케이스)을 작성하였습니다.
3. PR 피드백에 요청할 포인트도 사전에 정리하였습니다.
사용자 이름을 입력받고 입력 형식에 맞지 않으면 예외를 반환하도록 구현하였습니다.

1. 공백, 빈칸 입력
2. 허용되지 않은 구분자
3. 영어 외의 이름,
4. 2-5 글자가 아닌 이름의 경우
참가자 이름 입력 형식에 맞게 입력한 경우에 대해 테스트를 구현하였습니다.
참가자가 유효한 형식으로 주어졌을 때 Parsing 후에 Player 객체를 생성하는 기능을 구현하였습니다.
랜덤 카드를 뽑는 기능 중 랜덤 카드 모양 생성 세부 기능을 구현하였습니다.
랜덤 숫자를 1-4까지 생성후 카드 모양을 가져오는 방식으로 구현하였습니다.
랜덤 숫자를 생성 후에 코드 값을 통해 랜덤 카드 랭크를 생성하는 기능을 구현하였습니다.
완료한 기능에 대해 체크 표시하고
기능들을 검토후에 수정하였습니다.
랜덤 카드 랭크를 추출하기 위한 랜덤 숫자 생성기를 구현하였습니다.
랜덤으로 생성한 모양과 랭크를 통해 카드를 생성하는 기능을 구현하였습니다.
카드 생성시에 Repository에 저장하고 Repository에서 동일한 카드가 존재하는지 중복 검사하는 기능을 구현하였습니다.
플레이어와 딜러에게 카드 2장씩 랜덤으로 생성 한 뒤에 중복 검사를 거치고 덱을 생성합니다.
생성된 덱을 플레이어와 딜러에게 부여하는 기능을 구현하였습니다.
분배한 카드 2개씩을 출력한다.
이때, 딜러는 2장 중에서 나중에 생성된 카드만 오픈한다.
1. 카드 추가 배부 기능
2. 추가 배부시 합산 계산을 진행 진행
3. 21이 넘었을 때 버스트 판단 후 상태 변환
딜러와 플레이어의 최종 카드들과 총합을 출력합니다.
1. 사용자와 딜러의 버스트 유무를 먼저 비교하여 상태를 변경합니다.
2. 둘다 생존일 경우 덱의 합산을 비교하여 플레이어의 승패유무를 변경합니다.
1. 카드를 추가 분배하는 기능을 구현하였습니다.
2. 추가 분배 시에 합계를 다시 계산하는 로직을 구현하였습니다.
3. 점수 계산 시에 ACE를 1/11로 유리하게 계산되도록 구현하였습니다.
@include42
Copy link

안녕하세요 도우너, 이번에 리뷰를 맡게 된 카프카 라고 합니다.
미션 진행하시느라 고생 많으셨습니다!

리뷰는 금일 저녁 6시 전후로 가능할 듯 합니다.
남은 주말 푹 쉬어가시고, 이대로 미션 함께 파이팅하면 좋겠습니다. 😄

Copy link

@include42 include42 left a comment

Choose a reason for hiding this comment

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

안녕하세요 도우너, 리뷰어 카프카입니다.
작업해주신 내용 잘 확인하였습니다.
여러 가지를 시도해보고 고민해본 흔적이 많이 나는 작업이라,
지적보다도 구현의 이유를 묻는 질문이 많았습니다.
해당 부분들은 부담없이 의견 코멘트로 남겨주시면 좋겠습니다.
대부분 수정이 강제되지 않는 논의이니, 가능한 만큼 수정하고 다시 리뷰 요청 주시면 되겠습니다.

작업하시느라 고생 많으셨습니다! 😄

Comment on lines +146 to +152
## PR에 피드백 요청할 포인트

* 예외 처리 메시지에 대한 형식
* 자세히 or 포괄적으로..?
* Person에 대한 인터페이스로 추상화하는 것이 적절한가요
* TDD를 하는 과정에서 구멍이 난 테스트 코드에 대해서 눈에 들어올 때 다 구현 후에 마지막에 테스트 코드를 작성해야할까요?
* Deck에서 상태와 합산 등 책임을 가지고 있는데 이걸 여러개의 책임인지 책임의 기준이 무엇인지

Choose a reason for hiding this comment

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

우선 피드백 관련 내용 전달드립니다.

  1. 예외 처리 메시지는 간결한 편이 좋지만, 오류와 관련된 정보를 포함하는 것이 좋습니다. 예를 들어 입력 데이터 관련 오류라면, 해당 데이터를 보여주거나 준수하지 못한 룰(예: 특수문자 사용, 글자수 초과) 에 대한 정보를 전달하면 좋겠지요? 이렇게 구현할 경우 Validator쪽의 구현이 길어지는데, 관련해서 필수로 수정할 필요는 없습니다. 이번 미션에서는 이에 대한 고민만 해봐도 충분할 듯 하네요.

  2. 인터페이스로 구현해주신 것 자체는 좋습니다. 다만 이렇게 구현한 것과 추상 클래스로 구현한 것은 어떤 차이가 있을까요? 이에 대해 고민해보면 좋겠습니다. 그리고 덧붙이자면, 제가 느끼기에는 덱 관련 기능만 모여있어서 Person 이 적절한 네이밍같지는 않습니다. 예를 들어 WithDeck 같은 이름이면 어땠을까 싶네요. (제 의견이니, 도우너의 의견도 답신해주시면 좋겠습니다.)

  3. 구멍이 난 테스트 코드의 예시가 있을까요? 일단 TDD로 작업을 한다면, 정해진 스펙에 대해서는 구현 단계에서 테스트 코드가 먼저 완성되어야 한다고 생각됩니다. 다만 이부분은 따로 의견 적어주시면, 참고해서 논의해보면 좋겠습니다.

  4. 책임이 여러개인지에 대한 것보다는 해당 클래스가 해당 역할을 맡는게 올바르다고 여겨지는지가 중요하다고 저는 생각합니다. 이 경우엔 큰 문제가 없어보이긴 하네요. 다만 그와 별도로 해당 클래스에서 수정 필요한 부분이 있어 코멘트 해두었습니다.

Copy link
Author

Choose a reason for hiding this comment

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

  1. 지금 다시 코드를 보니 정말 덱 관련 기능만 모여있네요. 카프카 의견대로 Person 보다는 WithDeck이 더 적절한 네이밍 같아요!

Copy link

@include42 include42 Mar 10, 2026

Choose a reason for hiding this comment

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

이부분(WithDeck으로 인터페이스명 변경) 수정되지 않았는데, 해당 부분은 2단계에서 기능 추가하면서 수정될수도 있어 보입니다. 이번 단계에서는 수정하지 마시고, 다음 단계에서 수정이 필요할지 검토해주세요. 😄

import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;

public class InputViewTest {

Choose a reason for hiding this comment

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

해당 테스트는 InputValidatorTest가 되어야하지 않을까요?

다만 parameterizedTest 구현하신 부분이나, 성공 테스트 별도로 구현한 부분 매우 좋습니다. 👍

Copy link
Author

Choose a reason for hiding this comment

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

지금 보니 InputValidatorTest가 맞는 것 같네요..! 😅

Copy link

@include42 include42 Mar 10, 2026

Choose a reason for hiding this comment

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

이부분이 아직 수정되지 않아서, 함께 수정 요청드립니다. (테스트명 변경)

public class judgeTest {

@Test
void 게임_승패_판정_테스트() {

Choose a reason for hiding this comment

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

테스트 코드가 비어있는 이유가 있을까요?

  • 만약 미사용하는 테스트라면 @disabled 어노테이션을 붙이면 됩니다.
  • 이외의 경우라면 삭제하는 것이 좋겠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

실수로 테스트 코드 작성을 놓친것 같아요...!!
리팩토링하면서 추가해두겠습니다!

Comment on lines +15 to +27
class DefaultShapeNumberGenerator implements NumberGenerator {
@Override
public int generate() {
return 1;
}
}

class DefaultRankNumberGenerator implements NumberGenerator {
@Override
public int generate() {
return 2;
}
}

Choose a reason for hiding this comment

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

이부분 테스트에서 다른 제너레이터 만들어서 잘 구현해 주셨네요 👍

다만 PR 본문의 1번 질문과도 연결될텐데요,
저는 둘 다 괜찮은 방법이라고 생각됩니다.
다만 이 프로젝트에서는 두 방식 모두 연산 비용 혹은 메모리 사용의 범위가 허용 이상이 되지 않으니,
보다 코드의 가독성이 좋고 간단한 방법으로 구현하는게 좋을 것이라고 생각합니다.

구현해주신 방식은 이렇게 테스트하기에 편한 방식이라, 저도 좋다고 생각합니다. 😄

import view.InputView;
import view.OutputView;

public class DIConfig {

Choose a reason for hiding this comment

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

DIConfig를 만들어서 여러 클래스 및 의존성을 관리하고 있네요.
혹시 이렇게 구현하기로 페어와 결정한 이유와, 이 방식의 장점을 코멘트로 말씀해주실수 있으실까요?
이 방식과 BlackJackController등의 class 내부에서 필요한 class를 생성해서 사용하는 경우를 비교해볼 수 있다면 좋을듯 합니다. (이 부분은 수정 요청이나 지적이 아닌, 구현 이유를 확인하고자 하는 질문입니다.)

Copy link
Author

Choose a reason for hiding this comment

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

필요한 의존성을 한 눈에 볼 수 있다는 장점과 스프링의 IoC관련 지식과 접목 시킬 수 있다는 점에서 DIConfig를 만들어서 사용했습니다!!

private int code;
private String name;
private int value;
private int additionalValue;

Choose a reason for hiding this comment

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

아마도 CardRank의 구현은 두 가지 방법으로 나뉠텐데, ace score 를 이렇게 구현해주신 점 좋습니다.
이렇게 구현한 이유 혹은 생각하고 있는 장점이 있을까요? (저도 이 방식이 낫다고 생각해서, 확인차 코멘트 드렸습니다)

Copy link
Author

Choose a reason for hiding this comment

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

구조적으로 카드 내에서 두 가지의 점수를 가질 수 있다는 점을 쉽게 확인할 수 있고,
추가 점수가 필요할때 한번에 가져올 수 있다는 점에서 이렇게 구현했습니다.

Choose a reason for hiding this comment

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

말씀주신대로, 이렇게 작성한 경우 카드 점수의 성격과 두번째 점수를 CardRank 내에서 볼 수 있어서 좋다고 생각합니다.
그리고 추후에 (이번 미션에서는 그렇지 않으나) 다른 룰이 추가되는 경우 수정도 용이하겠지요. 고민해서 잘 결정해 주셨습니다 👍


public class Deck {

private int sum = 0;

Choose a reason for hiding this comment

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

Deck이 sum을 필드로 가져야 하는 이유가 있을까요?
getSum 호출시 바로 계산해주는 것과 비교했을때 장점이 무엇일지 궁금합니다.

Copy link
Author

Choose a reason for hiding this comment

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

합계에 대한 결과를 가져다 사용하는 부분이 많아, 재계산을 줄이기 위해 sum을 필드로 가져갔습니다!

Choose a reason for hiding this comment

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

말씀주신 부분이 장점이 된다고 저도 생각합니다.

다만 조심스러운 부분이, 그렇게 되면 Deck 내의 sum이 상수가 아닌 변수로 존재하게 되어서요,

  1. 만약 (그러면 안되지만) 계산 로직이 누락되면 잘못된 결과를 전달할 수 있습니다.
  2. 만약 setter가 열려 있어 sum이 변조되면 잘못된 결과를 전달할 수 있습니다.
  3. calculateSum과 calculateFinalSum 이 모두 sum을 수정하는 메소드라서, 외부에서 잘못 호출시 의도하지 않은 결과를 전달할 수 있습니다.

제 생각에는 이러한 면에서, getSum 호출시 계산해주는 면이 해당 시점의 점수를 변조 위험 없이 전달하는 방법이 아닐까 싶습니다. 다만 이렇게 할 경우, ace를 포함하는 final score 를 전달하는 getFinalScoreSum 과 같은 메소드가 하나 더 필요하겠네요.

다만 현재 프로젝트의 요구조건과 목표를 고려할때, 도우너가 구현한 방식이 틀렸다 라고 생각하지는 않습니다.
제가 적어드린 내용과 재연산의 비용 중 어느쪽이 중요한지 고려 후, 필요한 경우만 수정해주세요. 👍

Copy link
Author

Choose a reason for hiding this comment

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

생각해보니 조언해주신 부분이 더 중요한 것 같아요!! 만약 최종 점수 계산 메소드가 중복으로 실행된다면 잘못된 결과 값을 제공할 수도 있고요...!

위 상황을 막기 위해 sum 필드는 제거한 후 관련 로직들을 수정했어요! 코드가 좋은지는 모르겠지만 나름 열심히 구현했으니 확인 부탁드립니다!! 🙇‍♂️

Choose a reason for hiding this comment

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

이부분 수정 잘 해주셨습니다. 고생하셨습니다 👍

WIN("승"),
LOSS("패"),
DRAW("무"),
NONE("미정");

Choose a reason for hiding this comment

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

NONE을 만들어두신 것이 재미있네요. 이렇게 하면 게임 종료 전에 player의 status 조회시 NullPointerException이 발생하는걸 막을 수 있겠지요.

다만 궁금한 것은, Player가 PlayerStatus를 가지는 것이 결과값과 player 간의 매칭을 위해서일까요? 제 생각에는 PlayerResultDto.of(player)PlayerResultDto.of(player, status)로 수정하고 judgementService 에서 승패값을 구해 PlayerResultDto에 인자로 보내주면, player에서 해당값을 가질 필요가 없을듯해서 질문드렸습니다.

Copy link
Author

Choose a reason for hiding this comment

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

추후 기능 추가 시 Player가 PlayerStatus를 가지고 있어야 유용할 것 같아 이 구조를 사용했습니다. 😀

Choose a reason for hiding this comment

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

추후 수정을 고려해서 작업해주신 부분 좋습니다. 그렇다면 이 부분은 현재 구조 유지하는 것으로 생각하겠습니다.

private final CardRepository cardRepository;
private final NumberGenerator randomRankNumberGenerator;
private final NumberGenerator randomShapeNumberGenerator;
// TODO: 인스턴스 변수 2개까지 줄이기

Choose a reason for hiding this comment

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

코드 중간중간 TODO들이 보이는데, 이렇게 적어두면 intellij에서도 확인 가능해서 좋은 습관이라고 생각합니다.
다만 적어주신 내용은 다음 리뷰 요청전에 수정해 두거나, 혹은 지워주시는게 코드 퀄리티상 좋을 것으로 생각합니다.

Copy link
Author

Choose a reason for hiding this comment

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

다음부터는 리뷰 요청 전 수정해두겠습니다..!!! 👍

import java.io.IOException;
import java.io.InputStreamReader;

public class InputView {

Choose a reason for hiding this comment

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

구현 잘 해주셨습니다. 👍
다만 잘못된 값 입력시 예외 출력 후 다시 입력을 받을 수 있다면 좋을텐데요 (반복문으로 한번 감싸서...)
혹시 이렇게 구현하는 것은 어떻게 생각하시나요? 리뷰를 하면서 확인해보니 크루마다 구현여부가 갈리던데, 내일 다른 크루들과 논의해보고, 필요하다고 생각되시면 구현해보면 좋겠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

indent(인덴트, 들여쓰기) depth를 2를 넘지 않도록 구현한다. 1까지만 허용한다.
- 예를 들어 while문 안에 if문이 있으면 들여쓰기는 2이다.
- 힌트: indent(인덴트, 들여쓰기) depth를 줄이는 좋은 방법은 함수(또는 메서드)를 분리하면 된다.

이번 미션 요구사항에서 depth1까지만 허용이라 재입력 로직을 구현하지 못했습니다.
depth1을 유지하며 재입력을 받을 수 있는 방식에 대해서 힌트를 제공받을 수 있을까요..?😅

Copy link
Author

Choose a reason for hiding this comment

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

페어와 함께 고민해봤는데, 재귀문을 선언하고 그 안에서 try-catch로 계속해서 재입력 받는 형식은 어떤가요...?! 🙂

Copy link

@include42 include42 Mar 9, 2026

Choose a reason for hiding this comment

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

제 생각에도 그게 맞을듯 싶네요.
적어주신대로 구현 해주시고, 만약 indent 룰을 맞추기 어렵다면 코멘트로 따로 말씀해주세요. 😄

Copy link
Author

Choose a reason for hiding this comment

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

Controller에서 입력값을 검증할 때 재입력 받는 형식으로 구현해봤어요! 확인 부탁드립니다!! 😀

@include42
Copy link

안녕하세요 도우너, 리뷰어 카프카입니다.
금일 저녁에 리뷰 진행하고자 하였으나, 본 PR에서 변경사항 확인이 안되어서요,
작업하신 내용 반영 후에 다시 요청주시면 맞춰서 리뷰 진행하겠습니다. 😄

진행하시면서 어려우신 점 있으시면 코멘트 혹은 slack 통해 편하게 말씀주세요!

Copy link

@include42 include42 left a comment

Choose a reason for hiding this comment

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

안녕하세요 도우너, 리뷰어 카프카입니다.

리팩토링해주신 코드 잘 확인했습니다. 👍
전략패턴 사용이나 상속관계 고려 등 이번 미션에서 시도해보면 좋을 것들을 다양하게 적용해 보았네요.
코드 퀄리티도 좋고, 충분한 성취를 보여주고 있습니다.

다만 오류가 나는 부분을 테스트하면서 몇가지 발견해서, 코멘트를 남겨두었습니다.
코멘트 남긴 부분에 대해서 수정 및 관련 테스트 작성을 요청드립니다.
해당 오류들 해결되면, 확인 후 다음 단계로 넘어가는 것으로 하시죠 👍

작업하시느라 고생 많으셨습니다 💯

Comment on lines +56 to +68
private void judgeStatusByDeckSum(Player player, Dealer dealer) {
if (player.getDeckSum() > dealer.getDeckSum()) {
player.changeStatus(PlayerStatus.WIN);
}

if (player.getDeckSum() < dealer.getDeckSum()) {
player.changeStatus(PlayerStatus.LOSS);
}

if (player.getDeckSum() == dealer.getDeckSum()) {
player.changeStatus(PlayerStatus.DRAW);
}
}

Choose a reason for hiding this comment

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

Image

마지막에 player와 dealer 간 스코어를 비교할 때에, calculateFinalSum을 참조해야 하지 않을까요? 위 예시처럼 잘못된 결과가 출력될 수 있어 보입니다.

Choose a reason for hiding this comment

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

작업시 관련된 테스트 케이스도 함께 작성해주시면 좋을 것으로 생각됩니다.

Copy link
Author

Choose a reason for hiding this comment

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

제가 고려하지 못하고 넘어가버렸네요...! 수정했습니다 😅

}

@Test
void ACE_11이_유리한_경우_테스트() {

Choose a reason for hiding this comment

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

현재 이 테스트 실패하고 있는데, 수정이 필요해 보입니다.

Copy link
Author

Choose a reason for hiding this comment

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

calculateFinalSum 과 검증 확인을 했어야 했는데, 수정 전 getDeckSum을 가져와 실패했네요..!😅


import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

public class CardDistributorTest {
Copy link

@include42 include42 Mar 10, 2026

Choose a reason for hiding this comment

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

혹시 CardDistributor에서 카드를 52장 이상 뽑으면 어떻게 되나요?
물론 이 프로젝트에서는 불가능한 케이스로 보일 수도 있지만, 아래와 같은 케이스가 가능합니다.

  • 현재 플레이어 최대 수 제한을 두지 않아서 발생한 오류입니다.
// 입력 예시: 30명 플레이어 입력
ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab,ab

// 결과
Exception in thread "main" java.lang.StackOverflowError
	at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:526)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:513)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.FindOps$FindOp.evaluateSequential(FindOps.java:150)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.findFirst(ReferencePipeline.java:647)
	at domain.model.CardRank.getRank(CardRank.java:36)
	at util.RandomCardGenerator.generateRank(RandomCardGenerator.java:16)
	at domain.service.CardFactory.getCard(CardFactory.java:26)
	at domain.service.CardFactory.getCard(CardFactory.java:32)
   ...

Choose a reason for hiding this comment

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

⚠️ 아래 순서로 테스트 작성 요청드립니다.

  1. 카드를 52장 이상 뽑는 테스트 코드 추가
  2. 해당 테스트 수행해서 실패하는 것 확인
  3. CardDistributor의 메인 코드를 수정 (52장 이상 뽑는 경우 적절한 예외와 에러메시지 반환...)
  4. 테스트 코드 수행해서 성공하는 것 확인

Copy link
Author

Choose a reason for hiding this comment

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

말씀해주신 순서로 테스트를 작성하니,

52장 이상의 카드를 뽑을 때 StackOverflowError 발생을 확인했습니다.
(52장의 카드가 생성되면, 다음 카드 생성부터는 생성될 수 있는 경우의 수의 카드가 없어 무한 재귀를 돌게 됨)

위 문제를 해결하기 위해 52장의 카드가 생성된 상태라면 그 이후는 예외를 던지도록 구현했으며, 그에 맞는 테스트 코드를 수행하도록 했습니다. 😀

Choose a reason for hiding this comment

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

좋습니다, 전달해드린 flow로 오류 수정해보는 경험은 어떠셨나요?
TDD로 작업을 할때에 새로운 코드를 개발하는 과정도 중요하지만, 오류 혹은 스펙 변경을 인지하고 테스트부터 기민하게 작성하는 경험 역시 중요하다고 생각합니다. 👍

다만 확인해보니 stackoverflow 발생하는 것에 대한 테스트 작성 후, 메인 코드 -> 테스트 코드 순으로 수정하고 stack overflow 테스트 코드는 주석 처리해 주셨더라고요.

좀더 간단하게 아래와 같이 진행했다면 좋을듯 합니다.

  1. 테스트 코드에서 StackOverflowError 가 발생하는지 검증하지 말고, 52장 이상 드로우하면 IllegalArgumentException (message = "카드는 52장을 초과할 수 없습니다.")가 발생하는지 검증하는 테스트를 짠다.
  2. 해당 테스트 코드가 '실패하는 것을' 확인한다. (당연히 관련 로직이 없으므로...)
  3. 메인 코드를 수정해서, 52개를 넘는 경우 IllegalArgumentException 를 발생하도록 하고 올바른 메시지를 함께 넣어준다.
  4. 실패했던 테스트 코드가 성공하는 것을 확인한다.

여기서 포인트는, 1번 과정에 해당 오류에 대해 어떤 결과값을 줄지에 대한 스펙이 정의되어 있다는 점입니다.
이렇게 되면, 테스트를 통해 스펙을 먼저 정의하고, 코드가 그 스펙(테스트)를 따라가는 Test Driven 한 개발이 가능하겠지요.

제가 생각하는 테스트 주도의 오류 수정에 대해 적어두었는데,
이부분에 대해서는 다른 크루들과도 이야기 많이 나눠봤으면 좋겠습니다.
어쨌든, 의도한대로 잘 고쳐주셔서 구현내용에 대해서는 이의 없습니다. 💯

import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;

public class InputViewTest {
Copy link

@include42 include42 Mar 10, 2026

Choose a reason for hiding this comment

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

이부분이 아직 수정되지 않아서, 함께 수정 요청드립니다. (테스트명 변경)

Comment on lines +146 to +152
## PR에 피드백 요청할 포인트

* 예외 처리 메시지에 대한 형식
* 자세히 or 포괄적으로..?
* Person에 대한 인터페이스로 추상화하는 것이 적절한가요
* TDD를 하는 과정에서 구멍이 난 테스트 코드에 대해서 눈에 들어올 때 다 구현 후에 마지막에 테스트 코드를 작성해야할까요?
* Deck에서 상태와 합산 등 책임을 가지고 있는데 이걸 여러개의 책임인지 책임의 기준이 무엇인지
Copy link

@include42 include42 Mar 10, 2026

Choose a reason for hiding this comment

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

이부분(WithDeck으로 인터페이스명 변경) 수정되지 않았는데, 해당 부분은 2단계에서 기능 추가하면서 수정될수도 있어 보입니다. 이번 단계에서는 수정하지 마시고, 다음 단계에서 수정이 필요할지 검토해주세요. 😄


public class Deck {

private int sum = 0;

Choose a reason for hiding this comment

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

이부분 수정 잘 해주셨습니다. 고생하셨습니다 👍

Copy link

@include42 include42 left a comment

Choose a reason for hiding this comment

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

안녕하세요 도우너, 리뷰어 카프카입니다.

이전에 리뷰 드리면서 테스트 및 오류 관련 수정사항을 여러 가지 요청드렸는데,
빠르게 모두 수정해주셨네요. 확인 감사드립니다.
이전에 요청드린 사안 모두 문제 없고, 덱 드로우 오류 수정 관련해서만 추후 어떤 식으로 하면 더 좋을지 성장을 위한 코멘트 남겨두었습니다.

현재 프로젝트에서 요구되는 만큼 잘 성취해 주셔서, 이제 미션 approve 하고자 합니다. 💯
그동안 고생 많으셨고, 2단계에서도 함께 화이팅하시죠 😄


import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

public class CardDistributorTest {

Choose a reason for hiding this comment

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

좋습니다, 전달해드린 flow로 오류 수정해보는 경험은 어떠셨나요?
TDD로 작업을 할때에 새로운 코드를 개발하는 과정도 중요하지만, 오류 혹은 스펙 변경을 인지하고 테스트부터 기민하게 작성하는 경험 역시 중요하다고 생각합니다. 👍

다만 확인해보니 stackoverflow 발생하는 것에 대한 테스트 작성 후, 메인 코드 -> 테스트 코드 순으로 수정하고 stack overflow 테스트 코드는 주석 처리해 주셨더라고요.

좀더 간단하게 아래와 같이 진행했다면 좋을듯 합니다.

  1. 테스트 코드에서 StackOverflowError 가 발생하는지 검증하지 말고, 52장 이상 드로우하면 IllegalArgumentException (message = "카드는 52장을 초과할 수 없습니다.")가 발생하는지 검증하는 테스트를 짠다.
  2. 해당 테스트 코드가 '실패하는 것을' 확인한다. (당연히 관련 로직이 없으므로...)
  3. 메인 코드를 수정해서, 52개를 넘는 경우 IllegalArgumentException 를 발생하도록 하고 올바른 메시지를 함께 넣어준다.
  4. 실패했던 테스트 코드가 성공하는 것을 확인한다.

여기서 포인트는, 1번 과정에 해당 오류에 대해 어떤 결과값을 줄지에 대한 스펙이 정의되어 있다는 점입니다.
이렇게 되면, 테스트를 통해 스펙을 먼저 정의하고, 코드가 그 스펙(테스트)를 따라가는 Test Driven 한 개발이 가능하겠지요.

제가 생각하는 테스트 주도의 오류 수정에 대해 적어두었는데,
이부분에 대해서는 다른 크루들과도 이야기 많이 나눠봤으면 좋겠습니다.
어쨌든, 의도한대로 잘 고쳐주셔서 구현내용에 대해서는 이의 없습니다. 💯

@include42 include42 merged commit f62f1c8 into woowacourse:soojin6943 Mar 11, 2026
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.

3 participants