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

[블랙잭 - 2단계] 아토(이혜린) 미션 제출합니다. #17

Open
wants to merge 86 commits into
base: main
Choose a base branch
from

Conversation

hyxrxn
Copy link
Member

@hyxrxn hyxrxn commented Mar 16, 2024

아직 달린 리뷰가 없어 요청한 코드로 변경해서 다시 pr 보냅니닷

제이미 안녕하세요! 또 한 번 BE 6기 아토입니다.
변경된 기능만 충족할 수 있도록 코드를 수정해보았는데요, 관련해서 설명 드리겠습니다.


미션 진행 시 생각한 부분

1. 승패를 결정하던 MatchResult가 수익 비율을 결정

  • 요구사항에 의해 플레이어만 블랙잭인 경우가 승부 결과 Enum에 추가되었습니다. 그렇게 되며 카드의 수가 중요해져 점수로만 판단하는 것이 불가능해졌습니다. 그래서 Hand를 이용해 승패를 판단하고, 딜러의 Hand는 승부가 진행될 때 계속 사용되고 변경되지 않는 값이므로 매번 넣어주지 않고 생성할 때 넣어주기로 했습니다. 그런데 이렇게 되니 테스트가 복잡해졌습니다. 그리고 Player와 Hand 생성자가 디폴트에서 public으로 변경되었습니다. 그런데 Hand를 이용해 승패 판단을 하는 이상 당연한 결과인 것 같기도 합니다. 로직을 변경하는 게 좋을까요?

2. 플레이어별 배팅 금액을 저장하는 객체 PlayerBettingMoney

  • 플레이어를 key로, 배팅 금액을 value로 하는 Map에 정보를 저장합니다. 그런데 사실 객체가 하는 일은 Map이 하는 일과 거의 비슷합니다. 그러나 BlackJackGame에서 클래스로 의미를 나타내기 쉽다는 점, key가 겹칠 경우나 해당하는 key가 없는 경우에도 좀 더 의미를 담을 수 있다는 점에서 객체를 따로 두는 선택을 했습니다. 이 부분도 TDD가 아니었다면 테스트를 작성하지 않았을 것 같기는 합니다.

3. 결국 실패한 컨트롤러 분리... 😢

  • 입출력이 상당히 얽혀있어서 분리가 좀 힘드네요. (특히나 카드 더 받을지 묻는 부분...) 좀 더 쉬운 분리를 위해서라도 입력 가이드 출력 부분을 InputView로 옮기는 것이 좋을지 고민입니다...!

읽어보시고 의견 혹은 질문 많이많이 주세요!
즐거운 주말 보내시길 바랍니다~
감사합니다!

hyxrxn and others added 30 commits March 5, 2024 14:43
Co-authored-by: donghoony <[email protected]>
- 기능을 추가하며 클래스 분리 (Player -> Hand)

Co-authored-by: donghoony <[email protected]>
- `Card`에 포함되는 모양과 수는 변하지 않고, 불변 객체이므로 그대로 전달

Co-authored-by: donghoony <[email protected]>
- `toList`는 변경 불가능한 리스트를 반환하므로 `unmodifiableList` 제거

Co-authored-by: donghoony <[email protected]>
Co-authored-by: donghoony <[email protected]>
hyxrxn and others added 27 commits March 13, 2024 13:57
- Player의 Hand 접근제한자 변경
* docs: 컨벤션 및 기능 요구사항 작성

Co-authored-by: donghoony <[email protected]>

* chore: remove gitkeep

Co-authored-by: donghoony <[email protected]>

* feat(Card): 카드 점수 계산

Co-authored-by: donghoony <[email protected]>

* feat(Deck): 덱에서 카드 한 장 드로우

Co-authored-by: donghoony <[email protected]>

* feat(Deck): 비어있는 덱에서 드로우할 때 예외 처리

Co-authored-by: donghoony <[email protected]>

* feat(Player): 플레이어 점수 계산

Co-authored-by: donghoony <[email protected]>

* feat(Dealer): 딜러의 점수에 따른 카드 드로우

Co-authored-by: donghoony <[email protected]>

* feat(Dealer): 주어진 점수에 따른 승패 판단

Co-authored-by: donghoony <[email protected]>

* feat(Hand): Ace 점수를 유리한 방향으로 계산

- 기능을 추가하며 클래스 분리 (Player -> Hand)

Co-authored-by: donghoony <[email protected]>

* refactor(Player): 드로우 가능 여부 판단 메서드 위치 변경

Co-authored-by: donghoony <[email protected]>

