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단계 - 블랙잭] 머랭(김대현) 미션 제출합니다. #840

Merged
merged 43 commits into from
Mar 11, 2025

Conversation

cookie-meringue
Copy link

체크 리스트

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

프롤로그 링크

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

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

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

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

CardNumber 에서 List 를 사용해 해당 카드 번호가 가질 수 있는 값들을 저장하고 있습니다.
하지만, 이는 Rule 의 책임이라 생각이 듭니다.
또한, View 가 Model 을 지나치게 의존하고 있는 것 같습니다.
리뷰 남겨주시면 빠르게 리팩터링하겠습니다.

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

1. 유저와 딜러의 동작 구분을 위한 Role 사용

유저와 딜러의 기초적인 동작들(카드들을 받는다, 가능한 모든 점수를 반환한다)을 공통으로 분리하기 위해 추상 클래스를 사용했습니다.
그러나, 유저와 딜러의 동작이 달라야 하는 경우가 있었습니다(Rule 의 canDrawCards()). 이 기능을 구현하기 위해 두 가지 방법이 있다고 생각했습니다.

방법 1 : instanceOf 를 사용
방법 2: Player(추상클래스) 에 Role 필드를 두고 이를 사용

저는 방법 2를 선택했습니다. instanceOf 는 Player 의 구현체들에 대해 클래스 수준까지 알아야 하기 때문입니다(Dealer.class, User.class).
이 선택에 대해 어떻게 생각하시는지 궁금합니다.

2. Rule 을 의존하는 존재들

BlackJackGame 은 게임의 흐름을 책임지게 하기 위해 설계한 클래스입니다.
게임의 흐름과 구체적인 규칙은 다른 책임이라 생각해 Rule 클래스를 설계했습니다.
그러나, 구현하다 보니 외부�에서 Rule 을 사용하는 경우가 많아졌습니다.

시간이 없어 리팩터링하지는 못했지만, 저는 이것이 문제라고 생각했습니다.

Rule 은 BlackJackGame 을 진행하기 위한 요소인데 외부에서 사용하는 것이 올바른가? 에 대한 의문이 들었기 때문입니다.
이런 제 생각에 대해 어떻게 생각하시는지 궁금합니다.

3. 테스트만을 위한 기본 생성자

BlackJackGame 을 보시면, 두 가지 생성자가 있습니다.

  1. 기본 생성자.
  2. CardDeckInitializer 를 통한 생성자

프로덕션에서는 CardDeckInitializer 를 통한 생성자만 사용합니다.
그러나, CardDeckInitializer 를 통한 생성자만을 만들면, 테스트가 복잡해진다는 상황이 생겼습니다.

테스트만을 위한 기본 생성자를 public 으로 만드는 것에 대해 어떻게 생각하시는지 궁금합니다.

+ 객체지향적이지 못한 부분이 많이 있는 것 같습니다.. 피드백 주시면 빠르게 수정해나가겠습니다!

cookie-meringue and others added 30 commits March 4, 2025 15:33
- 초기화 시 랜덤으로 섞어 주입.
Copy link

@ddaaac ddaaac left a comment

Choose a reason for hiding this comment

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

안녕하세요 머랭 👋

잘 구현해주셨네요.

몇 가지 코멘트 남겼으니 확인해주세요.

import java.util.Map;
import java.util.function.Predicate;

public class Rule {
Copy link

Choose a reason for hiding this comment

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

지금 Rule이 하는 일을 보면

  • 딜러가 카드를 더 받을 수 있는지 판단
  • 플레이어가 카드를 더 받을 수 있는지 판단
  • 첫번째 공개 카드 결정
  • 점수 계산
  • 결과 계산

이 모든 걸 하고 있습니다. 너무 책임이 많아 보여요.
Rule을 제거하는 방향으로 리팩토링해보시면 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

네 감사합니다 ㅎㅎ
저는 블랙잭 게임의 룰은 얼마든지 바뀔 수 있다고 생각했습니다.
추후 여러 가지 룰이 생기게 되면 Rule 을 인터페이스로 분리해 외부에서 주입하면 좋지 않을까? 하는 생각에 Rule 을 분리했는데, 생각해보니 Rule 이 바뀔 일은 거의 없을 것 같고, 만약 바뀌더라도 굳이 Rule 이라는 클래스가 필요하지 않을 것 같습니다!

.orElse(player.getMinimumPoint());
}

