Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1단계 - 블랙잭] 에드(김진우) 미션 제출합니다. #833

Merged
merged 50 commits into from
Mar 12, 2025

Conversation

jinu0328
Copy link

@jinu0328 jinu0328 commented Mar 7, 2025

체크 리스트

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

프롤로그 링크

객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?

1~5점 중에서 선택해주세요.

  • 1 (전혀 충족하지 못함)
  • 2
  • 3 (보통)
  • 4
  • 5 (완벽하게 충족)

선택한 점수의 이유를 적어주세요.

  1. 도메인 객체의 getter 사용
    별도의 dto 없이 방어적 복사를 이용한 getter를 통해 View에서 도메인 객체를 직접 참조하는 것이 큰 문제가 없다고 판단되어 도메인 객체에 getter가 존재합니다.

  2. List를 그대로 사용함
    별도의 일급컬렉션 Players 없이 List를 그대로 사용하였습니다. 현재 요구사항에서 Player를 묶는 컬렉션을 일급 컬렉션으로 관리함으로써 얻는 것보다 이를 포장하고 view에서 getter를 한번 더 파고 들어가는 비용이 더 크다고 생각했기에 컬렉션을 그대로 사용하였습니다.

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

1. Gamer 추상 클래스에서 hit 메서드 관리 문제

현재 Gamer라는 추상 클래스를 만들고, PlayerDealer가 이를 상속하도록 구현하였습니다.

컨트롤러에서 List<Gamer>PlayerDealer를 함께 관리하려 했으나, hit 메서드를 호출하는 데 문제가 발생하였습니다. 결국 추상메서드 없이 Player와 Dealer에 각각 hit을 구현해주게 되었습니다.

시도해 본 해결 방법

  1. Gamer에 hit을 추상 메서드로 정의
    • hitGamer에 없으면 컨트롤러에서 호출할 수 없으므로, 추상 메서드로 선언하려 했습니다.
    • 하지만 Player.hit()void, Dealer.hit()boolean을 반환해야 해서 반환 타입을 통일할 수 없었습니다.
  2. 제네릭을 활용하여 반환 타입을 유연하게 구성
    • <T> T hit(Cards totalCard) 형태로 Gamer에 제네릭 메서드를 정의해 보았습니다.
    • 이를 통해 Player.hit()Void, Dealer.hit()Boolean을 반환하도록 구현할 수 있을 것이라 기대했습니다.
    • 하지만, Player에서 hit()void로 구현해야 할 때 문제가 발생했습니다.

발생한 문제

  • 제네릭 반환 타입의 한계
    • Player.hit()void로 구현하고 싶었으나, 제네릭 메서드는 반드시 값을 반환해야 하는 특성이 있습니다.
    • 즉, Void 타입을 반환하도록 구현하면, 반드시 return null;을 명시해야 하는 문제가 발생하였습니다.
    • 이는 hit()이 본래 반환값이 필요 없는 동작임에도 불구하고, 불필요한 null 반환을 강요하는 결과를 초래했습니다.

질문

이러한 상황에서 hitGamer의 공통 메서드로 관리할 수 있는 좋은 방법이 있을까요?

제네릭을 활용하는 것이 적절한 접근 방식이었을지, 혹은 다른 해결책이 있을지 고민됩니다.

더 나은 설계 방식이 있다면 조언 부탁드립니다!

2. Cards 클래스의 역할 분리 여부

현재 CardsCard 클래스의 일급 컬렉션으로 구성되어 있으며, 두 가지 용도로 활용되고 있습니다.

  1. 게임 전체에서 사용되는 52장의 카드 저장소 (덱 역할)
  2. Gamer가 뽑아 보유하는 카드 묶음

이로 인해 Cards 클래스에는 extract()add() 두 가지 주요 메서드가 존재하지만,

  • extract()게임 전체 카드(덱)에서 카드 한 장을 뽑을 때만 사용
  • add() → 덱의 초기화를 제외하고는 Gamer가 뽑은 카드를 보유할 때만 사용

즉, 같은 Cards 클래스 내에 있지만, 각 기능이 특정 용도에서만 쓰이는 상황입니다.

고민한 점

현재 Cards하나의 클래스로 유지하는 이유는 다음과 같습니다.

  • 카드를 묶어서 관리하는 객체가 이미 존재하므로, 굳이 덱과 플레이어의 카드 보유를 분리할 필요가 있는가?
  • 역할이 다르더라도, 기존 Cards 클래스에 필요한 메서드를 추가하여 충분히 관리할 수 있다면 하나의 클래스로 유지하는 것이 적절한가?

질문

현재처럼 하나의 Cards 클래스로 덱과 보유 카드를 함께 관리하는 것이 좋은 설계일까요?

아니면 덱과 Gamer의 보유 카드를 명확히 분리하는 것이 더 나은 방향일까요?

혹시 더 적절한 구조가 있다면 조언 부탁드립니다!

dye0p and others added 30 commits March 5, 2025 02:25
- List<Card>를 가지는 Cards를 통해 카드를 추가 및 뽑을 수 있다.
- Ace를 11로 계산하기 위해 SOFT_ACE(11)를 추가 하였음.
- 딜러는 처음에 받은 2장의 합계가 16이하면 반드시 1장의 카드를 추가로 받는다.
-처음 받는 2장의 합계가 17 이상이라면 추가로 받을 수 없다.
- number와 symbol 기반으로 중복되지 않는 카드뭉치 생성
- 셔플을 통해 카드의 순서를 랜덤으로 부여
- player의 hit() 메서드를 오버라이드 하지않고
boolean 타입을 반환하는 dealerHit 메서드로 변경
Copy link