* feat(Name): 이름 생성 및 검증

Co-authored-by: donghoony <[email protected]>

* feat(Players): 플레이어들 생성 및 검증

Co-authored-by: donghoony <[email protected]>

* feat(View): 콘솔 입출력

Co-authored-by: donghoony <[email protected]>

* feat(CardDisplay): 카드의 모양과 수의 표현 방식 설정

Co-authored-by: donghoony <[email protected]>

* refactor(Player): 게임 상수 추출 및 드로우 조건 수정

Co-authored-by: donghoony <[email protected]>

* feat(MatchResult): 게임 결과 판단

Co-authored-by: donghoony <[email protected]>

* feat(MatchResults): 게임 결과 저장

Co-authored-by: donghoony <[email protected]>

* feat(Deck): Shape와 Number의 조합으로 한 세트 생성

Co-authored-by: donghoony <[email protected]>

* feat(Command): 사용자의 명령어 변환

Co-authored-by: donghoony <[email protected]>

* refactor(OutputView): 매개변수로 `Card`를 받도록 수정

- `Card`에 포함되는 모양과 수는 변하지 않고, 불변 객체이므로 그대로 전달

Co-authored-by: donghoony <[email protected]>

* feat(Display): 플레이어의 결과 표현 방식 설정

Co-authored-by: donghoony <[email protected]>

* feat(BlackJackGame): 게임 흐름 제어

Co-authored-by: donghoony <[email protected]>

* feat(BlackJackMain): 의존성 설정 및 실행

Co-authored-by: donghoony <[email protected]>

* refactor(BlackJackGame): 메서드 분리

Co-authored-by: donghoony <[email protected]>

* refactor(Players): 불필요한 래핑 제거

- `toList`는 변경 불가능한 리스트를 반환하므로 `unmodifiableList` 제거

Co-authored-by: donghoony <[email protected]>

* style(Shape): 불필요한 세미콜론 제거

Co-authored-by: donghoony <[email protected]>

* refactor: 상수 추출

Co-authored-by: donghoony <[email protected]>

* refactor(OutputView): 메서드 분리

Co-authored-by: donghoony <[email protected]>

* refactor: 패키지 이동

Co-authored-by: donghoony <[email protected]>

* test(Player): 드로우 가능 여부 판단

Co-authored-by: donghoony <[email protected]>

* fix(Dealer): 카드를 가지고 있지 않은 경우 예외 처리

Co-authored-by: donghoony <[email protected]>

* refactor(Deck): 덱 생성 정적 메서드 추가

Co-authored-by: donghoony <[email protected]>

* refactor(Hand): 접근 제한자 변경

Co-authored-by: donghoony <[email protected]>

* refactor(Deck): 메서드 분리

Co-authored-by: donghoony <[email protected]>

* refactor(Display): 패키지 분리

Co-authored-by: donghoony <[email protected]>

* style(Number): 파일 끝 공백 추가

Co-authored-by: donghoony <[email protected]>

* refactor: 메서드 순서 변경

Co-authored-by: donghoony <[email protected]>

* refactor: 패키지 이동

- 컨벤션에 맞게 import 순서 변경

Co-authored-by: donghoony <[email protected]>

* style: 불필요한 개행 삭제

- 컨벤션에 맞게 import 순서 변경

Co-authored-by: donghoony <[email protected]>

* style(Hand): 가독성을 위한 변수 위치 및 줄바꿈 수정

Co-authored-by: donghoony <[email protected]>

* refactor(MatchResult): 승리 조건 별 메서드 분리

Co-authored-by: donghoony <[email protected]>

* test(MatchResult): 승리 조건 테스트 데이터 보강

Co-authored-by: donghoony <[email protected]>

* style: 테스트 클래스 접근제한자 삭제

Co-authored-by: donghoony <[email protected]>

* refactor(OutputView): 중복 코드 제거 및 메서드명 번경

Co-authored-by: donghoony <[email protected]>

* style: 개행 및 오타 수정

Co-authored-by: donghoony <[email protected]>

* docs: 기능 구현 사항 추가

* refactor(Card): this 사용 일관성 고려해 수정

- 파라미터와 구분이 되지 않는 경우만 사용

* refactor(InputView): 상수명 수정

* refactor(Display): 메서드명 수정

* test(Card): 에이스 여부 확인

* refactor(Hand): 점수 계산 로직 수정

* refactor: 드로우 관련 메서드명 변경

* refactor(Command): 클래스 내부 메서드로 No 여부 확인

* refactor(Rank): 클래스명 변경

* refactor(MatchResult): 메서드명 변경

