Step2 놀러와요 누누의 step2 블랙잭에 #9
Conversation
* docs: 기능 목록 작성 * test: Name 객체 유효성 테스트 및 도메인 생성 * feat: Shape 도메인 생성 * test: Card 테스트 * feat: Card 도메인 생성 * feat: shuffle 객체 추가 * feat: deck을 생성할 때 factory 형태로 생성하는 기능 추가 * feat: cardPocket 점수 계산 기능 추가 * feat: Participant 도메인 생성 * feat: dealer 카드 DRAW여부 확인 기능 추가 및 테스트 * feat: blackjack 상수 추가 * feat: participant 카드 getter 기능 추가 * refactor: 테스트에서 사용되는 fixture 메서드로 분리 * feat: Player 기능 추가 * feat: Players 기능 및 테스트 추가 * refactor: 빈 CardPocket 생성 기능 추가 * feat: 결과를 저장하는 객체를 추가 * feat: dealer 에서 결과 계산 기능 추가 * docs: readme 작성 * feat: 출력 기능 작성 * feat: 입력 기능 추가 * fix: 딜러 점수 계산 방식 오류 수정 * feat: hasName 메서드로 검색 가능하도록 추가 * feat: 출력을 위한 메서드들 추가 * feat: 게임 객체 추가 * feat: controller 작성 * docs: 클래스 주석 작성 * docs: 사용하지 않는 주석 제거 * refactor: remove blackjackGame to controller * refactor: Optional 안 사용하도록 변경 * refactor: 메서드 분리만 * refactor: controller indent 2 메서드 분리 * refactor: factory 내부 indent 2 제거 * chore: 사용하지 않는 메서드 제거 * chore: 매개변수 타입 명칭 변경 * style: reformat * refactor: 한번에 결과를 반환하도록 변경 * style : 자바 컨벤션 수정 * docs: 사용하지 않는 주석 제거 * feat: 줄바꿈 변경 * refactor: 접근 제어자 더 좁은 방향으로 변경 * feat: util 클래스 생성 방지를 위한 private 생성자 추가 * feat: participant 쪽에 공통으로 draw 하는 메서드 추가 * refactor: 안 쓰이는 equals, hashcode 제거 * refactor: isAce 메서드와, score 메서드를 통해 결합도를 낮춤 * test: isAce 메서드와, 점수 계산 테스트 추가 * test: ace 가 현재 점수에 따라 다르게 계산되는 것 검증 추가 * refactor: 메서드 네이밍 변경 * test: 52장 제거 테스트 추가 * refactor: for문을 flatMap 형태로 stream 으로 변경 * refactor: set 형태 대신, distinct 로 중복 검사 변경 * feat: initialCardResponseDto 추가 * feat: CardsScoreResponse 작성 * feat: enum 의 이름을 기준으로 가져올 수 있도록 변경 * fix: getter로 꺼내오는 메서드 제거 * feat: 결과를 변환하는 전략 추가 * feat: resultTypeResponse 추가 * refactor: InitialCardResponse 적용 * feat: PlayerCardsResponse 적용 * feat: playersCardsResponse 추가 * refactor: FinalResultResponse 를 사용하도록 변경 * chore: 안 쓰이는 dto 제거 * chore: 안 쓰이는 변환 제거 * refactor: String format 대신 MessageFormat 사용 * refactor: 바로 도메인 객체를 받아서 변환하도록 변경 * refactor: util 로 이동 * refactor: 상수를 각각의 클래스로 옮김 * chore: 클래스 네이밍 변경 * feat: 컨트롤러에서 사용하는 로직을 game 객체로 이동 * refactor: dto 로 변환하는 작업을 game 쪽으로 이동 * refactor: blackjack game 안쪽에서 Players 를 생성하도록 변경 * refactor: getter를 비즈니스 로직에서 덜 사용하도록 변경 * feat: blackjack Rule 객체 추가 * feat: dealer 에서 계산 로직 제거 * feat: 결과를 계산하는 로직을 담당하는 객체 추가 * style: 메시지 줄바꿈 추가 * feat: innerClass 로 particiPantResult 이동 * refactor: 패키지 이동 * feat: 계속 반복되는 CustomException 으로 분리 * style: 줄바꿈 변경 * chore: 카드 포켓 에러 메시지 변경 * chore: 메서드명 변경 * chore: 에러 메시지 변경 * refactor: deck, deckFactory 패키지 변경 * chore: 주석 제거 * refactor: blackjackGame 패키지 변경 * refactor: 메서드 인라인 * refactor: response 에 있던 정적 팩토리 제거 * feat: blackJackRule 인터페이스로 분리 * fix: 테스트 깨진 것 제거 * feat: 점수 저장 기능 BlackJackGame 으로 이동 * fix: 제거된 클래스 관련 메서드 제거 * chore: 메서드명 popCard 로 변경 --------- Co-authored-by: DESKTOP-FNVN0HH\USER <beatus96@naver.com>
| public void play(final DeckFactory deckFactory) { | ||
| final BlackJackGame blackJackGame = repeatUntilNoException( | ||
| () -> BlackJackGame.of( | ||
| inputPlayerNames(), | ||
| deckFactory, | ||
| new BlackJackRuleImpl()), | ||
| outputView::printError); | ||
| for (final String playerName : blackJackGame.getPlayerNames()) { | ||
| blackJackGame.addPlayerMoney(playerName, inputPlayerMoney(playerName)); | ||
| } | ||
|
|
||
| blackJackGame.distributeInitialCard(); | ||
| outputView.printInitialCards( | ||
| convertCard(blackJackGame.getDealerFirstCard()), | ||
| getPlayerCards(blackJackGame.getPlayers())); | ||
|
|
||
| for (final String playerName : blackJackGame.getPlayerNames()) { | ||
| drawPlayerCard(blackJackGame, playerName); | ||
| } | ||
| while (blackJackGame.isDealerDrawable()) { | ||
| blackJackGame.drawDealerCard(); | ||
| outputView.printDealerCardDrawMessage(); | ||
| } | ||
|
|
||
| outputView.printFinalStatusOfDealer(blackJackGame.getDealerScore(), | ||
| convertCards(blackJackGame.getDealerCards())); | ||
| outputView.printFinalStatusOfPlayers(convertPlayersCards(blackJackGame.getPlayersCards()), | ||
| blackJackGame.getPlayersScores()); | ||
| outputView.printFinalMoney(blackJackGame.calculatePlayersMoney()); |
There was a problem hiding this comment.
적절한 기능별로 메서드로 묶어서 추상화해보는 것은 어떨까요?
There was a problem hiding this comment.
리뷰어의 말을 듣고 이렇게 해보았는데요
적절한 단위로 묶어보는 것은 괜찮아 보이네요
조금 더 고민해볼게요
There was a problem hiding this comment.
컨트롤러에서, private 메서드로 작업들을 하는데, 그 과정에서 blackjackGame 이 계속 인자로 넘어가다보니 가독성이 안좋은 것 같다는 말을 듣고, 이렇게 작업을 했었는데요
처음에는 이런 느낌이었어요
이때 여기서도 직접 blackJackGame 을 호출하도록 하라는 피드백을 받고 이 방향으로 바꿔보고 어떤 느낌이 날지를 확인해보려고 했었는데요
직접 해보고 가독성을 보고, 천천히 생각해보니 이정도까지는 괜찮아 보이기도 하네요 메서드 네이밍만 조금 더 신경쓰면 괜찮을 것 같아보이네요
controller 를 없애면서, main 문으로 옮기면서 줄이 더 늘어나면서 복잡해서 가독성이 너무 안좋아진다면 묶어볼 것 같고, 아니라면 안 묶어도 괜찮겠다는 생각이 들기도 하네요
이대로도 괜찮을 수도 있겠다는 생각이 드네요
There was a problem hiding this comment.
확실히 blackjackGame이 게속 인자로 넘어가니 확실히 가독성이 안좋은 느낌이기도 하네
따라서 블랙잭 미션의 경우에는 필드에 있는 것도 괜찮다고 생각해!
확실히 메서드 네이밍에 신경쓴다면 읽는데 큰 문제가 없을 것 같은 느낌?
난 처음 것도 괜찮은 것 같아!
| while (blackJackGame.isPlayerDrawable(playerName) && playerChoice != DrawCommand.STAY) { | ||
| playerChoice = inputPlayerChoice(playerName); | ||
| if (playerChoice == DrawCommand.DRAW) { | ||
| blackJackGame.drawPlayerCard(playerName); | ||
| } | ||
| outputView.printCardStatusOfPlayer(playerName, convertCards(blackJackGame.getPlayerCards(playerName))); |
There was a problem hiding this comment.
이것도 리뷰어가... blackJackGame 을 인자로 넘기지 않았으면 좋겠다는 말을 듣고 시작하게 되었는데요
if 문을 물론 메서드로 분리할 수는 있겠지만, 의미를 못 드러내는 것 같아서 그냥 이대로 두어보려고 해요
There was a problem hiding this comment.
depth는 사실 연습을 위한 도구인 느낌이라 합당한 이유가 있다면 괜찮다고 생각해
| //이 클래스는 controller 의 역할이지만, controller 에다가 두면, 너무 많은 역할을 하게 되는 것 같아서 분리하였습니다 | ||
| //이 클래스가 단순하게 유틸리티 비슷한 클래스인데, 이대로 괜찮을지 여쭤보고 싶어요 |
There was a problem hiding this comment.
DTO로 변환시키는 것도 controller의 책임이라고 생각이 되는데
응집도를 생각했을 때, 해당 기능들을 controller에 위치시켜 보는 것은 어떨까?
There was a problem hiding this comment.
생각해보면, 지금은 도메인 객체를 그냥 내보냈는데, 사실 도메인 객체를 보호하기 위해서, 바로 안 내보낸다고 response를 내보내준다고 생각하면 response 객체를 blackjackGame 에서 바로 내보내도 괜찮을지도...?
아니면 도메인 객체를 바로 내보내는 방식으로 싹 갈아엎어봐도 괜찮을지도...?
고민이 계속 되네
| static Map<String, List<CardResponse>> getPlayerCards(final Players players) { | ||
| final List<String> playerNames = players.getPlayerNames(); | ||
| final Map<String, List<CardResponse>> playerCards = new HashMap<>(); | ||
| for (final String playerName : playerNames) { | ||
| final List<CardResponse> cardResponses = players.findPlayerByName(playerName) | ||
| .getCards() | ||
| .stream() | ||
| .map(CardResponse::from) | ||
| .collect(Collectors.toList()); | ||
| playerCards.put(playerName, cardResponses); | ||
| } | ||
| return playerCards; | ||
| } | ||
|
|
||
| static Map<String, List<CardResponse>> convertPlayersCards(final Map<String, List<Card>> playersCards) { | ||
| return playersCards.entrySet().stream() | ||
| .collect(Collectors.toMap( | ||
| Map.Entry::getKey, | ||
| entry -> entry.getValue().stream() | ||
| .map(CardResponse::from) | ||
| .collect(Collectors.toList()), | ||
| (oldValue, newValue) -> newValue, | ||
| LinkedHashMap::new | ||
| )); | ||
| } |
There was a problem hiding this comment.
List에서 DTO로 변환시키는 부분이 중복되는 것 같아서 다음과 같이 개선할 수 있을 것 같아~
동작 될지는 모름..!
static Map<String, List<CardResponse>> getPlayerCards(final Players players) {
return players.getPlayerNames.stream()
.collect(toMap(
Function::identity,
name -> generateCardResponses(players.findPlayerByName(name).getCards())
));
}
static List<CardResponse> generateCardResponses(final List<Card> cards) {
return cards.stream()
.map(CardResponse::from)
.collect(Collectors.toList());
}
static Map<String, List<CardResponse>> convertPlayersCards(final Map<String, List<Card>> playersCards) {
return playersCards.keySet().stream()
.collect(toMap(
Function.identity(),
name -> generateCardResponses(playersCards.get(name)),
(a, b) -> a,
LinkedHashMap::new
));
}| /** | ||
| * 이 클래스가 룰이 자주 변경될 수 있기에, 인터페이스를 둔다는 점은 좋은데요 | ||
| * <p> | ||
| * 사실상 한 클래스만 존재하는 상황에서, 이를 interface로 빼는 것은 별로 의미가 없을 수 있어보이는데, 어떻게 생각하시나요? |
There was a problem hiding this comment.
인터페이스가 필요하지 않은 상황에서 사용하는 것은 별로라고 생각해!
There was a problem hiding this comment.
또한 규칙같은 경우는 변경이 많은 부분이기 때문에, interface로 분리한다면 확장성 있는 설계가 될 수 있을것 같습니다.
라는 피드백을 듣고 interface로 분리하고 작업을 해보았는데요
변경이 잦은 부분인 것은 맞지만, 분리 하고 나서도 새로운 규칙을 만든 것이 아니라, 기존 규칙을 직접 변경한 것을 봐서 interface가 큰 의미가 없어서 다음 리뷰때는 없앨 예정입니다
고마워요 허브
|
|
||
| public class Player extends Participant { | ||
|
|
||
| private final int BUST_POINT = 21; |
There was a problem hiding this comment.
버스트 점수가 아닌 사실 블랙잭 점수(블랙잭 일 때도 더 이상 안뽑으니까)인데 변경해보는건 어떨까?
There was a problem hiding this comment.
그렇네 BUST_POINT 가 21점은 사실 아닐테니까 잘못된 네이밍이긴 하겠네
| boolean hasName(final String playerName) { | ||
| return name.getName() | ||
| .equals(playerName); | ||
| } |
There was a problem hiding this comment.
name에 equals & hashcode를 작성하는 건 어때? 매개변수도 Name으로 받고
There was a problem hiding this comment.
equals and hashcode 를 구현하는 것은 좋은 것 같아
이 메서드가 사용되는 곳이 Players 에서 사람을 찾을 때 쓰이는데, 이때를 생각해보면, 바깥에서 String 으로 소통하는 것을 거의 없애도 괜찮은 것 같기도 하고, String 으로 소통한다면, String 으로 놔두는 것도 좋은 것 같기도 하고
둘중 어떤 방향으로 갈지 고민을 해봐야 될것 같긴 하네 고마워
| private final Participants participants; | ||
| private final Deck deck; | ||
| private final BlackJackRule blackJackRule; | ||
| private final Map<String, Integer> playerMoney = new LinkedHashMap<>(); |
There was a problem hiding this comment.
Player 에다가 Money 를 넣고 매번 계산해서 봔환해줘야겠다 고마워
| return participants.getPlayers(); | ||
| } | ||
|
|
||
|
|
| } | ||
|
|
||
| @Test | ||
| void 카드_포켓에서_Ace_의_점수_계산이_11_과_1로_잘_계산된다() { |
There was a problem hiding this comment.
그렇네 한 테스트에서 너무 여러 작업들을 한꺼번에 한 느낌이 드네
2개로 나눠봐야겠다 고마워
| private static List<String> inputPlayerNames(final InputView inputView) { | ||
| return repeatUntilNoException(() -> { | ||
| final List<String> names = inputView.inputPlayerNames(); | ||
| Participants.validatePlayerNames(names); |
| WIN(1), | ||
| TIE(0), | ||
| LOSE(-1), | ||
| BLACKJACK_LOSE(-1); |
There was a problem hiding this comment.
그러게요 블랙잭으로 지는건가요? 뭔소리에요 도대체
리뷰를 이것도 못 하게 막았잖아요
플레이어가 블랙잭으로 이기기도 하면, 딜러가 블랙잭으로 이길 수도 있지 않을까요?
| final int countOfAce = countAce(); | ||
| int scoreOfCards = calculateMinimumScore(); | ||
| for (int i = 0; i < countOfAce; i++) { | ||
| scoreOfCards = calculateAceScore(scoreOfCards); |
There was a problem hiding this comment.
계산이 필요 없는 상황에서도 해당 메소드를 호출하는 경우가 있을 것 같은데 조건을 for문 혹은 while에 사용하여 불필요한 호출을 줄일 수 있는 방법이 있을 것 같네요!
| import blackjack.domain.card.Card; | ||
| import java.util.Objects; | ||
|
|
||
| public class CardResponse { |
| int profit(final double profit) { | ||
| return (int) (value * profit); | ||
| } |
| public class OutputView { | ||
|
|
||
| private static final String OUTPUT_DISTRIBUTE_MESSAGE = "딜러와 {0}에게 2장을 나누었습니다."; | ||
|
|
| @@ -0,0 +1,12 @@ | |||
| package blackjack.domain.card; | |||
|
|
|||
| public class CardFixture { | |||
| return Stream.of( | ||
| Arguments.of(List.of()), | ||
| Arguments.of(List.of("pobi", "crong", "honux", "wannte", "디디", "누누")) |
|
|
||
| @Test | ||
| void 참가자는_점수를_계산할_수_있다() { | ||
|
|
| CLOVER_ACE, | ||
| HEART_KING); |
There was a problem hiding this comment.
줄을 내린 이유가 있을까요?
Fixture로 둬서 굳이 내리지않아도 확인하기 용이할거같아요~
|
|
||
| public class Application { | ||
|
|
||
| public static void main(final String[] args) { |
There was a problem hiding this comment.
main 메소드가 길어서 전 안읽었는데 분리하는 것은 어때요? 분리 안한 이유가 궁금합니다
There was a problem hiding this comment.
ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ
읽어주세요
#9 (comment)
여기서 볼 수 있는 것처럼 나름의 고민을 했는데, 그냥 분리 안 했어요
| WIN(1), | ||
| TIE(0), | ||
| LOSE(-1), | ||
| BLACKJACK_LOSE(-1); |
There was a problem hiding this comment.
그러게요 블랙잭으로 지는건가요? 뭔소리에요 도대체
리뷰를 이것도 못 하게 막았잖아요
플레이어가 블랙잭으로 이기기도 하면, 딜러가 블랙잭으로 이길 수도 있지 않을까요?
|
|
||
| private void validateCardPocket(final List<Card> cards) { | ||
| if (cards == null) { | ||
| throw new IllegalArgumentException("카드에 null 값이 들어갈 수 없습니다"); |
There was a problem hiding this comment.
뷰에 보낼 메세지인데 null 이라고 하면 사용자는 뭐라 생각할까요?
|
|
||
| private void validateCards(final Queue<Card> cards) { | ||
| if (cards == null) { | ||
| throw new IllegalArgumentException("카드에 null 이 들어왔습니다"); |
| } | ||
| if (cards.size() != NUMBER_OF_CARDS_IN_DECK) { | ||
| throw new IllegalArgumentException( | ||
| "카드 숫자는 " + NUMBER_OF_CARDS_IN_DECK + "장이어야 합니다 현재 :" + cards.size() + "장"); |
| //애그리거트 루트로 생각하다보니, 여기에 검증하는 작업을 넣었는데, 한번에 이름, 돈이 들어오지 않기에, 생성시 검증이 불가능합니다 | ||
| //그러다보니 static 으로 작성하게 되었습니다 | ||
| public static void validatePlayerNames(final List<String> playerNames) { | ||
| new Names(playerNames); |
| //애그리거트 루트로 생각하다보니, 여기에 검증하는 작업을 넣었는데, 한번에 이름, 돈이 들어오지 않기에, 생성시 검증이 불가능합니다 | ||
| //그러다보니 static 으로 작성하게 되었습니다 | ||
| public static void validatePlayerNames(final List<String> playerNames) { | ||
| new Names(playerNames); |
There was a problem hiding this comment.
차라리 validator 같은 검증용 객체를 사용하는 것이 낫지 않을까요?
리뷰를 못달게 해주면 어떻게 댓글을 다나요
애그리거트루트라고 하는 개념을 도입했는데, 그때 검증용 객체가 외부에 노출되어 있다는 것이 도메인 개념에서 이상할 것 같아서 하지 않았습니다
| return players.stream() | ||
| .filter(player -> player.hasName(playerName)) | ||
| .findFirst() | ||
| .map(Player::isDrawable) | ||
| .orElseThrow(PlayerNotFoundException::new); | ||
| } | ||
|
|
||
| void draw(final String playerName, final Card card) { | ||
| final Player targetPlayer = players.stream() | ||
| .filter(player -> player.hasName(playerName)) | ||
| .findFirst() | ||
| .orElseThrow(PlayerNotFoundException::new); | ||
| targetPlayer.drawCard(card); | ||
| } | ||
|
|
||
| List<Player> getPlayers() { | ||
| return players; | ||
| } | ||
|
|
||
| List<CardResponse> getPlayerCards(final String playerName) { | ||
| return players.stream() | ||
| .filter(player -> player.hasName(playerName)) | ||
| .findFirst() | ||
| .map(Player::getCards) | ||
| .orElseThrow(PlayerNotFoundException::new); |
There was a problem hiding this comment.
Player player = findPlayer(playerName) 로 분리하는 건 어때요?
| @Test | ||
| void 카드의_Symbol을_반환한다() { | ||
| assertThat(card.getSymbol()) | ||
| .isEqualTo(symbol); | ||
| } | ||
|
|
||
| @Test | ||
| void 카드의_Shape를_반환한다() { | ||
| assertThat(card.getShape()) | ||
| .isEqualTo(shape); | ||
| } | ||
|
|
||
| @Test | ||
| void 카드의_점수를_반환한다() { | ||
| assertThat(card.getScore()) | ||
| .isEqualTo(symbol.getScore()); | ||
| } | ||
|
|
||
| @Test | ||
| void 카드가_Ace인지_확인한다() { | ||
| assertThat(card.isAce()) | ||
| .isEqualTo(symbol.isAce()); | ||
| } |
| void 생성시_null_이면_예외() { | ||
| assertThatThrownBy(() -> new Deck(null)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessage("카드에 null 이 들어왔습니다"); |
There was a problem hiding this comment.
모든 부분에 다 null 체크를 넣어볼까 라는 생각을 했지만 하지 않았습니다
이정도까지만 해도 충분할 것 같아보이네요
No description provided.