[2단계 - 블랙잭 베팅] 리비(이근희) 미션 제출합니다.#15
[2단계 - 블랙잭 베팅] 리비(이근희) 미션 제출합니다.#15Libienz wants to merge 91 commits intowoowacourse-6th-code-review-study:mainfrom
Conversation
* docs: 프로젝트 소개글 및 구현 기능 목록 작성 * feat: 카드의 모양 및 점수 이넘 생성 * feat: 소유한 카드 합 계산 구현 * feat: 카드를 추가하는 기능 구현 * feat: Card 동등성 비교 및 해싱 함수 재정의 * feat: CardDeck 카드를 뽑을 수 있는 기능 구현 * test: 특정 카드 추가 테스트 의미 개선 * feat: 카드의 수가 충분하지 않은 경우 pop메서드의 예외처리 구현 * feat: 플레이어 이름 구현 * feat: 플레이어 구현 * test: 테스트 도메인 설명 작성 * test: 테스트 편의 픽스처 구현 * feat: Player 도메인 동등성 비교 기능 구현 * feat: Player의 일급 컬렉션 도메인 Players 구현 * feat: Player 관련 도메인 Getter 프로퍼티 추가 * feat: Player가 가지고 있는 핸드 Getter 프로퍼티 추가 * feat: Card 게터 프로퍼티 추가 * feat: Player가 자신이 가지고 있는 카드의 합계를 반환할 수 있는 기능 추가 * feat: InputView 구현 * feat: 출력 메시지 리졸버 구현 * feat: OutputView 구현 * docs: 기능 구현 목록 정리 및 최신화 * feat: DrawDecision 구현 * feat: 이름만 받아서 빈핸드를 가진 참가자들을 생성하는 정적 팩토리메서드 구현 * feat: InputMapper 구현 * feat: 정해진 카드 구성으로 덱을 만드는 Creator 구현 * feat: 카드 숫자가 에이스에 해당하는 숫자인지 확인하는 기능 추가 * feat: 카드가 에이스인지 확인하는 기능 추가 * test: 카드 숫자가 에이스에 해당하는 숫자인지 검증하는 기능 테스트 추가 * fix: 딜러 핸드 합계 구하는 메서드 수정 * feat: 핸드에서 에이스 카드가 몇개인지 세는 기능 구현 * feat: Judge가 핸드로부터 21에 가장 가까운 값을 구하는 기능 구현 * feat: Hand가 버스트 되었는지 판별하는 기능 구현 * feat: 딜러 Hit 동작 구현 * feat: 딜러 핸드 게터 구현 * refactor: 딜러 관련 로직 파라미터 개선 * feat: 플레이어의 승패 결정 로직 구현 * feat: 딜러 게임 결과 데이터 클래스 작성 * feat: Players 플레이어 수 반환하는 기능 구현 * feat: Judge 딜러의 승패를 결정하는 로직 구현 * feat: 최종 승패 출력 로직 구현 * feat: CardNumber 열거형 필드 추가 * feat: Card의 숫자 이름을 반환하는 기능 구현 * fix: 카드의 숫자가 아닌 카드 이름을 사용하도록 뷰로직 변경 * fix: 완성된 카드 결과 출력시 플레이어의 이름과 함께 출력하도록 뷰로직 수정 * refactor: 플레이어들의 점수 출력시 점수를 계산하지 않고 전달받도록 개선 * feat: 컨트롤러 구현 * feat: 어플리케이션 진입점과 컨트롤러 연결 * refactor: Player에서 사용되지 않는 메서드 제거 개선 * refactor: 패키지 구조 세분화 개선 * feat: Score 클래스 구현 * feat: ScoreCalculator 구현 * style: ScoreCalculator -> ScoreCalculateStrategy 클래스 이름 수정 * refactor: Player가 전략을 전달받아 점수를 계산하는 기능 구현 * refactor: 포장한 Score를 전달받아 뷰를 생성하도록 파라미터 개선 * refactor: 메서드 분리 개선 및 점수 계산 책임을 Judge에서 이전 * test: 테스트 이전으로 인한 삭제 * refactor: 히트 가능 책임을 Judge에서 이전 * test: 책임 분리로 인해 유의미하지 않은 테스트 제거 * refactor: 히트 할 수 있는지 여부를 결정하는 전략 Judge로부터 분리 * feat: 점수가 파라미터보다 높은지 확인하는 기능 구현 * feat: 점수간 비교 기능 구현 * refactor: 점수 비교 승패 판정 로직 도메인에게 물어보도록 개선 * fix: 승패 판정, 카드 분배 문제 해결 * refactor: Judge 메서드들 책임 분리 개선 * fix: 딜러 초기 핸드를 한장만 노출하도록 뷰로직 수정 * refactor: 누락된 딜러 전체 카드 출력 메세지 구현 * refactor: 사용하지 않는 클래스 제거 개선 * fix: 공백 뷰로직 수정 * docs: 체크리스트 최신화 * test: 반복되는 테스트 데이터 생성 fixture이용하도록 개선 * refactor: 메서드 순서 배치 개선 * feat: Deck에서 Hand를 생성하는 HandCreator 구현 * fix: CardDeck에서 카드를 뽑을 때 불변리스트를 반환하지 않도록 수정 * feat: 게임 규칙에 맞게 플레이어를 생성하는 기능 구현 * refactor: PlayerCreator로 부터 플레이어와 딜러를 초기화하도록 개선 * feat: Players 클래스에 특정 점수 이하인 플레이어를 세는 기능 구현 * refactor: 구현한 기능을 이용하도록 책임 이전 개선 * refactor: 사용하지 않는 파라미터 제거 개선 * refactor: 책임 이전으로 사용하지 않는 메서드 제거 개선 * feat: Hand가 자신의 채점 기준을 속성으로 가지도록 구현 * feat: Hand가 자신을 채점하는 기능 구현 * refactor: 책임 이전 개선 적용 * refactor: 점수를 플레이어를 통해 계산하도록 개선 * refactor: Hand가 Hit전략을 속성으로 가지도록 개선 * refactor: Player에게 히트할 수 있는지 직접 물어보도록 개선 * refactor: 람다 개선 * refactor: inline개선 * test: 수정된 도메인에 따라 fixture 변경 * style: 우테코 스타일 포매팅 적용 * docs: 카드합 및 점수 관련 기능 구현 목록 추가 * refactor: 컨트롤러 레이어 삭제 및 관련 클래스 이름 변경 개선 * refactor: 불필요한 클래스 참조 제거 개선 * refactor: 에러메시지 구체화 개선 * refactor: 도메인 내에 뷰로직 제거 개선 * fix: 에러메시지 변경에 의한 테스트 코드 수정 * refactor: 덱에서 카드를 뽑을 때 마지막 카드를 뽑도록 개선 * refactor: 팩토리 클래스에서 정적 팩토리 메서드로 로직 응집 개선 * style: 메서드명 구체화 개선 * refactor: 생성 검증 불필요한 통합 제거 개선 * test: 메서드 소스 테스트 CsvSource로 단순화 개선 * style: Player -> Participant 클래스 이름 변경 * refactor: 상속을 위한 속성 접근제어자 수정 개선 * feat: Player 구현 * feat: 딜러 구현 * refactor: 상속 구조에 따른 일급컬렉션 클래스 변경 * feat: 카드덱으로부터 핸드를 생성하는 기능 추가 * refactor: 상속 구조 운용 결정을 통한 PlayerCreator 삭제 개선 * refactor: HitStrategy 로직 딜러와 플레이어에게 이전 * refactor: 추상 클래스 생성자 접근제어자 개선 * refactor: 점수 계산 기능 Hand 도메인으로 책임 이전 * refactor: 사용하지 않는 클래스 제거 개선 * refactor: 점수 계산 로직 Hand 도메인 응집으로 인한 연관 클래스 수정 * refactor: 도메인 내 뷰로직 제거 개선 * refactor: Hand 생성 로직 Hand도메인 안으로 응집 * refactor: 사용하지 않게 된 HandCreator 클래스 제거 개선 * feat: pop한 개수를 계산하는 기능 구현 * refactor: 몇장 더 뽑았는지 딜러에게 물어보도록 개선 * refactor: 딜러 전적 계산 Judge클래스로 책임 이전 * refactor: 사용하지 않는 메서드 제거 개선 * refactor: 상속 구조를 통한 파라미터 타입 개선 * refactor: 사용하지 않는 부분 삭제 개선 * test: TestFixture -> TestDataCreator로 이름 변경을 통한 의미 구체화 * refactor: 정적 팩토리 메서드를 가지는 클래스 생성 로직 퍼지지 않도록 생성자 접근 제어자 개선
leegwichan
left a comment
There was a problem hiding this comment.
리비! 1단계에 이어서 2단계 코드도 잘 보고가요~ 특히 MessageResoler쪽의 깔끔하게 정리된 로직이 마음에 드네요 ㅎㅎ. 큰 설계적인 부분이 마음에 들어서, 네이밍 쪽이 더 보였던 것 같아요. 제가 볼 수 있는 부분에서 코멘트 달아드렸어요~
추가적으로 말씀 드리자면,
.gitkeep 파일은 지우는 것을 추천드려요~ .gitkeep 파일은 깃허브에 폴더 구조를 올리고 싶을 때 사용하는 파일로 알고 있어요. .gitkeep 참고자료
| public class Score { | ||
|
|
||
| private static final int MAX_SCORE = 21; |
There was a problem hiding this comment.
같은 의미의 상수가 여러 곳에서 선언되고 있다는 생각이 들었습니다.
Score.MAX_SCORE, Player.HIT_THRESHOLD, Hand.BUST_THRESHOLD`
여기서 의미하는 21이 전부 같은 의미를 지니고 있다고 생각하는데요. 만약에 최대 점수 관련 요구 사항이 변경된다면, 여러 곳에서 점수를 바꾸어주어야 할 것 같아요. (ex. 버스트를 21점 초과일 때가 아닌, 20점 초과일 때로 바꾸기)
| private void validateRange(int amount) { | ||
| if (amount < MIN || MAX < amount) { | ||
| throw new IllegalArgumentException("[ERROR] 베팅 금액은 " + MIN + "부터 " + MAX + "이하까지 가능합니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
이건 그냥 제가 리뷰하면서 공부한 거 공유드려요~
| private void validateRange(int amount) { | |
| if (amount < MIN || MAX < amount) { | |
| throw new IllegalArgumentException("[ERROR] 베팅 금액은 " + MIN + "부터 " + MAX + "이하까지 가능합니다."); | |
| } | |
| } | |
| private void validateRange(int amount) { | |
| if (amount < MIN || MAX < amount) { | |
| throw new IllegalArgumentException( | |
| "[ERROR] 베팅 금액은 %d부터 %d이하까지 가능합니다.".formatted(MIN, MAX)); | |
| } | |
| } |
저는 보통 위에 같이 쓰는 편이 더 좋을까라고 생각하는데, 그냥 + 연산 쓰는 것보다 성능이 더 안좋다고 하네요;;
가독성 면에서도 리비가 사용한 코드가 더 좋은것 같기도 해서, 제 방법을 제안하려다가 철회했습니다;;
참고 자료
| public PlayerProfits calculateProfitResult(Dealer dealer) { | ||
| return new PlayerProfits(playerBets.keySet().stream() | ||
| .collect(Collectors.toMap( | ||
| player -> player, | ||
| player -> (calculatePlayerProfit(player, dealer))))); | ||
| } |
There was a problem hiding this comment.
괄호 정리 추천! (람다식에서 사용하지 않아도 되는 괄호 제거)
| public PlayerProfits calculateProfitResult(Dealer dealer) { | |
| return new PlayerProfits(playerBets.keySet().stream() | |
| .collect(Collectors.toMap( | |
| player -> player, | |
| player -> (calculatePlayerProfit(player, dealer))))); | |
| } | |
| public PlayerProfits calculateProfitResult(Dealer dealer) { | |
| return new PlayerProfits(playerBets.keySet().stream() | |
| .collect(Collectors.toMap( | |
| player -> player, | |
| player -> calculatePlayerProfit(player, dealer) | |
| ))); | |
| } |
| public Card popCard() { | ||
| if (cards.isEmpty()) { | ||
| throw new IllegalArgumentException("[ERROR] 남아있는 카드가 부족하여 카드를 뽑을 수 없습니다"); | ||
| } | ||
| return cards.remove(cards.size() - 1); | ||
| } |
There was a problem hiding this comment.
cards를 사용하는 연산이 cards.remove(cards.size() - 1) 밖에 없다면, Queue를 도입하는게 좋지 않을까?
| protected final PlayerName name; | ||
| protected final Hand hand; |
There was a problem hiding this comment.
저는 필드를 추상 클래스에서 필드를 protected로 주는 것을 선호하지 않습니다. 혹시나 나중에 Participant의 필드 타입 등이 변경되었을 때, 하위의 상속하는 모든 클래스에 영향을 줄 수 있기 때문입니다.
PlayerName은 하위에서 사용하는 클래스가 없어서private로 바꾸어 주어도 상관 없을 것 같습니다.Hand는 하위 클래스에서 필요한 기능이calculateCardSummation()밖에 없어서 해당 메서드만protected메서드로 만들어 제공해주어도 될 것 같습니다.
public abstract class Participant {
private final PlayerName name;
private final Hand hand;
...
protected final int calculateCardSummation() {
return hand.calculateCardSummation();
}
....
}
public class Dealer extends Participant {
...
@Override
public boolean canHit() {
return calculateCardSummation() <= HIT_THRESHOLD;
}
...
}| BLACKJACK_WIN(1.5, (dealer, player) -> dealer.hasNoBlackJackHand() && player.hasBlackJackHand()), | ||
| PLAYER_LOSE(-1.0, (dealer, player) -> player.isBusted() || dealer.hasScoreAbove(player)), | ||
| PLAYER_WIN(1.0, (dealer, player) -> (player.isNotBusted() && (dealer.isBusted() || player.hasScoreAbove(dealer)))), | ||
| PUSH(0.0, (dealer, player) -> player.isNotBusted() && dealer.isNotBusted() && player.hasSameScore(dealer)); |
There was a problem hiding this comment.
비겼을 때 이름을 도메인 용어를 사용한 것(PUSH)이 좋네요~!
| private final double profitLeverage; | ||
| private final BiPredicate<Dealer, Player> biPredicate; | ||
|
|
||
| GameResult(double profitLeverage, BiPredicate<Dealer, Player> biPredicate) { | ||
| this.profitLeverage = profitLeverage; | ||
| this.biPredicate = biPredicate; | ||
| } |
There was a problem hiding this comment.
BiPredicate는 자바의 util에서 제공한 이름이니까 "딜러와 플레이어에 따라 승패를 결정한다"는 것을 표현하는 다른 핃드 이름을 사용하는 것은 어떨까요?
| public String resolveInitialHandsMessage(Dealer dealer, Players players) { | ||
| return new StringBuilder() | ||
| .append(resolveHandOutEventMessage(players)) | ||
| .append(LINE_SEPARATOR) | ||
| .append(resolveDealerHandMessage(dealer)) | ||
| .append(LINE_SEPARATOR) | ||
| .append(resolvePlayersHandMessage(players)) | ||
| .toString(); | ||
| } |
There was a problem hiding this comment.
우와... 제 OuputView는 엄청 지저분해졌는데, 각 부분별로 메서드를 잘 나누시고, StringBuilder와 LINE_SEPERATOR를 도입하니까 정말 깔끔해 보이네요!!
| return Arrays.stream(values()) | ||
| .filter(drawDecision -> drawDecision.code.equals(code)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new IllegalArgumentException("[ERROR] " + YES.code + "또는 " + NO.code + "로 입력해주세요")); |
| private static final String DEALER_NAME = "딜러"; | ||
| public static final int HIT_THRESHOLD = 16; | ||
|
|
|
|
||
| public class BetAmout { | ||
|
|
||
| private static final int MAX = 1_000_000_000; |
There was a problem hiding this comment.
배팅 금액을 최대 10억으로 정해두셨는데, 참가자 3명이 모두 10억씩 배팅하면 30억이되어 오버플로우가 발생할 수도 있을 거 같아요!
|
|
||
| public BetAmout readBetAmount(Player player) { | ||
| System.out.println(String.format("%s의 배팅 금액은?", player.getName())); | ||
| return new BetAmout(Integer.parseInt(scanner.nextLine())); |
There was a problem hiding this comment.
여기서도 예외가 터질 수 있을 거 같은데 잡아주는건 어떨까요?
There was a problem hiding this comment.
숫자만 입력되는 것이 아니거나 혹은 Integer 범위를 초과하는 입력이 오거나
| void testAdd() { | ||
| Profit profit1 = new Profit(1000); | ||
| Profit profit2 = new Profit(-1000); | ||
| assertThat(profit1.add(profit2).getValue()).isEqualTo(0); | ||
| } | ||
|
|
||
| @DisplayName("수익을 반전시킨 결과를 볼 수 있다") | ||
| @Test | ||
| void testInverse() { | ||
| Profit profit = new Profit(1000); | ||
| assertThat(profit.inverse().getValue()).isEqualTo(-1000); |
There was a problem hiding this comment.
Profit에 equals 이 재정의되어있으니 getValue() 사용하지 않고 이런식으로 해보는건 어떤가요?
Profit profit = new Profit(1000);
Profit expected = new Profit(-1000);
assertThat(profit.inverse()).isEqualTo(expected);
3Juhwan
left a comment
There was a problem hiding this comment.
리비~ 리뷰가 많이 늦었죠 😅
리비 코드 설계가 저와 많이 비슷해서 읽기 수월하고 남길 리뷰가 많지 않았어요!
코드에서 몇 가지 인사이트를 얻어서 제 코드에 반영하고 싶은 마음이 급하네요 😀
몇 가지 코멘트 남겼으니까 확인해 주세요!
|
|
||
| public Profit findProfitOfPlayer(Player player) { | ||
| return playerProfits.get(player); | ||
| } | ||
|
|
| package blackjack; | ||
|
|
||
| import blackjack.domain.DrawDecision; | ||
| import blackjack.domain.player.PlayerName; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| public class InputMapper { | ||
|
|
||
| private static final String DELIMITER = ","; | ||
|
|
||
| public List<PlayerName> mapToPlayerNames(String target) { | ||
| String[] split = target.split(DELIMITER); | ||
| return Arrays.stream(split) | ||
| .map(PlayerName::new) | ||
| .toList(); | ||
| } | ||
|
|
||
| public DrawDecision mapToDrawDecision(String target) { | ||
| return DrawDecision.from(target); | ||
| } | ||
| } |
|
|
||
| private Profit calculatePlayerProfit(Player player, Dealer dealer) { | ||
| GameResult gameResult = GameResult.of(dealer, player); | ||
| BetAmout betAmout = playerBets.get(player); | ||
| return betAmout.calculateProfit(gameResult.getProfitLeverage()); | ||
| } | ||
| } |
No description provided.