[1단계 - 블랙잭 게임 실행] 몰리(김지민) 미션 제출합니다. #613
Conversation
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
…pe, Score 객체 생성 Co-authored-by: Minjoo522 <[email protected]>
Co-authored-by: Minjoo522 <[email protected]>
- 테스트 작성 - 기존에 작성된 테스트 중 예외 상황에 해당되어버리는 테스트 수정
| public Deque<Card> getDeck() { | ||
| return deck; | ||
| } |
There was a problem hiding this comment.
파랑! 이 부분과 관련해서 궁금한 점이 있어요!
그동안 TDD를 하면서, 한번씩 테스트에서만 사용되는 메서드들이 있었는데요 🥲
제가 생각하기에 정말 반드시 테스트가 필요한 부분이라고 생각되면 프로덕션 코드에서 사용되지 않더라도 남겨두고, 크게 중요하지 않는 부분이라고 생각되면 테스트를 제거해 버렸었습니다.
예시로, 위의 getDeck()를 남겨둔 이유는 getDeck()이 2가지를 테스트 하고 있는데,
- 배분 시에 가장 위에 카드부터 배분하는지
- 52개의 카드로 이루어져있는지
1번 같은 경우는 꼭 위의 카드가 배분되지 않더라도 프로그램 동작 자체에는 문제가 없기 때문에 반드시 테스트가 필요하지는 않지만,
2번은 하나의 덱은 사이즈가 52로 고정적이고 (요구사항이 변경된 경우 다른 카드집을 사용하게 되어 꼭 52는 아닐 수 있지만 고정적일 것이라고 생각했어요) 추가로 사이즈가 52개라는 것은 모든 shape과 score에 대해 1:1로 잘 매칭되었다 까지 예상할 수 있기 때문에 테스트가 반드시 필요한 부분이라고 생각했어요.
그래서 조금의 찝찝함을 감수하더라도 getDeck()을 남겨두게 되었는데요..!
말이 조금 길어졌지만 😂
결론적으로, 파랑에게 반드시 테스트가 필요한 부분에서 생겨진, 프로덕트에서는 사용하지 않는 메서드는 어떻게 해야하는지에 대해 물어보고 싶어요!
저는 3가지 방법을 생각해볼 수 있을 것 같아요...!
- 메서드를 제거
- 테스트를 제거
- 다른 프로덕션에 있는 메서드로 대체하여 테스트 (대체가 가능하다면)
| void createCard() { | ||
| assertDoesNotThrow(() -> new Card(Shape.DIA, Score.NINE)); | ||
| } |
There was a problem hiding this comment.
Card 객체를 정상적으로 생성할 수 있는지에 대한 테스트였습니다!
혹시 너무 과한, 또는 의미가 부족한 테스트라고 생각되시는지 궁금합니다..!!
| import blackjack.model.deck.Card; | ||
| import java.util.List; | ||
|
|
||
| public class Dealer extends Player { |
There was a problem hiding this comment.
맞아요...!
처음 Dealer가 Player를 상속받도록 구현한 이유 중 하나가 Dealer 또한 특수한 형태의 Player라고 생각했기 때문이에요!
또한 Players는 Dealer가 제외된 Player에 대한 일급 컬렉션이에요...!
Players라는 이름만 보기에는 오해가 발생할 수 있는 것 같아요🥲
수정하도록 하겠습니다!
| import java.util.Scanner; | ||
| import java.util.function.Supplier; | ||
|
|
||
| public class ConsoleReader implements Supplier<String> { |
There was a problem hiding this comment.
이 부분은 큰 고민을 해보지 않고 페어의 의견을 받아들인 것 같아요 🥲
지금 제가 생각하는 장점으로는, 콘솔 입력을 처리하는 로직이 분리되기 때문에 혹시나 입력값에 대해 테스트를 해야하는 상황이 올 경우 테스트 하기에 용이할 것 같다는 것 같네요..!
| private OutputView() { | ||
| throw new AssertionError("인스턴스를 생성할 수 없습니다."); | ||
| } |
There was a problem hiding this comment.
'자주 변경되는 view의 로직에 domain이 영향을 받아서는 안 된다, 두 계층을 분리해야 한다' 저도 동의하는 바인데요, 이렇게 작은 프로젝트에서 dto를 사용했을 때 불편한 점은 없었는지 궁금합니다.
사실 저는 NameCardsScore까지는 모델을 감추고 layer간의 분리를 위해 기꺼이 할 수 있는 수준이라고 생각했지만 display enum을 사용하는데에는 회의적인 입장이였어요.
파랑 말처럼 프로젝트 규모에 비해 생성 비용이 너무 크다고 생각했기 때문에요.
"이 정도 프로젝트에서는 출력값 변경 시 그냥 domain에 있는 enum의 값을 바꾸는 게 더 간단할 것 같은데?" 라는 주장으로 페어와 이야기해봤을 때, 페어의 입장은 "우리는 배우는 입장이기 때문에 이런 부분들도 분리를 해보는 연습을 해보아야 할 것 같다" 였어요.
이 말에 납득이 됐기 때문에 처음 비용이 크더라도 시도를 해봤던 것 같아요.
또 기껏 만들어둔 값객체들을 view에서는 전혀 활용하지 못하고 있는데요 요 부분에 대해서도 어떻게 생각하는지 궁금해요~!
이 부분에서 파랑이 말씀해주신 값객체가 어떤 어떤 것들인지 여쭤봐도 될까요??
이번 미션에서 view에서 도메인을 사용하는 것과 dto를 사용하는 것에 어떤 장단점이 있는지 비교해 보면 좋을 것 같습니다.
정리하자면, 제가 생각하는 장점은 dto를 사용함으로써 모델을 감출 수 있으며 layer간의 분리가 가능하기 때문에 view에대한 요구사항이 변경되더라도 모델이 변경되는 일이 없다.
(아직 요구사항이 변경되지 않아서 장점을 몸소 느끼지는 못했습니다🥲)
단점은 dto 생성, 변환 코드 등 추가적으로 발생하는 코드량이 증가한다. 입니다!
| } | ||
|
|
||
| @Override | ||
| public boolean equals(final Object o) { |
There was a problem hiding this comment.
맞아요! 제가 의도한 것은 파랑이 말한 것처럼 이름으로 player를 구분하고자 했어요.
이 부분과 관련해서 파랑에게 궁금한 점이 있어요!
파랑은 주로 equals과 hashCode()를 언제 재정의를 하는지, 어떤 상황에서 재정의를 해야한다고 생각하는지 궁금해요.
| import java.util.Objects; | ||
|
|
||
| public class Player { | ||
| private static final int HITTABLE_THRESHOLD = 21; |
There was a problem hiding this comment.
제가 생각하기에는 같은 21이지만 전달하고자 의미가 조금 다르다고 생각하여 분리하였습니다!
Hand의 21은 블랙잭 게임에서 블랙잭 스코어에 대한 의미를 담고 있지만
Player가 가지고 있는 21은 bust가 아닌 최대 스코어에 대한 의미라고 생각했었어요.
현재는 요구사항이 "21을 초과하지 않으면서 21에 가깝게 만들면 이긴다." 이지만, "21 초과에 상관없이 21에 가깝게 만들면 이긴다."로 변경된다면 Hand의 21의 유지, Player의 21은 제거되어야 한다고 생각했었기 때문이에요.
하지만 현재는 Hand 안에서 블랙잭 스코어에 대한 의미, bust인지에 대한 의미 두 가지가 혼합되어 사용되고 있어 문제인 것 같긴 하네요...! 좋은 피드백 감사합니다 파랑👍
| public List<Card> openCard() { | ||
| return List.of(hand.getFirstCard()); | ||
| } |
There was a problem hiding this comment.
파랑의 의견이 궁금한 부분이 있어요 🥹
이 부분을 구현하면서 페어와 의견이 조금 갈렸었는데요.
저는 Hand로부터 카드를 다 받아 숨길 부분을 제거하고 나머지를 공개하도록 한다. 공개할지 말지 조차 결정하는 것은 Dealer의 행동이다. 라고 생각했어요.
페어는 반대로 어차피 요구사항에 의해 한장만 공개를 할 것인데 애초에 한 장만 받아오는 것이 맞는 것 같다는 의견이였어요.
이 상황에 대해 파랑은 어떻게 생각하시는지 궁금합니다...!
There was a problem hiding this comment.
어쨌거나 오픈했을 때 한 장만 보이게 한다는 요구사항만 충족한다면 어떤 방식으로 구현할지는 개발자의 몫이라고 생각합니다 😅 페어의 의견을 반드시 따를 필요는 없습니다. 본인이 좋다고 생각하는 방향으로 구현해보시죠~~
| import java.util.Scanner; | ||
| import java.util.function.Supplier; | ||
|
|
||
| public class ConsoleReader implements Supplier<String> { |
There was a problem hiding this comment.
현재는 inputView 테스트에 사용되고 있어 두셔도 좋을 것 같아요. 하지만, 앞으로는 스스로 필요성을 느꼈을 때 기능을 추가해보는 것을 권장드립니다 👍
| private OutputView() { | ||
| throw new AssertionError("인스턴스를 생성할 수 없습니다."); | ||
| } |
There was a problem hiding this comment.
장단점을 잘 적어주셨네요. 적어보는 장점이 더 크게 느껴지시나요? 혹은 그 반대인가요? 마찬가지로 고민해봤을 때 필요성을 느끼지 못하는 코드라면 과감히 삭제해주세요~
이 부분에서 파랑이 말씀해주신 값객체가 어떤 어떤 것들인지 여쭤봐도 될까요??
Name이나 Shore 등 이미 불변으로 잘 사용하고 있는 객체가 있는데, dto를 사용하면서 이 값들을 미리 꺼내서 다시 원시값으로 사용해야 하는 게 아쉽더라구요!
| } | ||
|
|
||
| private void playPlayerTurn(final Player player, final Deck deck) { | ||
| if (!player.isBust() && player.canHit()) { |
There was a problem hiding this comment.
넵넵 정확히 21인 경우도 canHit에서 걸러주고 있기 때문에 player.canHit()만 있어도 정상 동작하겠네요.
| public Deque<Card> getDeck() { | ||
| return deck; | ||
| } |
There was a problem hiding this comment.
중요한 로직에 대한 테스트이고 대체가 어렵다면 getter 정도는 남겨도 괜찮다고 생각하긴 합니다. 테스트만을 위한 코드를 최소화하되, 항상 맥락을 고려해야 하니까요.
| import blackjack.model.deck.Card; | ||
| import java.util.List; | ||
|
|
||
| public class Dealer extends Player { |
There was a problem hiding this comment.
단순히 이름의 문제가 아닙니다. Dealer도 Player의 한 종류라면 Player가 쓰이는 모든 곳에는 Dealer도 똑같이 들어갈 수 있어야 합니다. 지금 그렇게 동작하고 있나요? 두 개가 별개의 클래스로 사용되어야 하며 Player가 쓰이는 모든 곳에 Dealer도 함께 쓰일 수 없다면 둘은 상속 관계가 되어서는 안 됩니다.
| Card expected = deck.getDeck().getFirst(); | ||
| assertThat(deck.distribute()).isEqualTo(expected); |
There was a problem hiding this comment.
실제 distribute 메서드가 의도하는 것은 맨 앞에 있는 카드를 가져오는 것이 아닌 것 같습니다.
맨 앞의 카드를 가져오는 메서드라면 getFirst 등의 이름이 되어야 하지 않을까요? 지금 게임 룰에서는 불필요한 제한으로 보입니다!
| public static Deck createByRandomOrder() { | ||
| return new Deck(makeRandomOrderDeck()); | ||
| } |
|
|
||
| import java.util.Objects; | ||
|
|
||
| public class Name { |
| public List<NameCardsScore> collectFinalResults() { | ||
| return players.stream() | ||
| .map(player -> new NameCardsScore(player.getName(), player.openCards(), player.getScore())) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
domain에서 dto를 만들도록 구현하셨네요. 어떤 문제가 발생할 수 있을까요?
There was a problem hiding this comment.
dto가 변경되었을 때 도메인의 코드도 변경돼 버린다는 문제가 발생할 것 같네요🥲
수정하도록 하겠습니다...!
| @Test | ||
| @DisplayName("딜러는 16이하이면 카드를 추가로 받는다.") | ||
| void canReceive() { | ||
| assertThat(dealer.canHit()).isTrue(); |
| private OutputView() { | ||
| throw new AssertionError("인스턴스를 생성할 수 없습니다."); | ||
| } |
There was a problem hiding this comment.
장단점을 잘 적어주셨네요. 적어보는 장점이 더 크게 느껴지시나요? 혹은 그 반대인가요? 마찬가지로 고민해봤을 때 필요성을 느끼지 못하는 코드라면 과감히 삭제해주세요~
상황에 따라 다른 선택을 할 것 같아요 ㅎㅎ
지금 시점에서는... 생성 당시의 비용(단) < 현재 다시 제거하는 비용(단) + layer간의 분리, Player 내용 감추기(장) 이므로 냅두도록 하겠습니다..ㅎㅎ
Name이나 Shore 등 이미 불변으로 잘 사용하고 있는 객체가 있는데, dto를 사용하면서 이 값들을 미리 꺼내서 다시 원시값으로 사용해야 하는 게 아쉽더라구요!
조금 다른 얘기이지만, 저는 처음에는 VO도 로직을 가질 수 있기 때문에 domain이다 -> domain이니 view에서 모르게 dto를 만들어줘야 한다 라고 생각해서 원시값을 제거했었는데요.
리뷰를 받고 다시 생각해보니, 애초에 domain을 view에서 모르게 하려고 했던 이유가 domain의 내용을 감추고 싶어서였는데 VO면 값이 변경될 일도 없고, 내용도 사실 공개된 객체이지 않은가 하는 생각에 딱히 그렇지 않아도 될 것 같다는 생각을 했어요.
이런 접근방식이 괜찮은지 궁금합니다 파랑!
| import java.util.Objects; | ||
|
|
||
| public class Name { | ||
| private final String rawName; |
| import blackjack.model.deck.Card; | ||
| import java.util.List; | ||
|
|
||
| public interface Playable { |
There was a problem hiding this comment.
인터페이스를 선택하셨군요!
어떤 선택지들을 고려했고 왜 최종적으로 인터페이스를 선택하게 되었는지 얘기 나눠보면 좋을 것 같아요ㅎㅎ
1단계는 이제 머지되니 2단계 PR에서 몰리의 생각을 공유해주시면 좋을 것 같습니다 👍
안녕하세요 파랑 :)
리뷰 잘 부탁드립니다!