[1단계 - 블랙잭] 제나(위예나) 미션 제출합니다.

[yenawee]

안녕하세요 소니 !
블랙잭 미션 1단계를 구현해 리뷰 요청드려요😊

이번 미션은 이전 미션들보다 좀 더 어려웠었는데 구현하면서 궁금했던 점이 있습니다 !
딜러와 플레이어의 중복 코드를 줄이기 위해서 두 클래스는 Participant 를 상속받습니다.
그러면 Player 는 상속받은 List<Card> cards; Score score; 와 자신의 Name name; 를 가지게 되는데 그러면 Player 클래스의 인스턴스 변수는 1개인지 3개가 되는건지 궁금합니다 !!

또 게임 진행 로직은 BlackJackGame 클래스를 만들어서 책임을 분배해주고 싶었는데 게임에서 "한장의 카드를 더 받겠습니까?" 의 입력을 계속 InputView 로 받아야 해서 모든 게임 진행 로직을 Controller 가 담당하게 한 것이 아쉽습니다 ....

리뷰 잘 부탁드립니다 🙇‍♀️

[sonypark]

제나 안녕하세요. 리뷰어 소니입니다 :)
이전 미션에 비해 조금 어려웠을텐데 1단계 미션 구현 잘해주셨네요!
아래 몇가지 코멘트 남겼으니 확인부탁드립니다~

1. 인스턴스 변수 3개 이상 기준
   음 아마 부모 클래스의 인스턴스까지 포함해서 말한것일것 같은데요. 필드를 여러개 갖는다는건 역할을 나누는 기준이 될 수 있어서 이러한 제약을 둔 것 같습니다. 개인적으로 3개까지는 괜찮다고 생각합니다. :) 부모 필드까지 포함하되 필드는 3개까지 가능하고 그 이상은 안된다라는 기준으로 작성해주세요!

2. "한장의 카드를 더 받겠습니까?" 의 입력 로직
   이 부분은 아래 코멘트에 남겼습니다. (enum 활용)

[sonypark]

찾아보니 카드게임에 쓰이는 카드를 영미권에서는 PlayingCards라고 부르고 Trump는 일본에서 쓰는 명칭이라고 하네요. 여기서 의미는 카드 한벌을 의미하신 것 같은데 Deck 이라는 명칭은 어떤가요? (네이밍은 도메인 이름을 찾아보시고 결정해주시면 될 것 같아요)

[yenawee]

Trump -> Deck
TrumpShape -> Suit
TrumpNumber -> Letter 로 수정했습니다 !

[sonypark]

List로 카드를 관리하고 있는데, Queue나 Stack을 사용해보는건 어떨까요? 그렇게 되면 pop으로 한장씩 카드를 빼서 사용하기 편할 것 같네요.

[yenawee]

Queue 로 관리해서 .poll() 메소드를 사용해 하나씩 카드를 뽑도록 수정했습니다

[sonypark]

카드를 섞는 동작을 하는 shuffle 이라는 메서드를 만들어 사용하는 것도 괜찮을 것 같네요.

• Collections.shuffle() 이라는 메서드를 제공하기도 합니다.

[yenawee]

Card 를 Queue 로 관리하는데 Collections.shuffle()을 하려니

private void generateDeck() {
        for (Suit suit : Suit.values()) {
            generateCardEachSuit(suit);
        }
        Collections.shuffle((List<?>) cards);
    }

Queue 는 shuffle 지원을 안해서 형변환이 되어서 들어가는데 혹시 소니가 의도하신게 우선 List에 카드를 더하고 shuffle 한 뒤 Queue 로 변환하는 것이었는지 궁금합니다 ..!

[sonypark]

아 그렇군요!
Stack과 Queue 중 어떤걸 사용하느냐에따라 구현이 조금 달라지긴 하겠네요.
우선 Stack을 사용하면 형변환 없이 사용가능할 겁니다.

private final Stack<Card> cards = new Stack<>();

    private void generateDeck() {
        for (Suit suit : Suit.values()) {
            generateCardEachSuit(suit);
        }
        Collections.shuffle(cards);
    }

또는 메서드로 빼서 사용

private final Stack<Card> cards = new Stack<>();

    private void generateDeck() {
        for (Suit suit : Suit.values()) {
            generateCardEachSuit(suit);
        }
    }

    public void shuffle() {
        Collections.shuffle(cards);
    }

이렇게 보니 CardFactory를 만들어서 아래와 같이 사용해도 좋을 것 같네요

	private Stack<Card> cards = new Stack<>();

	public Deck() {
		cards.addAll(CardFactory.create());
	}

	public void shuffle() {
		Collections.shuffle(cards);
	}

