[1단계 - 블랙잭 게임 실행] 다즐(최우창) 미션 제출합니다. #443

woo-chang · 2023-03-03T08:31:49Z

반갑습니다 토니 🥃 많이 배워가도록 하겠습니다 :)

TDD를 바탕으로 끝까지 진행하려고 했으나, 끝까지 진행하기에 시간이 조금 부족하여 마지막에는 제대로 진행하지 못했던 것 같습니다. 지금까지 미션 중 가장 어려웠던 것 같습니다 😂 그래도 페어와 함께 끝까지 마무리할 수 있어서 너무 재밌었습니다!

진행하면서 생긴 몇 가지 질문사항을 아래에 정리해 보았습니다!

정잭 팩토리 메서드 vs 팩토리 패턴

도메인의 필드를 생성자로 받는 것이 아닌 필드를 만드는 값을 받아 도메인을 만들기 위해 정적 팩토리 메서드를 사용하였습니다. 그 이유는 도메인의 생성 기능이기에 해당 도메인이 알아야 하고 그 책임 또한 도메인에 있다고 생각하였기 때문입니다.

한편으로는 정적 팩토리 메서드를 사용하게 되었을 때 해당 도메인이 생성에 대해 가지는 책임이 너무 커지게 되는 느낌도 들었습니다. 따라서 해당 책임을 분리하고자 도메인 생성 책임을 지는 팩토리 객체를 만드는 방법도 고민해 보았습니다. 팩토리 객체를 사용하게 되면 생성에 대한 책임은 줄어들지만, 도메인 생성 로직이 변경되었을 때 도메인과 팩토리 객체 모두 수정이 필요하여 유지보수에 어려움이 발생할 것으로 생각합니다.

이러한 상황일 때 어떤 기준으로 선택하면 좋을지 궁금합니다!

Draw

현재 딜러와 플레이어는 뽑을 수 있는 상태인지 확인 후에 뽑을 수 있는 동작을 진행하고 있습니다. 이는 뽑을 수 있는 상태인지 확인하는 역할과 뽑는 역할을 분리하기 위해 메서드를 분리하였고 그로 인해 다음과 같은 문제가 발생할 수 있습니다.

뽑기 가능한 상태일 때만 뽑아야 하는데 카드를 뽑는 메서드는 이를 검증하지 않기 때문에 메서드를 호출한다면 카드를 가지게 됩니다. 이는 예상하지 못한 결과를 유발할 수 있기에 조금 위험한 코드라는 생각이 들었습니다.

하지만 뽑는 것에서 뽑을 수 없는 것을 검증하여 예외를 던지기에는 메서드 의미와 다르게 너무 많은 역할을 가지고 있다는 생각이 들었습니다. 토니는 이와 같은 충돌이 발생할 때 어떤 선택을 하는지 궁금합니다!

딜러 이름 중복 관리

딜러는 딜러라는 이름을 가지게 되기에 플레이어는 딜러라는 이름을 가질 수 없습니다. 이를 검증하기 위해서는 Name 객체도 딜러라는 이름을 알아야 하고, 딜러 또한 딜러라는 이름을 알아야 하는 상황이 발생하였습니다.

하지만 2개의 필드로 관리하기에는 딜러의 이름을 바꾸게 되었을 때 유지보수에 있어 불편함이 존재한다고 생각합니다. 하지만 1개로 마땅히 관리할 방법이 떠오르지 않아 2개의 필드로 관리하는 방법을 선택하였습니다. 이런 경우 어떻게 해결할 수 있을까요?

Score에서 Result 반환

Score의 compare 메서드에서는 점수를 비교한 뒤, 그에 맞는 Result를 반환하도록 하였습니다. 맥락상으로는 자연스러워 보이지만, 코드상으로는 자신과 다른 인스턴스를 반환하는 게 어색하게 느껴졌습니다. 토니는 어떻게 생각하는지 궁금합니다!

그 외에도 개선이 필요한 부분이나 부족한 부분에 대해 리뷰해 주시면 감사하겠습니다. 리뷰 내용을 바탕으로 더 좋은 코드를 만들기 위해 고민해 나가고자 합니다. 다시 한번 감사합니다 ⛄️

Co-authored-by: woo-chang <clllickme@naver.com>

toneyparky

안녕하세요 다즐~ 조금 늦은 시간에 리뷰를 완료했네요! 최대한 빠르게 코멘트 드리고 싶어 자정이 넘어 알람이 가더라도 이해 부탁드려요 😂

커밋 내용에서 TDD를 최대한 잘 지키며 구현해주신 부분이 인상적이었어요. 특히 테스트 코드의 내부에 프로덕션 클래스를 만들어서 빠르게 테스트하고 프로덕션 클래스를 따로 분리하는 과정이요 👍 좋은 습관이라 생각합니다!

질문 주신 네가지 사항에 대해 답변과 제 질문을 달았으니 확인 후 답변 부탁드려요~!
굳나잇~~!

toneyparky · 2023-03-04T15:24:11Z

README.md

