[1단계 - 블랙잭] 기론(김규철) 미션 제출합니다. #226

Gyuchool · 2022-03-11T04:28:04Z

안녕하세요 또링!
우테코 4기 기론이라고 합니다! 😁
이번 미션을 진행하면서 궁금한 점이 생겨서 같이 질문드립니다.

질문

CardFactory에서 테스트를 진행할 때, 한 번 뽑은 카드는 카드 뭉치들 속에서 사라져야 한다는 것을 확인하기 위해 CARD_CACHE의 접근 지정자를 default로 지정해서 확인했습니다.
사실 실제 서비스에서는 카드 뭉치를 확인할 필요가 없어서 private으로 하는 게 좋아 보였는데 이렇게 되면 테스트를 위한 get()매서드를 만드는 방법으로 테스트를 해야 해서 불필요한 매서드가 생성되어서 더 안좋다고 생각헀습니다.
이에 대해 또링은 어떻게 하면 좋을지 의견이 궁금합니다!
Dto를 생성하는 중 dto안에 다른 dto가 들어있을때가 발생하더라구요.

//dto안에 dto구조
public class ResultDto{
    UserDto userDto;
    int score;
}
public class UserDto {
    private String name;
    private List<CardDto> cards;
}
// dto안을 원시값으로만 갖는 구조
public class ResultDto{
     private String name;
    private List<String> cards;
    int score;
}

이런 구조는 dto안에서 다시 dto로 감싸야해서 dto가 무거워진다고 생각해서 안 좋다고 생각했는데,
dto에 dto를 안넣고 원시값으로 넣으면 dto을 만들 때 필요한 인자가 많아지면서 덜 클린한 코드가 된다고 생각되었습니다.
또링은 dto안에 dto존재에 대해서 어떻게 생각하시는지 궁금합니다!

dealer와 player들 관계

controller내에 매서드들에서 dealer와 List<Player>를 인자로 갖는 부분이 많은데 이 부분을 dealer,List<Player>를 상태로 갖는 도메인을 만들어서 관리를 할지 아니면
현재처럼 controller에 코드는 좀 많지만 대부분 단순 반복문을 돌면서 결과들을 담는 형태여서 그대로 둬도 괜찮을 지에 대해서 고민이 있었습니다.
도메인을 만들면 뭔가 도메인을 관리하는 도메인(?)느낌이어서 도메인 관리하는건 service같은 곳에서 해야하지 않나?라는 생각을 하면서 도메인의 의미가 있는지 생각이 들었습니다.
또링은 도메인을 만들어 관리하는게 좋다고 생각하시나요?

아직 리펙토링할게 많아 보이는데 피드백 받으면서 빠르게 고쳐나가보겠습니다!
블랙잭 미션 리뷰 잘 부탁드립니다!
감사합니다🙏

jnsorn

안녕하세요 기론! 피드백 반영해주신 부분 확인했어요.
추가로 개선되면 좋을 부분에 대해 리뷰 남겼으니 확인해주시고, 궁금하거나 얘기 나누고 싶은 내용은 언제든 DM주세요!

src/main/java/BlackJack/domain/Card/CardFactory.java

src/main/java/BlackJack/controller/BlackjackController.java

src/main/java/BlackJack/domain/User/User.java

jnsorn · 2022-03-14T16:44:39Z

src/main/java/BlackJack/domain/Result.java