@Rok93 Rok93 left a comment

Choose a reason for hiding this comment

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

에드 안녕하세요. 블랙잭 미션을 함께하게된 로키입니다. 😃
잘 부탁드려요. 🙇‍♂️

1단계 구현 잘해주셨네요! 👍👍
질문 주신내용에 대한 제 생각 남겨두었습니다. 😃
추가적으로 몇몇 코멘트 남겨두었는데, 확인해서 반영 부탁드릴게요. 🙏

Comment on lines 12 to 18
public boolean hit(Cards totalCards) {
if (canHit()) {
add(totalCards);
return true;
}
return false;
}
Copy link

Choose a reason for hiding this comment

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

이러한 상황에서 hit을 Gamer의 공통 메서드로 관리할 수 있는 좋은 방법이 있을까요?
제네릭을 활용하는 것이 적절한 접근 방식이었을지, 혹은 다른 해결책이 있을지 고민됩니다.
더 나은 설계 방식이 있다면 조언 부탁드립니다!

당연하게도 메서드 시그니처가 다르다면 인터페이스가 다르기 때문에 추상 메서드로 둘 수 없습니다.
지금의 케이스에서 제네릭을 활용한 방식은 적절하지 않다고 생각합니다.

2와 같이 제네릭을 이용한 방식도 반환값이 둘 다 있는 경우라면 어찌저찌 억지로 해볼 수 있을 것 같은데,
아예 있냐 없냐는 제네릭으로 처리할 수 있는 상황은 아닌 것으로 보입니다.

저라면 메서드 시그니처를 통일하는 쪽으로 둘 것 같아요. (필요에 따라서 canHit() 메서드를 public으로 제공해줄 것 같아요)

저라면 차라리 아래같이 딜러가 hit을 실패한다면 예외를 던져주는 방식으로 변경하는게 깔끔한 해결책인 것 같습니다.

Suggested change
public boolean hit(Cards totalCards) {
if (canHit()) {
add(totalCards);
return true;
}
return false;
}
public void hit(Cards totalCards) {
if (canHit()) {
throw new IllegalStateException("예외 내용....");
}
return add(totalCards);
}

Copy link
Author

@jinu0328 jinu0328 Mar 9, 2025

Choose a reason for hiding this comment

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

메서드 시그니처를 통일하여 Gamer 클래스에 hit()을 추상 메서드로 선언하고, Dealer에서는 카드를 더 뽑을 수 없는 경우 예외를 발생시키도록 수정했습니다.
또한, Controller의 processDealerTurn() 메서드에서 try-catch를 활용하여 예외 발생 시 outputView를 통해 카드 뽑기 실패 메시지를 출력하도록 변경하였습니다.

기존 요구사항에는 예외 발생 시 실패 메시지를 출력하라는 명시적인 내용이 없었지만, try-catch에서 예외가 발생했음에도 아무 조치를 취하지 않으면 동료 개발자가 코드를 봤을 때 "예외가 발생했는데 왜 아무런 처리를 하지 않지?" 라는 혼란을 불러올 수 있다고 판단했습니다.
따라서, 코드의 명확성을 높이기 위해 제 임의로 outputView에서 실패 메시지를 출력하도록 추가하였습니다!

refactor: Gamer의 hit() 메서드 시그니처 통일 및 예외 처리 방식 적용

Copy link

Choose a reason for hiding this comment

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

저라면 메서드 시그니처를 통일하는 쪽으로 둘 것 같아요. (필요에 따라서 canHit() 메서드를 public으로 제공해줄 것 같아요)
저라면 차라리 아래같이 딜러가 hit을 실패한다면 예외를 던져주는 방식으로 변경하는게 깔끔한 해결책인 것 같습니다.

제가 위와 같이 말했던 의도는 아래와 Controller에서 아래와 같이 (원래 있었던) canHit() 메서드를 public으로 두고, 아래와 같이 두는 것을 생각해서 말씀드렸던거였어요. 😃

    private void processDealerTurn(Dealer dealer, CardDeck deck) {
        if (dealer.canHit()) {
            dealer.hit(deck);
            outputView.printDealerHitSuccess();
        }
    }

말씀하신 것처럼 예외를 던졌는데, catch하여 아무런 처리를 하지 않는게 더 안티패턴이라고 생각해요. 😃
하지만 정상적인 흐름에서 Catch를 하여 로직을 처리하는 것도 좋은 처리라고 생각되지는 않는 것 같아요. 🤔

아래의 글을 참고해보시면 좋을 것 같아요.
좋은 예외(Exception) 처리

Copy link
Author

Choose a reason for hiding this comment

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

자료 감사합니다!! 이번 피드백을 통해 예외를 활용하는 방식에 대해 다시 한번 고민해볼 수 있었습니다.
특히, 정상적인 흐름에서 catch를 사용하여 로직을 처리하는 것은 안티패턴이 될 수 있다는 점을 배울 수 있었습니다.

try-catch를 활용하여 예외가 발생했을 때 outputView에서 실패 메시지를 출력하는 방식으로 처리했지만,
이 방식은 예외를 단순한 흐름 제어 도구로 사용하는 것이 되어버려,
오히려 예외의 의미인 비정상적인 상황을 알리는 것과 맞지 않다는 문제가 있었던 것 같습니다.
로키의 피드백대로 현재 딜러의 canHit() 메서드를 public으로 두고, 컨트롤러에서 이를 활용해 hit()을 호출하도록 수정했습니다.

