[2단계 - 블랙잭 베팅] 부기(이창의) 미션 제출합니다. #881
Conversation
| public static void main(String[] args) { | ||
| final InputView inputView = new InputView(); | ||
| final OutputView outputView = new OutputView(); | ||
| final Blackjack blackjack = Blackjack.from(inputView.readPlayerNames()); | ||
| final WagerMoney wagerMoney = new WagerMoney(inputView.readPlayersMoney(blackjack)); | ||
|
|
||
| blackjack.initPickCard(); | ||
| outputView.printHandOut(blackjack); | ||
|
|
||
| blackjack.pickCardPlayersIfNotBust(inputView::readPlayerAnswer, outputView::printPlayerPickCard); | ||
| blackjack.pickCardDealerIfNotMax(); | ||
|
|
||
| outputView.printDealerPickCard(blackjack); | ||
| outputView.printDealerHandResult(blackjack); | ||
| outputView.printPlayerHandResult(blackjack); | ||
|
|
||
| blackjack.duel(); | ||
|
|
||
| outputView.printWagerResult(wagerMoney); | ||
| } |
There was a problem hiding this comment.
Console 프로그램에 맞지 않는 MVC 패턴을 제거하고, 객체에 대해서 더 집중해서 구현하려고 노력했습니다.
따라서, controller가 사라지고, 메인 영역에서 흐름제어를 진행하도록 했습니다.
또한, 패키지 구조또한 domain, controller가 제거되었고 각 카테고리에 따라 객체들을 분류해주었습니다.
왜 MVC 패턴을 제거하게 되었는지 궁금합니다 :)
| KING(Score.from(10)); | ||
|
|
||
| private static final Score ACE_MIN = Score.from(1); | ||
|
|
||
| private final Score score; | ||
|
|
||
| Rank(final Score score) { | ||
| this.score = score; | ||
| } |
There was a problem hiding this comment.
| KING(Score.from(10)); | |
| private static final Score ACE_MIN = Score.from(1); | |
| private final Score score; | |
| Rank(final Score score) { | |
| this.score = score; | |
| } | |
| KING(10); | |
| private static final Score ACE_MIN = Score.from(1); | |
| private final Score score; | |
| Rank(final int score) { | |
| this.score = Score.from(score); | |
| } |
int를 받아 생성자에서 Score를 생성해줘도 괜찮을 것 같아요. enum 정의하는 부분이 깔끔해져서요.
| public boolean isWin() { | ||
| return get(DuelResult.WIN).isGreaterThan(get(DuelResult.LOSE)) | ||
| && get(DuelResult.WIN).isGreaterThan(get(DuelResult.DRAW)); | ||
| } | ||
|
|
||
| public boolean isDraw() { | ||
| return get(DuelResult.DRAW).isGreaterThan(get(DuelResult.WIN)) | ||
| && get(DuelResult.DRAW).isGreaterThan(get(DuelResult.LOSE)); | ||
| } | ||
|
|
||
| public boolean isLose() { | ||
| return get(DuelResult.LOSE).isGreaterThan(get(DuelResult.WIN)) | ||
| && get(DuelResult.LOSE).isGreaterThan(get(DuelResult.DRAW)); | ||
| } |
There was a problem hiding this comment.
if (duelHistory.isWin()) {
return DuelResult.WIN;
}
if (duelHistory.isDraw()) {
return DuelResult.DRAW;
}
return DuelResult.LOSE;바로 DuelResult를 반환하지 않는 이유가 있을까요? isLose는 사용하지 않는 메서드네요.
| public class WagerMoney { | ||
| private final Map<Player, Money> money; | ||
|
|
||
| public WagerMoney(final Map<Player, Money> money) { | ||
| this.money = new LinkedHashMap<>(money); | ||
| } |
There was a problem hiding this comment.
모든 원시값과 컬렉션을 객체로 감싸주다보니 depth가 계속 깊어지는 문제가 발생했습니다.
그래서 마지막에 추가한, Wager의 경우 Map<Player, Money>로 외부에서 처리하도록 하였는데 이렇게 늘어난 depth의 복잡도를 줄이는 방식이 옳은 방식인지 잘 모르겠습니다.
원시값을 포장했을 때의 장점은 잘 알려져있습니다. 원시값을 사용했을 때보다 의미를 명확히 할 수 있고, 관련 로직을 해당 클래스 내에서 관리할 수 있고, 불변으로 만든다면 안정성도 보장되겠죠. 하지만 모든 설계에는 트레이드오프가 있습니다. 모든 원시값과 문자열을 포장했을 때 클래스가 과도하게 많아지면 관리해야 할 코드과 클래스도 그만큼 늘어나게 됩니다. 말씀하신 것처럼 depth도 깊어지게 되구요.
score.isLessThan(DEALER_PICK_CARD_SCORE_MAX) || score.equals(DEALER_PICK_CARD_SCORE_MAX) 이런 코드를 보면 <= 키워드로 쉽게 비교할 수 있는 부분이었는데 비교하는 메서드를 추가해야 하고, 메서드를 통해 어렵게 비교하게 되었죠.
모든 원시값을 포장하라는 원칙을 철저히 지키신 점 너무 좋습니다. 👍 하지만 항상 모든 원칙을 지키는 것이 정답도 아니고, 그게 가능하지도 않습니다. 모든 원시값을 포장했을 때와 하지 않았을 때 부기가 느낀 장단점을 통해 앞으로 더 유연한 설계를 할 수 있으면 좋겠습니다! 그런 의미에서 WagerMoney 를 Map을 사용해서 충분히 잘 구현해주셨다고 생각합니다.
| if (player.calculateDuelResult() == DuelResult.DRAW) { | ||
| return wager; | ||
| } | ||
| if (player.calculateDuelResult() == DuelResult.WIN && player.isBlackjack()) { | ||
| return wager.multiply(BLACKJACK_WIN_WAGER); | ||
| } | ||
| if (player.calculateDuelResult() == DuelResult.WIN) { | ||
| return wager.multiply(WAGER_WIN); | ||
| } | ||
| return wager.multiply(WAGER_LOSE); | ||
| } |
There was a problem hiding this comment.
| if (player.calculateDuelResult() == DuelResult.DRAW) { | |
| return wager; | |
| } | |
| if (player.calculateDuelResult() == DuelResult.WIN && player.isBlackjack()) { | |
| return wager.multiply(BLACKJACK_WIN_WAGER); | |
| } | |
| if (player.calculateDuelResult() == DuelResult.WIN) { | |
| return wager.multiply(WAGER_WIN); | |
| } | |
| return wager.multiply(WAGER_LOSE); | |
| } | |
| WagerResult wagerResult = Arrays.stream(values()) | |
| .filter(value -> value.duelResultPredicate.test(player)) | |
| .findFirst() | |
| .orElseThrow(IllegalArgumentException::new); | |
| return wager.multiply(wagerResult.multiplier); |
enum과 함수형 인터페이스를 적절히 사용하면 if 문 없이도 구현 가능할 것 같은데 어떻게 생각하시나요?
| public void printPlayersCard(final Map<String, List<String>> cardsByPlayerName) { | ||
| cardsByPlayerName.forEach(this::printPlayerCards); | ||
| private void printDealerHandOutResult(final Blackjack blackjack) { | ||
| final Card card = blackjack.getDealer().getParticipant().getCardHand().getCards().getFirst(); |
There was a problem hiding this comment.
OutputView에서 전체적으로 데이터를 가져오기 위한 호출 depth가 너무 기네요 😢 이러면 어떤 객체가 어떤 값을 갖고 있는지 내부 구조를 정확하게 알고 있지 않으면 원하는 값을 가져오기 힘들고, 코드를 볼 때에도 이해하기 어렵습니다. 개선할 수 있는 방법은 없을까요?
There was a problem hiding this comment.
위임을 하는 메서드에 대해서 테스트를 해야하는지 의문이 들었습니다.
예를 들어 Player와 Dealer는 Participant를 조합으로 가지고 있고 중복되는 메서드에 대해서 participant의 메서드를 그대로 위임했습니다.
따라서 이 부분에 대해서 테스트를 어떻게 적용해야하는지 의문이 있습니다.
- participant만 테스트하면 된다.
- 위임을 하는 메서드를 테스트하면 된다. (player, dealer 각각 테스트)
- participant도 테스트하고, 위임을 하는 메서드도 테스트해야한다.
3번으로 하는 방식이 가장 안전하다고 판단되지만, 결국 테스트가 중복된다고 느껴졌습니다.
어떤 방식으로 하는게 좋을까요?
이미 부기가 각 방식에 대한 장단점을 충분히 이해하고 있어 보여요. 모든 게 완벽한 방법은 없습니다 :) 부기는 어떤 방식이 가장 마음에 드나요?
| // final var actual = Rank.ifOverThanBustScoreAceIsMIN(score, aceCount, bustScore); | ||
|
|
||
| // then | ||
| assertThat(actual).isEqualTo(expected); | ||
| // assertThat(actual).isEqualTo(expected); |
| assertThat(duelHistory.getWinCount()).isEqualTo(expectedWinCount); | ||
| assertThat(duelHistory.getDrawCount()).isEqualTo(expectedDrawCount); | ||
| assertThat(duelHistory.getLoseCount()).isEqualTo(expectedLoseCount); |
There was a problem hiding this comment.
assertAll이나 assertSoftly를 사용할까요? 전체적으로 점검 부탁드립니다!
| package value; | ||
|
|
||
| import java.util.Map; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
|
|
||
| public record Count(int value) { |
There was a problem hiding this comment.
value라는 패키지명과 Count라는 클래스명 모두 너무 모호한 것 같아요. value는 어떤 값을 의미하는 거고 Count는 어떤 개수를 의미하는 건지 알 수가 없네요. 카드 개수를 세는 데 사용하는 클래스라면 클래스명을 CardCount로 변경하고 card 패키지에 넣어주는 게 더 클래스의 의미를 확실하게 보여줄 수 있을 것 같습니다. Score도 고민해보세용.
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?
1~5점 중에서 선택해주세요.
선택한 점수의 이유를 적어주세요.
객체들의 책임을 분리하고 했지만, Blackjack 클래스에 3개의 인스턴스 변수가 존재합니다.
하지만, 이 부분은 Blackjack이 관리해야할 역할이라고 판단하여 굳이 depth를 늘려 줄이지는 않았습니다.!
어떤 부분에 집중하여 리뷰해야 할까요?
객체들의 책임과 TDD 사이클에 대해 집중해서 리뷰부탁드립니다.
STEP 2에서 변경된 포인트
모든 원시값을 포장하라
모든 원시값을 포장함으로써 원시값들이 객체로써 스스로 자신의 역할을 다하도록 만들었습니다.
MVC 패턴을 제거하라
Console 프로그램에 맞지 않는 MVC 패턴을 제거하고, 객체에 대해서 더 집중해서 구현하려고 노력했습니다.
따라서, controller가 사라지고, 메인 영역에서 흐름제어를 진행하도록 했습니다.
또한, 패키지 구조또한 domain, controller가 제거되었고 각 카테고리에 따라 객체들을 분류해주었습니다.
궁금한 점
테스트의 범위
위임을 하는 메서드에 대해서 테스트를 해야하는지 의문이 들었습니다.
예를 들어 Player와 Dealer는 Participant를 조합으로 가지고 있고 중복되는 메서드에 대해서 participant의 메서드를 그대로 위임했습니다.
따라서 이 부분에 대해서 테스트를 어떻게 적용해야하는지 의문이 있습니다.
3번으로 하는 방식이 가장 안전하다고 판단되지만, 결국 테스트가 중복된다고 느껴졌습니다.
어떤 방식으로 하는게 좋을까요?
파랑의 생각이 궁금합니다! 🤔
depth의 증가
모든 원시값과 컬렉션을 객체로 감싸주다보니 depth가 계속 깊어지는 문제가 발생했습니다.
그래서 마지막에 추가한, Wager의 경우
Map<Player, Money>로 외부에서 처리하도록 하였는데 이렇게 늘어난 depth의 복잡도를 줄이는 방식이 옳은 방식인지 잘 모르겠습니다.파랑의 생각이 궁금합니다! 🤔
함수형 인터페이스 사용
player의 경우, player가 21점 이하인 경우 + 사용자의 응답이 'y'인 경우에만 카드를 뽑는데 이경우에 객체 입장에서는 player들을 관리하는 players, player 객체에서 이를 수행하도록 하는 것이 옳다고 판단했습니다.
하지만 입력을 받기 위해 getPlayers()를 통해 흐름 제어를 하는 곳에서 해야하는 문제가 발생했습니다.
그래서 함수형 인터페이스를 활용해 inputView의 메서드를 메서드 참조로 player에게 넘겨주어 내부에서 처리하도록 만들었습니다.
이렇게 하는 경우 제 생각에는
"함수형 인터페이스를 통해 입력받기 때문에 객체가 직접 view에 의존이 생기지는 않는다."고 판단했습니다. 문제되는 경우가 존재하는지 궁금합니다!!
파랑의 생각이 궁금합니다! 🤔
WagerMoney
현재 WagerMoney는 Player와 Money를 Map의 키와 밸류로 가지고 있습니다.
생각하다보니, Player는 Key로써 사용해야하는데 저는 Key를 이용해서 로직을 수행하고 있어 Player의 변경에 WagerMoney와 WagerResultCalculator가 취약하다고 생각되었습니다.
그래서, 차라리 Player의 인스턴스 변수가 3개가 되더라도 Calculator가 Player를 받는 것이 아닌 DuelResult와 isBlackjack, Money의 결과 값만 받아서 계산하도록 하는게 더 변경에 안전한 코드가 되지 않을까? 생각했습니다.
이 부분에 대해서 파랑의 생각이 궁금합니다! 🤔