@@ -0,0 +1,17 @@
+package BlackJack.domain;
+
+public enum Result {


결과를 구하는 로직이 여기 저기 흩어져 있어요. 결과와 관련된 로직을 Result에서 관리해볼까요?

src/test/java/BlackJack/domain/Card/CardFactoryTest.java

jnsorn · 2022-03-14T16:56:22Z

src/test/java/BlackJack/domain/utils/FixedCardFactory.java

+
+import java.util.List;
+
+public class FixedCardFactory extends CardFactory {


FixedCardFactory라는 클래스가 필요한 이유가 무엇일까요? Dealer 생성이 어렵다면 왜 테스트하기 어려운지 고민해보는 것도 좋을 것 같습니다. 과연 Dealer가 CardFactory를 받아 생성되어야 할까요?

처음에 Dealer(Cards cards)로 Controller에서 cardFactory.initCards()로 인자값에 넣어서 처리하려고 했는데,
뒤에 같이 User를 상속한 Player들을 생성할 땐 CardFactory를 인자로 받아서 처리하므로 Dealer도 생성자 형식을 맞춰주는게 통일성있지 않을까 생각해서 Dealer에게도 CardFactory를 인자로 받게 했었습니다.

쉽게 테스트하기 위해서

통일성과 상관없이 Cards를 인자로 받도록 해도 될까요? 아니면

public Dealer(Cards cards)와 같은 생성자를 하나 더 만들어도 괜찮을까요?

혹시 의도하신 방향이 다른 거라면 힌트 조금 받을 수 있을까요..😢

우선, 현재는 Cards를 인자로 받아 FixedCardFactory 없이 테스트 가능해졌네요.
제가 드린 리뷰의 의도를 설명드리자면 아래와 같습니다. 이 부분은 고민해보시고 2단계 때 다시 얘기 나눠도 좋을 것 같네요.

Q1. Dealer가 CardFactory를 받아 생성되어야 할까요?

A1. 코드를 생각하지 않고 객체들의 그림을 그려보면 좋을 것 같아요.

딜러와 플레이어가 게임을 시작한다. 초기 카드를 나누어준다. 플레이어들이 조건에 따라 카드를 뽑는다.(hit) ...

이렇게 생각했을 때 딜러나, 플레이어가 생성될 때 CardFactory가 필요할지에 대해 질문드렸어요. (물론 기론의 생각이 다를 수 있는데, 만약 다르다면 기론의 생각을 공유해주시면 좋을 것 같아요.)

Q2. 왜 테스트하기 어려운지 고민해보는 것도 좋을 것 같습니다.

A2. 그럼에도 결국 카드를 초기화 할 때 CardFactory가 필요해지고, FixedCardFactory가 나올 수 있을 것 같은데요. 여기서 왜 테스트하기 어려울까에 대해 생각해보면 좋을 것 같아요. (Hint. 로또 미션의 자동로또생성 테스트가 어려웠던 이유)

A1
플레이어 1인칭으로 보면 CardFactory를 아는 것은 불필요하겠네요. 플레이어는 그냥 카드를 받을 뿐이니깐요!
저는 처음 생각했을 때, CardFactory가 deck의 역할이어서 플레이어가 덱에서 카드를 가져온다고 생각해서 플레이어가 cardFactory를 안다고 생각하고 코드를 작성했는데, 이렇게 결합되버리니 테스트하기도 힘들고 더 복잡해진 것 같습니다..

A2
제가 로또 미션 때, 자동로또 생성할때는 LottoGenerator와 같은 인터페이스를 이용해서 자동로또, 수동로또 생성할 때, 전략 패턴을 통해서 구현했었습니다.
그런데 이번 미션은 카드 드로우를 인터페이스로 만들어서 사용하기엔 재사용성도 떨어져서 굳이 로또했을때 처럼 만들 필요가 없겠네요.

최대한 생성자에서 주입받을 때, 정해진 값들을 받아야 테스트하기에 편할 것 같습니다.

src/main/java/BlackJack/domain/Card/Cards.java

src/main/java/BlackJack/domain/User/User.java

This reverts commit bee8566.

jnsorn · 2022-03-16T00:47:28Z

src/main/java/blackJack/domain/Card/Card.java

+    public static Card valueOf(Shape shape, Number number) {
+        return CACHE.stream().filter(card -> card.isSameShape(shape) && card.isSameNumber(number))
+                .findFirst()
+                .orElseThrow(() -> new IllegalArgumentException(EMPTY_CARD));
+    }


Map을 이용하면 꺼내기 더 쉽지 않을까요?

jnsorn · 2022-03-16T00:53:36Z

src/main/java/blackJack/domain/Card/CardFactory.java

+    public Cards initCards() {
+        List<Card> cards = IntStream.range(0, INIT_CARD_SIZE)
+                .mapToObj(i -> drawOneCard())
+                .collect(Collectors.toList());
+
+        return new Cards(cards);
+    }


기론은 for문과 IntStream.range()를 어떤 기준으로 사용하시나요?
(참고) for-loop 를 Stream.forEach() 로 바꾸지 말아야 할 3가지 이유

저는 collection을 구할때 stream을 유용하게 사용해서 웬만한 반복문은 통일성 있게 stream으로 바꾸려고 했었습니다! 또는 코드 길이가 길어지면 코드의 길이를 줄이기 또는 indent를 줄이기 위해 사용하기도 했습니다!

forEach는 System.out.println와 같이 단순한 출력문일때만 사용하는게 좋다고 알고 있었는데 몇몇 군데 그냥 무심코 사용했던 곳이 보이네요😭
그리고 IntStream.range()는 Stream.forEach()와 별개인줄 알았는데 같이 성능상 문제가 있었군요!

jnsorn · 2022-03-16T00:55:33Z

src/main/java/blackJack/domain/Card/CardFactory.java

+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+public class CardFactory {


CardFactory보다는 Deck의 일급컬렉션처럼 보이는데, 기론은 어떻게 생각하시나요?

그러네요.. 객체 생성보다는 카드들을 관리하고 분배하니 일급컬렉션에 가까워보입니다.
사실상 deck의 역할을 하고 있으니 아예 네이밍을 deck으로 수정하겠습니다!

src/main/java/blackJack/domain/Card/Card.java

src/main/java/blackJack/domain/Card/Cards.java

jnsorn · 2022-03-16T01:45:13Z

src/main/java/blackJack/dto/DealerResultDto.java

+    public static DealerResultDto from(Map<String, String> result) {
+        int winCount = 0;
+        int loseCount = 0;
+        for (String value : result.values()) {
+            winCount = getWinCount(winCount, value);
+            loseCount = getLoseCount(loseCount, value);
+        }
+        int drawCount = result.size() - (winCount + loseCount);
+        return new DealerResultDto(winCount, drawCount, loseCount);
+    }


딜러의 결과를 구하는 건 DTO의 역할일까요?

DTO는 담는 그릇의 역할만 해야 하는데 너무 안일하게 생각했었네요 😭

src/main/java/blackJack/domain/User/User.java

jnsorn · 2022-03-16T01:47:55Z

src/main/java/blackJack/domain/Card/Cards.java

+    public boolean add(Card card) {
+        return cards.add(card);
+    }


메서드가 한 가지 일만 하고 있나요?

코드를 줄이려고 욕심부리다 보니 자꾸 퇴보하는거 같네요...😂
수정했습니다!

jnsorn · 2022-03-16T01:54:05Z

src/main/java/BlackJack/controller/BlackjackController.java

+    public void run() {
+        List<String> inputPlayerNames = InputView.inputPlayerNames();
+        Dealer dealer = new Dealer(CardFactory.drawTwoCards());
+        List<Player> players = joinGame(inputPlayerNames);
+        OutputView.printDrawMessage(inputPlayerNames);
+        OutputView.printTotalUserCards(convertToListDto(dealer, players));
+
+        OutputView.printTotalResult(playGame(dealer, players));
+
+        List<PlayerResultDto> resultDtos = calculatePlayerResult(dealer, players);
+        DealerResultDto dealerDto = calculateDealerResult(dealer, players);
+        OutputView.printFinalResult(resultDtos, dealerDto);
+    }


Player에게 카드를 받을 수 있는 상태인지 물어볼 수 있을 것 같네요 😃

src/main/java/blackJack/controller/BlackjackController.java

jnsorn · 2022-03-16T01:55:57Z

src/main/java/BlackJack/domain/Card/CardFactory.java

+    static List<Card> CARD_CACHE = new ArrayList<>();
+
+    static {
+        Arrays.stream(Shape.values())
+                .forEach(shape -> Arrays.stream(Number.values())
+                        .forEach(number -> CARD_CACHE.add(new Card(shape, number))));
+        Collections.shuffle(CARD_CACHE);
+
+    }


Deck을 static을 통해 공유한다면 여러 게임이 진행될 때 Deck의 상태를 보장할 수 없어요. 또 말씀해주신 생성 비용은 (구현하신 것처럼) 카드 캐싱을 통해 줄일 수 있을 것 같네요.

jnsorn

안녕하세요, 기론! 리뷰 반영하느라 고생하셨어요. 몇 가지 코멘트 남겼는데, 이건 2단계 미션 구현하면서 같이 고민해주세요. 😃
추가로 처음 본문에 남겨주신 질문에서 여전히 답이 나오지 않은 질문이 있다면, 2단계 리뷰 요청할 때 같이 공유해주시구요!
2단계에서 만나요~ 👋

jnsorn · 2022-03-16T01:57:53Z

src/main/java/blackJack/domain/Card/Card.java

+    static {
+        Arrays.stream(Shape.values())
+                .forEach(shape -> Arrays.stream(Number.values())
+                        .map(number -> new Card(shape, number))
+                        .forEach(CACHE::add));
+    }


추가로 가독성을 위해 메서드로 분리해봐요 :) #226 (comment)

이 부분에 대한 코멘트였습니다 :)

jnsorn · 2022-03-16T02:05:39Z

src/main/java/blackJack/domain/PlayerResult.java

+        if (isLose(dealer, player)) {
+            return PlayerResult.WIN;
+        }


isLose가 true면 Win을 반환하네요. 이 네이밍은 누구 기준일까요?

명확한 이름이 필요하겠군요!

jnsorn · 2022-03-16T02:38:33Z

src/test/java/BlackJack/domain/utils/FixedCardFactory.java

+
+import java.util.List;
+
+public class FixedCardFactory extends CardFactory {


우선, 현재는 Cards를 인자로 받아 FixedCardFactory 없이 테스트 가능해졌네요.
제가 드린 리뷰의 의도를 설명드리자면 아래와 같습니다. 이 부분은 고민해보시고 2단계 때 다시 얘기 나눠도 좋을 것 같네요.

Q1. Dealer가 CardFactory를 받아 생성되어야 할까요?

A1. 코드를 생각하지 않고 객체들의 그림을 그려보면 좋을 것 같아요.

딜러와 플레이어가 게임을 시작한다. 초기 카드를 나누어준다. 플레이어들이 조건에 따라 카드를 뽑는다.(hit) ...

이렇게 생각했을 때 딜러나, 플레이어가 생성될 때 CardFactory가 필요할지에 대해 질문드렸어요. (물론 기론의 생각이 다를 수 있는데, 만약 다르다면 기론의 생각을 공유해주시면 좋을 것 같아요.)

Q2. 왜 테스트하기 어려운지 고민해보는 것도 좋을 것 같습니다.

A2. 그럼에도 결국 카드를 초기화 할 때 CardFactory가 필요해지고, FixedCardFactory가 나올 수 있을 것 같은데요. 여기서 왜 테스트하기 어려울까에 대해 생각해보면 좋을 것 같아요. (Hint. 로또 미션의 자동로또생성 테스트가 어려웠던 이유)

Juhyung990122 added 30 commits March 8, 2022 20:15

test: 테스트 작성

5aa725d

feat: 추상클래스 Car 생성

67807ba

feat: 종류별 자동차 도메인 구현

ee1a9fe

feat: 연료 계산 및 주입 기능 구현

b28bbe9

docs: 기능 요구 사항 작성

7161dc5

feat: 참가자 이름 입력 기능 구현

2ffc15c

feat: 유저 도메인 생성

6592e9d

feat: 카드, 카드팩토리 도메인 생성 및 캐싱

6f32dec

feat: Player,Dealer 도메인 생성 및 Dto변환 구현

ec380dd

feat: 카드 넘버에 이름 추가

65ea3ec

feat: CardDto 변환로직 구현 및 UserDto 리턴타입 수정

807fbf7

feat: 딜러와 참가자 카드 출력 기능 구현

898beda

feat: player 생성 및 카드 분배 기능 구현

bd394f5

feat: 딜러 카드 공개 로직 추가

f7f5050

feat: 한장의 카드 받는 기능 구현

66f520b

refactor: 뷰의 책임을 컨트롤러로 이전

ea9fa87

test: 카드분배기능 테스트 작성

ac35b89

feat: 카드 추가 기능 구현

05827d6

feat: 카드 추가 질의 기능 구현

8750ed4

feat: 플레이어 소지 카드 출력 구현

0542747

chore: 안내메세지 추가

002889e

feat: 카드 리스트를 일급컬렉션으로 변환

bf87a2e

feat: 카드 점수 계산 기능 구현

b39dc59

refactor: 접근제어자 수정

e4e796a

test: Cards 테스트 추가

a64fdc1

feat: 딜러 카드 추가 조건 구현

3557611

feat: 최종 결과 출력 기능 구현

f58dd8f

refactor: 미사용 코드 삭제

07a3b75

feat: 최종 승패 출력 기능 구현

8c7ee51

feat: 결과 DTO 생성

e76aa89

Gyuchool added 5 commits March 14, 2022 17:40

refactor: dealer도 cardFactory를 통해서 카드 받도록 수정

2d334d3

refactor: y와 n 매직 넘버 제거

2e80acc

refactor: dealer카드를 더 받는지 확인하는 매서드 네이밍 수정

83b7f5b

refactor: get함수 제거

5d622a5

test: 통계 테스트 추가

87a951e

jnsorn requested changes Mar 14, 2022

View reviewed changes

Gyuchool added 19 commits March 15, 2022 16:22

refactor: 디렉토리 이름 변경 및 카드에 캐싱 적용

4022d7d

refactor: drawOneCard를 통한 initCards매서드 수정

79540cd

refactor: 매서드 분리 및 매서드 메서드 네이밍 수정

1268c7b

refactor: bustLine검증 책임 cards로 이전

08781bb

refactor: 우승자 결정 로직 PlayerResult로 옮김

3b9a5e5

fix: 블랙잭 규칙에 맞게 수정

51f79ec

refactor: hit일때 boolean값을 반환하도록 수정

178d91b

refactor: 네이밍 수정

2eb9509

refactor: dealer Cards를 받아 생성하도록 수정

e8c5cc1

test: UserTest 추가

dc38f44

refactor: 딜러 승패무 횟수 얻는 로직 수정

2d4c447

refactor: 승패 로직 수정

63786b2

refactor: Card를 VO로 만든다.

860426a

refactor: 무승부일때 로직 수정 및 코드 포멧팅

10ec7f0

refactor: dealer lose할 경우의 수 추가

bee8566

Revert "refactor: dealer lose할 경우의 수 추가"

ebe0b74

This reverts commit bee8566.

refactor: result Test 추가 및 dealer lose 경우의 수 추가

3e8852b

refactor: 접근 지정자 private으로 수정

91dbac5

refactor: add 반환 타입 boolean으로 변경

2a85843

jnsorn reviewed Mar 16, 2022

View reviewed changes

jnsorn approved these changes Mar 16, 2022

View reviewed changes

jnsorn merged commit 6741a73 into woowacourse:gyuchool Mar 16, 2022

Gyuchool mentioned this pull request Mar 18, 2022

[블랙잭 미션] 기론 1단계 리뷰 정리 woowacourse-study/2022-back-end-code-review-study#18

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1단계 - 블랙잭] 기론(김규철) 미션 제출합니다. #226

[1단계 - 블랙잭] 기론(김규철) 미션 제출합니다. #226

Gyuchool commented Mar 11, 2022

jnsorn left a comment

jnsorn Mar 14, 2022

jnsorn Mar 14, 2022

Gyuchool Mar 15, 2022

jnsorn Mar 16, 2022

Gyuchool Mar 16, 2022

jnsorn Mar 16, 2022

jnsorn Mar 16, 2022

Gyuchool Mar 16, 2022 •

edited

jnsorn Mar 16, 2022

Gyuchool Mar 16, 2022

jnsorn Mar 16, 2022 •

edited

Gyuchool Mar 17, 2022

jnsorn Mar 16, 2022

Gyuchool Mar 16, 2022

jnsorn Mar 16, 2022

jnsorn Mar 16, 2022

jnsorn left a comment •

edited

jnsorn Mar 16, 2022

jnsorn Mar 16, 2022

Gyuchool Mar 16, 2022

jnsorn Mar 16, 2022

		@@ -0,0 +1,17 @@
		package BlackJack.domain;

		public enum Result {


		import java.util.List;

		public class FixedCardFactory extends CardFactory {

[1단계 - 블랙잭] 기론(김규철) 미션 제출합니다. #226

[1단계 - 블랙잭] 기론(김규철) 미션 제출합니다. #226

Conversation

Gyuchool commented Mar 11, 2022

질문

jnsorn 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

Gyuchool Mar 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnsorn Mar 16, 2022 • edited

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

jnsorn left a comment • edited

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

Gyuchool Mar 16, 2022 •

edited

jnsorn Mar 16, 2022 •

edited

jnsorn left a comment •

edited