[1단계 - 블랙잭 게임 실행] 조조(조은별) 미션 제출합니다.#658
Conversation
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: eun-byeol <joeunbyeol98@gmail.com> Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
Co-authored-by: lilychoibb <choiyoonseo02@gmail.com>
- 이름의 마지막 글자가 모음이면 '는' 붙여 이름 출력 - 모음 이외의 문자는 '은' 붙여 이름 출력
DealerResult 클래스 추가
- 기존 players = players.hit(player, card) 로직에서 player.hitCard(card)으로 수정 - BlackjackGame players 멤버의 final 키워드 추가 - Player hand 멤버의 final 키워드 삭제
- Dealer hand 멤버 final 키워드 삭제
DealerResult 정적 팩토리 메서드 추가
GameResult, DealerResult 패키지 이동
- Players 이름 중복 검사 로직 수정 - Player equals, hashCode 메서드 삭제 - Dealer name 정적 필드 추가 - RandomCard.pickCards 반환 타입 수정
- assertAll 테스트 라이브러리 추가 - ParameterizedTest 추가 - 테스트 이름 수정
|
영이, 안녕하세요! ⭐️ 리팩터링 시 신경 쓴 점1. 네이밍
2. 클래스의 역할 분리
3. OutView 비즈니스 로직 제거
💭 추가 질문 목록 |
choijy1705
left a comment
There was a problem hiding this comment.
안녕하세요 조조!
최초의 카드를 생성하는 과정이 쉽게 이해가 잘안가는것 같아요
피드백 참고해서 카드 묶음(deck)을 만들고 이를 게임중에 관리하는 방법에 대해 고민해보면 좋을것 같습니다!
| public static Players preparePlayers() { | ||
| return retryOnException(() -> { | ||
| List<String> playerNames = askPlayerNames(); | ||
| return Players.from(playerNames); | ||
| }); | ||
| } |
There was a problem hiding this comment.
재입력로직은 domain 에서는 있으면 안될것 같네요! 도메인에는 관련된 비즈니스로직만 있어야 할것 같아요
이상적인것은 inputView에서 받는것이 가장 좋을것 같네요!
| return new BlackjackGame(dealer, players); | ||
| } | ||
|
|
||
| private void setting(BlackjackGame blackjackGame) { |
There was a problem hiding this comment.
init은 초기화라는 사전에 등재된 단어라서
축약이 아니지 않을까요?
| private static final String INVALID_PLAYER_NAMES_UNIQUE = "플레이어 이름은 중복될 수 없습니다."; | ||
| private static final int CARDS_PER_PLAYER = 2; | ||
|
|
||
| private final List<Player> group; |
There was a problem hiding this comment.
클래스명과 필드명이 같은걸 지양해야 한다에 가독성 좋은 어색하게 group으로 바꾼게 아닐까요??
| hitToPlayers(blackjackGame); | ||
| hitToDealer(blackjackGame); |
There was a problem hiding this comment.
| hitToPlayers(blackjackGame); | |
| hitToDealer(blackjackGame); | |
| blackjackGame.hitToPlayers(); | |
| blackjackGame.hitToDealer(); |
이런 느낌으로 가야하지 않을까요? game을 파라미터로 받는 어색한 구조인것 같아요
hitToPlayers()라는 명칭도 조금 어색하네요
blackjackGame.play()라는 메서드안에서 이 로직들을 수행해보면 어떨까요?
There was a problem hiding this comment.
전체적으로 game클래스에서 진행할법한 로직들이 controller에서 수행하고 있는 느낌이네요
현재 구조에서 game의 메서드들은 로직은 따로없고 player, delaer에거 호출을 전파하는 역할만 하고 있는것 같습니다
There was a problem hiding this comment.
전반적으로 컨트롤러에 있던 로직을 blackjackGame으로 옮겼습니다!
단, hitToPlayers() 로직은 hit 여부 응답을 받는 InputView 로직이 포함되어있어, 컨트롤러에 남겨두었습니다.
| private void initGame(BlackjackGame blackjackGame) { | ||
| int cardCount = blackjackGame.determineInitCardCount(); | ||
| List<Card> cards = RandomCard.pickCards(cardCount); | ||
| blackjackGame.initHand(cards); | ||
| OutputView.printInitHand(blackjackGame); | ||
| } |
There was a problem hiding this comment.
얘도 blackJackGame 안에서 초기화하는게 안어색하지 않을까요?
| import java.util.List; | ||
| import java.util.stream.Stream; | ||
|
|
||
| public class RandomCard { |
There was a problem hiding this comment.
해당 클래스의 로직이 쉽게 이해가 안가는데
설명해줄 수 있나요??
There was a problem hiding this comment.
해당 로직을 완전히 수정하여 제로에서 재설계했습니다. 이에 대한 내용은 아래에 정리해두었습니다.
| int cardCount = blackjackGame.determineInitCardCount(); | ||
| List<Card> cards = RandomCard.pickCards(cardCount); | ||
| blackjackGame.initHand(cards); |
There was a problem hiding this comment.
최초 카드를 배분하는 로직에 대한 내용입니다. 해당 로직을 전부 BlackjackGame 클래스로 이동시켰습니다!
| public int determineInitCardCount() { | ||
| return (players.count() + DEALER_COUNT) * INITIAL_CARD_COUNT; | ||
| } | ||
|
|
||
| public void initHand(List<Card> cards) { | ||
| dealer.hitCards(cards.subList(0, INITIAL_CARD_COUNT)); | ||
| players.hitCards(cards.subList(INITIAL_CARD_COUNT, cards.size())); | ||
| } |
There was a problem hiding this comment.
BlackjackGame 관련 로직을 전면 수정했습니다. 아래 메시지에 답변 정리해두었습니다.😊
| import model.card.Card; | ||
| import model.card.Hand; | ||
|
|
||
| public class Player { |
There was a problem hiding this comment.
dealer와 player는 인터페이스를 정의해서 관리해보는 건 어떨까요?
There was a problem hiding this comment.
결론적으로, 상속과 인터페이스를 모두 사용했습니다.
인터페이스를 사용한 이유
- 블랙잭 게임에서 요구되는 행동을 인터페이스로 관리할 수 있습니다. 참가자들은 이 인터페이스를 extends하기만 하면 됩니다.
- 다른 액션이 추가되는 경우 다중 상속을 통해 쉽게 확장시킬 수 있습니다.
상속을 사용한 이유
- Player와 Dealer의 중복 코드를 줄이기 위함입니다.
- Participant 추상 클래스를 생성하고, 인터페이스(HitAction)를 구현하고 있습니다.
public abstract class Participant implements HitAction {| import java.util.Map; | ||
| import java.util.stream.IntStream; | ||
|
|
||
| public class CardDispenser { |
There was a problem hiding this comment.
카드 묶음(Deck)을 만드는 로직이 변경해주신 부분도 쉽게 이해하기 어려운것 같습니다
- 모양과 번호에 따라 다른 카드를 어떻게 초기화하여 52장(?)을 만들지
- 해당 카드에서 순서대로 카드를 뽑는다고 했을때 어떤 자료구조를 사용하면 좋을지
- 만들어진 카드 묶음(Deck)을 어느 객체에서 관리하면 좋을지
변경해주신 randomCard부분도 너무 복잡한것 같습니다 😭
제로에서 다시한번 설계해보면 어떨까요??
- 컨트롤러 비즈니스 로직 BlackjackGame으로 이동 - BlackjackGame에서 CardShuffler, Dealer 인스턴스 변수로 갖도록 수정 - 랜덤 라이브러리 변경으로 인하여 ApplicationTest 삭제
- group -> players로 변경
- blackjackgame 패키지 -> game로 이름 수정 - dealer, player 패키지 -> participant 패키지로 이동
Player, Dealer가 HitAction 인터페이스를 구현하도록 수정
- ParticipantScore -> CardsScore 클래스로 대체 - 딜러의 승무패 판단 로직 수정
- Participant 추상 클래스 생성 - Participant에서 HitAction 인터페이스를 구현하도록 수정
|
안녕하세요 영이! 조조입니다😃 🤔 리팩토링 시 신경 쓴 점1. 랜덤 카드 로직 단순화피드백 주신 가정 : 랜덤 카드 묶음은 유한하다. 블랙잭 게임 시작 시에, 다음 로직이 수행됩니다.
2. 컨트롤러에 혼재된 BlackjackGame 역할 분리
주요 변경사항입니다.
3. Player와 Dealer의 중복 코드 제거상속을 통해 구현했습니다. 또한, public abstract class Participant implements HitAction { |
choijy1705
left a comment
There was a problem hiding this comment.
조조 피드백 잘 반영해주셨습니다!👍
추가 피드백 있는데
2단계 진행하면서 반영해주시면 좋을것 같습니다!
| public static HitChoice findHitChoice(String choice) { | ||
| return Arrays.stream(values()) | ||
| .filter(hitChoice -> hitChoice.displayName.equals(choice)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new IllegalArgumentException("y 혹은 n 중 하나를 입력해 주세요.")); | ||
| } |
There was a problem hiding this comment.
yes or no 인데
if 문으로 분기하는게 더 간결하고 가독성 좋지 않을까요?
|
|
||
| private static final int HIT_CONDITION = 22; | ||
|
|
||
| private final String name; |
| private static void validate(List<String> playerNames) { | ||
| validateEmpty(playerNames); | ||
| validatePlayerNamesUnique(playerNames); | ||
| } | ||
|
|
||
| private static void validateEmpty(List<String> playerNames) { | ||
| if (playerNames.isEmpty()) { | ||
| throw new IllegalArgumentException("플레이어 수는 1명 이상이어야 합니다."); | ||
| } | ||
| } | ||
|
|
||
| private static void validatePlayerNamesUnique(List<String> playerNames) { | ||
| Set<String> uniqueNames = new HashSet<>(playerNames); | ||
| if (uniqueNames.size() < playerNames.size()) { | ||
| throw new IllegalArgumentException("플레이어 이름은 중복될 수 없습니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
validate 메서드안에서 모든 검증을 처리하도록 하나로 합치면 어떨까요?
검증이 하나둘 추가되면 메서드도 늘어나고 클래스가 커져서 복잡해질것 같아요
| import java.util.List; | ||
| import java.util.stream.Stream; | ||
|
|
||
| public class Deque { |
There was a problem hiding this comment.
해당 클래스의 존재 이유를 잘 모르겠습니다!
CardShuffler에서 초기화시에 initCards 로직을 수행하면 안되는건가요?
그리고 자료구조를 Class명으로 가지는것도 어색하네요😭
| import java.util.NoSuchElementException; | ||
| import java.util.Queue; | ||
|
|
||
| public class CardShuffler { |
There was a problem hiding this comment.
CardShuffler라는 네이밍이 매번 카드를 뽑을때마다 카드를 섞을것 같은 느낌을 주는데
CardDeck과 같은 네이밍이 어떤가요?
| while (dequeCount > 0) { | ||
| cards.addAll(new Deque().getCards()); | ||
| dequeCount--; | ||
| } |
There was a problem hiding this comment.
for문 처리가 좋지 않을까요?
while은 무한루프의 가능성이 있기 때문에 정말 어쩔 수 없는 경우 아니면 사용하지 않는것이 좋은것 같아요
| IntStream.range(0, players.count()) | ||
| .forEach(order -> dealCardsToPlayer(players, order)); |
There was a problem hiding this comment.
players.deal(cardShuffler)
같은 형태로 처리하는게 자연스럽지 않을까 싶습니다 ㅎㅎ
| IntStream.range(0, INITIAL_CARD_COUNT) | ||
| .forEach(count -> dealer.hitCard(cardShuffler.drawCard())); |
There was a problem hiding this comment.
IntStream은 요즘 트렌드인가 봅니다...?
for문이 더 가독성 좋지 않나요?
| this.results = Collections.unmodifiableMap(results); | ||
| } | ||
|
|
||
| public static PlayerResults from(GameResult gameResult) { |
There was a problem hiding this comment.
GameResult안에서 getPlayerResults 하면 Map<String, ResultStatus> 가 반한되도록 하면 좀 더 심플해지지 않을까요?
map을 포장하면 결과를 내는 과정이 많이 복잡해진것 같아요
DealerResult도 마찬가지입니다!

영이, 안녕하세요! 조조입니다🐥
중간부터 페어의 건강상 문제로 부득이하게 혼자 진행하게 되었습니다.
저는 24시 언제든 slack으로 응답이 가능합니다. 반대로, 영이의 시간규칙(?)을 알려주시면 좋을 것 같아요!(ex. 몇 시 이후에는 dm 자제 등)
🥔 본인의 상황(feat. TMI)
🤔 코드를 작성할 때 신경 쓴 점
1. 쉬운 테스트를 위한 인터페이스 사용 자제
저는
테스트를 쉽게 작성할 수 있어야 한다는 기준을 지향합니다.특히 랜덤 테스트를 할 때, 인터페이스를 재정의하여 테스트 코드를 작성하는 방식이 오히려 테스트를 어렵게 하는 것 같습니다.
따라서,
랜덤으로 카드를 뽑는 로직(CardDispenser)과 연관이 있는 도메인을 어떻게 쉽게 테스트 할 수 있을까?하는 고민에 대해,외부에서 카드를 생성하고 인자로 받아 처리하는 방식을 선택했습니다.
BlackjackGame클래스의 메서드 인자를 통해Cards를 받고 있습니다.2. 불변 객체 만들기
자동차 미션 때와 유사하게, 딜러와 플레이어의 카드는 계속 변합니다.
불변 객체를 만들기 위해, 카드가 늘어날 때마다 새로운 객체를 생성해서 반환했습니다.
3. 카드 합 계산 로직 단순화
특히 카드 합을 계산하는 로직이 까다로웠습니다.
ACE가 2개의 값을 가질 수 있다보니 Enum 클래스에서 처리하는 것에 한계를 느껴, 다음과 같이 단순화했습니다.
1. default 값으로,
ACE는1의 값을 갖는다.Hit 질문 여부를 판단하기 위해, ACE의 최소 조건으로 카드 합을 계산했습니다.
2. 승패를 낼 때,
ACE가11값을 고려하여 최적화 한다.GameScore클래스에서 1번에서 계산된 값에서 +10을 하면서 21에 가까운 값을 찾도록 했습니다.💭 질문
1.
모든 원시 값과 문자열을 포장한다어디까지 지켜야 할까?요구사항에 명시된 조건입니다.
여기서 의미하는
원시 값은 어느 범위까지 해당 되는 지에 대해 고민이 있었습니다.Collection으로 선언된 변수를 비롯하여 모든 원시값을 감싸다 보면, 불필요한 getter 과정이 반복되고 코드가 복잡해지는 문제가 있었습니다.
아래 코드처럼, 파라미터 변수도 포장 대상이 되는지 궁금합니다.
2. 재입력 로직을 View로 이동,
View가 Model을 알고 있는 것이 괜찮을까?MVC에서 View가 Model을 알고 있는 것이 가능하다고 공부했지만,
View에서 객체를 생성할 만큼 알아도 괜찮을까? 하는 의문이 있었습니다.
이에 대한 의견 궁금합니다🤔
플레이어를 입력 받는
InputView클래스입니다.