Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[블랙잭 - 1단계] 아토(이혜린) 미션 제출합니다. #3

Open
wants to merge 68 commits into
base: main
Choose a base branch
from

Conversation

hyxrxn
Copy link
Member

@hyxrxn hyxrxn commented Mar 8, 2024

제이미 안녕하세요! BE 6기 아토입니다.
처음 예상보다 오래 걸렸던 미션이네요.
기능 구현은 하루 반 정도 걸린 것 같은데 기능들을 모으는 데 시간이 더 많이 든 것 같습니다.
미션 진행하며 신경 쓴 부분과 질문들 정리해보겠습니다!


미션 진행 시 생각한 부분

1. 요구사항에 명시되어 있지 않은 것들

  • 딜러는 16점을 초과할 때까지 카드를 계속 뽑습니다. 보통의 블랙잭 게임에서 그렇게 하기 때문에 이렇게 정했습니다.
  • 플레이어에게 카드를 더 받을지 물어본 후, 카드를 받지 않으면 카드 내역을 출력하지 않습니다. 실행결과 예시에는 혼용되어 있지만, 새로 뽑지 않으면 이전 내역에서 최종 카드를 알 수 있다고 생각해 출력하지 않기로 했습니다.
  • 플레이어명 뒤 , 출력은 은(는)으로 통일합니다. 출력부에서 이름별로 어떤 것이 붙어야 할지 판단하는 것이 크게 필요하지 않은 과정이라고 생각했기 때문입니다.

2. 점수 계산은 Hand가, 과정은...

  • 플레이어가 직접 본인의 점수를 보고 뽑을지 말지 정한다는 의미에서 플레이어의 Hand가 들고 있는 카드를 이용해 점수를 계산해줍니다. 그런데 이 때, 에이스가 점수 계산에 두 갈래 길을 만듭니다. 그리고 카드를 뽑을 때마다 에이스를 어떤 점수로 선택할지 정해야 합니다. 그런데 이 때, 에이스가 두 장 이상 있어도 한 장만 11점으로 선택될 수 있습니다. 그래서 에이스가 1로 계산되는 것이 minimumScore, 에이스가 있을 때 10을 더한 것이 maximumScore입니다. 그리고 maximumScore가 21을 넘기지 않으면 점수로 maximumScore를, 넘긴다면 점수로 minimumScore를 선택합니다. 그런데 이 부분은 게임의 최대 점수가 21점이 아닌 22점만 되더라도 바뀔 수 있는 부분이고, 약간 알고리즘적(답만 구하면 된다는 마인드...) 코드가 될 수 있다고 생각합니다. 그런데 에이스가 2장 이상 있을 경우 점수 선택의 경우의 수를 일일히 계산하게 변경하는 것보다 저희 계산 로직을 직접 설명하는 데 쓰이는 비용이 더 작다고 생각해 이런 선택을 했습니다. 약간 찜찜하긴 합니다만, 어쩔 수 없는 트레이드오프라고 생각했습니다. 이 부분에 대해 어떻게 생각하시나요?

3. 전체 덱 생성은 정적 메서드로! 그리고 여기서의 indent 문제

  • 카드는 Number(수)와 Shape(모양)으로 이루어져 있습니다. 이 수와 모양을 각각 Enum으로 선언해 각 순서쌍을 모두 가지는 전체 덱을 만들었습니다. 이 때 생성을 위해 정적 메서드를 사용했습니다. 모든 카드 순서쌍을 덱 생성자에서 만드는 것이 역할을 너무 많이 가진다고 생각했고, 사용되는 덱의 종류가 한 가지라 변경될 일이 없었기 때문입니다. 이 때 가장 처음 작성한 코드는 모양별 for문 안에서 수별 for문을 돌리며 해당 카드를 만들어 넣어주는 방식이었습니다. 그런데 여기서 indent가 2가 되며 요구사항을 지키지 못하게 되었어요. 그래서 모양별 for문 안에서 stream을 사용해보았지만 가독성이 현저히 떨어진다고 느꼈습니다. 이를 해결하기 위해 수별 for문 안에서 생성을 모양별로 4번 하는 방식도 생각해보았습니다. 하지만 이 또한 조금 웃긴 것 같아 고민하다가... 모양별 전체 카드를 생성하는 메서드로 추출해 이 이름을 열심히 정해보자는 결론을 냈습니다. 그런데 여전히 이중 for문의 가독성이 더 좋다고 느껴요. indent에 대한 제한을 요구사항에 두는 것이 가독성, 메서드 분리 이 두 가지의 이유가 있다고 생각하는데요, 이번 카드를 만드는 부분은 오히려 indent가 커져야 의미가 더 잘 보이는 부분인 것 같습니다. 만약 제한이 2였다면 그냥 이중 for문을 사용했을 거에요! 제이미의 생각은 어떤가요?

