-
Notifications
You must be signed in to change notification settings - Fork 389
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단계 - 블랙잭] 후디(조동현) 미션 제출합니다. #273
Conversation
- 테스트 코드 작성 - getName, getFuelNeeded 메소드 구현
- 테스트 코드 작성 - create 팩토리 메서드 구현 - addCar, generateReport 메서드 구현
- Sonata 도메인을 Car의 구현체로 수정 - SonataTest에서 CarTest로 테스트 수정
- 테스트 코드 작성
- 테스트에 Sonata 이외의 Car 구현체 내용 추가
- 테스트 코드 작성 - 하트, 스페이드, 다이아몬드, 클로버 심볼 구분 - getDisplayName 메서드 구현
- 테스트 코드 작성 - ACE, 2 ~ 10, JACK, QUEEN, KING 랭크 구분 - getDisplayName 메서드 구현
- 테스트 코드 작성 - 내부 클래스를 통한 캐시의 구현
- 테스트 코드 작성 - 초기화 시점에 52개의 카드를 생성하고 셔플 - pop 메서드 구현 - Card 도메인 toString 메서드 출력 형식 가독성 개선
- 테스트 코드 작성 - 인스턴스 생성을 위한 valueOf, add 메서드 - toInt 메서드 구현 - compareTo를 통한 value 필드 값 기반 객체 비교 구현 - 내부 클래스를 통한 캐쉬 구현
- 테스트 코드 작성 - getValue 구현 - ACE의 기본 value를 1로 설정
- 테스트 코드 작성 - of 팩토리 메서드를 통해 카드 2개짜리 인스턴스 생성 기능 구현 - add를 통한 카드 추가 기능 및 중복에 대한 예외처리 구현 - getScore를 통한 카드 점수 합계 반환 기능 구현 - Card에서 getRankValue를 통한 랭크 값 반환 기능 구현
- 테스트 코드 작성 - WIN, LOSE, DRAW 세 가지 결과 타입 구현 - displayName 필드 구현
- 테스트 코드 작성 - 인스턴스 초기화시 값은 0 - increment 메서드를 통해 값은 1씩만 증가
- 테스트 코드 작성 - 인스턴스 생성시 승/패/무 정보는 각각 0으로 초기화 - getCountOf 메서드를 통해 특정 결과 정보를 반환 - incrementCountOf 메서드를 통해 특정 결과 정보의 값을 1씩만 증가
- 테스트 코드 작성 - receiveCard 를 통한 카드 추가 기능 구현 - canReceive 를 통해 게임 진행 여부 판단 기능 구현
- 테스트 코드 작성 - receiveCard 를 통한 카드 추가 기능 구현 - canReceive 를 통해 게임 진행 여부 판단 기능 구현
- CardDeck을 CardStack의 구현체로 설정 - CardStack으로부터 카드를 한장씩 꺼내는 pop 메서드 설정
- 테스트에서 중복으로 사용되는 카드 VO 상수화
- CardStackGenerator.ofReverse를 통해 카드 스택 생성 - 생성될 때 입력된 카드들의 순서대로 pop되는 구조
- 테스트 코드 작성 - 인스턴스 생성시 카드 덱, 딜러, 플레이어 초기화 - 플레이어명이 전달되지 않으면 예외 발생 - 딜러에게 카드 전달 기능 구현 - 카드 덱에서 카드 한장 뽑는 기능 구현
- 깨진 테스트 코드 수정
- InputView를 통해 플레이어명 받아 문자열 리스트로 반환 - 각 플레이어명의 좌우 공백은 제거 - BlackjackController를 통해 게임 인스턴스 생성하여 반환
- 테스트 코드 작성 - toString 리팩토링
- blackjackGame.getDealer 구현 - dealer.getOpenCard 구현
- 기존 로직은 딜러 점수가 16이하일 경우 단 한번만 카드를 추가로 가져가도록 되어 있었음
- Dealer 와 Player 에서 name 과 hand 를 getter 를 통해 접근하도록 로직 변경
- 테스트 코드 작성
…aCardToPlayer) 추가 - giveExtraCardToPlayer 에 대한 테스트 코드 작성 - BlackjackController의 givePlayerCards 2단계 depth 문제 해결
- OutputView 에 예외 메세지 출력을 위한 printException 메소드 구현
- 테스트 코드 작성 - 잘못된 이름 입력 시 재입력 받도록 컨트롤러 로직 수정
- 테스트 코드 작성
public class ParticipantDto { | ||
private final String name; | ||
private final HandDto handDto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
필드 수 제한 (2개까지 허용) 때문에 부득이하게 HandDto 라는 추가적인 DTO 를 만들어 필드로 갖게 하였습니다. DTO 안에 DTO... 무언가 어색한 것 같은데, 괜찮은 구조로 보이는지 앨런의 의견이 궁금합니다 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵! 데이터가 계층적으로 내려가는군요? 괜찮다고 생각합니다.
개인적인 생각으로 DTO는 데이터를 전달의 목적이므로 3개이상 필드를 사용해도 괜찮을 것 같아요. �
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 필드 제한 규칙때문에 억지로 만들어낸 DTO 라서, 앨런 의견대로 3개이상의 필드를 DTO 한정으로 허용한다면, 필드를 3개 갖고있는 것이 더 좋은 구조라고 생각되네요. HandDto
를 제거하고 ParticipantDto
가 3개의 필드를 가지는 형태로 개선하겠습니다 ^-^
private void giveCardsToPlayer(Player player, BlackjackGame game) { | ||
try { | ||
giveSingleCardToPlayerOnMoreCardInput(player, game); | ||
} catch (IllegalArgumentException exception) { | ||
OutputView.printException(exception); | ||
giveCardsToPlayer(player, game); | ||
} | ||
} | ||
|
||
private void giveSingleCardToPlayerOnMoreCardInput(Player player, BlackjackGame game) { | ||
if (InputView.requestMorePlayerCardInput(player.getName())) { | ||
giveSingleCardToPlayer(player, game); | ||
} | ||
} | ||
|
||
private void giveSingleCardToPlayer(Player player, BlackjackGame game) { | ||
if (game.giveExtraCardToPlayer(player)) { | ||
OutputView.printParticipantHand(ParticipantDto.from(player)); | ||
giveCardsToPlayer(player, game); | ||
return; | ||
} | ||
|
||
printHandAndBustMessage(player); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depth 와 길이를 줄이기 위해 giveCardsToPlayer 를 3가지 메소드로 분리했는데, (잘못된 입력의 재입력 처리를 위한) 재귀호출 덕분에 giveCardsToPlayer -> giveSingleCardToPlayerOnMoreCardInput -> giveSingleCardToPlayer -> giveCardsToPlayer ...
와 같이 메소드의 실행흐름이 다소 복잡해졌습니다. 이 구조와 관련한 앨런의 의견이 궁금합니다!
public static void main(String[] args) { | ||
BlackjackController blackjackController = new BlackjackController(); | ||
BlackjackGame blackjackGame = blackjackController.initializeGame(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Application
이 Controller 뿐 아니라 BlackjackGame
이라는 도메인도 가지고 있습니다. 괜찮은 구조일까요?
blackjackController.printInitialHand(blackjackGame); | ||
blackjackController.giveCardsToAllPlayer(blackjackGame); | ||
blackjackController.giveExtraCardToDealer(blackjackGame); | ||
blackjackController.printFinalHandAndScore(blackjackGame); | ||
blackjackController.printDealerMatchDto(blackjackGame); | ||
blackjackController.printPlayersMatchDto(blackjackGame); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/woowacourse/java-blackjack/pull/273/files#r826630109 에서 언급한 문제 뿐 아니라, BlackjackController 의 메소드가 모두 blackjackGame 을 파라미터로 전달받고 있습니다.
이를 해결하기 위해 BlackjackGame
을 싱글톤으로 만들까 고민중인데 이에 대한 앨런의 의견이 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음,,, 그전에 왜 싱글톤을 고민하게 되셨을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어플리케이션 실행 중 단 한번만 생성되는 객체이기도 하고, 싱글톤으로 생성하면 위 코드처럼 계속 컨트롤러에 파라미터로 넘겨줄 필요도 없다고 생각했습니다 (정적 메소드로 인스턴스를 가지고오면 되어서).
그런데, 밑에 피드백 남겨주신대로 BlackjackController
에 run
메소드를 생성하고, Application
의 코드를 옮겨두니 문제가 다소 해결된 것 같아 싱글톤은 필요 없다고 생각이 됩니다 :)
public class DealerMatchDto { | ||
private static final Referee REFEREE = new Referee(); | ||
|
||
private final Map<ResultType, ResultCount> matchResult; | ||
private final String name; | ||
|
||
private DealerMatchDto(Map<ResultType, ResultCount> matchResult, String name) { | ||
this.matchResult = matchResult; | ||
this.name = name; | ||
} | ||
|
||
public static DealerMatchDto of(Dealer dealer, List<Player> players) { | ||
return new DealerMatchDto(REFEREE.generateDealerResult(dealer, players), dealer.getName()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DTO 인 DealerMatchDto
와 PlayerMatchDto
가 도메인인 Referee
를 호출하여 그 반환값으로 자기자신을 생성하고 있는데, 이 구조에 대해 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가로, Referee
가 사실상 유틸 클래스의 역할만 하고 있는 것 같은데, 아예 Referee
의 모든 메소드를 static 으로 변경해 유틸성 클래스로 만들어버리고 DealerMatchDto
와 PlayerMatchDto
에서 사용하는 편이 더 자연스러울 것 같기도 한데, 위 질문과 함께 답변주시면 감사드리겠습니다 😄 😄 😄 !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모든 메소드를 static 으로 변경해 유틸성 클래스로 만들어버리고
네 이 부분이 일반적이긴한데요. 이런형태의 DTO보단, 결과값 저장과 결과계산을 해주는 객체를 별도로 만드는게 어떨까요?
(지금으로 치면 DealerMatchDto과 Referee를 합친다고 볼 수 있겠네요)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀을 듣고보니 Referee
가 해당 책임을 갖는 것 보다는 DTO 를 도메인으로 만든 다음 각각의 책임을 각 도메인으로 옮겨주었습니다! (PlayerMatchDto -> PlayerMatchResult, DealerMatchDto -> DealerMatchResult)
안녕하세요, 앨런! 두번째 리뷰 요청입니다. 고민과 질문은 코멘트로 달아두었습니다. TODO 코멘트를 보시면 아시겠지만, 사실 아직 고치고 싶은곳이 조금 많기는 합니다... 만 다 고치다가는 리뷰 간격이 매우 길어질 것 같아 일단 이쯤에서 리뷰요청 드립니다! 이번 리뷰도 잘 부탁드립니다! 😄 |
public Score getScore() { | ||
int maximumScore = cards.stream() | ||
.map(Card::getRankValue) | ||
.reduce(Score.valueOf(0), Score::add) | ||
.getValue(); | ||
|
||
int aceCount = (int) cards.stream() | ||
.filter(Card::isAce) | ||
.count(); | ||
|
||
return calculateScoreIncludingAce(maximumScore, aceCount); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hand
의 getScore
는 플레이어의 패에서 ACE 를 1로 계산할지, 11로 계산할지 판단하여 패의 총 점수를 계산하는 메소드입니다. Hand
의 Score
를 계산하는 로직이지만, 로직 자체는 Score
클래스와 더 관련있다고 생각됩니다. 이 로직은 어쩌면 Score
클래스에 있는 것이 더 자연스럽다는 생각도 드는데요, 이에 대한 앨런의 의견이 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵, Score객체를 만들어주셨으니 계산로직은 해당부분에 위임하는게 좋아보이는군요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 후디!
코드에 많은 변화가 일어났네요. 고민하신부분들이 보입니다.
코멘트는 2단계에서 적용해주세요. 고생하셨습니다 👍
BlackjackController blackjackController = new BlackjackController(); | ||
BlackjackGame blackjackGame = blackjackController.initializeGame(); | ||
|
||
blackjackController.printInitialHand(blackjackGame); | ||
blackjackController.giveCardsToAllPlayer(blackjackGame); | ||
blackjackController.giveExtraCardToDealer(blackjackGame); | ||
blackjackController.printFinalHandAndScore(blackjackGame); | ||
blackjackController.printDealerMatchDto(blackjackGame); | ||
blackjackController.printPlayersMatchDto(blackjackGame); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BlackjackController blackjackController = new BlackjackController(); | |
BlackjackGame blackjackGame = blackjackController.initializeGame(); | |
blackjackController.printInitialHand(blackjackGame); | |
blackjackController.giveCardsToAllPlayer(blackjackGame); | |
blackjackController.giveExtraCardToDealer(blackjackGame); | |
blackjackController.printFinalHandAndScore(blackjackGame); | |
blackjackController.printDealerMatchDto(blackjackGame); | |
blackjackController.printPlayersMatchDto(blackjackGame); | |
new BlackjackController().run(); |
이런 형태로 가고, 컨트롤러의 역할은 모두 run()에 위임하는게 좋을 것 같네요.
blackjackController.printInitialHand(blackjackGame); | ||
blackjackController.giveCardsToAllPlayer(blackjackGame); | ||
blackjackController.giveExtraCardToDealer(blackjackGame); | ||
blackjackController.printFinalHandAndScore(blackjackGame); | ||
blackjackController.printDealerMatchDto(blackjackGame); | ||
blackjackController.printPlayersMatchDto(blackjackGame); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음,,, 그전에 왜 싱글톤을 고민하게 되셨을까요?
public class BlackjackController { | ||
public BlackjackGame initializeGame() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 컨트롤러의 public 메서드들이 순서의 의존적이에요. 다른사람은 public 메서드의 순서를 알기 어려울 것 같아요.
만약 후디가 아닌 다른사람이 Application코드를 작성하는데, BlackjackController의 메서드 순서를 잘 활용할 수 있을까요?
Application 코멘트 남겨드렸던것처럼, BlackjackController.run()으로 메서드들을 한곳으로 모으는게 중요해보입니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BlackjackController
의 run
메소드로 옮겼습니다!
static Set<Card> cache = new HashSet<>(); | ||
|
||
static Card getCache(CardRank rank, CardSymbol symbol) { | ||
return cache.stream() | ||
.filter(card -> card.rank == rank) | ||
.filter(card -> card.symbol == symbol) | ||
.findAny() | ||
.orElseGet(() -> createNewCache(rank, symbol)); | ||
} | ||
|
||
static Card createNewCache(CardRank rank, CardSymbol symbol) { | ||
Card newCard = new Card(rank, symbol); | ||
cache.add(newCard); | ||
|
||
return newCard; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분도 캐시를 하나씩 추가하는것보다, 프로그램 시작시점에 모두 만들어주는게 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
카드는 CardDeck
에서 어차피 모두 사용되므로 앱이 시작되면서 모든 카드가 캐싱되도록 개선하였습니다!
Set<Card> initialCards = new HashSet<>(Set.of(card1, card2)); | ||
return new CardBundle(initialCards); | ||
return new Hand(initialCards); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
카드의 순서가 중요할 것 같은데, Set자료구조로 괜찮을까요? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확인해보니 새로운 카드를 플레이에 추가할 때 마다 순서가 다르게 표시되네요! List 자료구조로 개선하였습니다 :)
int maximumScore = cards.stream() | ||
.map(Card::getRankValue) | ||
.reduce(Score.valueOf(0), Score::add) | ||
.getValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapToInt로 Intstream 형태로만들면, sum() 메서드를 사용할 수 있습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int maximumScore = cards.stream()
.mapToInt(card -> card.getRankValue().getValue())
.sum();
와 같이 깔끔하게 바꾸었습니다 😄 감사합니다!
private void validatePlayerNamesNotEmpty(List<String> playerNames) { | ||
if (playerNames.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (Objects.isNull(playerNames) || playerNames.isEmpty()) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코멘트를 보고, null 처리에 대해 조금 검색해보았습니다. 통계에서도 NPE 가 두번째로 많이 발생하는 런타임 이슈라고 하네요! 안그래도 이전부터 null 체크가 중요하다고 이야기를 많이 들어오기도 했습니다.
하지만 아직 얼마나 철저하게 널체크를 해야하는지는 아직 감이 잡히지 않네요 🥲 코멘트 주신 코드같은경우 결국 입력값의 출처가 콘솔 입력이기 때문에 빈 리스트가 들어올 지언정 null 이 들어올 것이라고는 생각하지 않았거든요. 혹시 제가 미처 생각하지 못한 playerNames
가 null 이 될 상황이 있을까요?
return Set.copyOf(cards); | ||
} | ||
|
||
public Score getScore() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get이라는 이름은 getter로 오해할 수 있는데요. 이 부분은 get이라는 네이밍과 어울리지 않는 것 같습니다 🥲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 로직을 Hand
에서 Score
클래스로 옮기고, public static Score calculateSumFrom(Hand hand)
로 메소드를 변경하였습니다 :)
public boolean isBusted() { | ||
return hand.getScore().isGreaterThan(Score.BLACKJACK); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public boolean isBusted() { | |
return hand.getScore().isGreaterThan(Score.BLACKJACK); | |
} | |
public boolean isBusted() { | |
return hand.getScore().isBust(); | |
} |
이런식 메시지를 보낼 수 있겠네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Score
클래스에 isBusted
와 isOverDealerHitThreshold
메소드를 추가하였고, isLessThan
, isGreaterThan
과 같은 메소드를 제거하였습니다!
if ((playerScore > otherScore && !isBusted()) || (!isBusted() && other.isBusted())) { | ||
return ResultType.WIN; | ||
} | ||
if ((playerScore < otherScore && !other.isBusted()) || (isBusted() && !other.isBusted())) { | ||
return ResultType.LOSE; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo대로 조건이 복잡해서.. 메서드로 분리해주는게 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public ResultType compareWith(Participant other) {
if (isWin(other)) {
return ResultType.WIN;
}
if (isLose(other)) {
return ResultType.LOSE;
}
return ResultType.DRAW;
}
private boolean isWin(Participant other) {
int playerScore = getCurrentScore().getValue();
int otherScore = other.getCurrentScore().getValue();
boolean isGreaterThanOtherAndNotBusted = playerScore > otherScore && !isBusted();
boolean isNotBustedButOtherIsBusted = !isBusted() && other.isBusted();
return isGreaterThanOtherAndNotBusted || isNotBustedButOtherIsBusted;
}
private boolean isLose(Participant other) {
int playerScore = getCurrentScore().getValue();
int otherScore = other.getCurrentScore().getValue();
boolean isLessThanOtherAndOtherIsNotBusted = playerScore < otherScore && !other.isBusted();
boolean isBustedButOtherIsNotBusted = isBusted() && !other.isBusted();
return isLessThanOtherAndOtherIsNotBusted || isBustedButOtherIsNotBusted;
}
위와 같이 메소드로 분리했고, 각 조건도 보기좋게 변수로 추출하였습니다 :)
안녕하세요, 앨런! 🙌 🙌
이번 미션 리뷰 요청 드리게 될 후디라고 합니다. 만나서 정말 반가워요!!
쉽지 않은 미션인 것 같습니다. 처음에는 여유있게 시작했는데, 리뷰 요청 기간이 가까워질 수록 성급하게 코드를 작성하게 됐네요 🥲
특히, 후반부에 구현에 치중하다보니 컨트롤러나 뷰 부분의 메소드가 영수증마냥 길게 되어버렸는데... 차차 리팩토링 해보도록 하겠습니다.
미션 진행하며 궁금했던 부분은 코멘트로 남겨두겠습니다!