[1단계 - 블랙잭 게임 실행] 채채(신채원) 미션 제출합니다. #410

chaewon121 · 2023-03-03T07:40:47Z

채채 (페어: 메리) 미션 제출합니다!

고민한 부분들

controller의 역할
블랙젝 게임은 계속해서 입력을 받고 그 입력으로 인해 값들이 변경됩니다.
BlackJackGame 이라는 클래스를 추가하여 컨트롤러의 역할을 도와주는 클래스를 추가하려고 하였는데
그렇게 하면 BlackJackGame은 서비스 패키지가 되는 것일까요? 컨트롤러의 역할에 대해 고민이 많이 되었습니다.
이부분에 대해 의견 주시면 감사하겠습니다!
자료 관리 부분에 대해
이번 미션에서 딜러의 결과, 플레이어들의 결과 이 두가지의 결과값을 갖게되어 두가지 결과값을 따로 관리를 하려고 했습니다. 그러나 고민을 해보니 이 두 결과값이 서로 깊게 연관이 되어있고 플레이어의 결과값이 딜러의 결과값에 영향을 준다는 것을 깨닫고 따로 관리하는 것은 비효율 적이지 않을까 고민하게 되었습니다. 그래서 플레이어의 결과값만 저장을 하고 딜러의 결과값을 계산하는 메소드만 추가를 하여 데이터 관리를 하였습니다..
결과값 관리에 대한 피드백 주시면 감사하겠습니다!

리뷰 잘부탁드립니다!

choijy1705

질문주신 내용에 대한 답변 및 몇가지 코멘트 남겼습니다!
확인부탁드려요
그리고 딜러의 결과값이 조금 이상하게 나옵니다.
이 부분도 체크해봐주시면 좋을것 같아요

choijy1705 · 2023-03-06T11:18:19Z

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