+
+## 도메인 다이어그램
+
+```mermaid


mermaid 를 활용하시어 객체간의 의존도를 표현해주신 부분 좋네요~! 현업에서도 문서화는 중요한 요소기에 이렇게 미리미리 연습해두시는 거 좋습니다 👍

다만, 문서는 꾸준히 관리하지 않으면 죽은 것과 다름 없기에 리팩터링이 진행될 때에 꼭 관심을 가지고 관리해주죠~

리팩터링 이후 놓칠 수 있는 문서 관리를 해주셨네요 💯

toneyparky · 2023-03-04T15:25:05Z

src/main/java/blackjack/controller/BlackJackController.java

+
+public class BlackJackController {
+
+    private static final int INIT_DRAW_COUNT = 2;


Suggested change

private static final int INIT_DRAW_COUNT = 2;

private static final int INITIAL_DRAW_COUNT = 2;

축약어가 아닌 단어를 그대로 사용하면 어떤 점이 좋을까요?

의미를 명확하게 전달할 수 있다는 장점이 있을 것 같습니다 🔥

만약 담아야하는 의미가 너무 많아서 축약하지 않았을 때, 오히려 가독성을 떨어뜨린다면 토니는 어떤 선택을 하시나요?

개발 컨벤션에 대한 부분이라고 생각해요. 그렇기에 팀마다 선택이 다를 수 있습니다.

팀의 구성 인원의 숙련도, 도메인 용어가 정립된 수준 등에 맞춰서 축약어를 사용할 수도 있다고 생각해요. 하지만 저는 처음 코드를 접하는 사람이 이를 오해하지 않도록 하는 게 더 중요하다고 생각하기에 단어를 그대로 사용하길 제안할듯해요.

물론 가독성이 너무 떨어지는 경우에는 코드리뷰를 통해 합의해나갈거 같습니다 :)

toneyparky · 2023-03-04T15:25:30Z

src/main/java/blackjack/controller/BlackJackController.java

+public class BlackJackController {
+
+    private static final int INIT_DRAW_COUNT = 2;
+    private static final int DEALER_LIMIT = 16;


DEALER_LIMIT은 어떤 의미에요? 딜러의 무엇을 제한하는걸까요?

딜러는 처음 받은 카드 2장의 합이 16이하일 때만 추가적으로 카드를 받을 수 있습니다.

이를 제약하기 위한 값이었는데 Controller가 이를 알기보다는 Dealer가 알고 있어야할 것 같습니다!

적절한 객체로 이동시킴과 더불어 2장의 합이 16이하이라는 숫자의 개념이 들어가도록 상수 이름도 수정하면 좋겠네요 😎

toneyparky · 2023-03-04T15:26:55Z

src/test/java/blackjack/domain/card/CardsTest.java

+@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
+class CardsTest {
+
+    private static Stream<Arguments> generateCardsWithoutACE() {


다즐은 프로젝트 내에서 에이스를 Ace로 사용하시나요 ACE로 사용하시나요? 같은 의미의 용어를 다른 문자 표현 컨벤션으로 사용할 경우 코드를 읽는/작성하는 사람은 어떻게 느낄거 같으세요~?

프로젝트 내에서 일관성은 중요한 요소라고 생각합니다 😂 ACE보다는 Ace가 직관적으로 다가오기에 통일이 필요해 보입니다!

굳굳 나중에 팀 프로젝트를 할 때에는 여러 사람이 개발하다보니 요런 부분에 대해서 명확하게 설정하지 않는다면 프로젝트가 중구난방이 되기 쉽상이랍니다

toneyparky · 2023-03-04T15:28:15Z

src/test/java/blackjack/domain/card/DeckFactoryTest.java

+        assertThatThrownBy(deck::draw)
+                .isInstanceOf(IllegalStateException.class);


코드를 읽는 사람은 부분이 assert를 하기에 테스트에서 검증하고 싶은 내용을 나타낸다고 인지할거에요. 하지만 테스트의 이름은 덱을 생성한다라서 혼란스러울 수 있겠네요. 다즐은 이 테스트에서 어떤 내용을 검증하고 싶으셨을까요~?

덱을 생성한 것에 대한 검증이 필요하였는데, 검증을 위해 getter를 여는 것이 불필요하다고 생각하였습니다!

그렇기에 1장짜리 덱을 생성 후 카드 2장을 뽑았을 때 예외가 발생하도록 하여 1장짜리 덱이 생성되었는지 확인하려고 했습니다. 이런 경우 getter를 열어서 테스트를 진행하는 것이 맞는건지 궁금합니다 🤔

1장짜리 덱을 생성 후 카드 2장을 뽑았을 때 예외가 발생하도록 하여 1장짜리 덱이 생성되었는지 확인

이 부분이 테스트의 설명에 드러나지 않아서 이해하기 어려웠어요. 설명해주셔서 감사합니다!
게터를 열어 테스트하는건 좋지 않다고 생각해요. 테스트의 범위를 확장하여 설명에 컨텍스트를 추가하는 방법도 있겠어요.

혹은 assertThatNoException()와 같은 검증문을 사용하여 DeckFactory.createWithCount이 예외 없이 동작하는지 정도로 검증을 마칠 수 있구요.

그런데 두가지 모두 애매하다고 느껴진다면 그 테스트는 필요 없는 테스트일 수도 있다는 생각까지 확장할 수 있습니다.

테스트를 작성해야 한다면 assertThatNoException()를 사용하여 생성에 대해서만 작성해줄 것 같네요.

toneyparky · 2023-03-04T15:47:54Z

src/main/java/blackjack/domain/result/Score.java

+    public Result compare(final Score score) {
+        final int result = this.compareTo(score);
+        if (value > 21) {
+            if (score.getValue() > 21) return DRAW;
+            return LOSE;
+        }
+
+        if (result > 0) {
+            return WIN;
+        }
+        if (result < 0) {
+            return LOSE;
+        }
+        return DRAW;
+    }


Score에서 Result 반환 질문에 대해 이야기해봐요!

우선 질문에 대한 답변에 앞서 한가지 질문을 드릴게요. Score 객체의 책임은 뭐라고 생각하시나요? 또한 Result 객체의 책임은 뭘까요? 다즐의 생각을 적어주세요! 적어주신 내용을 바탕으로 바로 추가 답변드릴테니 같이 논의해보죠 😊

처음에는 점수를 알고 이를 관리하는 것이 Score의 책임이라고 생각했습니다. 점수를 관리하고 있기에 점수를 받아 비교해서 결과를 반환하는 것도 Score의 당연한 책임이라고 생각했습니다. 때문에 Result는 값 전달을 위한 값 객체가 되어버렸습니다.

블랙잭 도메인에 대한 이해가 조금 높아진 지금으로는 결과를 생성하는 것은 Dealer 책임인 것 같습니다. 따라서 Player 점수를 딜러에게 전달해서 Result를 반환하도록 리팩토링해보려고 합니다.

토니의 생각은 어떤가요? 🤔

오호~ 도메인을 학습하여 현실의 블랙잭처럼 딜러가 점수를 카운팅한다는면을 반영하시려했군요! 괜찮은 방법이네요~!

하지만 우리가 꼭 현실의 블랙잭을 따를 필요는 없다고 생각해요. 현실의 블랙잭에서 딜러만이 점수 계산이라는 행위를 하는건 또 아니니까요. 저라면 점수는 그냥 int로 관리하고 이 값을 Result 이넘이 받아서 비교하여 결과를 내주는 로직을 구현할 것도 같네요~

제 생각을 여쭤보셨기에 남겨봤습니다 😀 정답은 없기에 다즐이 구현하고 싶은 대로 리팩터링해보셔도 좋지 않을까 싶어요~
전해드리고 싶은 말은 현실을 100% 따르지 않더라도 적절한 복잡도로 프로젝트의 요구사항을 해결할 수 있도록 구현하자입니다.

현실 세계를 온전히 구현하더라도 복잡도가 증가해버리면 좋은 소프트웨어라고 할 수 없겠네요 .. ! 너무 멋있는 말인 것 같습니다 👍

Result에서 int값을 받아 결과를 내주는 것도 생각해 보았습니다!

Result of(final int dealerScore, final int playerScore) { //결과 계산 로직 }

위와 같은 코드로 작성할 것이라 생각이 되는데, dealerScore와 playerScore 중 어느 것을 기준으로 결과를 생성할지에 대해 애매하다는 생각을 하게 되었습니다. 이 부분에 대해서는 어떻게 생각하시나요?

Result 객체 입장에서는 dealer, player를 구분할 필요가 있을까 싶어요. 그냥 점수, 비교 점수 이렇게 둘을 받아서 결과를 계산해줄 수 있지 않을까요? 대신 요러면 사용하는 입장에서 주의를 기울여야겠죠. 이런 측면에서는 다즐이 제안해주신 Dealer 방식도 명확하게 이해하기에 좋다고 생각되네요!!

toneyparky · 2023-03-04T15:48:38Z

src/main/java/blackjack/domain/card/DeckFactory.java

+
+public class DeckFactory {
+
+    public static Deck createWithCount(final Stack<Card> pack, int count) {


count를 왜 받고 있는거에요~? 요구사항에서 어떤 유즈케이스를 대응하기 위한 구현인가요?

요구사항에서는 존재하지 않지만, 확장성을 고려하여 트럼프 팩을 여러개 섞어 사용할 때 사용할 수 있게 만들기 위해 메서드를 다음과 같이 작성해 보았습니다! 오버엔지니어링에 해당하는걸까요 😂

제가 카드 게임을 잘 몰라 요런 확장성이 있을거라고 생각해보지 못했네요 ㅋㅋㅋ

오버엔지니어링인지 여부를 판단하기는 약간 어렵네요. 음 그렇다면 코드를 읽는 사람에게 의도를 전달할 수 있도록 메서드 이름이나 인자의 이름으로 최대한 표현해줄 수 있겠어요. 이게 어려우면 지금의 도메인에서는 이런 코드가 필요 없다는 의미기도할테니 제거하는거구요.

toneyparky · 2023-03-04T16:03:44Z

src/main/java/blackjack/domain/card/DeckFactory.java

+
+import java.util.Stack;
+
+public class DeckFactory {


정적 팩토리 메서드 vs 팩토리 패턴 질문에 대해 이야기해봐요~

도메인의 필드를 생성자로 받는 것이 아닌 필드를 만드는 값을 받아 도메인을 만들기 위해 정적 팩토리 메서드를 사용하였습니다. 그 이유는 도메인의 생성 기능이기에 해당 도메인이 알아야 하고 그 책임 또한 도메인에 있다고 생각하였기 때문입니다.

=> 이 부분은 이해하기 조금 어려웠어요. 생성자도 메서드이기에 도메인 객체에 포함되어 있죠. 생성자로 만드나 정적 팩토리 메서드로 만드나 도메인이 알고 있고 책임도 도메인에 있지 않을까요? 제 이야기에 공감이 되었다면 정적 팩터리 메서드는 어떤 장점이 있길래 사용하는걸까요? 다즐의 생각을 적어주세요~

한편으로는 정적 팩토리 메서드를 사용하게 되었을 때 해당 도메인이 생성에 대해 가지는 책임이 너무 커지게 되는 느낌도 들었습니다. 따라서 해당 책임을 분리하고자 도메인 생성 책임을 지는 팩토리 객체를 만드는 방법도 고민해 보았습니다. 팩토리 객체를 사용하게 되면 생성에 대한 책임은 줄어들지만, 도메인 생성 로직이 변경되었을 때 도메인과 팩토리 객체 모두 수정이 필요하여 유지보수에 어려움이 발생할 것으로 생각합니다.

=> 위의 질문에 대한 답변을 차치하고 정적 팩터리 메서드가 생성에 대한 책임을 많이 지게 된다는 사실은 맞아요. 생성 로직이 복잡해질 수록 책임도 커지죠. 다즐이 적어준 팩토리 객체의 장점과 단점에 대해서도 동의합니다~! 💯

여기서 핵심은 어떤 경우에 도메인의 생성에 대한 책임이 커질까/복잡해질까?라고 생각해요. 이 부분이 다즐의 궁금증인 정적 팩토리 메서드와 팩토리 패턴을 사용하는 기준을 결정지을 것 같구요. 기준에 대해서 여쭤보셨으니 제가 생각하는 기준을 말씀드릴게요. 저는 기본적으로 생성에 대한 로직 자체는 해당 객체가 가지고 있는게 응집도가 높다고 생각하여 정적 팩토리 메서드를 사용할거에요. 그리고 팩터리 패턴은 팩터리 메서드(혹은 생성자)를 사용하여 객체를 생성하는 쪽의 로직이 복잡해질 때에 사용할 거에요. 이에 대한 예시는 참고 글을 확인해주세요.

정리하자면 둘은 같은 비교 대상이 아니라고 생각해요. 정적 팩토리 메서드는 객체 하나를 생성할 때에 객체 내부에서 사용하는 방법이구요. 팩터리 패턴은 객체를 생성하는 쪽에서 객체를 생성하는 메서드를 사용하는 경우에 발생하는 복잡도를 제어하는 방법이라 생각해요.

이해가 안되거나 이야기 더 나누고 싶은 부분은 편하게 답변주시거나 DM 부탁해요 🙂

이 부분은 이해하기 조금 어려웠어요. 생성자도 메서드이기에 도메인 객체에 포함되어 있죠. 생성자로 만드나 정적 팩토리 메서드로 만드나 도메인이 알고 있고 책임도 도메인에 있지 않을까요? 제 이야기에 공감이 되었다면 정적 팩터리 메서드는 어떤 장점이 있길래 사용하는걸까요? 다즐의 생각을 적어주세요~

생성자는 특수한 메서드이기에 너무 많은 책임을 가지면 안된다고 생각합니다. 필드로 가지는 값을 생성자로 받아 검증 후 필드를 생성하는 것만이 생정가 가지는 특수성이라고 이해했습니다. 예를 들면 Players는 List를 필드로 가질 때 List을 받아 List를 만들어서 필드에 주입하는 방법은 생성자에서 진행하기는 너무 많은 책임이라는 생각입니다. 따라서 해당 경우에는 정적 팩터리 메서드로 책임을 넘겨줄 수 있지 않을까라는 의견이었습니다.

public class Players { private List<Player> players; public Players(final List<Player> players) { validate(players); this.players = players; } public static Players from(final List<String> names) { final List<Player> players = generatePlayers(names); return new Players(players); } }

위와 같은 경우에도 생성자에서 수행해도 괜찮을지 궁금합니다!

정리하자면 둘은 같은 비교 대상이 아니라고 생각해요. 정적 팩토리 메서드는 객체 하나를 생성할 때에 객체 내부에서 사용하는 방법이구요. 팩터리 패턴은 객체를 생성하는 쪽에서 객체를 생성하는 메서드를 사용하는 경우에 발생하는 복잡도를 제어하는 방법이라 생각해요.

명확하게 차이를 이해하지 못하고 있었는데 토니의 말을 듣고 나니 명확하게 다가오는 것 같습니다!

public class Ladder { private List<Line> lines; public Ladder(final List<Line> lines) { validate(lines); this.lines = lines; } public static Ladder of(final DirectionGenrator directionGenerator, final Height height, final int playerNumber) { //List<Line> 생성 return new Ladder(lines); } }

이번 예는 이전 사다리 미션을 참고했습니다. 사다리를 생성하기 위해서는 각 지점에서 움직일 방향을 생성하는 생성기, 사다리 높이, 플레이어 수 등 다양한 정보를 바탕으로 라인을 생성해야 합니다. Players 예시와 다르게 생성 로직이 복잡하기에 이를 담당하는 객체를 만들어 책임을 넘기고자 팩터리 객체를 고려할 수 있을 것 같습니다. 이런 경우에도 토니는 정적 팩터리 메서드를 선호하나요? 🤔

Players 코드 예시에 대하여

음 List<Player>를 인자로 받는 기본 생성자 이외에 List<String>를 인자로 받는 생성자를 추가하는 것과 예시 코드의 차이는 없다고 생각해요.

정적 팩터리 메서드의 장점은 생성자라는 만드는 규약이 없으므로 오는 장점이 있습니다. 이 규약은 생성자의 이름은 클래스 이름과 같아야 한다, 생성자는 꼭 해당 클래스를 반환해야한다와 같은 내용이죠. 이런 장점은 관련글 (1, 2) 를 참고해주세요.

결론은 제 기준에서는 생성자에서 수행해도 상관 없을듯합니다!

Ladder 예시에 대하여

생성 로직이 복잡하기에 요 부분은 이전 Players는 문자열을 Player 객체로 바꾸는 로직이 단순함에 비해 DirectionGenrator, Height, int와 같은 타입의 인자를 활용하여 Ladder를 생성해야하기에 복잡하다고 하신걸까요~?

만일 맞다면 다즐이 생각하는 복잡함의 기준은 인자의 개수인걸까요?

생성 로직이 복잡하기에 요 부분은 이전 Players는 문자열을 Player 객체로 바꾸는 로직이 단순함에 비해 DirectionGenrator, Height, int와 같은 타입의 인자를 활용하여 Ladder를 생성해야하기에 복잡하다고 하신걸까요~?

맞습니다! 문자열을 객체로 바꾸는 로직은 단순하지만, 사다리 같은 경우 여러 인자들을 받아 사용하게 되고 내부적으로도 복잡한 로직으로 구성되기에 복잡하다고 느낀 것 같습니다 .. !

그렇기에 그러한 복잡한 로직이 생성자에 들어가도 되는지 의문이 들게 되었습니다 '^'

객체를 생성하는 생성자 로직은 객체에게 중요하기에 객체가 가지고 있게 두지 않을까 싶어요. 오히려 로직이 복잡하니까 그만큼 객체 내부에서 관리하지 않을까해요. 너무 복잡하다는건 객체를 분리할 여지가 있음을 의미할지도 모르구요.

다즐이 생각하는 팩터리 객체의 예시를 보여주셔도 좋겠네요~

가장 대표적으로 떠오르는 팩터리 객체의 예시는 아래와 같습니다!

public final class DeckFactory { public static void create(final DeckType deckType) { if (deckType == RANDOM) { return RandomDeck(); } if (deckType == NORMAL) { return NormalDeck(); } //... } }

다형성을 이용해서 원하는 형태의 덱을 생성하여 반환하도록 합니다. 분기에 따른 생성 책임을 팩터리 객체가 가지게 하는 것이 가장 일반적인 팩터리 객체의 역할이라고 생각합니다.

토니가 알려준 글에서 객체 생성 자체가 중요한 역할이라면 팩터리 객체로 분리하는 것을 고려한다라는 내용을 보게 되었습니다. 그래서 생성 역할 팩터리 객체가 가져도 괜찮지 않을까라는 생각을 하게 되었습니다.

토니는 객체 생성 자체가 중요한 역할이기에 객체가 가지고 있게 한다고 하셨는데, 그렇다면 언제 팩터리 객체로 분리하는 것을 고려해야할까요? 🤔

오~ 보내주신 예시처럼 특정 타입을 기반으로 분기를 쳐서 추상 클래스/인터페이스를 구현한 구현체를 반환하는 경우에는 사용할거 같아요~

저는 인자의 개수와 같은 측면보다 분기로 인해 여러 객체가 반환 될 수 있는 상황을 복잡하다고 여기고 팩터리 패턴을 적용할 것 같습니다!

toneyparky · 2023-03-04T16:08:14Z

src/main/java/blackjack/controller/BlackJackController.java

+                .collect(Collectors.toList());
+    }
+
+    private void cardDraw(final List<Player> players, final Deck deck) {


자바에서는 메서드를 동사로 시작하는 컨벤션이 있죠! 이에 따라 메서드명을 수정해보죠!

toneyparky · 2023-03-04T16:15:46Z

src/main/java/blackjack/domain/participant/Participant.java

+    public void drawCard(final Card card) {
+        cards.addCard(card);
+    }


Draw 질문에 대해 이야기해봐요!

뽑기 가능한 상태일 때만 뽑아야 하는데 카드를 뽑는 메서드는 이를 검증하지 않기 때문에 메서드를 호출한다면 카드를 가지게 됩니다. 이는 예상하지 못한 결과를 유발할 수 있기에 조금 위험한 코드라는 생각이 들었습니다.

=> 동의합니다. 일반적인 Participant는 그냥 draw할 수 있을지몰라도 Dealer, Player는 각자의 로직이 있으니까요!! 좋은 판단이에요.

하지만 뽑는 것에서 뽑을 수 없는 것을 검증하여 예외를 던지기에는 메서드 의미와 다르게 너무 많은 역할을 가지고 있다는 생각이 들었습니다. 토니는 이와 같은 충돌이 발생할 때 어떤 선택을 하는지 궁금합니다!

=> 음... 요 부분은 아래의 코드를 보고 생각해보면 어때요?

public Participants(final List<Participant> participants) { validate(participants); this.participants = List.copyOf(participants); }

생성자는 객체를 생성하는 메서드인데 validate라는 행위도 수행하죠. 이처럼 카드를 뽑는 행위도 뽑을 수 있는 상황에서만 뽑도록 validate 해준다는 관점으로 말이에요.

생성자 코드를 보니 조금 의미없는 고민이었다는 생각이 들었습니다 ㅎㅎ..

public Card draw() { if (cards.isEmpty()) { throw new IllegalStateException(); } return cards.pop(); }

덱에서도 다음과 같은 코드를 작성했는데, 일관성이 없었던 고민이라 부끄럽습니다 ,,,

생성자 이외의 행위를 하는 메서드에서도 추가 로직이 들어있군요!
제가 제안한 예시 이외에 다즐이 본인 코드에서 유사한 부분을 찾아 제안주셨다는 점에서 제가 남긴 코멘트를 충분히 이해하셨다는 생각이 드네요 👍

woo-chang · 2023-03-06T03:02:36Z

리뷰 내용을 바탕으로 리팩토링을 진행해 보았고, 테스트가 부족했던 코드에 대해서는 테스트 코드를 추가해 보았습니다!
재미있는 고민거리들을 너무 많이 알려주셔서 주말 동안 재밌게 리팩토링할 수 있었던 것 같습니다 😆

다시 한번 리뷰 잘 부탁드립니다 토니 🍸

toneyparky

안녕하세요 다즐! 제가 리뷰한 부분 이외의 코드도 꼼꼼하게 리팩터링해주신 모습이 좋네요 😀

저희가 이전에 나눴던 대화와 제가 추가로 여쭤보고 싶은 부분에 대해서 질문 남겼으니 답변해주시면 감사하겠습니다..! 코드에서 수정할 사항은 별로 없지만 필요하다면 다음 단계를 진행하며 부탁드립니다~!

고생하셨어요!

toneyparky · 2023-03-06T12:18:33Z

README.md

+
+## 도메인 다이어그램
+
+```mermaid