Queue를 사용한다면 CardFactory로 List<Card>를 만들어 shuffle 후 Queue에 담아줄 수 있을 것 같네요.

	private Queue<Card> cards = new ArrayDeque<>();

	public Deck() {
		cards.addAll(Collections.shuffle(CardFactory.create()));
	}

[sonypark]

• 플레이어를 만드는 로직은 PlayerFactory 클래스를 만들어 위임하면 어떨까요?
• 플레이어명을 검증하는 로직을 Validator 클래스에 두는게 나을까요? Player를 생성할때만 필요한 검증일것 같은데 그렇다면 어디에 두는게 좋을까요?

[yenawee]

• Player 들을 관리하는 Players 클래스가 이미 있어서 해당 클래스에 정적 팩토리 메소드로 각각 Player 들을 만들어 List 를 생성하도록 했습니다!!
• 플레이어들의 수, 플레이어들의 이름 중복을 검증하는 로직이라서 Players 클래스에 함께 두었습니다. 이전에 Validator 클래스를 따로 두었던 이유는 InputView 에서 받은 값들을 컨트롤러에서 검증해서 도메인에 넘기려고 했는데 혹시 Validator 클래스를 따로 두는 것 보다 도메인에서 검증하는게 더 나을까요 ..? 유효성 검증은 항상 할때마다 위치가 고민이 됩니다..ㅠ

[sonypark]

네 그렇습니다. Validator 클래스는 이름만 봐도 너무 추상적이고, 해당 클래스에 어떤 검증 로직이 들어있는지 또는 들어갈지 판단하기 어렵습니다. (앞으로 플레이어 이외에 게임 내 다양한 검증 로직이 더 늘어날 경우 Validator에 계속 추가될까요? Validator에 추가될 로직은 어떤 기준에서 정해질까요?) 또한 Validator 클래스를 따로 만들었다는건 여러곳에서 범용적으로 쓰일 수 있어야하는데 플레이어 검증은 플레이어를 생성할때 이외에는 사용되지 않아서 클래스로 따로 빼는건 과도한 분리라고 생각했습니다 :) 도메인에 대한 규칙과 검증은 해당 도메인이 갖고 있는게 코드 응집성, 가독성, 유지보수 측면에서 좋을 것 같습니다.😃

[sonypark]

딜러와 플레이어를 출력하는 메서드가 각각 있을 필요가 있을까요?
예를들어, 이런식으로 쓸 수 있을 것 같아서요 :)

public static void printInitialCardDistribution(Dealer dealer, Players players) {
		String playersName = String.join(", ", players.getNames());
		System.out.printf("%s와 %s에게 두 장의 카드를 나누었습니다.\n", dealer.getName(), playersName);
		printCards(dealer.getName(), dealer.getFirstCard());
		players.forEach(player -> printCards(player.getName(), player.getCards()));
	}

[yenawee]

딜러: 3다이아몬드
pobi카드: 2하트, 8스페이드
jason카드: 7클로버, K스페이드

이런 식으로 출력 사항이 약간씩 달라서 각각 구현해줬었는데 아래 승패 결과는 또 '딜러 카드' 라고 나와 메소드 분리해서 똑같이 output view 를 맞춰야 할 필요성이 없는 것 같아 말씀 주신대로 딜러와 플레이어를 출력하는 메소드를 통합했습니다 😺

[sonypark]

카드를 더 받을지 말지에 대한 입력값은 네, 아니오 둘 중 하나일텐데요.
특정 상수에 해당하는지 여부 + 해당하지 않으면 예외 발생 -> 이와 같은 로직은 enum으로 구현해도 좋을 것 같은데 어떠신가요?

public enum AddCardOrNot {
	YES("y"),
	NO("n");

	private final String type;

	AddCardOrNot(String type) {
		this.type = type;
	}

	public static AddCardOrNot of(String type) {
	}

[yenawee]

추천해주신 방법으로 구현해봤더니 코드가 매우 간결해졌습니다 감사합니다

[sonypark]

이건 마이너한 부분이긴 한데 입력값이 대문자로 들어올 경우도 고려해서 toLowerCase 또는 toUpperCase 로 변경후 값을 비교해주면 어떨까요~?

[sonypark]

플레이어의 의사(카드를 더 받을지 말지) + 더 받을 수 있는 상태 -> 를 판단하는 로직은 Player 내부에서 판단하면 되지 않을까요?

[yenawee]

더 받을 수 있는지(점수 21점 이하인지) 는 플레이어 내부에서 판단하고
더 받을 수 있다고 대답했는지 판단은 inputView 에서 받아야 해서 위에 올려주신 enum 구현을 활용해서 컨트롤러에서 판단할 수 있게끔 수정했습니다 !!

[yenawee]

안녕하세요 소니 !

1단계 리팩토링은 남겨주신 리뷰를 반영하여 너무 비대했던 컨트롤러 코드를 줄이는데 집중해봤습니다.!

• 책임 분배