public Map<Player, Map<Result, Integer>> calculateResult(final Dealer dealer, final List<User> users) {
Copy link

Choose a reason for hiding this comment

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

자료구조가 중첩된 형태네요. 이런 타입은 가독성을 떨어트립니다. Map<Result, Integer>를 감싸는 클래스를 하나 만들어도 좋아보여요.

Copy link
Author

Choose a reason for hiding this comment

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

그렇게 하는 편이 더 좋아보입니다! 감사합니다.

super(name, Role.DEALER);
}

public Card getFirstCard() {
Copy link

Choose a reason for hiding this comment

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

딜러는 첫 번째에 하나의 카드만 여는 걸 도메인으로 잘 표현해주셨네요.

이런 걸 Player의 추상 메서드로 표현해보는 건 어떨까요?

"유저는 첫 번째 카드 공개 때 2장을 열고, 딜러는 첫 번째 카드 공개 때 1장을 연다"를 코드로 명확하게 표현해볼 수 있을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

Rule 이 없어지니 자연스럽게 말씀해주신대로 되네요 ㅎㅎ Rule 제거와 함께 리팩터링했습니다.

Rule rule = new Rule();
BlackJackGame blackJackGame = new BlackJackGame(cardDeckInitializer, rule);

Dealer dealer = new Dealer("딜러");
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

@cookie-meringue cookie-meringue Mar 9, 2025

Choose a reason for hiding this comment

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

좋은 방법인 것 같습니다 생성자를 추가하는 방법으로 수정했습니다!


private void userDrawMoreCards(final Rule rule, final BlackJackGame blackJackGame, final User user) {
while (rule.canDrawMoreCard(user) && inputView.readUserDrawMoreCard(user)) {
blackJackGame.drawMoreCard(user);
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.

감사합니다

return possiblePoints;
}

private void addPossiblePoint(List<Integer> possiblePoints, int index, int sum) {
Copy link

Choose a reason for hiding this comment

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

블랙잭의 룰을 잘 조사해보면 ACE의 포인트를 복잡하게 계산할 필요가 없다는 걸 알 수 있습니다.

단순히 ACE가 있고 합이 11이하면 +10을 해주는 식으로 구현하면 될 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

저는 ACE 가 1, 11 을 가지고 있다는 룰이 있다고 생각했습니다.
추후 최소한의 확장(다른 카드들도 여러 값을 가질 수 있다)성을 고려해 ACE 임을 명시하지 않았습니다.

이 부분에 대해서 어떻게 생각하시는지 알려주시면 감사하겠습니다!

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.

틀린 말이라는 생각이 들지 않네요 ㅎㅎ 수정하는 편이 좋을 것 같습니다!

+++ 두 번째 리뷰 신청에서 수정한다는 것을 까먹고 리뷰신청을 해 버렸네요.. 세 번쨰 리뷰 신청에 반영해놓겠습니다!

Copy link

Choose a reason for hiding this comment

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

아하 다음 리뷰때 반영해주세요~


protected final String name;
protected final Cards cards;
protected final Role role;
Copy link

Choose a reason for hiding this comment

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

저는 방법 2를 선택했습니다. instanceOf 는 Player 의 구현체들에 대해 클래스 수준까지 알아야 하기 때문입니다(Dealer.class, User.class). 이 선택에 대해 어떻게 생각하시는지 궁금합니다.

instanceOf를 사용하고 싶지 않은 이유가 무엇인가요? 각 Player 추상 클래스의 구현체가 추가될 때마다 Role에도 값을 추가해줘야한다면 instanceOf와 다를 게 없지 않을까요?

구현체 별로 동작이 달라진다면 추상 메서드를 활용해보면 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

Rule 을 제거하면서 추상 메서드로 분리했습니다!

this.rule = rule;
}

public BlackJackGame(final CardDeckInitializer cardDeckInitializer, final Rule rule) {
Copy link

Choose a reason for hiding this comment

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

테스트만을 위한 기본 생성자를 public 으로 만드는 것에 대해 어떻게 생각하시는지 궁금합니다.

잘 구현해주신 것 같아요.

테스트도 클라이언트 코드이고, 클라이언트에서 필요한 건 객체의 책임이 될 수 있다고 생각해서 필요하면 만드는 편입니다.

돌아서 테스트를 만들어야할만큼 getter를 만들지 말아야할지는 잘 모르겠네요.

중요한 건 "getter로 값을 꺼내서 로직을 구현하지 않는다"인 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다!

this.rule = rule;
}

public BlackJackGame(final CardDeckInitializer cardDeckInitializer, final Rule rule) {
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.

생성자 체이닝은 처음 알게 되었네요 알려주셔서 감사합니다!

Copy link

@ddaaac ddaaac left a comment

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.Map;

public class BlackJackGame {
Copy link

Choose a reason for hiding this comment

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

좀 고민되는 게 BlackJackGame의 메서드들을 보면 딜러나 플레이어를 다 받고 있어요.
그럴거면 딜러와 플레이어를 받아서 초기화해도 되지 않나 싶기도 하네요.

뭔가 플레이어가 카드를 뽑을 떄 View와 연관이 있어서 이렇게 구현했나 싶기도 한데, 어떻게 잘 해결할 수 있지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

게임이 딜러와 유저들을 가지고 있는 것은 좋은 방법이라 생각했습니다.

하지만, 현재 구조는 view 를 통해 유저가 계속 뽑을 지 결정해야 하는 구조입니다.
이를 해결하기 위해 Predicate<User> 를 받는 drawUserCards(Predicate<User> predicate) 를 만들고자 계획했습니다.
Controller 에서는 다음과 같이 호출할 수 있을 것입니다.
blackJackGame.drawUserCards(inputView.readUserDrawMoreCard);
그리고 drawUserCards() 메서드 내부에서는 predicate.test() 가 실패할 때까지 유저의 카드를 더 뽑을 것입니다.
그러나, 카드를 더 뽑는 메서드가 Predicate 를 매개변수로 전달받은 것이 올바른 방법인가? 에 대한 의문이 들었습니다.

계속해서 플레이어에게 물어보며 카드를 뽑는 것은 View 의 영역이고 Model 이 이를 신경쓸 필요가 없지 않은가? 하는 결론으로 도달했는데요, 범블비의 의견을 더 들려주시면 감사하겠습니다!

Copy link

Choose a reason for hiding this comment

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

좋은 질문이네요!

지금과 다르게 BlackJackGame이 유저를 갖고 있게 구현하는 방법은 크게 두 가지일텐데

  • 말씀해주신대로 View 메서드를 함수로 넘기는 방법
  • 아니면 blackjackGame.getUsers() 처럼 getter를 활용하는 방법이 있겠네요.

카드를 더 뽑는 메서드가 Predicate 를 매개변수로 전달받은 것이 올바른 방법인가?

이 질문을 다시 해석해보면 BlackJackGame#drawUserCards(Predicate<User> predicate) 이런 시그니처가 자연스러운가? 객체의 책임을 잘 나타내는가?로 해석할 수 있습니다.
이 코드를 처음 보는 사람 입장에서는 꽤나 해석하기 어려운 시그니처이기도 하구요.

조금 더 다듬자면 자바 표준인 Predicate 대신에 직접 Funtional Interface를 정의해서 사용하는 방법도 있긴 합니다. UserAnswer 등과 같은 이름으로 감싸면 "파라미터로 유저의 대답이 넘어오겠구나" 라는 생각까지 해볼 수 있죠.

계속해서 플레이어에게 물어보며 카드를 뽑는 것은 View 의 영역이고 Model 이 이를 신경쓸 필요가 없지 않은가?

요 부분은 사실 "플레이어가 원할 떄까지 카드를 뽑는다" 라는 요구사항을 어떻게 해석할지에 따라 다른 것 같긴 해요. 솔직히 말하면 어느 쪽이든 크게 상관 없다고 생각하지만, 이렇게 생각해주셨다면 지금 구현 방향이 맞을 것 같네요.

다만, 다른 방법도 한 번 직접 구현해보시면 좋을 것 같아요. 직접 구현해보고 판단하는 것과 상상 속에서 판단하는 건 많이 다르니깐요.

Copy link
Author

Choose a reason for hiding this comment

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

생각하지 못한 방법이 있었네요! 시도해보겠습니다!

.orElse(getMinimumPoint());
}

public abstract Cards openInitialCards();
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.

감사합니다 ㅎㅎ


private void dealInitialCards(final List<User> users, final Player dealer, final BlackJackGame blackJackGame) {
List<Player> allPlayers = new ArrayList<>(users);
allPlayers.add(dealer);
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.

감사합니다!

return possiblePoints;
}

private void addPossiblePoint(List<Integer> possiblePoints, int index, int sum) {
Copy link

Choose a reason for hiding this comment

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

어차피 요구사항은 우리가 예상한대로 변화하지 않습니다. 그래서 요구사항이 어떻게 변할건지 예측하는 건 크게 의미가 없습니다. 오버 엔지니어링에 대한 글을 읽어봐도 좋을 것 같아요.

경우에 따라 다르겠지만, 블랙잭 룰은 크게 변할 것 같진 않아서 저라면 가독성을 높이고 유연함을 낮추는 방향으로 구현할 것 같네요.

}

public boolean isDraw(final Player challenger) {
if (isBust() && challenger.isBust()) {
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.

룰을 잘 몰랐네요.. 수정해보겠습니다!

return calculateOptimalPoint() == challenger.calculateOptimalPoint();
}

public boolean isWin(final Player challenger) {
Copy link

Choose a reason for hiding this comment

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

이 시그니처에 대해서도 생각해볼만 합니다.

블랙잭은 항상 플레이어(User)와 딜러의 대결입니다.

지금 시그니처로 봐서는 딜러 vs 딜러, 플레이어 vs 플레이어의 대결도 가능해보여요. 도메인 지식을 명확하게 드러내는 시그니처를 사용하는 건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

수정해보겠습니다!

Copy link

@ddaaac ddaaac left a comment

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.Map;

public class BlackJackGame {
Copy link

Choose a reason for hiding this comment

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

좋은 질문이네요!

지금과 다르게 BlackJackGame이 유저를 갖고 있게 구현하는 방법은 크게 두 가지일텐데

  • 말씀해주신대로 View 메서드를 함수로 넘기는 방법
  • 아니면 blackjackGame.getUsers() 처럼 getter를 활용하는 방법이 있겠네요.

카드를 더 뽑는 메서드가 Predicate 를 매개변수로 전달받은 것이 올바른 방법인가?

이 질문을 다시 해석해보면 BlackJackGame#drawUserCards(Predicate<User> predicate) 이런 시그니처가 자연스러운가? 객체의 책임을 잘 나타내는가?로 해석할 수 있습니다.
이 코드를 처음 보는 사람 입장에서는 꽤나 해석하기 어려운 시그니처이기도 하구요.

조금 더 다듬자면 자바 표준인 Predicate 대신에 직접 Funtional Interface를 정의해서 사용하는 방법도 있긴 합니다. UserAnswer 등과 같은 이름으로 감싸면 "파라미터로 유저의 대답이 넘어오겠구나" 라는 생각까지 해볼 수 있죠.

계속해서 플레이어에게 물어보며 카드를 뽑는 것은 View 의 영역이고 Model 이 이를 신경쓸 필요가 없지 않은가?

요 부분은 사실 "플레이어가 원할 떄까지 카드를 뽑는다" 라는 요구사항을 어떻게 해석할지에 따라 다른 것 같긴 해요. 솔직히 말하면 어느 쪽이든 크게 상관 없다고 생각하지만, 이렇게 생각해주셨다면 지금 구현 방향이 맞을 것 같네요.

다만, 다른 방법도 한 번 직접 구현해보시면 좋을 것 같아요. 직접 구현해보고 판단하는 것과 상상 속에서 판단하는 건 많이 다르니깐요.

return possiblePoints;
}

private void addPossiblePoint(List<Integer> possiblePoints, int index, int sum) {
Copy link

Choose a reason for hiding this comment

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

아하 다음 리뷰때 반영해주세요~

@ddaaac ddaaac merged commit 2efcce0 into woowacourse:cookie-meringue Mar 11, 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