다만, 이 과정에서 Gamer의 hit()을 추상 메서드로 둘 필요가 있을까? 하는 고민이 생겼고,
결국 hit() 자체의 동작은 동일하고, 차이는 행위를 수행하기 전의 검증 로직에서 발생한다는 점을 발견했습니다.

기존에는 Player와 Dealer가 hit()을 오버라이드하여 차이를 두었지만,
리팩토링 후에는 Gamer의 hit()을 공통 메서드로 두고, canHit()을 추상 메서드로 만들어 개별적인 검증을 수행하도록 변경했습니다.

// Gamer
    public void hit(CardDeck deck) {
            validateHitState();
            addFrom(deck);
        }

    public void validateHitState() {
        if (!canHit()) {
            throw new IllegalStateException("[ERROR] 카드를 더 뽑을 수 없는 상태입니다.");
        }
    }

    public abstract boolean canHit();

// Player
    @Override
    public boolean canHit() {
        return !isBurst();
    }

// Dealer
    @Override
    public boolean canHit() {
        return getHand().calculateTotalPoint() <= HIT_THRESHOLD && !isBurst();
    }


public class Cards {

private static final int BURST_SCORE = 22;
Copy link

Choose a reason for hiding this comment

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

버스트 점수가 존재한다기보다는 블랙잭 점수가 존재하고 이를 넘는 경우가 버스트가 되는게 아닐까요? 🤔

Copy link
Author

@jinu0328 jinu0328 Mar 9, 2025

Choose a reason for hiding this comment

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

블랙잭에서 버스트는 특정 점수가 아닌, 최고 점수를 초과하는 개념이므로, 로키의 말대로 BURST_SCORE보다는 최고 점수인 21을 상수화하는 것이 더 적절할 것 같습니다!

추가로 블랙잭 게임에 대해 다시 알아보니, 단순히 21을 '블랙잭 점수'라고 부르는 것이 아니라, 처음 분배받은 2장의 합이 21일 때만 '블랙잭'이라고 한다는 점을 알게 되었습니다.
따라서, 혼동을 줄이기 위해 21을 HIGHEST_SCORE라는 이름으로 상수화하였습니다.

상수화 기준에 대해 다시 한번 고민해볼 수 있었습니다. 감사합니다!

Copy link

Choose a reason for hiding this comment

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

저희 둘다 하나씩 놓친부분이 있었군요? 😁
매우 좋은 네이밍이라 생각됩니다. 👍👍


private final InputView inputView;
private final OutputView outputView;
private final Cards deck;
Copy link

Choose a reason for hiding this comment

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

Controller는 도메인 객체가 아닌데, deck 이라는 도메인 객체를 상태로 가지고 있네요. 🤔
메서드 내에서 필요한 시점에 생성해서 사용하는 것은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

기존에 deck을 DI로 받았던 이유는 한 게임 내에서 deck이 하나만 존재해야 한다고 생각했기 때문입니다.
그러나 다시 살펴보니, run() 메서드에서 초기에 생성하더라도 한 게임 내에서 deck은 충분히 하나만 유지되는 구조였습니다.
오히려 게임을 여러 번 진행하는 경우, 이전 게임의 deck을 그대로 참조하게 되는 문제가 발생할 가능성이 있다는 점을 발견했습니다.
따라서, 로키의 피드백대로 run() 메서드 내에서 매번 새로운 deck을 생성하는 것이 더 적절하다고 판단했습니다.

다만, 리팩토링 과정에서 컨트롤러가 CardsInitializer를 멤버로 가지게 되었습니다.

CardsInitializer는 처음 52장의 카드를 생성하는 역할을 하며, 서비스 성격이 강하다고 생각됩니다.
실제로 블랙잭에서 초기 카드 세팅 방식이 여러 가지 존재한다는 점을 고려했을 때,
Initializer를 컨트롤러에서 멤버로 유지하는 것이 더 유연한 설계라고 판단했습니다.
또한, CardDeck에서 생성자 시점에 직접 초기 카드를 생성하는 방식도 고려했지만,

초기 카드 생성이 private로 감춰지면서 테스트가 어려워진다는 문제
추후 카드 생성 방식이 변경될 경우, CardDeck을 직접 수정해야 한다는 단점
이러한 이유로, 외부에서 List를 받아 CardDeck을 생성하는 방식을 채택했습니다.
결과적으로, 컨트롤러에서 CardsInitializer를 멤버로 유지하는 것이 더 적절하다고 판단했습니다!

Copy link

Choose a reason for hiding this comment

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

또한, CardDeck에서 생성자 시점에 직접 초기 카드를 생성하는 방식도 고려했지만,
초기 카드 생성이 private로 감춰지면서 테스트가 어려워진다는 문제
추후 카드 생성 방식이 변경될 경우, CardDeck을 직접 수정해야 한다는 단점
이러한 이유로, 외부에서 List를 받아 CardDeck을 생성하는 방식을 채택했습니다.

좋은 판단이라고 생각해요. 💯💯

Copy link

@Rok93 Rok93 Mar 10, 2025

Choose a reason for hiding this comment

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

다만, 리팩토링 과정에서 컨트롤러가 CardsInitializer를 멤버로 가지게 되었습니다.
CardsInitializer는 처음 52장의 카드를 생성하는 역할을 하며, 서비스 성격이 강하다고 생각됩니다.

먼저 에드에세 질문드리고싶은데요.
서비스는 아직 우테코 교육 과정에서는 배우지 않는 개념일 것 같은데요.

에드는 서비스가 뭐라고 생각하시나요? 🤔

실제로 블랙잭에서 초기 카드 세팅 방식이 여러 가지 존재한다는 점을 고려했을 때,
Initializer를 컨트롤러에서 멤버로 유지하는 것이 더 유연한 설계라고 판단했습니다.

유연한 설계라고 생각하신 이유를 조금 더 말씀해주시면 좋을 것 같아요. 🤔
저는 왜 Controller의 멤버 변수로 CardsInitializer를 두는 것이 유연한 설계인지 잘 이해가 안되는데요.
CardsInitializer를 멤버로 가지는게 유연하다는 부분이 잘 이해가 안되는 것 같습니다.

저는 CardsInitializer를 (상태 없이 static 메서드만 존재하는) 팩터리 메서드로 만든다면, 굳이 상태로 두지 않고도 지금의 구조와 비슷하게 쓸 수 있을 것 같아서요. 🙂

Copy link
Author

Choose a reason for hiding this comment

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

에드는 서비스가 뭐라고 생각하시나요? 🤔

서비스는 도메인 객체들의 행동을 조합하여 더 큰 기능을 수행하며, 자체적인 상태를 가지지 않는 역할이라고 생각합니다.
즉, 개별 도메인 객체(Card, Deck 등)의 상태를 관리하기보다는, 그 객체들을 활용하여 특정한 비즈니스 로직을 수행하는 클래스가 서비스에 해당한다고 생각합니다!

CardsInitializer는 초기 카드 생성을 담당하는 비즈니스 로직을 포함하고 있지만, 내부적으로 별도의 상태를 유지하지 않으며, 단순히 Card 객체들을 생성하여 List<Card>를 반환하는 역할을 수행하기에 서비스에 가까운 역할을 한다고 판단하였습니다.

유연한 설계라고 생각하신 이유를 조금 더 말씀해주시면 좋을 것 같아요. 🤔

CardsInitializer를 인터페이스로 분리하고, 52장의 카드를 생성하는 기본 방식(StandardCardsInitializer)을 별도의 구현체로 두면,
추후 다양한 카드 생성 방식이 필요할 때 새로운 구현체를 추가하는 방식으로 유연하게 대응할 수 있다고 생각했습니다.

예를 들어,