  • Player 객체 생성 책임을 Players 로 위임
  • 플레이어마다 카드를 더 받을지 판단하는 로직을 enum 을 사용. 입력이 유효한지 검증도 해당 enum 사용

• 출력 로직 outputView 로 이동

  • 사실 뷰에서 도메인 객체를 받는거보다 getter 로 원시값만 넘겨주고 싶어서 가공하는 과정에서 controller 의 코드가 길어졌었는데 출력하는 Dealer, Player 의 상태인 Cards, Name, Score 와 GameResult 의 상태인 승패 정보 모두 출력 대상이라서 객체를 view 에 넘겨주더라도 응답에 필요하지 않은 데이터를 넘겨주는 일은 없을 것 같아 가독성을 위해 output 에 객체를 넘기도록 리팩토링 하였습니다.
  • 도메인의 내부 구현이 뷰에 노출이 되긴 해서 맘에 약간 걸립니다 😿
  • 의문점 : 딜러는 이름이 없는데 플레이어와 같은 메소드를 사용하려고 getName() 메소드를 호출하면 outputView 에 정의되어 있는 DEALER_NAME 을 반환합니다. 이렇게 구현이 된거면 도메인이 뷰를 의존하는 상태가 된 것인가요..? 딜러를 "딜러" 라고 출력 안하고 "dealer" 라고 출력해야 한다면 outputView 만 수정하면 되도록 했는데 domain 에 import OutputView 가 들어가니까 이런 방식을 지양해야 하는지 궁금합니다 !!

리뷰 감사드립니다!!

[sonypark]

제나 안녕하세요~~
피드백 반영 잘해주셨네요!👍
질문 남기신 부분에 답글 달았으니 확인 부탁드려요~ 코멘트 남긴부분은 2단계에 반영해주시면 감사하겠습니다. 2단계 진행해주세요~

[sonypark]

아 그렇군요!
Stack과 Queue 중 어떤걸 사용하느냐에따라 구현이 조금 달라지긴 하겠네요.
우선 Stack을 사용하면 형변환 없이 사용가능할 겁니다.

private final Stack<Card> cards = new Stack<>();

    private void generateDeck() {
        for (Suit suit : Suit.values()) {
            generateCardEachSuit(suit);
        }
        Collections.shuffle(cards);
    }

또는 메서드로 빼서 사용

private final Stack<Card> cards = new Stack<>();

    private void generateDeck() {
        for (Suit suit : Suit.values()) {
            generateCardEachSuit(suit);
        }
    }

    public void shuffle() {
        Collections.shuffle(cards);
    }

이렇게 보니 CardFactory를 만들어서 아래와 같이 사용해도 좋을 것 같네요

	private Stack<Card> cards = new Stack<>();

	public Deck() {
		cards.addAll(CardFactory.create());
	}

	public void shuffle() {
		Collections.shuffle(cards);
	}

Queue를 사용한다면 CardFactory로 List<Card>를 만들어 shuffle 후 Queue에 담아줄 수 있을 것 같네요.

	private Queue<Card> cards = new ArrayDeque<>();

	public Deck() {
		cards.addAll(Collections.shuffle(CardFactory.create()));
	}

[sonypark]

네 그렇습니다. Validator 클래스는 이름만 봐도 너무 추상적이고, 해당 클래스에 어떤 검증 로직이 들어있는지 또는 들어갈지 판단하기 어렵습니다. (앞으로 플레이어 이외에 게임 내 다양한 검증 로직이 더 늘어날 경우 Validator에 계속 추가될까요? Validator에 추가될 로직은 어떤 기준에서 정해질까요?) 또한 Validator 클래스를 따로 만들었다는건 여러곳에서 범용적으로 쓰일 수 있어야하는데 플레이어 검증은 플레이어를 생성할때 이외에는 사용되지 않아서 클래스로 따로 빼는건 과도한 분리라고 생각했습니다 :) 도메인에 대한 규칙과 검증은 해당 도메인이 갖고 있는게 코드 응집성, 가독성, 유지보수 측면에서 좋을 것 같습니다.😃

[sonypark]

이건 마이너한 부분이긴 한데 입력값이 대문자로 들어올 경우도 고려해서 toLowerCase 또는 toUpperCase 로 변경후 값을 비교해주면 어떨까요~?