[블랙잭 - 2단계] 아토(이혜린) 미션 제출합니다.#17
[블랙잭 - 2단계] 아토(이혜린) 미션 제출합니다.#17hyxrxn wants to merge 86 commits intowoowacourse-6th-code-review-study:mainfrom
Conversation
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
- 기능을 추가하며 클래스 분리 (Player -> Hand) Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
- `Card`에 포함되는 모양과 수는 변하지 않고, 불변 객체이므로 그대로 전달 Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
- `toList`는 변경 불가능한 리스트를 반환하므로 `unmodifiableList` 제거 Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
Co-authored-by: donghoony <aru0504@naver.com>
- Player의 Hand 접근제한자 변경
* docs: 컨벤션 및 기능 요구사항 작성 Co-authored-by: donghoony <aru0504@naver.com> * chore: remove gitkeep Co-authored-by: donghoony <aru0504@naver.com> * feat(Card): 카드 점수 계산 Co-authored-by: donghoony <aru0504@naver.com> * feat(Deck): 덱에서 카드 한 장 드로우 Co-authored-by: donghoony <aru0504@naver.com> * feat(Deck): 비어있는 덱에서 드로우할 때 예외 처리 Co-authored-by: donghoony <aru0504@naver.com> * feat(Player): 플레이어 점수 계산 Co-authored-by: donghoony <aru0504@naver.com> * feat(Dealer): 딜러의 점수에 따른 카드 드로우 Co-authored-by: donghoony <aru0504@naver.com> * feat(Dealer): 주어진 점수에 따른 승패 판단 Co-authored-by: donghoony <aru0504@naver.com> * feat(Hand): Ace 점수를 유리한 방향으로 계산 - 기능을 추가하며 클래스 분리 (Player -> Hand) Co-authored-by: donghoony <aru0504@naver.com> * refactor(Player): 드로우 가능 여부 판단 메서드 위치 변경 Co-authored-by: donghoony <aru0504@naver.com> * feat(Name): 이름 생성 및 검증 Co-authored-by: donghoony <aru0504@naver.com> * feat(Players): 플레이어들 생성 및 검증 Co-authored-by: donghoony <aru0504@naver.com> * feat(View): 콘솔 입출력 Co-authored-by: donghoony <aru0504@naver.com> * feat(CardDisplay): 카드의 모양과 수의 표현 방식 설정 Co-authored-by: donghoony <aru0504@naver.com> * refactor(Player): 게임 상수 추출 및 드로우 조건 수정 Co-authored-by: donghoony <aru0504@naver.com> * feat(MatchResult): 게임 결과 판단 Co-authored-by: donghoony <aru0504@naver.com> * feat(MatchResults): 게임 결과 저장 Co-authored-by: donghoony <aru0504@naver.com> * feat(Deck): Shape와 Number의 조합으로 한 세트 생성 Co-authored-by: donghoony <aru0504@naver.com> * feat(Command): 사용자의 명령어 변환 Co-authored-by: donghoony <aru0504@naver.com> * refactor(OutputView): 매개변수로 `Card`를 받도록 수정 - `Card`에 포함되는 모양과 수는 변하지 않고, 불변 객체이므로 그대로 전달 Co-authored-by: donghoony <aru0504@naver.com> * feat(Display): 플레이어의 결과 표현 방식 설정 Co-authored-by: donghoony <aru0504@naver.com> * feat(BlackJackGame): 게임 흐름 제어 Co-authored-by: donghoony <aru0504@naver.com> * feat(BlackJackMain): 의존성 설정 및 실행 Co-authored-by: donghoony <aru0504@naver.com> * refactor(BlackJackGame): 메서드 분리 Co-authored-by: donghoony <aru0504@naver.com> * refactor(Players): 불필요한 래핑 제거 - `toList`는 변경 불가능한 리스트를 반환하므로 `unmodifiableList` 제거 Co-authored-by: donghoony <aru0504@naver.com> * style(Shape): 불필요한 세미콜론 제거 Co-authored-by: donghoony <aru0504@naver.com> * refactor: 상수 추출 Co-authored-by: donghoony <aru0504@naver.com> * refactor(OutputView): 메서드 분리 Co-authored-by: donghoony <aru0504@naver.com> * refactor: 패키지 이동 Co-authored-by: donghoony <aru0504@naver.com> * test(Player): 드로우 가능 여부 판단 Co-authored-by: donghoony <aru0504@naver.com> * fix(Dealer): 카드를 가지고 있지 않은 경우 예외 처리 Co-authored-by: donghoony <aru0504@naver.com> * refactor(Deck): 덱 생성 정적 메서드 추가 Co-authored-by: donghoony <aru0504@naver.com> * refactor(Hand): 접근 제한자 변경 Co-authored-by: donghoony <aru0504@naver.com> * refactor(Deck): 메서드 분리 Co-authored-by: donghoony <aru0504@naver.com> * refactor(Display): 패키지 분리 Co-authored-by: donghoony <aru0504@naver.com> * style(Number): 파일 끝 공백 추가 Co-authored-by: donghoony <aru0504@naver.com> * refactor: 메서드 순서 변경 Co-authored-by: donghoony <aru0504@naver.com> * refactor: 패키지 이동 - 컨벤션에 맞게 import 순서 변경 Co-authored-by: donghoony <aru0504@naver.com> * style: 불필요한 개행 삭제 - 컨벤션에 맞게 import 순서 변경 Co-authored-by: donghoony <aru0504@naver.com> * style(Hand): 가독성을 위한 변수 위치 및 줄바꿈 수정 Co-authored-by: donghoony <aru0504@naver.com> * refactor(MatchResult): 승리 조건 별 메서드 분리 Co-authored-by: donghoony <aru0504@naver.com> * test(MatchResult): 승리 조건 테스트 데이터 보강 Co-authored-by: donghoony <aru0504@naver.com> * style: 테스트 클래스 접근제한자 삭제 Co-authored-by: donghoony <aru0504@naver.com> * refactor(OutputView): 중복 코드 제거 및 메서드명 번경 Co-authored-by: donghoony <aru0504@naver.com> * style: 개행 및 오타 수정 Co-authored-by: donghoony <aru0504@naver.com> * 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 <aru0504@naver.com>
- 플레이어의 수익 확인 - 딜러의 수익 확인
| 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); |
There was a problem hiding this comment.
이번 미션 진행하면서 이 컨트롤러가 많이 커져서 불편하지 않았나요? ㅎㅎㅎ
저도 너무 커져서 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 이라는 필드에 넣어 컨트롤러의 무게를 조금 줄이는 방법으로 해보았는데, 나쁘진 않은 거 같더라구요!
| 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; | ||
| } |
There was a problem hiding this comment.
이 부분을 BiPredicate 를 활용하여 구현할 수도 있을 거 같아요!
| private static final int MIN_NAME_LENGTH = 2; | ||
| private static final int MAX_NAME_LENGTH = 5; |
There was a problem hiding this comment.
이 클래스 이름에서 이미 NAME 이 들어가기에 MIN_LENGTH, MAX_LENGTH로 해두어도 나쁘지 않을 거 같아요!
| private void validateInteger(String input) { | ||
| try { | ||
| Integer.parseInt(input); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new IllegalArgumentException("[ERROR] 정수가 아닙니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
여기에서 걸리는건 IllegalArgumentException 이 아니라 NumberFormatException 일 거 같은데 맞을까요..?
그리고 추가로 Integer 범위가 넘어가는 수가 입력되어도(ex 3000,000,000) 예외가 터질거 같은데 이때에는 "정수가 아닙니다."라는 메시지가 맞지 않을 거 같아요..!
There was a problem hiding this comment.
오!!!! 그러네요..... 수정하겠습니다
그런데 왜 catch가 되는걸까요...?
| 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) | ||
| )); |
There was a problem hiding this comment.
제가 기억하기론 Hand 한 클래스에 묶어두셨던 거 같은데 다시 분리하신 이유가 있을까요??
There was a problem hiding this comment.
질문이 이해가 안갑니다..!! 뭘 분리를 했을까요 제가.....??
| public class Money { | ||
|
|
||
| private final int money; | ||
|
|
There was a problem hiding this comment.
Money 의 타입을 int로 두었는데, 배팅 금액의 상한선을 정해두지 않으면 계산 과정에서 오버 플로우가 발생할 수 있습니다!
seokmyungham
left a comment
There was a problem hiding this comment.
아토 고생 많으셨습니다!
이번 step2는 따로 구현할게 많이 없었죠~
다음 체스 미션도 화이팅해봐요 👍
| public enum Command { | ||
| YES("y"), | ||
| NO("n"); |
There was a problem hiding this comment.
만약 카드를 더 받을지 말지 결정하는 명령어를 "y"에서 다른 문자열 요구사항으로 변경된다면
아토는 어느 패키지를 먼저 열어볼 것 같으신가요??
현재 사용자 입력을 받는 view 패키지가 따로 존재하고 BlackJackGame이 Controller 역할을 도맡아 하고있다면
추후 유지보수 측면에서 생각해봤을 때 Command의 현재 위치는 적절하지 않아보입니다
There was a problem hiding this comment.
저도 약간 고민했던 부분인데...
inputView에서 커맨드를 이용해 불리안 값을 리턴해주는 방식을 고려중입니다!
| public class MatchResults { | ||
|
|
||
| private final Hand dealerHand; | ||
| private final Map<Player, Integer> results; | ||
|
|
||
| public MatchResults(Hand dealerHand) { |
There was a problem hiding this comment.
아토는 객체의 필드로 선택하는 본인만의 기준이 있는지 궁금해요~
저는 필드가 객체의 상태를 나타내는지, 필드가 한 객체 내에서 공통적으로 사용되는지를 종합적으로 고려해서 선택해요
DealerHand가 MatchResults의 상태, 속성에 해당한가? 하면은 저는 조금 의문이 들거든요 😕
어떻게 생각하시나용~?
There was a problem hiding this comment.
약간 결과 계산해주는 심판 느낌으로 만든건데
결과 판단에 사용되는 딜러는 항상 같은 딜러니까
한 게임 전체의 결과를 심판하기 시작할 때 사용되는 딜러의 카드가 무엇인지를 알려주면 어떨까?
하는 생각이었습니다
약간 의도가 잘 안담긴 것 같기는 하네요 😹
|
|
||
| private static boolean isPlayerWinningCondition(Hand playerHand, Hand dealerHand) { | ||
| Score playerScore = playerHand.calculateScore(); | ||
| Score dealerScore = dealerHand.calculateScore(); | ||
|
|
There was a problem hiding this comment.
isDealerWinningCondition 메서드에서도 플레이어와 딜러의 점수를 계산하던데 계산을 여러번 하는 이유가 궁금합니다
There was a problem hiding this comment.
hand가 아니라 score를 넘겨줄까 고민을 했었는데 그러니까 메서드 호출부가 너무 길어져서 이런 선택을 했습니다...!
그렇다고 calculateRateOfPrize에서 변수로 만들면 10라인 제한을 어기게 되더라구요...
|
|
||
| public class MatchResults { | ||
|
|
||
| private final Hand dealerHand; | ||
| private final Map<Player, Integer> results; |
There was a problem hiding this comment.
원시 값을 포장한 Money 클래스를 만든 만큼
블랙잭 게임 결과 Map에서도 Money를 사용할 수 있었으면 좋겠어요.
아마 음수의 Money 생성을 막기 위한 검증 때문이었을까요?
There was a problem hiding this comment.
맞습니다.... 머니를 베팅머니로 바꾸는게 좋겠어요
아직 달린 리뷰가 없어 요청한 코드로 변경해서 다시 pr 보냅니닷
제이미 안녕하세요! 또 한 번 BE 6기 아토입니다.
변경된 기능만 충족할 수 있도록 코드를 수정해보았는데요, 관련해서 설명 드리겠습니다.
미션 진행 시 생각한 부분
1. 승패를 결정하던
MatchResult가 수익 비율을 결정2. 플레이어별 배팅 금액을 저장하는 객체
PlayerBettingMoney3. 결국 실패한 컨트롤러 분리... 😢
읽어보시고 의견 혹은 질문 많이많이 주세요!
즐거운 주말 보내시길 바랍니다~
감사합니다!