  • 무작위 카드를 제외하여 덱을 생성하는 방식
  • 총 4개의 덱(208장)으로 게임을 진행하는 방식 (카드 카운팅을 막기 위해 이러한 방식도 사용된다고 합니다!)

과 같은 요구사항이 생길 경우, 새로운 구현체만 추가하면 쉽게 적용 가능합니다.

1차 리팩토링 당시에는 이 점을 고려했지만, 실제 코드에는 반영하지 않아서 로키에게 전달력이 부족했던 것 같습니다.
이번 리팩토링을 진행하면서 이 개념을 적용해보았고,
컨트롤러가 카드 초기화 방식을 주입받는 것이 아닌 생성 시점에서 직접 주입하는 방식(new CardDeck(new StandardCardsInitializer()))으로 변경하였습니다.
이 방식이 CardDeck에 카드 초기화 책임을 주면서도 초기화 방식이 바뀌었을 경우 유동적으로 대처할 수 있는 방식이라고 생각하였습니다!

이러한 방식으로 변경하면서 컨트롤러의 책임을 줄이고, CardDeck이 초기화 방식을 명확하게 관리할 수 있도록 개선하였습니다!

Copy link

Choose a reason for hiding this comment

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

이렇게 많은 생각(?)들을 하신지는 전혀 몰랐었어요. 😲
다소 전달하기 어려운 내용이었을 것 같은데 잘 전달해주셔서 의도한 바가 잘 이해됐습니다. 😉

결론은 두 사람이 모두 만족하는 결과를 잘 이끌어내셨군요? 👍💯


## 도메인

- [x] 딜러와 플레이어에게 카드를 2장씩 나누어준다.
Copy link

Choose a reason for hiding this comment

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

기능 구현 목록표 꼼꼼하게 잘 작성해주셨네요. 👍💯

import java.util.List;
import java.util.Objects;

public class Cards {
Copy link

Choose a reason for hiding this comment

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

질문
현재처럼 하나의 Cards 클래스로 덱과 보유 카드를 함께 관리하는 것이 좋은 설계일까요?
아니면 덱과 Gamer의 보유 카드를 명확히 분리하는 것이 더 나은 방향일까요?
혹시 더 적절한 구조가 있다면 조언 부탁드립니다!

저는 역할에 따라 객체를 분리하는 것이 더 좋다고 생각해요. 😃

카드를 묶어서 관리하는 객체가 이미 존재하므로, 굳이 덱과 플레이어의 카드 보유를 분리할 필요가 있는가?
역할이 다르더라도, 기존 Cards 클래스에 필요한 메서드를 추가하여 충분히 관리할 수 있다면 하나의 클래스로 유지하는 것이 적절한가?

위와같은 사고를 거쳐서 Cards로 플레이어가 보유한 카드와 덱 뭉치를 동일하게 사용하고 계신데요.
이렇게 생각하고 구현하게되면 기존에 구현해둔 객체는 계속해서 비대해지게 될거고, 추후 자연스럽게 여러 역할을 수행하게될거에요. 이런 관점에서 역할이 다르다면 다른 객체로 구현하는 것이 좋다고 생각하는데, 말씀하신 것처럼 Deck은 사실 Cards의 기능과 비슷한 요소도 꽤 많아보이는 것 같아요. 이런 경우에는 Deck이 Card를 상태로 가지는 조합 방식을 고민해볼 수 있을 것 같아요. 😃

Copy link
Author

@jinu0328 jinu0328 Mar 9, 2025

Choose a reason for hiding this comment

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

그동안 저는 객체에 책임을 위임할 때, 해당 책임을 수행하기에 가장 적절한 데이터를 보유한 객체에 위임하는 것을 우선적으로 고려했습니다.

Deck에서 전체 카드를 관리하고, 한 장씩 카드를 빼는 기능은 카드 리스트(List)를 관리하는 Cards가 가장 적절하다고 판단했습니다.
Player의 Cards도 마찬가지로 플레이어가 자신의 카드를 관리하며 점수를 계산하는 책임을 가지는 것이 자연스럽다고 생각했습니다.
따라서 모든 책임을 Cards에 위임하는 것이 별도의 클래스로 분리하는 것보다 더 설득력 있다고 판단했었습니다.

그러나 실제로 Cards에서 모든 책임을 가지고있다보니 다음과 같은 문제가 있었습니다.