+import java.util.Collections;
+import java.util.List;
+
+public class Cards {


static 하게 사용하는것보다는 그냥 생성자로 생성할수 있도록 하는게 어떨까요?

우선 확장 가능성이 없을것 같아요. application하나당 오직 하나의 cards만 존재하는데 만약 서비스가 커져서 여러개의 게임방을 만든다고 하면 모두 바꿔야할것 같습니다

또한 언제 어디서든 card를 뽑아 쓴다는게 독이 될수 있을것 같은데요. 지금은 혼자 코드를 관리하지만 여러명에서 코드를 관리한다 생각해보면 어느 시점에 카드가 사용될지 예측할수 없을것 같습니다

static으로 관리하는 것은 상태가 전혀없는 함수들을 static으로 관리하면 좋을것 같아요. 예를 들면 자동차 게임에서 랜덤 숫자를 생성하는 로직 같은 util 기능들은 상태가 없기 때문에 언제 어디서든 호출하여도 문제가 없을것 같습니다.

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

choijy1705 · 2023-03-06T11:24:40Z

src/main/java/blackjack/game/RunningGame.java

+import java.util.List;
+import java.util.Map;
+
+public class RunningGame {


컨트롤러인거 같은데 명시적으로 Controller라는 이름으로 표현하면 좋을것 같습니다!

src/main/java/blackjack/game/BlackJackGame.java

choijy1705 · 2023-03-06T11:47:12Z

src/main/java/blackjack/game/RunningGame.java

+        Players players = blackJackGame.getPlayers();
+
+        OutputView.printReadyMessage(names);
+        printUserFirstCards(players.getPlayers(), dealer);


Suggested change

printUserFirstCards(players.getPlayers(), dealer);

printUserFirstCards(users);

같은 User일텐데 Users로 관리해보는 건 어떤가요?
Users에 getDealer, getPlayers, getUsers를 구현해서 사용하면 파라미터로는 항상 Users만 넘기면 될것 같습니다

플레이어들과 딜러를 넘기는 경우는 저 함수 하나만 존재하는데 users라는 클래스를 추가하는것이 크게 장점으로 느껴지지 않습니다. 데이터관리 면에서나 또다른 장점이 존재한다면 말씀해주시면 감사하겠습니다!

Users클래스를 새로 만들다기보다 players 클래스를 users로 관리하면 어떨지의 의견이었습니다.
파라미터의 숫자를 일단 줄일수 있을것 같네요!

choijy1705 · 2023-03-06T11:52:30Z

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

+
+import java.util.*;
+
+public class GameResult {


말씀주신대로 dealer와 player의 결과는 항상 연관된 결과값이라고 생각합니다. 근데 player와 dealer의 결과 타입이 달라서 쉽게 눈에 들어오지 않는 것 같아요. 결과는 항상 게임이 끝나고 나서 계산이 될것이고 계산된 결과값은 변하지 않을거에요.
굳이 key값이 Player, 결과값이 ResultMatcher일 필요는 없을것 같습니다.
Map<String, String> 으로 최종 결과를 저장해서 하나의 result 변수에 dealer와 player의 결과를 저장하면 어떨까요?

딜러의 경우 결과값 승 무 패 의 개수로 저장이 되어야 하는데 혹시 딜러의 결과값을 Map<String, String> 여기에 어떻게 저장하라는 말씀이신지 설명해주시면 감사하겠습니다! 필요한 결과값만 저장하는 좋은 방안을 제시해주셔서 감사합니다!

result.put("딜러", "1승 1패");
result.put("유저1", "승");
결과 계산을 다하고 요렇게 map에 담아주면 player, dealer를 동시에 관리할 수 있을것 같아서요

choijy1705 · 2023-03-06T11:57:21Z

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

+    public static void printPlayerCurrentCards(Player player) {
+        System.out.println(player.getPlayerName() + CARD_STRING + OutViewHelper.getUserCards(player));
+    }


도메인을 view에서 다알고 있네요

Suggested change

public static void printPlayerCurrentCards(Player player) {

System.out.println(player.getPlayerName() + CARD_STRING + OutViewHelper.getUserCards(player));

}

public static void printPlayerCurrentCards(String playerName, List<String> cardNames) {

System.out.println(playerName + CARD_STRING + String.join(",", cardNames));

}

지금은 문제가 없겠지만. 만약 개인정보가 있을때 도메인 자체를 view로 넘기게 되면 개발자도구를 통해 중요정보를 다 볼 수 있을것 같아요.

choijy1705 · 2023-03-06T11:59:29Z

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

+        CardSymbol symbol = CardSymbol.HEART;
+
+        // then
+        Assertions.assertThatNoException().isThrownBy(() -> new Card(number, symbol));


예외가 안던져지는지 체크하는것보다
생성한 card에 값이 잘들어갔는지를 체크해보면 좋을것 같아요
지금은 파라미터만 받고 필드에 저장하는 로직을 빼먹어도 통과할것 같아요

생성하는것 보다 값이 잘 들어갔는지 테스트 하는 것이 맞는 것 같네요 수정하겠습니다!

src/main/java/blackjack/view/InputView.java

choijy1705

안녕하세요 채채!
피드백 반영잘해주셨는데요👍
2단계 진행하시면서 남긴 코멘트들 반영해주시면 될것 같아요

choijy1705 · 2023-03-10T06:40:32Z

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

+    }
+
+    public boolean isAce() {
+        return cardNumber.equals(CardNumber.ACE);


Suggested change

return cardNumber.equals(CardNumber.ACE);

return cardNumber == CardNumber.ACE;

enum의 경우 equals도 가능하긴 하지만 ==이 더 안전할것 같아요. 여기에 대해서는 한번 생각해보면 좋을것 같아요

choijy1705 · 2023-03-10T06:44:20Z

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

+    public static CardNumber of(String inputNumber) {
+        return Arrays.stream(CardNumber.values())
+                .filter(cardNumber -> cardNumber.number.equals(inputNumber))
+                .findFirst()
+                .orElseThrow(() -> new IllegalArgumentException("찾을 수 없는 카드넘버입니다."));
+    }


테스트에서만 사용하는 코드인것 같아요

CardNumberTest에서 CardNumber.valueOf(value) 를 테스트 하고싶은거 같은데
굳이 모든 경우를 다 테스트해볼 필요가 있을까요?
하나만 해도 테스트가 충분히 되고 CardNumber.of(number)를 그냥 실제 CardNumber.ACE 처럼 확인해보면 어떤가요

choijy1705 · 2023-03-10T06:46:13Z

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

+    private static final int CARD_TOTAL_SIZE = 48;
+
+    private static Stack<Card> cards;
+    private static final Deck DECK = new Deck();


이전에 리뷰한거 같은데 static을 계속 쓰는 이유가 궁금합니다

choijy1705 · 2023-03-10T06:51:04Z

src/main/java/blackjack/domain/player/Hand.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class Hand {


Hand는 너무 뜬금없는 클래스명 같아요

일급컬렉션은 Collection이외에 어떠한 필드변수가 없는게 정의 아닌가요?
totalScore를 변수로 가지고 있으면 일급 컬렉션 위반일것 같습니다

choijy1705 · 2023-03-10T06:57:03Z

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

+
+import java.util.*;
+
+public class GameResult {


result.put("딜러", "1승 1패");
result.put("유저1", "승");
결과 계산을 다하고 요렇게 map에 담아주면 player, dealer를 동시에 관리할 수 있을것 같아서요

choijy1705 · 2023-03-10T07:01:14Z

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

+    @ParameterizedTest(name = "input : {0}")
+    @CsvSource(value = {"A:ACE", "2:TWO", "3:THREE", "4:FOUR", "5:FIVE", "6:SIX", "7:SEVEN", "8:EIGHT", "9:NINE", "K:KING", "Q:QUEEN", "J:JACK"}, delimiter = ':')
+    @DisplayName("of()는 카드넘버를 문자로 받아 값을 반환해 준다.")
+    void get_value_of_card_number_test(String number, String value) {
+        // given & when
+        CardNumber expected = CardNumber.valueOf(value);
+
+        // then
+        Assertions.assertThat(CardNumber.of(number)).isEqualTo(expected);
+    }


위에서 언급드렸지만
테스트를 위한 코드를 만들어서 까지 parameterizedTest를 굳이 할 필요없을것 같습니다.
parameterizedTest는 테스트의 시간이 길어지게 하는 주요 원인이 될 수 잇을것 같아요

Suggested change

@ParameterizedTest(name = "input : {0}")

@CsvSource(value = {"A:ACE", "2:TWO", "3:THREE", "4:FOUR", "5:FIVE", "6:SIX", "7:SEVEN", "8:EIGHT", "9:NINE", "K:KING", "Q:QUEEN", "J:JACK"}, delimiter = ':')

@DisplayName("of()는 카드넘버를 문자로 받아 값을 반환해 준다.")

void get_value_of_card_number_test(String number, String value) {

// given & when

CardNumber expected = CardNumber.valueOf(value);

// then

Assertions.assertThat(CardNumber.of(number)).isEqualTo(expected);

}

@DisplayName("of()는 카드넘버를 문자로 받아 값을 반환해 준다.")

@Test

void get_value_of_card_number_test(String number, String value) {

// given

String value = "ACE";

// when

CardNumber expected = CardNumber.valueOf(value);

// then

Assertions.assertThat(CardNumber.ACE).isEqualTo(expected);

}

choijy1705 · 2023-03-10T07:02:10Z

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

+    @ParameterizedTest(name = "input : {0}")
+    @CsvSource(value = {"하트:HEART", "다이아몬드:DIAMOND", "스페이드:SPADE", "클로버:CLOVER"}, delimiter = ':')
+    @DisplayName("of()는 심볼 이름를 받아 해당 enum 을 반환한다.")
+    void test_(String symbolName, String symbol) {


마찬가지로 고려해주세요

choijy1705 · 2023-03-10T07:06:59Z

src/main/java/blackjack/domain/player/Hand.java

+    public int getTotalScore() {
+        return totalScore.getTotalScore();
+    }


Suggested change

public int getTotalScore() {

return totalScore.getTotalScore();

}

public int getTotalScore() {

int newTotalScore = 0;

for (Card card : playerCards) {

newTotalScore += card.getCardNumber().getValue();

}

return updateAceScore(newTotalScore);

}

요렇게만 있으면 totalScore에 관한 모든 로직은 필요없지 않나요?

choijy1705 · 2023-03-10T07:11:56Z

src/main/java/blackjack/game/BlackJackGame.java

+        if (cards.size() < MIN_CARD_SIZE) {
+            throw new IndexOutOfBoundsException("뽑을 수 있는 카드가 없습니다.");
+        }
+        return cards.remove();


stack 은 vector 를 상속한 클래스인데요
vector가 성능저하의 주요 원인중 하나라서 vector사용을 권장하지 않습니다. 공식 문서상에서도요.
이를 구현한 stack도 차라리 deque를 써라고 권장하고 있습니다.
여유가 된다면 직접 클래스를 들어가보면 적혀있는 주석을 읽어보시면 좋을것 같아요

chaewon121 added 30 commits February 28, 2023 15:47

docs: 기능 구현 목록 작성

baf0c6b

docs: 기능 구현 목록 수정

c0bb682

feat: 카드 생성자 구현

821ac81

feat: 카드 생성 메서드 구현

355ec44

feat: 카드 넘버 value 반환 메서드 구현

20c1487

feat: 카드 Symbol 이름 반환 메서드 구현

24a228e

refactor: 카드 생성자 자료형 변경

6d4ed3a

feat: 카드 팩토리 구현

fe9860d

refactor: 카드 팩토리 함수 리팩터링

1f35d3a

feat: 생성한 카드 저장 및 개수 유효성 검사

238a877

refactor: 이중 for문 리팩토링

e66a393

feat: 카드 뽑기 메소드 구현

08532a1

feat: 카드 번호, 심볼 getter 추가

14fddc7

refactor: 카드 패키지 생성

2b58d1a

feat: 플레이어가 카드를 받을 때마다 점수 계산 및 업데이트 기능 구현

7ea72fc

refactor: 카드 생성 시 개수 유효성 검사 이동

7c50bb2

feat: 카드 점수 계산 구현

a25f133

refactor: 점수 계산 리팩토링

6020e4b

feat: 유저, 플레이어, 딜러 클래스 생성

0067714

feat: 유저, 딜러 점수 확인 구현

7072eb2

feat: 두장의 카드를 나눠주는 기능 구현

2dcae00

refactor: 유저 추상클래스, 딜러 리팩토링

61fe3ae

refactor: 유저 추상클래스, 딜러 리팩토링

67f562d

feat: 게터 추가

341c024

fix: 카드 점수 계산 오류 수정

ccecd4e

feat: 이름 입력, 게임 command 입력 기능 구현

3deab13

feat: 플레이어 클래스 구현

5fb76f4

feat: 과정 출력 및 컨트롤러 구현

f3b151f

refactor: 파일 이동

6ac9f1e

feat: 결과 계산 구현

99a398a

chaewon121 added 19 commits March 5, 2023 23:16

fix: 게터 수정으로 인해 발생한 오류 수정

064eb9f

refactor: 반대 결과를 반환해 주는 메소드 추가

41a721a

refactor: 클래스 명 수정으로 인한 변경

7c8b6da

refactor: stream 수정

7bc04e0

style: 함수 위치 이동

b6512f7

refactor: OutView 클래스 책임 분리

cf6aa3c

refactor: 클래스 이름 수정

6a01028

style: 불필요한 주석 제거

9628065

fix: 예외처리 오류 수정

b19eb8d

test: Command 클래스 테스트 추가

2db7f02

refactor: 카드 생성 방법 수정

9954aab

refactor: 생성자를 한번만 만들 수 있도록 생성자 막는 매소드 추가

493e4da

refactor: 카드 리스트 수정

bbe8965

refactor: 자식 인자 수정

2d5c8b9

refactor: 점수 업데이트와 카드 더하는 메소드 책임 분리

5155696

refactor: 자료구조 수정

d6d1564

refactor: TotalScore 클래스 생성

3ef9273

fix: 오류 수정

2cb4fdb

refactor: 싱글톤 적용하여 카드를 한번만 생성하도록 수정

64d7703

choijy1705 requested changes Mar 6, 2023

View reviewed changes

chaewon121 added 8 commits March 10, 2023 12:30

refactor:이름 변경 및 draw기능 추가

4532c79

refactor: 예외처리 수정 및 command 클래스 삭제

6aef4e7

test: 테스트 과정 수정

0d72e39

refactor: 클래스 이름 수정

b08f5f6

refactor: 인자 class -> string으로 수정

4ddea22

refactor: final 추가

3d07622

refactor: 예외처리 이동

7bbb7f2

refactor: 서비스단 컨트롤러로 이동

9133370

choijy1705 approved these changes Mar 10, 2023

View reviewed changes

choijy1705 merged commit 91fcf3f into woowacourse:chaewon121 Mar 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1단계 - 블랙잭 게임 실행] 채채(신채원) 미션 제출합니다. #410

[1단계 - 블랙잭 게임 실행] 채채(신채원) 미션 제출합니다. #410

chaewon121 commented Mar 3, 2023 •

edited

Loading

choijy1705 left a comment

choijy1705 Mar 6, 2023

choijy1705 Mar 6, 2023

choijy1705 Mar 6, 2023

chaewon121 Mar 10, 2023

choijy1705 Mar 10, 2023

choijy1705 Mar 6, 2023 •

edited

Loading

chaewon121 Mar 7, 2023

choijy1705 Mar 10, 2023

choijy1705 Mar 6, 2023

choijy1705 Mar 6, 2023

chaewon121 Mar 9, 2023

choijy1705 left a comment

choijy1705 Mar 10, 2023

choijy1705 Mar 10, 2023

choijy1705 Mar 10, 2023

choijy1705 Mar 10, 2023 •

edited

Loading

choijy1705 Mar 10, 2023

choijy1705 Mar 10, 2023

choijy1705 Mar 10, 2023

choijy1705 Mar 10, 2023

choijy1705 Mar 10, 2023

	printUserFirstCards(players.getPlayers(), dealer);
	printUserFirstCards(users);

	return cardNumber.equals(CardNumber.ACE);
	return cardNumber == CardNumber.ACE;

-    public int getTotalScore() {
-        return totalScore.getTotalScore();
-    }
+    public int getTotalScore() {
+        int newTotalScore = 0;
+        for (Card card : playerCards) {
+            newTotalScore += card.getCardNumber().getValue();
+        }
+        return updateAceScore(newTotalScore);
+    }

[1단계 - 블랙잭 게임 실행] 채채(신채원) 미션 제출합니다. #410

[1단계 - 블랙잭 게임 실행] 채채(신채원) 미션 제출합니다. #410

Conversation

chaewon121 commented Mar 3, 2023 • edited Loading

choijy1705 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

choijy1705 Mar 6, 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

choijy1705 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

choijy1705 Mar 10, 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

chaewon121 commented Mar 3, 2023 •

edited

Loading

choijy1705 Mar 6, 2023 •

edited

Loading

choijy1705 Mar 10, 2023 •

edited

Loading