* refactor(Players): 메서드 간소화

* refactor: 예외 메시지에서 상수 사용

* refactor: 상호 참조 제외

- 도메인에서 컨트롤러 참조하지 않게 수정

* refactor: 예외 발생시 메시지 출력 후 종료

* refactor(Display): 메서드 리턴 타입 변경

* refactor(OutputView): 상수 추출

* feat(Score): 점수 덧셈

* feat(Score): 버스트 여부 판단

* feat(Score): 점수 대소 비교

* feat(Score): 에이스 보정 점수 계산

* refactor: Score 이용

- Player의 Hand 접근제한자 변경

* test(Deck): 덱 생성

* refactor(Score): 메서드명 번경

---------

Co-authored-by: donghoony <[email protected]>
- 플레이어의 수익 확인
- 딜러의 수익 확인
Copy link
Member

@reddevilmidzy reddevilmidzy 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 +22 to +31
Deck deck = Deck.createShuffledFullDeck();
Dealer dealer = new Dealer();
Players players = createPlayers();
PlayerBettingMoney playerBettingMoney = decideBettingMoney(players);
initializeGame(deck, dealer, players);
proceedPlayersTurn(deck, players);
proceedDealerTurn(deck, dealer);

showCardsWithScore(dealer, players);
showMatchResult(dealer, players, playerBettingMoney);
Copy link
Member

Choose a reason for hiding this comment

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

이번 미션 진행하면서 이 컨트롤러가 많이 커져서 불편하지 않았나요? ㅎㅎㅎ

저도 너무 커져서 Controller있고 아예 다른 Game이라는 객체를 만들어 요 객체 필드에 Dealer와 Players를 가지고 있게 구현하는 방식으로 리팩터링 해보았어요

initializeGame(deck, dealer, players);
proceedPlayersTurn(deck, players);
proceedDealerTurn(deck, dealer);
showCardsWithScore(dealer, players);

이 메서드들 모두 deck, dealer, players로 동일한데 매번 파라미터로 넘겨주는 형태가 뭔가 조금 이상한 거 같더라구요

그래서 저는 deck을 dealer 필드로 넣고, dealer와 players를 Game 이라는 필드에 넣어 컨트롤러의 무게를 조금 줄이는 방법으로 해보았는데, 나쁘진 않은 거 같더라구요!

Comment on lines +19 to +30
public static double calculateRateOfPrize(Hand playerHand, Hand dealerHand) {
if (isPlayerBlackJackCondition(playerHand, dealerHand)) {
return PLAYER_BLACKJACK.rateOfPrize;
}
if (isPlayerWinningCondition(playerHand, dealerHand)) {
return PLAYER_WIN.rateOfPrize;
}
if (isDealerWinningCondition(playerHand, dealerHand)) {
return DEALER_WIN.rateOfPrize;
}
return TIE.rateOfPrize;
}
Copy link
Member

Choose a reason for hiding this comment

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

이 부분을 BiPredicate 를 활용하여 구현할 수도 있을 거 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

오 한 번 알아보겠습니다

Comment on lines +5 to +6
private static final int MIN_NAME_LENGTH = 2;
private static final int MAX_NAME_LENGTH = 5;
Copy link
Member

Choose a reason for hiding this comment

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

이 클래스 이름에서 이미 NAME 이 들어가기에 MIN_LENGTH, MAX_LENGTH로 해두어도 나쁘지 않을 거 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

그러네용

