[1단계 - 블랙잭 게임 실행] 리비(이근희) 미션 제출합니다. #7
Conversation
|
|
||
| public CardDeck create() { | ||
| List<Card> cards = createCards(CardShape.values(), CardNumber.values()); | ||
| Collections.shuffle(cards); | ||
| return new CardDeck(cards); | ||
| } | ||
|
|
There was a problem hiding this comment.
CardDeckCreator은 CardDeck을 생성하는 로직을 품고 있군요!
CardDeck이 담당해도 괜찮은 역할 아닐까요? 분리한 이유가 궁금해요 💭
There was a problem hiding this comment.
도메인을 규칙들(enum으로 부터 생성한다, 섞는다)로부터 분리하여 순수히 유지하고 싶었어요
단순히 enum으로 부터 카드를 생성하는 역할과 카드를 섞는 것은 CardDeck의 책임이 아니라고 생각한 것이기도 합니다.
다만 다음의 단점이 있을 것 같다고 생각해서 현재는 바꾼 상태입니다 🫠
- 파생되는 클래스가 많음
- 도메인에서 여러 규칙이 분리되어 다른 개발자가 의도하지 않은대로 도메인을 사용할 위험성이 있음
| private List<Card> createCards(CardShape[] cardShapes, CardNumber[] cardNumbers) { | ||
| return Arrays.stream(cardShapes) | ||
| .flatMap(shape -> Arrays.stream(cardNumbers) | ||
| .map(number -> new Card(shape, number))) | ||
| .collect(Collectors.toList()); | ||
| } | ||
| } |
There was a problem hiding this comment.
카드 모양과 숫자에 대해 가능한 모든 조합을 생성하는 로직을 stream 이용해서 잘 풀어내셨네요 👍
다른 크루에게도 질문 드렸는데요! 중첩 반복문을 사용하는 거에 대해 어떻게 생각하시나요? 저는 중첩 반복문이 스트림을 이용하는 것보다 명확하다고 생각했어요. 만약, indent<2 라는 요구 사항이 없어도 스트림을 사용하실 건지 궁금해요 💭
There was a problem hiding this comment.
이전에 어떤 PR을 염탐하다가 어떤 리뷰어분이 다음과 같이 코멘트 남겼던 것이 기억에 남네요 🤔
- for문으로 iterate하는 것은 명령형
- stream으로 도는 것은 선언형
개인적으로는 어떤 알고리즘이 돌아야 하는지 명령하는 것(HOW를 작성하는 것)보다 동작을 선언하는 것(WHAT을 작성하는 것)이 더 가독성 좋다고 생각해서 요구 사항이 없어도 스트림을 사용할 것 같습니다 👍
| ACE(1, "A"), | ||
| TWO(2, "2"), | ||
| THREE(3, "3"), | ||
| FOUR(4, "4"), | ||
| FIVE(5, "5"), | ||
| SIX(6, "6"), | ||
| SEVEN(7, "7"), | ||
| EIGHT(8, "8"), | ||
| NINE(9, "9"), | ||
| TEN(10, "10"), | ||
| JACK(10, "J"), | ||
| QUEEN(10, "Q"), | ||
| KING(10, "K"); |
There was a problem hiding this comment.
이 부분에 대해 저도 동일하게 처리했지만, 리비의 의견이 궁금해서 코멘트 남겨요!
2를TWO로 출력해 주세요. (국제화)
위와 같이 name 필드에 대한 비즈니스 정책이 변경된다면, 개발자는 어떤 파일을 먼저 열어볼까요?
There was a problem hiding this comment.
뷰를 열어볼 것 같습니다 🫠
처음에는 카드 숫자가 이름을 가지고 그 이름을 꺼내는 것이 왜 뷰로직이야?라고 생각했는데 망쵸가 해주신 질문과 비슷한 것을 듣고 저는 수정하게 되었네요 😅
|
|
||
| public class ScoreCalculateStrategy { | ||
|
|
||
| private static final int BLACK_JACK = 21; |
There was a problem hiding this comment.
변수명이 조금 더 명확하면 좋을 것 같아요! CONDITION 같은 suffix는 어떠신가요?
There was a problem hiding this comment.
BUST_CONDITION과 같이 수정해볼 수 있겠네요 💪🏻
| public PlayerCreator(HandCreator handCreator) { | ||
| this.handCreator = handCreator; | ||
| } | ||
|
|
||
| public Player createPlayerFrom(PlayerName playerName, CardDeck cardDeck) { | ||
| return new Player(playerName, handCreator.createPlayerHandFrom(cardDeck)); | ||
| } | ||
|
|
||
| public Player createDealerFrom(CardDeck cardDeck) { | ||
| return new Player(new PlayerName(DEALER_NAME), handCreator.createDealerHandFrom(cardDeck)); | ||
| } | ||
| } |
There was a problem hiding this comment.
정적 팩토리 메서드를 고려해 보는 것은 어떨까요?
호출하는 부분을 확인해 보면, 생성만을 담당하는 PlayerCreator의 인스턴스가 불필요하게 생성된다는 생각이 들어요!
There was a problem hiding this comment.
- Player는 어떤 전략을 가지고 탄생하는지 (히트 가능 숫자 21이하)
- 딜러는 어떤 전략을 가지고 탄생하는지 (히트 가능 숫자 16이하)
처음에는 위의 두 정보를 Player가 모르도록 하는 것이 좋은 설계라고 생각했어요 (책임 분리 관점)
그리고 Player가 어떤 전략을 가지고 탄생하는지 정해주는 팩토리 클래스가 있으면 좋겠다고 생각했죠
하지만 정적 팩토리 메서드를 사용하면 Player는 게임의 룰 전략 등 알아야 하는 것이 너무 많아진다고 생각했습니다.
다만 도메인을 의도한 대로 사용해주도록 하려면 너무 많은 분리는 오히려 독이 될 수도 있다는 것을 깨닫게 되었네요 😂
There was a problem hiding this comment.
의견 감사합니다! 리비가 많이 고민했다는 것이 느껴지네요 👍
첨언을 하자면, 저는 언급해 주신 두 정보를 Player와 Dealer가 알도록 했는데요! 실제 게임에서 Player와 Dealer는 합리적으로 룰을 숙지하고 있다고 생각했기 때문이에요.
이미 수정하신 것 같아서.. 🏃🏃
| public enum DrawDecision { | ||
|
|
||
| YES("y"), | ||
| NO("n"); | ||
|
|
||
| private final String code; | ||
|
|
||
| DrawDecision(String code) { | ||
| this.code = code; | ||
| } | ||
|
|
||
| public static DrawDecision from(String code) { | ||
| return Arrays.stream(DrawDecision.values()) | ||
| .filter(drawDecision -> drawDecision.code.equals(code)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new IllegalArgumentException("[ERROR] 해당하는 값을 찾지 못하였습니다.")); | ||
| } | ||
| } |
| private final List<Card> cards; | ||
| private final ScoreCalculateStrategy scoreCalculateStrategy; | ||
| private final HitStrategy hitStrategy; |
There was a problem hiding this comment.
요구사항을 벗어나는 것 같아요 😀 개인적으로 이런 거 좋아합니다.. (틀에서 벗어나는 것)
많이 고민하고 작성하셨을 것 같은데, 리비의 의견이 궁금해요!
추가된 요구 사항: 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.
There was a problem hiding this comment.
추가된 요구 사항: 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.
해당 요구 사항이 등장하게 된 맥락을 생각해보았어요 🤔
저한테는같은 관심사를 가진 애들끼리만 연관관계를 맺도록 고려해봐라와 같이 해석되는 것 같습니다.
작성할 당시 3 필드는 같은 관심사를 공유하는 것으로 해석했고 위와 같은 코드가 탄생하게 되었네요 😀
| import blackjack.domain.DealerGameResult; | ||
| import blackjack.domain.player.Player; | ||
| import blackjack.domain.player.Players; | ||
| import blackjack.domain.rule.Score; |
| public String resolveHandOutEventMessage(Players players, int handOutCount) { | ||
| String namesMessage = resolveNamesMessage(players); | ||
| String message = String.format("딜러와 %s에게 %d장을 나누었습니다.", namesMessage, handOutCount); | ||
| return String.join("", LINE_SEPARATOR, message); | ||
| } |
| public static void main(String[] args) { | ||
|
|
||
| InputView inputView = new InputView(); | ||
| OutputView outputView = new OutputView(new MessageResolver()); | ||
|
|
||
| BlackJackController blackJackController = new BlackJackController(inputView, outputView); | ||
| blackJackController.run(); | ||
| } |
There was a problem hiding this comment.
OutputView와 InputView를 만들어서 Controller에 주입해주는 이유가 있을까요?
Controller에서 OutputView와 InputView를 만들어도 되지 않을까요?
There was a problem hiding this comment.
필드 주입의 단점을 의식한 것이긴 해요 (DI 측면)
다만 변경될 가능성이 극히 낮음에도 YAGNI원칙을 무시한 부분이 아닐까라는 생각이 들기는 하네요 😅
| private final CardShape cardShape; | ||
| private final CardNumber cardNumber; |
There was a problem hiding this comment.
CardShape, CardNumber로 역할을 잘 나누셨네요!
저는 Shape, Number라고 쓰는 편이 더 간결하면서도 역할을 충분히 표현해줄 수 있다고 생각하는데, 리비는 어떻게 생각하세요?
There was a problem hiding this comment.
도메인에서 어느정도 노출되는 정보가 있으니 커찬의 의견 합리적인 것 같습니다.
취향의 영역이라는 생각도 드네요 🤔
| @DisplayName("존재하는 코드명이면 적절한 상수를 반환받는다") | ||
| @ParameterizedTest | ||
| @MethodSource("provideEnumFromCsv") | ||
| void testEnumFromValidCode(String code, DrawDecision drawDecision) { | ||
| assertThat(DrawDecision.from(code)).isEqualTo(drawDecision); | ||
| } | ||
|
|
||
| private static Stream<Arguments> provideEnumFromCsv() { | ||
| return Stream.of( | ||
| Arguments.of("y", DrawDecision.YES), | ||
| Arguments.of("n", DrawDecision.NO) | ||
| ); | ||
| } |
There was a problem hiding this comment.
이건 취향일 수도 있을 것 같은데 메서드를 구현해야 되는 @MethodSource보다 어노테이션 안에서 사용할 수 있는 @CsvSource도 추천해요!
@DisplayName("존재하는 코드명이면 적절한 상수를 반환받는다")
@ParameterizedTest
@CsvSource({"y, YES", "n, NO"})
void testEnumFromValidCode(String code, DrawDecision drawDecision) {
assertThat(DrawDecision.from(code)).isEqualTo(drawDecision);
}There was a problem hiding this comment.
읽는 사람의 부수적인 피로도를 낮출 수 있는 좋은 방법이라고 생각해요 🧐
추천 감사합니다 😊
| @DisplayName("덱에서 카드를 뽑을 수 있다") | ||
| @Test | ||
| void testPopCardFromDeck() { | ||
| List<Card> cards = new ArrayList<>(); | ||
| Card card1 = new Card(CardShape.HEART, CardNumber.TWO); | ||
| Card card2 = new Card(CardShape.CLUB, CardNumber.THREE); | ||
| Card card3 = new Card(CardShape.DIAMOND, CardNumber.FOUR); |
There was a problem hiding this comment.
한 클래스 내에서 같은 카드들이 반복적으로 사용되는데, 상수로 뽑아서 관리하는게 좀더 수월하지 않을까요?
@DisplayName("총 카드 덱 테스트")
class CardDeckTest {
private static final Card CARD1 = new Card(CardShape.HEART, CardNumber.TWO);
private static final Card CARD2 = new Card(CardShape.CLUB, CardNumber.THREE);
private static final Card CARD3 = new Card(CardShape.DIAMOND, CardNumber.FOUR);
@DisplayName("덱에서 카드를 뽑을 수 있다")
@Test
void testPopCardFromDeck() {
List<Card> cards = List.of(CARD1, CARD2, CARD3);
CardDeck cardDeck = new CardDeck(cards);
Card popped = cardDeck.popCard();
assertThat(popped).isEqualTo(CARD1);
}
....
}There was a problem hiding this comment.
지금 보니 요거를 fixture로 관리했어야 하는데 커찬이 정확히 지적해주셨네요
훨씬 좋아보입니다 👍
| @DisplayName("카드의 합을 계산할 수 있다.") | ||
| @Test | ||
| void testHandSummation() { | ||
| Hand hand = HandFixture.of(2, 2, 2); | ||
| int expected = hand.sum(); | ||
|
|
||
| assertThat(expected).isEqualTo(6); | ||
| } |
There was a problem hiding this comment.
숫자를 통해서 Hand를 손쉽게 생성할 수 있는 HandFixture의 도입은 정말 좋은 것 같아요.
하지만, 테스트 하는 객체를 알기 위해서는 HandFixture의 로직까지 알아야 해서 인지 비용이 드는 것 같네요.
Hand를 구현하는 것이 HandTest에 있어야 좀 더 가독성이 좋은 코드가 되지 않을까요? 리비의 생각이 궁금해요!
c.f. 어떤 어려움이 있어서 HandFixture를 도입했는지 공유해주었으면 좋겠어요! 저도 좋은 인사이트를 얻어갈 수 있을 것 같네요~
There was a problem hiding this comment.
- 한 테스트 클래스에서 자꾸 재사용되는 테스트 데이터가 있음
- 테스트 클래스 내 메서드로 추출하자
- 쓰다 보니 다른 클래스에서도 자꾸 재사용되네
- 클래스로 크리에이터를 추출하자
위와 같은 흐름이었습니다.
다만 fixture개념은 이번에 처음 학습했고 fixture의도에 맞지 않게 사용했다는 것이 결론이에요
위에서 커찬이 말씀하신 것처럼 반복해서 사용되는 데이터를 고정시켜 fixture로 관리하고 있었다면 의도가 맞겠지만 현재는 Creator에 가깝지 않나 싶네요 😅
HandFixture는 미리 생성된 데이터를 관리하고 있어서 (생성로직을 담고 있는 것이 아니라) 인지 비용이 없어야 하는 것이 맞는 것 같습니다
| class JudgeTest { | ||
|
|
||
| private Judge judge; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| judge = new Judge(); | ||
| } | ||
|
|
||
| @DisplayName("딜러와 플레이어 중 누가 이겼는지 알 수 있다") | ||
| @Test | ||
| void testSelectWinner() { | ||
| Score dealerScore = new Score(10); | ||
| Score playerScore = new Score(5); | ||
| assertThat(judge.isPlayerWin(dealerScore, playerScore)).isFalse(); | ||
| } | ||
| } |
There was a problem hiding this comment.
테스트가 하나 밖에 존재하지 않는다면, @BeforeEach를 굳이 도입하지 않아도 되지 않나요?
혹시 @BeforeEach를 도입하는 기준이 따로 있으실까요?
There was a problem hiding this comment.
필드가 있으면 써왔는데 그 의도가 반복을 줄이기 위한 것이라면 커찬의 말씀이 맞는 것 같습니다 🤔
불필요하게 확장을 항상 먼저 생각하는 것이 제 고약한 버릇인데 제가 알고 있던 지점 외에서 이런 버릇이 또 등장하는 것을 찾았네요 😀
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (o == null || getClass() != o.getClass()) { | ||
| return false; | ||
| } | ||
| Player player = (Player) o; | ||
| return Objects.equals(name, player.name); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(name); | ||
| } | ||
|
|
||
| public String getName() { | ||
| return name.getValue(); | ||
| } | ||
|
|
||
| public Hand getHand() { | ||
| return hand; | ||
| } |
There was a problem hiding this comment.
저는 equals() 랑 getter 중에서 getter를 더 위에 두는 편이에요.
equals는 Object 클래스의 메서드를 상속하는 것이어서 더 범용적이고, getter는 class의 특성들을 반환한다고 생각해서 더 한정적이라고 생각해서요.
리비는 equals()를 더 위에 두는 이유가 있나요?
There was a problem hiding this comment.
Getter가 가장 무게가 낮다고 생각했어요 🤔
equals는 커스텀 될 수 있는 여지가 있고 getter는 그마저의 느낌도 없다는 개인적인 생각입니다 👍
안녕하세요 6기 크루분들 리비입니다 🙌
블랙잭 1단계 PR 리뷰 요청 드립니다 🙇🏻♂️
해당 미션은 페어인 제리와 함께 진행했습니다 👍
이번 블랙잭 미션에서 저희는 역할과 책임 분리를 통한 객체지향적인 설계와 엄밀한 TDD 수행을 목표로 두어봤어요 💪🏻
하지만 미션 난이도가 확 올라서 그런지 객체지향적인 설계, TDD모두 쉽지 않았습니다. 🥲
특히 설계는 정답인지 확신을 내릴 수 없는 상태에서 페어와 고민만 하는 경우가 매우 많았네요 😭
마지막 즈음에 설계를 개선할 때에는 거꾸로 돌아가고 있다는 느낌도 들었던 적 있습니다 🤔
길을 잃은 것 같아요 ... 도와주시면 감사하겠습니다 🙇🏻
따끔한 리뷰도 괜찮습니다!
리뷰 주시면 열심히 학습해보겠습니다 💪🏻