[블랙잭] 레몽(이규명) 미션 제출합니다. #1

lgm1007 · 2024-06-01T07:32:44Z

각 행위의 역할을 어떻게 할당해야 할지에 대한 고민이 어려웠던 미션같아요~😄
결국 전반적인 게임의 진행에 대한 역할은 GameController에게 위임하게 되었고,
딜러라는 객체를 참가자와 따로 생성할까 생각했지만, 딜러도 결국 게임의 참가자이며 참가자 객체에서 딜러인지 체크하는 메서드를 둠으로써 참가자들과 딜러를 구분지을 수 있어 결국 따로 생성하지는 않았어요!

기능 요구사항

사용자에게 블랙잭 게임에 참여할 플레이어를 입력받는다.
- 블랙잭 게임의 참가자는 딜러 + 플레이어이다.
게임 처음에는 게임의 참가자들은 카드덱에서 무작위로 카드 2장을 받는다.
- 카드덱은 하트, 클로버, 스페이드, 다이아몬드 4개의 모양이 있고 각각 ACE, 2~9, KING, QUEEN, JACK이 있다.
- ACE는 1또는 11로 계산할 수 있고 (합계가 21이 넘으면 1로 계산, 아니면 11), KING, QUEEN, JACK은 각각 10으로 계산한다.
플레이어는 처음에 받은 두 장의 카드의 숫자 합계가 21을 넘지 않으면 얼마든지 카드를 더 받을 수 있다.
딜러는 처음에 받은 카드 2장의 합계가 16이하라면 반드시 1장의 카드를 추가로 받아야 하고, 17점 이상이면 추가로 받을 수 없다.
21이 초과된 플레이어는 무조건 패배한다.
21에 가장 가까운 플레이어가 승리한다.
게임을 완료한 후 각 플레이어별로 승패를 출력한다.

lgm1007 · 2024-06-03T12:02:08Z

src/main/java/card/model/CardDeck.java

+	private static final List<CardSuit> CARD_EMBLEMS = List.of(CardSuit.CLOVER, CardSuit.HEART, CardSuit.SPADE, CardSuit.DIAMOND);
+	private static final List<CardRank> CARD_NUMBER = List.of(CardRank.ACE, CardRank.TWO, CardRank.THREE, CardRank.FOUR, CardRank.FIVE, CardRank.SIX, CardRank.SEVEN, CardRank.EIGHT, CardRank.NINE, CardRank.JACK, CardRank.QUEEN, CardRank.KING);


엇 이 부분은 처음에 카드 문양을 EMBLEM, 숫자를 NUMBER로 네이밍했었습니다만,
찾아보니 트럼프 카드에서 문양을 SUIT, 숫자를 RANK로 명칭한다는 관례를 찾게되어 네이밍을 변경했는데, 미쳐 해당 변수명은 변경하지 못한 부분입니다..!

haero77

레몽 안녕하세요! 리뷰어 해로(김선호)입니다.
먼저 마지막 미션까지 완주하신 것 축하드려요! 🥳

처음 미션을 시작하실 때보다, 클래스를 어떻게 하면 역할에 맞게 나눌지 고민하고 계신다는 느낌을 많이 받았습니다..!

고민하시던 내용에 초점에 맞춰 간단히 리뷰를 남겨봤어요. 고생 많으셨습니다 :)

haero77 · 2024-06-06T08:12:09Z

src/main/java/card/controller/DrawCardRequest.java

+import java.util.List;
+
+public class DrawCardRequest {
+	private static final List<String> DRAW_YES = List.of("y", "Y");


이미 DrawCard enum에 'y' 가 잘 정의되어있어서 상수로 추가로 더 정의해주는 것은 중복으로 보여요. enum.name()과 String.equalsIgnoreCase 를 활용해서 중복을 없애봐도 좋을 것 같습니다 :)

haero77 · 2024-06-06T08:15:18Z

src/main/java/card/model/Card.java

+package card.model;
+
+public record Card (
+	CardSuit suit,
+	CardRank rank
+) {
+	@Override
+	public String toString() {
+		return "%s%s".formatted(rank.getRank(), suit.getName());
+	}


모델이 UI(뷰)로직에 의존하고 있어요.
모델이 UI로직에 의존할 경우 어떤 문제가 생길 수 있을까요?
toString() 오버라이딩은 언제, 어떻게 하는 것이 좋을지도 같이 찾아보시면 좋을 것 같아요 :)