리팩터링 이후 놓칠 수 있는 문서 관리를 해주셨네요 💯

toneyparky · 2023-03-06T12:19:58Z

src/main/java/blackjack/controller/BlackJackController.java

+
+    public void run() {
+        final Participants participants = new Participants(new Dealer(), gatherPlayers());
+        final Deck deck = Deck.createUsingTrump(1);


이해하기 훨씬 쉬워졌네요~!

toneyparky · 2023-03-06T12:26:41Z

src/main/java/blackjack/view/OutputView.java

+        printHandedCardsWithoutScore(dealer);
+        players.forEach(this::printHandedCardsWithoutScore);
+
+        System.out.println();


그냥 println 할 때와 lineSeparator를 출력할 때는 어떤 차이가 있을까요~?

System.out.println(); System.out.println("hello");

System.out.println(LINE_SEPARATOR + "hello");

위의 코드보다 아래의 코드가 읽기 편한 것 같아서 추가하였습니다!

차이점을 알기 위해 println 코드를 확인해 보았는데, 내부에서도 System.lineSeparator를 사용하여 줄바꿈을 하고 있었습니다. 버퍼를 비우는 동작도 하고 있었는데, println을 2번 사용하는 것보다 1번 사용하면 버퍼를 비우는 동작을 1번만 수행하기에 조금이나마 성능상이 이점이 존재할 것 같습니다!

그 외에 어떤 차이가 존재하는지 궁금합니다!

이 부분은 저도 궁금하여 여쭤보았어요. 두가지를 혼용하여 사용하시기에 다즐만의 기준이 있는지 궁금했습니다~

toneyparky · 2023-03-06T12:27:02Z

src/test/java/blackjack/domain/card/CardTest.java

+        assertThat(card.getScore()).isEqualTo(1);
+    }
+
+    @Nested