  • Cards가 Deck의 역할을 수행할 때는 점수 계산, 카드 추가 등의 기능이 필요하지 않음
  • 반대로, Cards가 플레이어의 카드 목록 역할을 할 때는 extract()(카드 제거) 같은 기능이 필요하지 않음

즉, 하나의 클래스가 "덱"과 "플레이어의 카드 보유"라는 서로 다른 역할을 동시에 수행하게 되면서 불필요한 책임까지 포함하게 되었습니다.
이 과정을 통해, 클래스를 분리할 때는 "책임"뿐만 아니라 "역할"까지 고려해야 자연스러운 구조가 나온다는 점을 깨닫게 되었습니다.

현재 블랙잭의 용어를 찾아보고 플레이어의 카드목록은 Hand 클래스가 전체 카드 뭉치는 CardDeck에서 관리하고있습니다.
리팩토링 된 내용이 로키가 의도한 바가 맞는 지 궁금합니다!

Copy link

Choose a reason for hiding this comment

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

이 코멘트를 남겼을 때는 Cards를 이미 만드셨기 때문에 CardDeck에서도 Player나 Dealer에서도 쓰는 쪽을 고려해서 말씀드렸는데, 사실 처음의도는 에드가 말씀하신 의도에 더 가깝긴합니다. 😃
엄밀하게 말하면 아예 역할을 나누기 때문에 상태는 같더라도 다른 책임을 갖는 것이 맞다고 생각해요

}
}

@Override
Copy link

Choose a reason for hiding this comment

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

Player는 이름만 같으면 동등하다고 판단되는군요? 🤔
저는 동명이인의 케이스라던지 같은 이름이라도 카드 갯수가 다르면 동등하다고 판단하기 어렵다고 생각하는데, 에드는 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

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

처음 요구사항을 정리할 때, 플레이어의 이름 중복을 예외 상황으로 고려하여 구현한 부분입니다.

하지만 로키의 말처럼, 동명이인의 케이스를 생각해보았을 때, 이름을 기준으로 플레이어를 구분하는 것은 적절치 않다고 생각합니다!

또한, 이름과 카드가 모두 동일하더라도 실제로는 다른 사람일 가능성이 있기 때문에, 플레이어 객체의 동등성을 이름만으로 판단하는 것은 적절하지 않다고 생각됩니다.

equals를 오버라이드 하는 부분을 제거 완료하였습니다!

this.state = state;
}

public static Result judge(Cards dealerCards, Cards playersCards) {
Copy link

Choose a reason for hiding this comment

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

요구사항 중 함수(또는 메서드)의 길이가 10라인을 넘어가지 않도록 구현한다. 라는 요구사항이 있는데,
요구사항에 맞게 수정해보시면 좋을 것 같아요.

return WIN;
}