네! 해당 리뷰를 보고 toString() 오버라이딩은 어떨 때 하는 것인지 찾아보게 되었어요.
이전에는 toString()은 객체가 가진 주요 정보를 사람이 읽기 쉬운 형태로 담아 출력하는 용도로 사용한다면 오케이라고 생각했어요.
하지만 관련 스택오버플로우를 찾아보니 toString() 메서드는 특별한 경우를 제외하고는 디버깅 용도로 사용하고 프로덕션 로직에서는 사용하지 않는 것이 좋다는 글을 보며 생각을 다시 하게 되었어요!👍

참고 문서

Java toString for debugging or actual logical use

Is toString() only useful for debugging?

haero77 · 2024-06-06T08:19:39Z

src/main/java/card/model/CardDeck.java

+	private static final List<CardSuit> CARD_EMBLEMS = List.of(CardSuit.CLOVER, CardSuit.HEART, CardSuit.SPADE, CardSuit.DIAMOND);
+	private static final List<CardRank> CARD_NUMBER = List.of(CardRank.ACE, CardRank.TWO, CardRank.THREE, CardRank.FOUR, CardRank.FIVE, CardRank.SIX, CardRank.SEVEN, CardRank.EIGHT, CardRank.NINE, CardRank.JACK, CardRank.QUEEN, CardRank.KING);


CardSuit, CardRank 에 이미 모든 카드 문양과 카드 숫자가 잘 정의 되어있어서,
상수로 다시 정의되어 있어 불필요한 중복으로 보여요. Enum.values를 적절히 활용해보면 어떨까 싶습니다 :)

아하 Enum.values 배워갑니다~!

haero77 · 2024-06-06T08:21:22Z

src/main/java/card/validator/DrawCardInputValidator.java

@@ -0,0 +1,9 @@
+package card.validator;
+
+public class DrawCardInputValidator {


정적 유틸 클래스를 만들 때는 실수로 인스턴스 생성을 하지 않도록 유의해주세요!

haero77 · 2024-06-06T08:25:20Z

src/main/java/card/validator/DrawCardInputValidator.java

+public class DrawCardInputValidator {
+	public static void validate(final String drawCardInput) {
+		if (drawCardInput == null || drawCardInput.isBlank()) {
+			throw new IllegalArgumentException("카드를 더 뽑을지 y 또는 n을 입력해주세요.");
+		}
+	}


drawCardInput 에 대한 단순 empty string 검사하는 로직을 별도 클래스로 추출하다보니, drawCard에 관한 로직이 산재되어 응집도가 떨어진다고 느꼈어요. DrawCardRequest와 이 로직이 같이 있으면 더 자연스러울 것 같아요 :)

네 validator에 단순 문자열이 비어있는지 검사하는 로직만 있다보니 그렇게 느껴졌을 것 같아요!
사용자 input에 대해 유효성을 검사하는 역할을 validator에게 주도록 하다보니 클래스 분리를 했지만, 유효성 검사가 하나일 경우에는 해당 검사를 사용하는 클래스에 있어도 괜찮겠다는 생각을 하게 됐습니다!

haero77 · 2024-06-06T08:50:34Z

src/main/java/game/domain/GameJudgement.java

+	private final Participant dealer;
+	private final List<Participant> participantWithoutDealers = new ArrayList<>();
+
+	public GameJudgement(final List<Participant> participants) {
+		this.dealer = participants.stream()
+			.filter(Participant::isDealer)
+			.findAny()
+			.orElseThrow(() -> new IllegalArgumentException("게임에 딜러가 존재하지 않습니다."));
+		this.participantWithoutDealers.addAll(participants.stream()
+			.filter(participant -> !participant.isDealer())
+			.toList()
+		);
+	}


딜러와 딜러가 아닌 참가자를 구분하는 코드가 소스 전반적으로 자주 보여요.
이런 구조는 결국 계속 딜러와 딜러가 아닌 참가자를 개발자가 지속적으로 신경써줘야하기 때문에 실수할 여지가 많고,
실수를 하지 않기 위한 인지 부하도 높은 구조로 느껴집니다.

실수 방지와 버그 유발을 낮추기 위해서, '딜러가 아닌 참가자'를 나타내는 값 객체를 등장시켜보면 어떨까 싶어요 :)
(어떤 Participants를 파라미터로 건네도, '딜러가 아닌 참가자'의 인스턴스를 생성하는 시점에서 해당 객체 안에 딜러가 없음을 보장하면 그 객체를 믿고 써볼 수 있지 않을까요?)

haero77 · 2024-06-06T08:55:11Z

src/main/java/view/OutputView.java