Comment on lines +25 to +31
private void validateInteger(String input) {
try {
Integer.parseInt(input);
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException("[ERROR] 정수가 아닙니다.");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

여기에서 걸리는건 IllegalArgumentException 이 아니라 NumberFormatException 일 거 같은데 맞을까요..?

그리고 추가로 Integer 범위가 넘어가는 수가 입력되어도(ex 3000,000,000) 예외가 터질거 같은데 이때에는 "정수가 아닙니다."라는 메시지가 맞지 않을 거 같아요..!

Copy link
Member Author

Choose a reason for hiding this comment

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

오!!!! 그러네요..... 수정하겠습니다
그런데 왜 catch가 되는걸까요...?

Comment on lines +18 to +35
private static final Hand handBlackJack = new Hand(List.of(
new Card(Shape.CLOVER, Rank.ACE),
new Card(Shape.DIAMOND, Rank.TEN)
));
private static final Hand hand22 = new Hand(List.of(
new Card(Shape.HEART, Rank.TEN),
new Card(Shape.CLOVER, Rank.TEN),
new Card(Shape.DIAMOND, Rank.TWO)
));
private static final Hand hand21 = new Hand(List.of(
new Card(Shape.CLOVER, Rank.JACK),
new Card(Shape.DIAMOND, Rank.TEN),
new Card(Shape.HEART, Rank.ACE)
));
private static final Hand hand20 = new Hand(List.of(
new Card(Shape.CLOVER, Rank.JACK),
new Card(Shape.DIAMOND, Rank.TEN)
));
Copy link
Member

Choose a reason for hiding this comment

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

제가 기억하기론 Hand 한 클래스에 묶어두셨던 거 같은데 다시 분리하신 이유가 있을까요??

Copy link
Member Author

Choose a reason for hiding this comment

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

질문이 이해가 안갑니다..!! 뭘 분리를 했을까요 제가.....??

Comment on lines +3 to +6
public class Money {

private final int money;

Copy link
Member

Choose a reason for hiding this comment

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

Money 의 타입을 int로 두었는데, 배팅 금액의 상한선을 정해두지 않으면 계산 과정에서 오버 플로우가 발생할 수 있습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

그러네요
정해야겠습니닷

Copy link
Member

@seokmyungham seokmyungham left a comment

Choose a reason for hiding this comment

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

아토 고생 많으셨습니다!
이번 step2는 따로 구현할게 많이 없었죠~

다음 체스 미션도 화이팅해봐요 👍

Comment on lines +6 to +8
public enum Command {
YES("y"),
NO("n");
Copy link
Member

Choose a reason for hiding this comment

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

만약 카드를 더 받을지 말지 결정하는 명령어를 "y"에서 다른 문자열 요구사항으로 변경된다면
아토는 어느 패키지를 먼저 열어볼 것 같으신가요??

현재 사용자 입력을 받는 view 패키지가 따로 존재하고 BlackJackGameController 역할을 도맡아 하고있다면
추후 유지보수 측면에서 생각해봤을 때 Command의 현재 위치는 적절하지 않아보입니다

Copy link
Member Author

Choose a reason for hiding this comment

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

저도 약간 고민했던 부분인데...
inputView에서 커맨드를 이용해 불리안 값을 리턴해주는 방식을 고려중입니다!

Comment on lines +8 to +13
public class MatchResults {

private final Hand dealerHand;
private final Map<Player, Integer> results;

public MatchResults(Hand dealerHand) {
Copy link
Member

Choose a reason for hiding this comment

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

아토는 객체의 필드로 선택하는 본인만의 기준이 있는지 궁금해요~
저는 필드가 객체의 상태를 나타내는지, 필드가 한 객체 내에서 공통적으로 사용되는지를 종합적으로 고려해서 선택해요
DealerHandMatchResults의 상태, 속성에 해당한가? 하면은 저는 조금 의문이 들거든요 😕
어떻게 생각하시나용~?

Copy link
Member Author

Choose a reason for hiding this comment

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

약간 결과 계산해주는 심판 느낌으로 만든건데
결과 판단에 사용되는 딜러는 항상 같은 딜러니까
한 게임 전체의 결과를 심판하기 시작할 때 사용되는 딜러의 카드가 무엇인지를 알려주면 어떨까?
하는 생각이었습니다
약간 의도가 잘 안담긴 것 같기는 하네요 😹

Comment on lines +35 to +39

private static boolean isPlayerWinningCondition(Hand playerHand, Hand dealerHand) {
Score playerScore = playerHand.calculateScore();
Score dealerScore = dealerHand.calculateScore();

Copy link
Member

Choose a reason for hiding this comment

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

isDealerWinningCondition 메서드에서도 플레이어와 딜러의 점수를 계산하던데 계산을 여러번 하는 이유가 궁금합니다

Copy link
Member Author

Choose a reason for hiding this comment

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

hand가 아니라 score를 넘겨줄까 고민을 했었는데 그러니까 메서드 호출부가 너무 길어져서 이런 선택을 했습니다...!
그렇다고 calculateRateOfPrize에서 변수로 만들면 10라인 제한을 어기게 되더라구요...

Comment on lines +7 to +11

public class MatchResults {

private final Hand dealerHand;
private final Map<Player, Integer> results;
Copy link
Member

Choose a reason for hiding this comment

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

원시 값을 포장한 Money 클래스를 만든 만큼
블랙잭 게임 결과 Map에서도 Money를 사용할 수 있었으면 좋겠어요.
아마 음수의 Money 생성을 막기 위한 검증 때문이었을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

맞습니다.... 머니를 베팅머니로 바꾸는게 좋겠어요

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