4. 출력에 넘기는 파라미터의 형식

  • 처음에는 int, String등의 간단한 형식만 넘겨야 한다고 생각했습니다. 도메인 객체 자체를 뷰에 넘기면 예상치 못한 문제가 생길 수 있다고 생각했기 때문입니다. 그런데 출력을 위해 코드를 작성하던 중, 카드의 출력 형식에 맞게 정보를 변환하는 것이 BlackJackGame의 역할이 아니라는 생각이 들었습니다. 당연히 출력 형식을 Card가 알고 있을 필요도 없구요. 그리고 Card는 필드로 Enum 두 개만을 가지고, 이를 변화시킬 방법이 없기 때문에 불변성이 유지된다고 보아 이를 직접 넘기고 뷰에서 변환하자는 결론을 냈습니다. 그래서 출력을 위한 디스플레이 Enum이 따로 존재합니다! 마찬가지로 플레이어의 결과로 출력될 , , 또한 모델이 알고 있을 필요 없는 출력이고, 이것도 뷰에서 변환하여 출력합니다. 혹시 다른 좋은 방법이 있었을까요?

5. 접근 제한자 설정

  • 이번 페어와는 접근 제한자에 신경을 많이 쓰려 했습니다. 이를 위해 패키지를 제대로 구분하려 했고, 결과적으로 Players만 외부에서 생성이 가능하고 나머지 Player, Hand 등은 패키지 밖에서는 생성이 불가합니다. 응집력을 높여 역할을 잘 나누고 접근을 잘 막았다는 생각에 굉장히 만족했는데, 저희가 잘 생각한 것인지 궁금합니다.

6. 전체를 하나로 묶는 패키지

  • 관례적으로 모든 코드가 포함된 상위 패키지가 있다는 것은 알고 있었으나, 이에 대한 이유는 잘 몰랐습니다. 실제로 앞의 미션에서는 최상위 패키지를 따로 두지 않았어요. 그런데 이번에 import문 순서를 신경쓰다 보니 전체 패키지로 묶어야 import를 보았을 때 java.~~~ 가 사이에 끼지 않고 맨 위나 맨 아래에 모일 수 있다는 것을 알게 되었어요! 그래서 상위 패키지를 만들었습니다.

7. TDD 중 생기는 테스트를 위한 생성자

  • 테스트를 먼저 하다 보니 테스트에만 사용되는, 카드를 강제로 들리는 생성자가 세 곳에서 등장하게 되었습니다. 물론 기능으로 존재하는데 게임 흐름 구현에서 사용되어 테스트를 놓쳐 TDD를 하지 못한 부분도 Player에서 하나 있기는 합니다. 하지만 만약 TDD를 했더라도 비슷한 방식으로 작성되었을 것 같아요. 예를 들어, Deckcard패키지에 있어 Player 테스트에서 이를 사용하게 되면 테스트가 복잡해지고, 생성자를 새로 만드는 것에 비해 불필요한 접근이 등장하기 때문입니다. 자연스러운 일인가요? 다른 방법이 있었을까요?

페어와 의도를 잘 담은 코드를 작성하기 위해 노력했습니다!
보시면서 알아보기 힘든 부분이나 잘 읽히지 않는 부분이 있으시면 무엇이든 편하게 질문이나 의견 주세요!
감사합니다 😊

hyxrxn and others added 30 commits March 5, 2024 14:43
Co-authored-by: donghoony <[email protected]>
- 기능을 추가하며 클래스 분리 (Player -> Hand)

Co-authored-by: donghoony <[email protected]>
- `Card`에 포함되는 모양과 수는 변하지 않고, 불변 객체이므로 그대로 전달

Co-authored-by: donghoony <[email protected]>
- `toList`는 변경 불가능한 리스트를 반환하므로 `unmodifiableList` 제거

Co-authored-by: donghoony <[email protected]>
Co-authored-by: donghoony <[email protected]>
Copy link

@HaiSeong HaiSeong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다들 컨트롤러를 잘짜시네요.
몇가지 의견 남겼습니다.

if (isBurst(playerScore)) {
return true;
}
return !isBurst(dealerScore) && dealerScore > playerScore;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

부정형에 && 조건이 있어 코드의 의미가 바로 파악되지 않습니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

더 고민해 보겠습니다.

Comment on lines +5 to +7
DEALER_WIN,
PLAYER_WIN,
TIE;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 WIN, LOSE 이런식으로 적은것 같은데 의미가 더 잘 전달되는 좋은이름이네요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

열심히 고민했답니다.

package blackjack.player;

import blackjack.card.Card;
import blackjack.game.BlackJackGame;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