@Nested를 적절하게 활용해주셨네요 굳굳~

사용해보니 어떤 점을 느끼셨을까요?

응집성 있는 테스트 코드를 작성할 수 있었고, 어떤 동작을 수행하고 어떤 결과를 낼 것이다라는 명확한 의미를 담은 테스트 코드를 작성할 수 있었습니다. 자주 애용하게 될 것 같습니다 👍

저도 계층을 활용하면 원하는 테스트를 명확하게 표현할 수 있다는 장점이 있다고 생각해요. 그로 인해 가독성도 좋아지구요.
물론 코드 양이 늘어나지만요. 이를 해결하기 위한 라이브러리도 있다고 알고 있는데 @Nested면 충분하다고 생각합니다~
굳굳

toneyparky · 2023-03-06T12:35:35Z

src/main/java/blackjack/controller/BlackJackController.java

+                .collect(Collectors.toList());
+    }
+
+    private List<ParticipantResponse> getParticipantResponses(final List<? extends Participant> participants) {


제네릭의 기능을 사용하셨군요~ List<? extends Participant>는 어떤 의미를 지니는가요~?

?는 와일드 카드로 제한없이 모든 타입이 가능하다는 것을 의미합니다. 저는 참여자들의 리스트만 받기 위한 제한이 필요하였고 상한 제한을 설정하여 Participant를 상속하는 클래스만 올 수 있도록 위와 같은 표현을 사용하였습니다!

굳굳~ 제네릭을 잘 이해하고 계시네요 👍

toneyparky · 2023-03-06T12:42:37Z

src/main/java/blackjack/controller/BlackJackController.java

+    private ParticipantResponse getHiddenDealerResponse(final Dealer dealer) {
+        final List<Card> hiddenCards = dealer.getCards().subList(0, INITIAL_DRAW_COUNT - 1);
+        final CardsResponse cardsResponse = new CardsResponse(-1, getCardInfos(hiddenCards));
+        return new ParticipantResponse(dealer.getName(), cardsResponse);
+    }


컨트롤러가 많이 복잡해졌네요.

그 요인을 살펴보면 객체를 DTO로 변환하는 부분이 많아보여요. 요런 부분에 대한 책임을 DTO에 정적 팩터리 메서드를 만들어서 넘기기도 해요. ParticipantResponse.from(dealer) 정도로 DTO를 바로 생성하는거죠. (적용하실 필요는 없습니다~!)

저희가 정적 팩터리 메서드로 이야기를 많이 나눴기에 다즐의 생각이 궁금하네요.

너무 좋은 방법인 것 같습니다 ⛄️

컨트롤러에서 dto 생성자를 이용해 생성한 이유는 컨트롤러가 도메인과 뷰 사이의 중간 매개체로 이어주는 역할을 한다고 생각하였습니다. (dto가 도메인을 알지 못하도록)

하지만 뷰에 데이터 전달을 위한 dto도 컨트롤러 레이어로 해당하기에 위와 같은 역할을 가져도 된다고 생각합니다. 그래서 알려주신 방법대로 리팩토링 진행해보고자 합니다!

네, 기존에 컨트롤러가 변환의 역할을 담당하지만 이는 컨트롤러 객체가 아닌 컨트롤러 레이어죠. 그렇기에 해당 레이어에 속한 DTO에 책임을 넘길 수 있겠다 싶어 리뷰드렸어요.

변경사항이 조금 있을 것 같은데 요 부분까지 반영하고 머지할걸 그랬군요 😂

toneyparky · 2023-03-06T12:43:22Z

src/main/java/blackjack/view/dto/DealerResultResponse.java

+        return name;
+    }
+
+    public Map<Result, Integer> getResult() {


DTO에서 모델 레이어의 객체인 Result를 반환하고 있네요. 그리고 이를 OutputView에서 사용하고 있구요. 이 부분에 대해서 어떻게 생각하시나요~?

도메인과 같이 노출하면 안되는 중요한 값을 가지고 있지 않고, Enum을 데이터를 감싸서 전달하는 값 객체로 볼 수 있다고 생각합니다! String으로 어떤 의미인지 구체적이지 않은 값을 전달하기보다는 Enum을 전달하는 방법을 선택하였는데 토니는 어떻게 생각하는지 궁금합니다!

Enum 객체 내부에 로직이 들어갈 경우에는 뷰에서 해당 객체를 조작할 수 있겠죠? 예를들면 reverse 함수를 사용할 수 있을거에요. 그러면 이미 값 이상의 행위를 하고 있다고 생각되어요. 그렇기에 원시값을 사용할듯합니다.

로직이 들어가 있기에 사이드 이펙트가 발생할 가능성이 존재하네요! 이 부분도 수정해보도록 하겠습니다 🔥

kokodak and others added 30 commits February 28, 2023 17:07

docs: 요구사항 명세 작성

93adb74

feat: 이름의 길이 검증 기능 구현

4d669bd

feat: 금지된 이름 검증 기능 구현

ab04caa

docs: 기능 구현 목록 수정

76e478d

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 이름 도메인 생성

c5575b7

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 문양 도메인 생성

aeb41cc

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 숫자 도메인 생성

660568b

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 문양 이름 확인 기능 구현

5529812

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 숫자 이름 확인 기능 구현

f42b755

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 점수 확인 기능 구현

59212a2

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 점수 계산 기능 구현

365c189

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 에이스인지 확인하는 기능 구현

14cb9da

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 점수 합계 계산 기능 구현

d668b8c

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 점수 최댓값 초과 여부 확인 기능 구현

07e5c72

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 이름 확인 기능 구현

36dacd5

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 블랙잭 여부 확인 기능 구현

fe3be1e

Co-authored-by: woo-chang <clllickme@naver.com>

fix: 메서드명 수정 및 잘못된 로직 수정

bf4991f

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 카드 추가 여부 확인 기능 구현

ed70701

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 카드 추가 기능 구현

4c494a0

Co-authored-by: woo-chang <clllickme@naver.com>

chore: 테스트 어노테이션 추가

43b6ec5

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 카드를 받는 기능 구현

35fd4fc

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 점수 확인 기능 구현

d3a6af1

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 개수 확인 기능 구현

58f1fae

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 카드 추가 여부 확인 기능 구현

81f907a

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 점수 확인 기능 구현

96833fd

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 카드를 받는 기능 구현

5138dc4

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 딜러 도메인 생성

4e58c1d

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 참가자 도메인 생성

e3ae0a0

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 딜러와 플레이어가 참가자 상속

4cdb38c

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 플레이어의 이름 중복 검증 기능 구현

2370e13

Co-authored-by: woo-chang <clllickme@naver.com>

kokodak and others added 6 commits March 3, 2023 16:46

feat: 이름으로만 생성될 수 있는 기능 구현

70230b2

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 덱에서 카드를 뽑아 참가자들에게 나눠주는 기능 구현

0363e8e

Co-authored-by: woo-chang <clllickme@naver.com>

feat: 블랙잭 게임 실행 기능 구현

bf2e4bf

Co-authored-by: woo-chang <clllickme@naver.com>

fix: 게임 결과 로직 수정

0ad7a72

Co-authored-by: woo-chang <clllickme@naver.com>

refactor: TRUMP는 덱이 알도록 수정

6518a49

refactor: 입력 기능 가독성 위해 메서드명 수정

24e9e03

toneyparky requested changes Mar 4, 2023

View reviewed changes

woo-chang added 19 commits March 5, 2023 15:12

refactor: 축약하지 않은 변수명으로 수정

57cb277

refactor: 딜러가 카드를 추가로 뽑을 수 있는 점수는 딜러가 알도록 수정

5f77913

refactor: ACE -> Ace 일관성있는 표현으로 수정

f7eb062

refactor: 테스트 목적을 명확히하도록 수정

90d09d3

refactor: 계층 구조 테스트로 의미를 명확하게 전달하도록 수정

9bddfc3

refactor: 자바 컨벤션에 따라 메서드명 수정

8f294bb

refactor: 카드 뽑을 때 검증하도록 수정

ed5db59

refactor: NPE 방지 및 변수, 메서드명 수정

0223fca

refactor: 참가자에게 딜러 여부에 대한 메시지를 던지도록 수정

7eeedc2

refactor: 거짓 성공을 막기위해 검증부에는 하드코딩 하도록 수정

1a85ff7

refactor: TRUMP 생성 시 flatMap 사용 및 덱 생성 시 섞도록 수정

cf364cf

refactor: 플레이어 이름을 명시하도록 Name -> PlayerName 수정

b15c4c2

refactor: 참여자 목록 생성 책임을 생성자로 수정

6506c31

refactor: Deck 생성 책임은 Deck이 가지도록 수정

60cd11c

refactor: 블랙잭 게임 컨트롤러 수정

2458f44

refactor: 딜러 승패 여부 확인 로직 수정

4daf6f8

refactor: 카드 목록에서 사용되지 않는 메서드 제거

24f34f1

refactor: 딜러가 결과 확인하는 로직 수정

be64ca7

test: 테스트하지 못했던 코드에 대한 테스트 코드 작성

601b98d

feat: 제네릭 미션 구현

44bd964

toneyparky approved these changes Mar 6, 2023

View reviewed changes

toneyparky merged commit 095f92f into woowacourse:woo-chang Mar 6, 2023


		public class BlackJackController {

		private static final int INIT_DRAW_COUNT = 2;

	private static final int INIT_DRAW_COUNT = 2;
	private static final int INITIAL_DRAW_COUNT = 2;

		assertThatThrownBy(deck::draw)
		.isInstanceOf(IllegalStateException.class);


		public class DeckFactory {

		public static Deck createWithCount(final Stack<Card> pack, int count) {

[1단계 - 블랙잭 게임 실행] 다즐(최우창) 미션 제출합니다. #443

[1단계 - 블랙잭 게임 실행] 다즐(최우창) 미션 제출합니다. #443

Conversation

woo-chang commented Mar 3, 2023 • edited Loading

정잭 팩토리 메서드 vs 팩토리 패턴

Draw

딜러 이름 중복 관리

Score에서 Result 반환

toneyparky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toneyparky Mar 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toneyparky Mar 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woo-chang Mar 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woo-chang Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

toneyparky Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woo-chang Mar 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woo-chang commented Mar 6, 2023

toneyparky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woo-chang Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woo-chang commented Mar 3, 2023 •

edited

Loading

toneyparky Mar 4, 2023 •

edited

Loading

toneyparky Mar 5, 2023 •

edited

Loading

woo-chang Mar 5, 2023 •

edited

Loading

woo-chang Mar 7, 2023 •

edited

Loading

toneyparky Mar 7, 2023 •

edited

Loading

woo-chang Mar 5, 2023 •

edited

Loading

woo-chang Mar 7, 2023 •

edited

Loading