if (dealerScore > playerScore) {
Copy link

Choose a reason for hiding this comment

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

dealer 기준에서 승패를 판단하는걸까요? 🤔
그렇다면 dealer 객체에게 승패를 판단하는 역할을 위임해주면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

BlackJackMatchResult에서 온전히 담당했던 승패 판단 책임을 일부 Hand에 위임하는 것으로 리팩토링 하였습니다! 현재 Hand에서는 버스트 된 경우의 승패 결과를 직접 반환하며 이외의 경우에는 내부적으로 BlackJackMatchResult의 judge() 메서드를 호출하여 승패결과를 반환하고 있습니다.

로키의 피드백대로 딜러가 승패를 판단하도록 위임하는 방식도 고려해보았습니다.
그러나 현재 제 구조상 승패를 결정하는 기준은 딜러가 아닌 Hand끼리의 비교하는 과정에서 결정되기 때문에 Hand가 비교 기능을 직접 가지는 것이 더 적절하다고 판단하였습니다!

적절한 리팩토링이었는지 조언 부탁드립니다!

refactor: Hand가 승패 판별 책임을 일부 가지도록 변경

@@ -0,0 +1,47 @@
package domain;

public enum Result {
Copy link

Choose a reason for hiding this comment

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

네이밍이 조금 포괄적이라는 생각이 들어요. 🤔
추후 result 같은 네이밍은 자주 나올 수 있을 것 같은데요. 어떤 result인지 구분되는 네이밍을 지정해보면 어떨까요?
(Number도 유사한 것 같네요)

Copy link
Author

Choose a reason for hiding this comment

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

Result -> BlackJackMatchResult,
Number -> CardRank로
네이밍 개선하였습니다!

}

public boolean isAce() {
return number.equals(Number.ACE);
Copy link

Choose a reason for hiding this comment

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

enum class의 경우 동일성 판단을해도 동일한 결과를 얻을 수 있어요.

Suggested change
return number.equals(Number.ACE);
return number == Number.ACE;

Copy link
Author

@jinu0328 jinu0328 Mar 9, 2025

Choose a reason for hiding this comment

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

각 인스턴스가 1개만 존재하는 enum의 경우 equals보다는 메모리 주소로 비교하는 ==이 미세하지만 성능적인 이점이 존재하며 null의 경우도 안전하게 체크 가능하다는 것을 알게 되었습니다! 감사합니다

jinu0328 added 8 commits March 9, 2025 16:01
- Gamer 클래스에서 hit() 메서드를 추상 메서드로 통일
- Dealer의 hit()에서 카드를 뽑을 수 없는 경우 예외를 던지도록 수정
- processDealerTurn()에서 예외 발생 시 실패 메시지를 출력하도록 변경
- 블랙잭 승패를 나타내는 Enum 이름을 `Result` → `BlackjackGameOutcome`으로 변경하여 의미 명확화
- 카드 숫자를 표현하는 Enum 이름을 `Number` → `CardRank`로 변경하여 J, Q, K 포함 개념을 반영
- 플레이어와 딜러가 보유한 카드는 Hand에서 관리
- 전체 게임에서 쓰이는 카드 뭉치는 CardDeck으로 관리
- 기존 BlackjackMatchResult에서 처리하던 버스트 로직을 Hand에서 수행하도록 변경
- handleBurstCases() 메서드를 추가하여 핸드 객체가 자신의 상태를 판단하도록 개선
- 점수 비교는 기존처럼 BlackjackMatchResult에서 수행하되, Hand에서 직접 호출하도록 변경
- 코드 가독성 및 역할 분리를 통해 유지보수성 향상
Copy link

@Rok93 Rok93 left a comment

Choose a reason for hiding this comment

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

에드 피드백 반영 잘해주셨어요. 👍👍
조금 더 짚고 넘어가면 좋을 부분이있어서, 추가로 코멘트 남겨두었습니다.
확인 부탁드릴게요. 🙏

Map<Player, BlackjackMatchResult> playerResult = new HashMap<>();

for (Player player : players) {
BlackjackMatchResult result = dealer.getHand().compareWith(player.getHand());
Copy link

Choose a reason for hiding this comment

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

로키의 피드백대로 딜러가 승패를 판단하도록 위임하는 방식도 고려해보았습니다.
그러나 현재 제 구조상 승패를 결정하는 기준은 딜러가 아닌 Hand끼리의 비교하는 과정에서 결정되기 때문에 Hand가 비교 기능을 직접 가지는 것이 더 적절하다고 판단하였습니다!

dealer 객체에게 메시지를 보내보면 어떨까요?

현재는 dealer 객체의 상태를 직접 외부로 꺼내서 처리하고있는데요.
dealer 객체에게 메시지를 보내서 dealer가 스스로 판단하여 처리하도록 해보면 좋을 것 같습니다. 😃

Copy link
Author

Choose a reason for hiding this comment

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

피드백 감사합니다! 객체에 메시지를 먼저 보내서 결과값을 도출해내는 것을 먼저 고려했어야 하는데 getter가 이미 존재하다보니 무의식적으로 빠르게 구현 가능한 방법으로 구현해버린 것 같습니다. 다음과 같은 메서드 구현 완료하였습니다!

// Dealer

public BlackjackMatchResult determineMatchResultAgainst(Player player) {
        return getHand().determineMatchResultAgainst(player.getHand());
    }

cards.add(card);
}

public BlackjackMatchResult compareWith(Hand other) {
Copy link

Choose a reason for hiding this comment

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

적절한 리팩토링이었는지 조언 부탁드립니다!

이 부분도 Hand 객체에게 승패 판단 로직을 온전히 위임하게되면, 승,패,무는 누구의 기준에서 결정이 된걸까요?

지금 같은 경우에는 호출되는 순서가 바뀌기도 너무 쉬운 상태라고 생각해요.
(ex. dealer.getHand().compareWith(player.getHand())와 player.getHand().compareWith(dealer.getHand())의 결과 값을 외부에서 봤을 떄, 누구의 승패인지 바로 파악하기에는 조금 어렵지 않을까요?)

Copy link
Author

Choose a reason for hiding this comment

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

로키의 말대로 호출 순서에 따라 결과값이 바뀌며, 승패의 주체가 누구인지 파악하기 어려운 점이 문제라고 생각했습니다.

이에 따라, 승패의 결과를 딜러에게 메시지를 보내 얻는 방식으로 리팩토링하면서,
메서드의 의미를 보다 명확하게 하기 위해 다음과 같이 메서드명을 구체화하였습니다.

  • dealer.determineMatchResultAgainst(player)
    -> 매개변수로 전달된 player에 대한 딜러의 승패 결과를 반환한다는 의미가 직관적으로 드러나도록 수정하였습니다.

또한, 딜러의 승패가 결정되면 플레이어의 승패는 자연스럽게 반대가 되므로, 별도로 중복 계산할 필요가 없다고 판단했습니다.
이를 위해 BlackjackMatchResultreverse() 메서드를 추가하여, 승패 결과를 쉽게 반전시킬 수 있도록 하였습니다.

public BlackjackMatchResult reverse() {
    if (this == WIN) return LOSE;
    if (this == LOSE) return WIN;
    return DRAW;
}

Copy link

Choose a reason for hiding this comment

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

괜찮은 방식이네요. 👍💯

Comment on lines 43 to 44


Copy link

Choose a reason for hiding this comment

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

불필요한 줄바꿈은 제거해주세요. 🙏

Suggested change

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 16 to 36
public BlackjackMatchResult compareWith(Hand other) {
BlackjackMatchResult burstResult = handleBurstCases(other);
if (burstResult != null) {
return burstResult;
}

return compareScores(other);
}

private BlackjackMatchResult handleBurstCases(Hand other) {
if (this.isBurst() && other.isBurst()) {
return BlackjackMatchResult.DRAW;
}
if (this.isBurst()) {
return BlackjackMatchResult.LOSE;
}
if (other.isBurst()) {
return BlackjackMatchResult.WIN;
}
return null;
}
Copy link

Choose a reason for hiding this comment

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

null 반환을 한 뒤, null 체크를 하는 부분을 개선해보면 좋을 것 같은데요. 😃
아래와 같이 변경해보는건 어떨까요? (네이밍은 예시이니 더 좋은 네이밍이 있다면 그 네이밍을 사용해주세요. 😉)

Suggested change
public BlackjackMatchResult compareWith(Hand other) {
BlackjackMatchResult burstResult = handleBurstCases(other);
if (burstResult != null) {
return burstResult;
}
return compareScores(other);
}
private BlackjackMatchResult handleBurstCases(Hand other) {
if (this.isBurst() && other.isBurst()) {
return BlackjackMatchResult.DRAW;
}
if (this.isBurst()) {
return BlackjackMatchResult.LOSE;
}
if (other.isBurst()) {
return BlackjackMatchResult.WIN;
}
return null;
}
public BlackjackMatchResult compareWith(Hand other) {
if (this.isBurst() || other.isBurst()) {
return checkBurstResult(other);
}
return compareScores(other);
}
private BlackjackMatchResult checkBurstResult(Hand other) {
if (this.isBurst() && other.isBurst()) {
return BlackjackMatchResult.DRAW;
}
if (this.isBurst()) {
return BlackjackMatchResult.LOSE;
}
return BlackjackMatchResult.WIN;
}

Copy link
Author

Choose a reason for hiding this comment

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

피드백 감사합니다!
확실히 로키가 추천해주신 방식이 불필요한 null 체크를 제거할 수 있고,
둘 중 하나라도 버스트일 경우에만 checkBurstResult()를 호출하도록 분기가 나뉘어 있어
가독성이 훨씬 좋아졌다는 점을 실감했습니다.

private int sumPointWithoutAce() {
return cards.stream()
.filter(card -> !card.isAce())
.mapToInt(card -> card.getNumber().getPoint())
Copy link

Choose a reason for hiding this comment

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

이전 코멘트를 참고해주시면 좋을 것 같아요.

this.cards = cards;
}

public Card extractCard() {
Copy link

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.

가장 일반적으로 사용되는 용어인 "drawCard"로 변경하였습니다!

return hand.calculateTotalPoint();
}

public void addFrom(CardDeck deck) {
Copy link

Choose a reason for hiding this comment

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

Dealer와 Player 에서만 사용되는 것 같은데요.
캡슐화를 완벽하게할 수는 없지만, 가능한 접근 제한자를 제한해주면 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

변경 완료하였습니다!

@@ -0,0 +1,18 @@
package domain;

public enum Symbol {
Copy link

Choose a reason for hiding this comment

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

마찬가지로 이 부분도 네이밍적으로 CardSymbol 처럼 두는게 조금 더 명확한 네이밍이될 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

CardSymbol로 네이밍 개선 완료하였습니다!

Comment on lines 14 to 23
Hand dealerHand = new Hand();
Hand playerHand = new Hand();

dealerHand.add(new Card(Symbol.COLVER, CardRank.KING));
dealerHand.add(new Card(Symbol.COLVER, CardRank.KING));
dealerHand.add(new Card(Symbol.COLVER, CardRank.TWO));

playerHand.add(new Card(Symbol.COLVER, CardRank.KING));
playerHand.add(new Card(Symbol.COLVER, CardRank.KING));
playerHand.add(new Card(Symbol.COLVER, CardRank.THREE));
Copy link

Choose a reason for hiding this comment

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

Hand 객체를 조금 더 편리하게 만들 수 있도록 테스트를 위한 fixture 함수를 만들어보면 좋을 것 같아요.

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 36 to 39
//when

//then
assertThat(five.getPoint()).isEqualTo(5);
Copy link

Choose a reason for hiding this comment

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

given, when, then 주석을 적으셨으니, 가능하면 given, when, then 구조를 지켜서 테스트 코드를 짜보시면 좋을 것 같아요. 😃
테스트 코드도 유지보수 대상이라는 측면에서 이 구조를 지키며 테스트 코드 짜는 것이 매우 의미있다고 생각해요. 😉

Suggested change
//when
//then
assertThat(five.getPoint()).isEqualTo(5);
//when
int actual = five.getPoint()
//then
assertThat(�actual).isEqualTo(5);

Copy link
Author

Choose a reason for hiding this comment

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

좋은 피드백 감사합니다!
given, when, then 구조가 단순한 템플릿이 아닌 유지보수 측면에서 더 의미 있는 방식이라는 점을 알 수 있었습니다.
피드백 반영하여 개선해보겠습니다!

jinu0328 added 12 commits March 11, 2025 15:04
- Gamer의 hit()을 공통메서드로 변경 후 추상메서드 canHit() 구현
- Dealer의 hit() 호출 시 예외를 활용한 흐름 제어 제거
- canHit()을 활용하여 미리 검증하는 방식으로 변경
- CardsInitializer를 인터페이스로 추출하여 초기화 방식 확장 가능하도록 변경
- StandardCardsInitializer를 기본 구현체로 추가
- CardDeck이 생성 시점에서 초기화 방식을 주입받도록 수정
- Card 객체의 필드는 enum 뿐이므로 불필요한 객체 재생성 제거하고 방어적 복사 유지
Copy link

@Rok93 Rok93 left a comment

Choose a reason for hiding this comment

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

에드 피드백 반영 잘해주셨습니다. 👍💯
1단계는 이정도면 충분한 것 같아서 이만 머지하도록하겠습니다. 👏👏
추가적으로 몇몇 코멘트 남겨두었으니 다음 단계 진행하시면서 같이 확인 부탁드릴게요. 😃

return hand.calculateTotalPoint();
}

private void addFrom(CardDeck deck) {
Copy link

Choose a reason for hiding this comment

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

개인적인 생각으로는 addFrom 이라던지, validateHitState 메서드의 경우 굳이 메서드로 뽑지 않아도 hit 메서드 내에 정의하는게 더 코드가 잘 읽히지 않을까 싶은데요.

addFrom은 prepareGame에서도 사용되고있으니, 이해가되는 것 같은데, validateHitState는 메서드로 뽑지 않아도 될 것 같아서요. 🤔 에드는 어떻게 생각하시나요?

Comment on lines +9 to +10
private static final int HIGHEST_SCORE = 21;
private final List<Card> cards = new ArrayList<>();
Copy link

Choose a reason for hiding this comment

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

상수와 상태는 줄바꿈을 통해서 구분해주면 가독성이 더 좋아질 것 같아요. 😃

Suggested change
private static final int HIGHEST_SCORE = 21;
private final List<Card> cards = new ArrayList<>();
private static final int HIGHEST_SCORE = 21;
private final List<Card> cards = new ArrayList<>();

Comment on lines +24 to +38
private BlackjackMatchResult checkBurstResult(Hand other) {
if (this.isBurst() && other.isBurst()) {
return BlackjackMatchResult.DRAW;
}
if (this.isBurst()) {
return BlackjackMatchResult.LOSE;
}
return BlackjackMatchResult.WIN;
}

private BlackjackMatchResult compareScores(Hand other) {
int thisScore = this.calculateTotalPoint();
int otherScore = other.calculateTotalPoint();
return BlackjackMatchResult.judge(thisScore, otherScore);
}
Copy link

Choose a reason for hiding this comment

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

BlackJackMatchResult에서 온전히 담당했던 승패 판단 책임을 일부 Hand에 위임하는 것으로 리팩토링 하였습니다! 현재 Hand에서는 버스트 된 경우의 승패 결과를 직접 반환하며 이외의 경우에는 내부적으로 BlackJackMatchResult의 judge() 메서드를 호출하여 승패결과를 반환하고 있습니다.

로키의 피드백대로 딜러가 승패를 판단하도록 위임하는 방식도 고려해보았습니다.
그러나 현재 제 구조상 승패를 결정하는 기준은 딜러가 아닌 Hand끼리의 비교하는 과정에서 결정되기 때문에 Hand가 비교 기능을 직접 가지는 것이 더 적절하다고 판단하였습니다!

이 부분은 Hand로 직접 비교하게될 경우에 Dealer와 Player를 반대로 점수를 주입하게되면 승패가 바뀌게 되는 문제가 발생해서 이 부분을 우려한거였는데요. 실질적으로 Dealer가 내부적으로 포워딩하는 방식으로 승패를 판단하고있군요. 😃
그렇다면 크게 우려되는 부분은 없는 것 같습니다.

cards.add(card);
}

public BlackjackMatchResult compareWith(Hand other) {
Copy link

Choose a reason for hiding this comment

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

괜찮은 방식이네요. 👍💯

assertThat(actual).isEqualTo(expected);
}

@DisplayName("Ace가 있을 때, 11로 간주해도 21을 초과하지않을 경우 11로 간주한다.")
Copy link

Choose a reason for hiding this comment

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

DisplayName이 모두 동일한 것 같은데요.
동료가 이 DisplayName을 보면서 테스트를 파악할 때, 이 테스트가 어떤 테스트를 진행한 것인지 조금 이해하기 어렵지 않을까요? 🤔

아마 DisplayName이 동일한 이유는 같은 테스트지만 parameter만 다른 케이스라서 그런 것 같아요. 😃
이런 경우에는 Parameterized Test를 활용해서 하나의 테스트 케이스로 동일한 테스트 케이스를 테스트할 수 있을 것 같아요. 😉

Comment on lines +123 to +127
List<Card> cards = new ArrayList<>();
cards.add(card1);
cards.add(card2);
cards.add(card3);
cards.add(card4);
Copy link

Choose a reason for hiding this comment

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

컬렉션 팩토리를 활용해볼 수 있을 것 같아요.

Suggested change
List<Card> cards = new ArrayList<>();
cards.add(card1);
cards.add(card2);
cards.add(card3);
cards.add(card4);
List<Card> cards = List.of(card1, card2, card3, card4);


private final InputView inputView;
private final OutputView outputView;
private final Cards deck;
Copy link

Choose a reason for hiding this comment

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

이렇게 많은 생각(?)들을 하신지는 전혀 몰랐었어요. 😲
다소 전달하기 어려운 내용이었을 것 같은데 잘 전달해주셔서 의도한 바가 잘 이해됐습니다. 😉

결론은 두 사람이 모두 만족하는 결과를 잘 이끌어내셨군요? 👍💯

@Rok93 Rok93 merged commit fcca80b into woowacourse:jinu0328 Mar 12, 2025
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