game의 클래스들이 player의 클래스들을 의존하는데
player패키지에 있는 클래스들이 game의 상수를 사용하네요

이렇게되면 패키지 끼리 순환참조를 하고 있는것 인가요?
패키지 구조가 잘못 된것인지, 의도하신 구조인지 궁금합니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

상수 21을 한 곳으로 모아보려는 시도를 해보겠습니다.

Comment on lines 41 to 42
boolean isAce = cards.stream()
.anyMatch(Card::isAce);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변수명이 조금 애매한것 같습니다. "Hand에 Ace가 있는지" 라는 의미를 잘 표현했으면 합니다.
isAce는 에이스 인가? 이런 의미인가요? 저는 변수가 어떻게 사용되는지는 알겠는데 도메인 지식이 부족한 사람이 본다면 이 변수명 때문에 의도를 파악하지 못할것 같네요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasAce 메서드로 분리했습니다.

Comment on lines 8 to 12
public class Player {

private final Name name;
protected final Hand hand;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

protected final Hand hand protected를 사용하면 같은 패키지 내에서 hand를 사용할 수 있게 되는데 이게 괜찮은것인지 고민해보시면 좋겠습니다.

스크린샷 2024-03-09 오후 3 51 04

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasDrawbleScore를 오버라이딩 하며 생긴 문제인데 더 고민해보겠습니다.

Comment on lines 24 to 33
Deck deck = Deck.createShuffledFullDeck();
Dealer dealer = new Dealer();

Players players = createPlayers();
initGame(deck, dealer, players);
playersDrawMore(deck, players);
dealerDrawMore(deck, dealer);

showCardsWithScore(dealer, players);
showMatchResult(dealer, players);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@reddevilmidzy reddevilmidzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

블랙잭 미션 수고 많으셨습니다!

디스크립션에 남긴말 보고 질문이 생겨 추가로 질문 남겨요
저는 패키지는 언제든지 이동될 수 있다고 생각하여 디폴트 접근제한자를 지양해왔었는데, 아토는 어떻게 생각하시나요?

Comment on lines +6 to +7
1. 클래스 정의 다음 줄은 공백으로 한다.
2. test code에 사용하는 메서드는 `static import`한다.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

옹 이렇게 페어와 지킬 컨벤션을 작성해두는것도 좋은 거 같네요!!

@@ -0,0 +1,28 @@
package blackjack.card;

public enum Number {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실 ace, jack, queen, king 은 number라는 네이밍에는 어울리지 않는다고 생각하는데 아토는 어떤가요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 크루들의 코드를 보니 Number, Rank 중에 하나더라구요.
Rank가 좀 더 맞는 표현인 것 같기는 합니다!

Comment on lines 40 to 47
int score = calculateMinimumScore();
boolean isAce = cards.stream()
.anyMatch(Card::isAce);

if (isAce) {
return score + ADDITIONAL_ACE_SCORE;
}
return score;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

만약 ace 가 한장이 아니라 한장 이상 있다면 어떻게 될까요?

4, A, A, A, K 이렇게 있다면 4, 1, 1, 1, 10 이 됩니다!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요거 제가 착각했네요! 4,1,1,1,10 으로 잘 동작하네요👍

다만 cards 에서 isAce라고 네이밍 하는 것보다 cards에서 ace를 가지고 있다는 의미로 hasAce로 바꿔보는건 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasAce 메서드로 분리했습니다!

}

private void validateNameLength(String name) {
if (name.length() < MIN_NAME_LENGTH || name.length() > MAX_NAME_LENGTH) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

부등호를 사용할 때 방향을 맞춰주는 것이 읽기가 더 좋아요!

if (name.length() < MIN_NAME_LENGTH || MAX_NAME_LENGTH < name.length()) 

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요 의견 저번 리뷰어에게 들었었는데.. 저는 방향 맞추는 것은 && 일 때만 해당 값이 가운데로 모이기 때문에 읽기 좋다고 생각해서요!
레디는 name.length()가 양쪽 끝으로 가도 이게 더 읽기 편한가요?

Comment on lines +32 to +34
String getName() {
return name;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우옷 게터까지 디폴트로 제한하셨군요 디폴트로 만들어진 게터를 본적이 없어 신박하다는 생각이 들었습니다😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

페어와 이번에는 최대한 보수적으로 가보자는 결론을 냈습니닷....! ㅎㅎ
필요 없는 곳은 다 막아두었어요.

Copy link
Member Author

@hyxrxn hyxrxn Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

패키지가 이동된다면 그 때 접근제한자를 수정하면 된다고 생각해요!
실제로 이번에 Hand가 card에 있다가 player로 옮겨왔는데 그렇게 하며 접근제한자가 퍼블릭에서 디폴트가 되었답니다.

Comment on lines 31 to 33
public boolean hasDrawableScore() {
return hand.calculateScore() < BlackJackGame.BLACKJACK_MAX_SCORE;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 로직을 hand 안으로 집어넣는건 어떤가요? 그리고 calculateScore() 메서드처럼 player에선 호출만 하는 방식으로

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

드로우 가능한지 여부는 hand가 아니라 플레이어 / 딜러가 직접 결정한다는 의미로 이렇게 작성했는데...
일단은 21을 모으는 것이 목표라 이런 의견도 고려해서 수정을 해보아야겠어요!

Comment on lines 38 to 45
private void validateUniqueNames(List<String> playerNames) {
int distinctNameCount = (int) playerNames.stream()
.distinct()
.count();
if (distinctNameCount != playerNames.size()) {
throw new IllegalArgumentException("[ERROR] 이름은 중복될 수 없습니다.");
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (Set.copyOf(playerNames).size() != playerNames.size())

이런 식으로 사용한다면 한줄로 깔쌈하게 중복을 확인할 수 있습니다!

import blackjack.card.Number;
import java.util.Arrays;

public enum CardNumberDisplay {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 이렇게 분리하셨군요!
사실 고민했던 부분인데 재미있네요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

출력 형식은 뷰만 알 필요가 있다는 것을 열심히 구현하려 했습니다!


public class BlackJackGame {

public static final int BLACKJACK_MAX_SCORE = 21;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 상수를 꼭 필요한 곳으로 옮겨보는건 어떤가요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좀 더 고민해서 모아보도록 하겠습니다.

private final Map<String, MatchResult> results;

public MatchResults() {
this.results = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HashMap을 사용하시면 순서가 유지되지 않는다는 것을 알고 계신가요?!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

몰랐습니다!
그런데 지금 상황에선 큰 상관이 없네요!

Copy link
Member

@seokmyungham seokmyungham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 이미 리뷰달고 싶은 부분들을 로키와 레디가 잘 남겨주었네요 !!
미션 화이팅입니다! 고생하셨어용😄

Comment on lines 24 to 30
int minimumScore = calculateMinimumScore();
int maximumScore = calculateMaximumScore();

if (maximumScore > BlackJackGame.BLACKJACK_MAX_SCORE) {
return minimumScore;
}
return maximumScore;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

구현 방식에 정답은 없지만, 개선할 수 있는 여지가 명확히 보여서 리뷰 남겨드립니다!

현재 한 번에 할 수 있는 일을 두 번 반복하는 느낌이 듭니다.
지금 위 로직은 패의 최소 점수최대 점수를 각각 구해서 비교하고 있는 것 같습니다.

isAce라는 메서드를 이미 calculateMaximumScore()에서 사용하고 계신다면,
Ace의 점수를 미리 정해놓지 말고 카드의 점수를 계산 한 다음, 적절한 Ace 점수를 적용시키는 방식은 어떻게 생각하시는지 궁금합니다.

그런데 에이스가 2장 이상 있을 경우 점수 선택의 경우의 수를 일일히 계산하게 변경하는 것보다 저희 계산 로직을 직접 설명하는 데 쓰이는 비용이 더 작다고 생각해 이런 선택을 했습니다

이 부분은 전적으로 주관의 영역이기도 한데,
오히려 블랙잭 게임 룰을 아는 개발자라면 이렇게 계산하는 방식이 더 이해하기 힘들 수도 있을 것 같다 생각합니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에이스가 없다면 맥스스코어 따로 계산하지 않고 있더라도 이미 게산한 결과 이용할 수 있도록 바꿔보았습니다!


private void validateNameLength(String name) {
if (name.length() < MIN_NAME_LENGTH || name.length() > MAX_NAME_LENGTH) {
throw new IllegalArgumentException("[ERROR] 이름은 2글자 이상 5글자 이하여야 합니다.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이름의 길이를 상수로 지정하셨으니
오류 메시지에 상수를 사용하여, 상수 사용의 이점을 높이고 어떤 기준으로 실패했는지 메시지를 풍부화 해줄 수 있을 것 같습니다.

Comment on lines +57 to +59
public List<Player> getPlayers() {
return players;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일급 컬렉션이 현재 내부의 참조를 그대로 반환했는데, 이는 setter 를 허용한 것과 마찬가지라고 생각합니다.
방어적 복사를 활용하는 방법은 어떻게 생각하시나요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toList()를 사용하였습니다!


private void validateSize(List<String> playerNames) {
if (playerNames.size() < MIN_PLAYER_COUNT || playerNames.size() > MAX_PLAYER_COUNT) {
throw new IllegalArgumentException("[ERROR] 플레이어의 수는 1명 이상 10명 이하여야 합니다.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분도 위에서 정의한 상수를 활용할 수 있을 것 같습니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants