[블랙잭 게임 1단계] 리뷰 요청 드립니다. #1

chunghyeon-kimm · 2021-07-29T10:26:36Z

학습로그

1. StringUtils

StringUtils
- StringUtils 클래스에 String을 다루는 여러가지 편리한 메서드가 정의되어 있다

2. Collectors

Collectors
- Stream 최종연산 collect에서 사용하는 Collectors 클래스의 여러가지 메서드

3. Parameterized Test

Parameterized Test
- @ParameterizedTest로 선언
- @valuesource, @EnumSource, @MethodSource 등을 이용해서 인자를 주입한다
- 테스트코드의 중복을 제거할 수 있다

KIMSIYOUNG

안녕하세요 충현님.

처음 블랙잭 구현할 때 룰도 너무 어렵고, 구현도 너무 어려웠던 기억이 있는데 잘 구현해주셨네요!

코멘트 몇 가지 남겼으니 확인 부탁드립니다🙏

ps. 블랙잭에 원래 배팅 금액 입력하고 수익률 구하는거도 있었지 않나요? 요번 미션에 없었나용

KIMSIYOUNG · 2021-08-03T13:11:34Z

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

+import java.util.List;
+
+public class BlackJackController {
+    private final List<Player> players;


Controller를 Entry-point로써 사용하고 있는데, 현재와 같이 해당 값들을 이를 멤버변수로 가지게 되면 해당 게임을 여러명이 동시에 요청하는 경우 데이터 부정합이 발생할 수 있어요. 멤버변수를 지역변수로 가져가는 것이 좋아 보여요🙂

여러명이 동시에 요청하는 경우를 고려하지 않았네요. 서비스 개발자로서 항상 생각해야 할 부분인 것 같습니다.
view를 제외한 클래스변수들을 로컬변수로 수정했습니다.

KIMSIYOUNG · 2021-08-03T13:11:49Z

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

+    }
+
+    public void run() {
+        drawTowCards();


메소드 분리가 잘 되어 있어서, 가독성이 좋네요🙂

KIMSIYOUNG · 2021-08-03T13:12:31Z

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

+    }
+
+    private void drawCardToPlayer(Player player) {
+        DrawCardResponseDTO drawCardResponse = inputView.getPlayersResponse(player);


DTO도 자신이 가지고 있는 데이터에 대해서, 어떤 행위/판단을 할 수 있다고 생각해요. 입력받은 값이 Y/N인지 판단하는 로직은 DTO로 가도 된다고 생각합니다.

다만 해당 로직이 여러 곳에서 사용되거나, 이를 하나의 도메인 행위로 정의한다면 DTO -> Domain으로 전환한 다음 도메인에서 판단하는 것도 좋을 것 같아요. 현재는 해당 부분에 대한 도메인 로직이 Controller에 나와 있기에 원하시는 형태로 포장해주시면 좋을 것 같습니다🙂

추가로 네이밍은 팀바팀이고 사바사지만, 개인적으로 프로그램에게 넘어온 Input을 담는 DTO는 Request/우리 프로그램에서 반환해주는DTO는 Response로 사용하고 있어요. response로 작성해주신 맥락은 알겠으나, Client-Server가 항상 유동적으로 달라질 수 있다면 DTO가 많아졌을 때 헷갈릴 수 있다고 생각해요🙂 (추가로 DTO는 > Dto로 적고 있긴합니다 - 취향)

Dto 이름을 DrawCardRequestDto로 변경하고 판단로직을 Dto로 이동했습니다.
Dto가 가진 데이터만을 이용한 간단한 로직은 Dto에서 처리해도 된다는 것을 알게 되었습니다.

Dto 이름을 받아온 것인지, 반환하는 것인지에 따라 통일하면 일관성이 있어 헷갈릴 일이 없어서 좋은 방법인 것 같습니다.

KIMSIYOUNG · 2021-08-03T13:13:52Z

src/main/java/blackjack/utils/StringUtil.java

+                .collect(Collectors.toList());
+    }
+
+    public static String validateYesOrNo(String response) {


Utils성 클래스는 일반적으로 정말 유틸리티한 것들만 기능을 담는 것이 일반적이에요. 현재 유틸로직이 Y/N이라는 어떤 룰에 대해서 너무 많이 알고 있는 상태인 것 같습니다

도메인 혹은 DTO의 로직으로 변경하는 것이 좋아보이고, Util에서 사용하신다면 검증 대상이 되는 Y/N도 파라미터로 받는다면 조금 더 범용적으로 사용할 수 있는 메소드가 될 것 같아요.

로직을 DrawCardRequestDto로 이동했습니다.

KIMSIYOUNG · 2021-08-03T13:14:19Z

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

+    private final List<Card> deck;
+
+    public Deck() {
+        this.deck = new LinkedList<>(DeckFactory.createDeck());


LinkedList로 선언하신 이유는 카드를 한장씩 삭제할 때 ArrayList가 비효율적이라서 그렇게 하신걸까요? 👏

넵 그렇습니다.

KIMSIYOUNG · 2021-08-03T13:21:56Z

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

+    DEALER_HIGHER(((player, dealer) -> player.getCardsSum() < dealer.getCardsSum()), WinningResult.LOSE),
+
+    // 무승부 결정
+    TIES(((player, dealer) -> player.getCardsSum() == dealer.getCardsSum()), WinningResult.TIE);


요 로직들이 순서에 따라 영향을 받을 것 같아요. 원래는 플레이어가 버스트인 경우 항상 패배로 간주되는데 무승부 Enum을 맨 위로 올리면, 비긴거로 간주되는 형식으로여! 그렇다면 기존 Enum의 순서가 중요하다는 것을 명시적으로 드러내거나, 다른 방법을 사용해줘야 다른 사람이 실수할 여지를 줄일 수 있을 것 같아요ㅎㅎ(여긴 가장 중요한 로직이니까 더더욱이요!)

verificationOrder 라는 int 필드를 추가하고 RuleComparator클래스를 생성해서 비교하기 전에 enum을 정렬하도록 했습니다.

Rule enum 자체에서 comparable 인터페이스를 구현하고 compareTo 메서드를 오버라이딩 하려고 시도해봤는데
Enum은 상수를 선언한 순서대로 정렬되고, compareTo 오버라이딩이 불가능하다는 것을 알게 되었습니다.

https://stackoverflow.com/questions/519788/why-is-compareto-on-an-enum-final-in-java
참고했습니다.

KIMSIYOUNG · 2021-08-03T13:22:10Z

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

+    PLAYER_BUST(((player, dealer) -> player.isBust()), WinningResult.LOSE),
+    DEALER_BUST(((player, dealer) -> dealer.isBust()), WinningResult.WIN),
+
+    // 카드 값의 합이 21을 넘지 않으면서, 값이 더 큰쪽이 승패결정(이미 여기서 결정, BlackJack 체크 필요없음)


친절한 주석 좋네요. 해당 메소드는 participant에게 값을 비교하는 형태로 리팩토링할 수 있을 것 같아요

이 부분 이해를 못했습니다. 조금만 더 설명해주실 수 있을까요?

KIMSIYOUNG · 2021-08-03T13:22:20Z

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

+        this.winningResult = winningResult;
+    }
+
+    public Boolean compare(Player player, Dealer dealer) {


해당 메소드는 비교하는 메소드가 아니라, 어떤 Rule인지를 찾는거에 가깝네요. 요것도 메소드 이름 수정해주시면 좋을 것 같아요.

findMatchingRule로 수정했습니다.

KIMSIYOUNG · 2021-08-03T13:23:01Z

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

+        this.result = result;
+    }
+
+    public WinningResult reverse() {


요거 제가 전에 할 때, 안 됐던건데 WinningResult가 WinningResult 멤버변수를 가지고, 반대로 선언하면 안되더라구요. 왜 안되는지 바로 이해가시면 Enum을 잘 이해하고 계신 것 같습니다ㅋㅋㅋ

WIN("승", LOST) // 요런식으로여

WIN을 정의하기 위해 LOST가 필요하고, 다시 LOST를 정의하기 위해 WIN이 필요하고... 이런식으로 무한반복되기 때문에 안 되는 것일까요 ? ㅎㅎ

제가 Enum에 아직 익숙하지 않아서 간단하게 설명해주시면 감사하겠습니다.

네넹ㅎㅎㅎ이해하신게 맞습니다. 윈을 로드하는 시점에 로스트가 정의되어있지 않기 때문에 발생하는 에러입니다ㅎㅎ

KIMSIYOUNG · 2021-08-03T13:23:28Z

src/test/java/blackjack/domain/result/GameResultTest.java

+class GameResultTest {
+
+    @ParameterizedTest
+    @EnumSource(WinningResult.class)


EnumSource를 활용해주신 것은 좋은데, 요거보단 개인적으로 ArgumentProvider/CsvSource등을 통해, 카드들도 같이 명시해주는게 좋아보이는데, 어떠신가용?

Parameterized Test를 실행하는 다양한 방법들이 있다는 것을 알게 되었습니다.
말씀해주신 방법을 찾아봤는데 아직 어떻게 쓰는것인지 잘 모르겠습니다. 좀 더 찾아보고 구현해보겠습니다.

…Dto로 이동

… 이름 수정

chunghyeon-kimm · 2021-08-15T01:57:10Z

배팅 금액 관련된 부분은 2단계에서 구현하도록 되어있습니다. ㅎㅎ

chunghyeon-kimm and others added 30 commits July 25, 2021 12:55

docs: 구현 기능 목록 작성

cd61a5b

feat: Type, Denomination Enum으로 카드 객체 생성

9a8ad41

feat: Deck 객체, 테스트 작성

49385ff

feat: Player 클래스 작성

5850dd0

feat: Player가 카드 하나를 받는 기능 구현

f5e6914

refactor: 롬복 추가, 오타 수정

c43c521

feat: 덱에서 카드를 하나 뽑는 메서드 작성

55f99ee

feat: 카드의 숫자값을 Score 객체로 생성하고 Enum 클래스들에 롬복 적용

0d0b13c

refactor: 한장의 카드를 Deck에서 뽑아주는 기능 리팩토링

e9ed25c

feat: 딜러 객체 생성 및 카드합이 16이하일 때까지 카드를 한 장씩 뽑을 수 있는 로직 구현

2b3f5c5

docs: 객체 설계와 로직 구현에 대한 구상

a6c12c5

Implements missing components to a commit

b2198e8

feat: 게임종료 후 카드값의 합을 구하는 로직을 util성 클래스로 구현

5ec0118

feat: Participant 인터페이스 추가 및 게임 참여자가 가진 카드 덱 값의 합을 구하는 로직 구현

f0cbf79

refactor: Participant 추상클래스를 생성하여 Player와 Dealer가 각각 상속받도록 리팩토링

28c5a8d

feat: 입력받은 이름을 ,를 기준으로 잘라 Player 목록을 생성하는 로직 구현

ac18db0

feat: InputView를 생성하여 사용자 이름을 입력받는 기능 구현

b1e1a25

feat: Player에게 카드 한장을 더 받을지 묻는 기능 구현

7b7ed47

refactor: 누락된 사항 commit

8dab5e0

feat: Controller와 Application을 생성, 카드를 두 개씩 분배하는 기능 구현

f5829e0

docs: 설계, 로직 구현에 대한 메모

f0ec5f4

feat: OutputView 객체 생성, 게임에서 처음 카드를 두 개씩 분배하는 문구 출력 기능 구현

2cc96f3

feat: 딜러와 플레이어가 처음 분배받은 카드 목록의 출력 기능 구현

e99f18b

feat: 플레이어가 21을 넘지 않는 선에서 카드를 한장씩 받는 로직 구현

9bfe228

feat: 딜러 카드목록의 합이 17미만이면 카드를 추가로 한 장 받는 로직 구현

02ba420

refactor: 카드, 게임참가자, 결과로 package 분리

02b8932

feat: winningResult, Rule을 Enum으로 선언하여 TotalResult 객체를 생성해 저장하도록 구현

6765478

feat: 플레이어와 딜러의 승패를 판가름하여 플레이어 기준으로 승패를 발표하는 기능 구현

d7e50f4

feat: 플레이어와 딜러의 승패를 판가름하여, 딜러기준으로 승패를 발표하는 로직 구현 및 누락test 커밋

33dc3e5

feat: 게임 종료 후 모든 플레이어의 카드 목록, 카드값의 합 출력하는 로직 구현

56880bb

Ariel429 and others added 13 commits July 28, 2021 17:38

feat: 게임 종료 후 최종 결과를 출력하는 로직 구현

f3f0d12

docs: 누락된 커밋 추가

3260c5d

refactor: 카드 덱을 생성하는 클래스를 DeckFactory로 분리, 이중 depth 제거

b71d341

refactor: 이중 depth 제거 (do-while문 사용)

6f547a3

refactor: 사용자의 이름 입력에 대한 예외처리

1177903

refactor: 카드 덱에서 52장을 초과하는 카드를 뽑는 경우에 대한 예외처리

74e1443

feat: 사용자 이름에 대한 예외 처리

3aa80e6

feat: 사용자의 이름 입력(중복 이름이 입력된 경우)에 대한 예외처리

22f26dd

refactor: 플레이어가 카드를 한장 더 뽑겠다는 대답의 유효성 검사 로직 추가

68db2ce

refactor: 불필요한 주석 및 쓰이지 않은 import정리

aa1ab86

refactor: Controller의 drawCardToPlayer 메서드 수정

e5a2991

refactor: 문자열 상수처리

4eed0a8

refactor: 테스트 정리

b21caf0

KIMSIYOUNG requested changes Aug 3, 2021

View reviewed changes

chunghyeon-kimm added 12 commits August 9, 2021 21:32

refactor: Dto 클래스 이름 변경, DrawCard 입력이 yes인지 확인하는 메서드를 DrawCardRequest…

61696cb

…Dto로 이동

refactor: DrawCard 응답의 유효성 검사 메서드를 DrawCardRequestDto로 이동 및 테스트메서드 이동

b00b293

refactor: CardTest printCardTwoDiamond 메서드 수정

dc99787

refactor: Player클래스 toString 오버라이딩 삭제, outputView에 플레이어 게임결과 출력메서드 추가

8cd41f3

refactor: 메서드 이름 수정 (win -> resultVersusDealer)

634acab

refactor: Rule 메서드 이름 변경 (compare -> findMatchingRule)

3818a06

refactor: static block 추가

49d48e7

feat: Rule 비교 로직 추가

080bc70

refactor: 카드 뽑을 때 RandomNumber 생성하는 부분을 분리

4b3f64b

refactor: Controller 클래스변수를 지역변수로 변경

0aebc8c

refactor: get -> orElseThrow

bfe22bb

refactor: player와 dealer를 비교해서 게임 결과를 반환하는 로직을 Player에서 Rule로 옮기고 메서드…

13b9421

… 이름 수정

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[블랙잭 게임 1단계] 리뷰 요청 드립니다. #1

[블랙잭 게임 1단계] 리뷰 요청 드립니다. #1

chunghyeon-kimm commented Jul 29, 2021

KIMSIYOUNG left a comment

KIMSIYOUNG Aug 3, 2021

chunghyeon-kimm Aug 15, 2021

KIMSIYOUNG Aug 3, 2021

KIMSIYOUNG Aug 3, 2021

chunghyeon-kimm Aug 15, 2021

KIMSIYOUNG Aug 3, 2021

chunghyeon-kimm Aug 15, 2021

KIMSIYOUNG Aug 3, 2021

chunghyeon-kimm Aug 15, 2021

KIMSIYOUNG Aug 3, 2021

chunghyeon-kimm Aug 15, 2021

KIMSIYOUNG Aug 3, 2021

chunghyeon-kimm Aug 15, 2021

KIMSIYOUNG Aug 3, 2021

chunghyeon-kimm Aug 15, 2021

KIMSIYOUNG Aug 3, 2021

chunghyeon-kimm Aug 15, 2021

KIMSIYOUNG Aug 16, 2021

KIMSIYOUNG Aug 3, 2021

chunghyeon-kimm Aug 15, 2021

chunghyeon-kimm commented Aug 15, 2021

[블랙잭 게임 1단계] 리뷰 요청 드립니다. #1

Are you sure you want to change the base?

[블랙잭 게임 1단계] 리뷰 요청 드립니다. #1

Conversation

chunghyeon-kimm commented Jul 29, 2021

학습로그

1. StringUtils

2. Collectors

3. Parameterized Test

KIMSIYOUNG 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

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

chunghyeon-kimm commented Aug 15, 2021