+	public void printGameJudgement(final GameJudgement gameJudgement) {
+		System.out.println("## 최종 승패");
+		System.out.println(gameJudgement.fetchDealerJudge());
+		gameJudgement.fetchParticipantJudge().forEach(System.out::println);
+	}


뷰 객체가 도메인 객체에게 결과를 뽑는 메시지를 보내서 결과를 출력하는게 좋은지는 의문이에요.
(뷰 객체가 결과를 뽑기 위해서 어떤 메시지를 보내야할지 알아야할 필요가 있을까요?)

뷰는 이미 계산된 결과를 출력하는 정도의 책임만 갖고 있는게 적당하다고 생각합니다 :)

haero77 · 2024-06-06T08:58:58Z

src/main/java/game/domain/GameJudgement.java

+	public List<String> fetchParticipantJudge() {
+		return participantWithoutDealers.stream()
+			.map(participant -> {
+				String loseMessage = LOSE_PARTICIPANT_MESSAGE.formatted(participant.getName());
+				if (participant.isOverWinningMaxScore()) {
+					return loseMessage;
+				}
+
+				int participantScore = participant.calculateCurrentScore();
+				if (participantWithoutDealers.stream()
+					.filter(p -> !(p.getName().equals(participant.getName()) ||
+						p.isOverWinningMaxScore()))
+					.allMatch(innerParticipant -> participantScore > innerParticipant.calculateCurrentScore())) {
+					return WIN_PARTICIPANT_MESSAGE.formatted(participant.getName());
+				}
+
+				return loseMessage;
+			})
+			.toList();
+	}


stream 안에 if 문이 중첩되고, 조건식 자체에 stream이 사용되어 있어서 이 메서드 로직을 파악하기가 많이 어려워요. 의미 있는 단위로 이름을 붙여서 가독성을 더 챙겨보면 좋을 것 같아요!

haero77 · 2024-06-06T09:20:08Z

src/main/java/participant/domain/ParticipantRegister.java

+		this.participantRequest = participantRequest;
+	}
+
+	public List<Participant> registerParticipants() {


참여자를 '등록' 시키는 메서드가 실제로 어딘가에 '등록' 시킨다기보다, 단순히 참여자 이름을 통해 Participant 객체를 생성해서 리턴하는 걸로 보여요. (물론 이 과정 자체를 '등록'이란 과정으로 추상화를 하신 것 같기도 합니다.) 이런 로직이라면 ParticipantRequest.toParticipants 처럼 풀어나가도 괜찮아 보입니다 :)

haero77 · 2024-06-06T09:56:04Z

src/main/java/participant/domain/ParticipantRegister.java

+		// 참여자 리스트 순서 정렬 - 딜러가 앞에 오도록 정렬
+		return participants.stream()
+			.sorted(Comparator.comparing(Participant::getName).reversed())
+			.collect(Collectors.toList());


(오늘 리뷰 중 가장 중요하게 생각하는 리뷰에요) ⭐️

리스트에 저장된 순서에 따라서 누가 딜러인지, 누가 플레이어인지 정하고 있어요.
그 말은 곧 registerParticipants라는 메시지를 보내는 객체는 항상 딜러가 가장 앞에 와야한다는 것을 계속 알고 있어야한다는 의미이기도 합니다. 즉, 이 메서드를 사용하는 개발자는 항상 이 메서드의 구현을 상세하게 알아야만 올바르게 이 메서드를 사용할 수 있어요. (순서에 따라 딜러인지, 플레이어인지 누군지 정확하게 모르면 잘못 구현하기가 쉽고(=실수하기가 쉽고), 이는 곧바로 버그로 이어지겠죠.)

소스 전반적으로 딜러와 플레이어를 구분하는 로직이 여러 곳에 걸쳐 등장하고, 이에 따라 실수하기 쉬운 구조가 계속 만들어지는데, 저는 이것이 딜러와 플레이어 구분없이 Participant 하나로 설계를 하는 것에서 비롯한 비용이라고 생각해요.

딜러라는 객체를 참가자와 따로 생성할까 생각했지만, 딜러도 결국 게임의 참가자이며 참가자 객체에서 딜러인지 체크하는 메서드를 둠으로써 참가자들과 딜러를 구분지을 수 있어 결국 따로 생성하지는 않았어요!

딜러/플레이어를 체크하는 메서드를 둔다는 것은, 이 메서드의 구현을 항상 살펴봐야한다는 것이고(위 메서드도 이 예에 해당하죠.), 이는 곧 개발자가 직접, 지속적으로 신경써야하는 인지 부하 비용을 항상 떠안게 되게 됩니다.

딜러도 결국 게임의 참가자이며

딜러가 게임의 참가자라고 해서, 플레이어와 구분될 필요가 없는지, 구분해서 얻을 수 있는 장단점은 무엇이고 구분하지 않아서 생기는 장단점은 무엇인지 고민해보시면 좋을 것 같습니다 :D (앞으로 역할 관점에서 객체를 설계할 때 많이 도움될 거에요!)

맞습니다. 사실 이번 미션에 딜러와 플레이어 간 발생하는 로직을 중복되지 않게 처리하기 위해 딜러와 플레이어를 같은 Participant로 구분하게 되었는데, 중복이 발생하지 않도록 하기 위한 조치가 복잡한 구조를 만들어버리는 아이러니에 빠져버렸죠.

게임의 로직을 생각해보면 딜러와 플레이어 간 동일한 동작은 결국 1. 게임 시작 시 두 장의 카드를 받는 것 2. 가지고 있는 카드의 점수 계산 2가지 뿐이었고, 그렇다면 이 두 가지 동작만 중복되지 않게 처리했다면 지금보다 덜 신경써도 되는 구조가 되지 않았을까 생각이 듭니다.

개발하면서도 가장 고민하던 부분이었는데, 역시나 잘 캐치하셔서 좋은 리뷰 남겨주신 점 감사합니다!😄

lgm1007 added 15 commits May 25, 2024 12:47

docs: 기능 요구사항 README 추가

ab35e08

feat: Card 관련 객체 생성

75f23cb

modify: Card 객체 record

1dd9777

feat: 게임 참가자 객체 추가 및 참가자의 현재 카드 점수 계산 기능 추가

efe67af

refactor: 카드 문양 : suit, 카드 숫자 : rank 네이밍 변경

414858b

feat: 게임 참가자 사용자 입력 받기

a291a12

modify: 카드덱 카드 뽑기 방법 수정

fa3d675

modify: 카드덱 카드 뽑기 방법 수정

0d620eb

feat: 참여자 리스트를 필드로 받는 GamePlayground (게임장) 객체 추가

eff7337

modify: Card 객체에 toString() 오버라이드 - 카드 종류 출력하기 위함

94efede

feat: GameController 동작 구성 - 게임의 전반적인 진행 과정 개발

778c323

feat: 게임 승패 판단 GameJudgement 추가 및 A카드 점수 계산 로직 수정

c4d9413

refactor: 카드 모델 클래스 패키지 이동

2d7cb6d

test: Output 출력 테스트 추가

a1efa10

test: Participant 모델 테스트코드 추가

b37874e

lgm1007 requested review from sc0116 and haero77 June 1, 2024 07:32

lgm1007 commented Jun 3, 2024

View reviewed changes

haero77 reviewed Jun 6, 2024

View reviewed changes

haero77 assigned lgm1007 Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[블랙잭] 레몽(이규명) 미션 제출합니다. #1

[블랙잭] 레몽(이규명) 미션 제출합니다. #1

lgm1007 commented Jun 1, 2024

lgm1007 Jun 3, 2024

haero77 left a comment •

edited

Loading

haero77 Jun 6, 2024

haero77 Jun 6, 2024

lgm1007 Jun 10, 2024

haero77 Jun 6, 2024

lgm1007 Jun 10, 2024

haero77 Jun 6, 2024

haero77 Jun 6, 2024

lgm1007 Jun 10, 2024

haero77 Jun 6, 2024

haero77 Jun 6, 2024

haero77 Jun 6, 2024

haero77 Jun 6, 2024

haero77 Jun 6, 2024

lgm1007 Jun 10, 2024

		private static final List<CardSuit> CARD_EMBLEMS = List.of(CardSuit.CLOVER, CardSuit.HEART, CardSuit.SPADE, CardSuit.DIAMOND);
		private static final List<CardRank> CARD_NUMBER = List.of(CardRank.ACE, CardRank.TWO, CardRank.THREE, CardRank.FOUR, CardRank.FIVE, CardRank.SIX, CardRank.SEVEN, CardRank.EIGHT, CardRank.NINE, CardRank.JACK, CardRank.QUEEN, CardRank.KING);

		@@ -0,0 +1,9 @@
		package card.validator;

		public class DrawCardInputValidator {

[블랙잭] 레몽(이규명) 미션 제출합니다. #1

Are you sure you want to change the base?

[블랙잭] 레몽(이규명) 미션 제출합니다. #1

Conversation

lgm1007 commented Jun 1, 2024

기능 요구사항

Choose a reason for hiding this comment

haero77 left a comment • 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

haero77 left a comment •

edited

Loading