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

[2단계 - 블랙잭(베팅)] 연로그(권시연) 미션 제출합니다. #321

Merged
merged 36 commits into from
Mar 20, 2022

Conversation

yeon-06
Copy link

@yeon-06 yeon-06 commented Mar 17, 2022

안녕하세요 로운!
2단계 구현을 완료하여 제출합니다
이번에도 잘 부탁드립니다 😄

추가된 기능 요구사항

  • 사용자의 배팅 금액 입력
  • 결과 승/무/패 대신 게임 결과에 따른 배팅 금액 출력

변경사항

  • 불필요한 메소드, 클래스 삭제 또는 적절한 위치로 이동
  • 추가된 요구사항 구현
    • 참가자들의 배팅 금액 입력
    • 최종 수익 출력
  • CardDistributor에 전략 패턴 적용
  • 참가자들의 카드 초기화 방식 변경 (처음부터 2장을 받지 않고 생성 후 메소드 호출을 통해 2장 받도록)

고민사항

DTO의 도입

1단계에서 이어지는 질문인데요 원래는 이번에 DTO를 도입해보려고 했습니다.
사유는 아래 두가지인데요

  1. 도메인의 로직을 호출할 가능성을 아예 없애기 위해
  2. 뷰의 변경으로 인해 도메인의 변경 상황을 막고 싶어서

게더에서 다른 분의 의견도 들어봤는데요

  1. 현 프로젝트에서 dto를 도입하는건 불필요하다 (오버 프로그래밍)
    (=도메인 넘기는 것으로 충분하다)

위 의견도 많았어서 고민이 됩니다.

또 하나 고민되는 것이
만약에 dto를 도입해서 하트3을 출력해야한다고 가정해보겠습니다.

  1. 도메인에서 suit+number를 조합해서 dto에 보내준다.
  2. dto에서suit+number를 조합한다.
  3. dto에넌 suit, number 따로 담고 view에서 조합한다.

요 부분에 대해서도 고민이 됩니다
저는 2가 적당하다고 생각했는데 이 경우 view의 로직이 dto에 담기는 것 같아서 불편하더라고요
3을 선택하자니 dto를 사용하는 의미를 잃는 것 같습니다

로운이라면 어떻게 하실지 궁금해요!

  • dto를 도입할지
  • 도입한다면 문자열의 조합을 어디서 할지

기타 고민한 부분에 대해서는 파일에 직접 코멘트 남기겠습니다!

yeon-06 added 24 commits March 16, 2022 17:51
- 메소드 순서 변경
- for -> stream 변경
- Participants의 데이터를 get해서 보내기보다는 해당 객체 자체를 보내 처리

import blackjack.domain.card.Cards;

public class ParticipantGameInfo {
Copy link
Author

Choose a reason for hiding this comment

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

해당 클래스는 GameResult 내부에서만 사용되는 클래스입니다.
Map.Entry처럼 내부 클래스로 만들까 하다가 GameResult의 코드가 너무 길어져 클래스 로직을 파악하는게 힘들어지진 않을까? 라는 생각에 톱레벨 클래스로 빼두었습니다.
이러한 이유로 내부 클래스 대신 톱레벨 클래스로 생성해도 괜찮을까요?😂

Copy link

Choose a reason for hiding this comment

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

코드가 길어지는 것 때문에 분리보다는 역할이랑 책임을 더 고려하는 것 같아요.
이 객체를 보면서 가지고 있는 필드들을 cards가 가지고 있을수도 있겠는데 라는 생각을 했거든요.

연로그는 cards와 GameResult의 역할과 책임을 어떻게 정했을까요??

Copy link
Author

Choose a reason for hiding this comment

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

Cards

  • 카드 목록을 갖고 있음
  • 카드 목록의 상태(ex: 추가) 관리

GameResult

  • 게임 규칙을 알고 실행 결과에 대한 책임

위처럼 생각했습니다!

score을 뺀 이유
👉 ACE의 경우 1일수도 11일수도 알 수 있다는 규칙을 GameResult가 알게 하도록

status를 뺀 이유
👉 status를 알기 위해 score을 반드시 알고 있어야 하기 때문

Copy link

@lowoon lowoon Mar 19, 2022

Choose a reason for hiding this comment

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

개인적인 생각인데요.
위의 책임이라면 이 객체의 메서드들이 GameResult로 넘어가고 해당 클래스가 사라져도 될거 같다는 생각이 들었습니다.

private MatchResult playResult(Player player, Dealer dealer) {
Cards playerCards = player.getCards();
Cards dealerCards = dealer.getCards();
private Long elicitBettingResult(Player player, Dealer dealer) {
Copy link
Author

@yeon-06 yeon-06 Mar 17, 2022

Choose a reason for hiding this comment

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

결과를 도출하다는 의미로 elicitResult를 썼는데 elicit이라는 단어가 많이 생소하네요💦
(검색했을 때 get도 있었는데 저희가 일반적으로 선언하는 getter와 혼동될 것 같아 사용하지 않았습니다)
로운이 추천해주시는 또는 자주 쓰시는 단어가 있을까요?

Copy link

@lowoon lowoon Mar 17, 2022

Choose a reason for hiding this comment

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

네 저도 보고 생소했네요 😅
getter는 저도 단지 값을 가져오는 것이라 사용하지 않는것 같고요.
calculate나 create등을 썼던거 같아요. figure out 등도 생각나지만 너무 복잡하게 생각하지 말고 간단하게 나타내도 좋을거 같아요.
playResult를 말씀드렸던건 의미(결과를 플레이하다)가 맞지 않은 느낌이 들었던 거라서요.

int index = players.indexOf(player);
return players.get(index);
public void drawCard(Participant participant, Card card) {
int index = players.indexOf(participant);
Copy link
Author

Choose a reason for hiding this comment

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

클래스 타입을 비교하지 않으려고 로직을 수정하다보니 List에서 Participant를 찾으려는 상황이 발생했습니다.
현재 Suspicious call to 'List.indexOf' 라고 노란 전구가 뜨고 있는데 너무 불편하네요
개선 방법에 대해 힌트를 주실 수 있을까요?😂

Copy link

@lowoon lowoon Mar 17, 2022

Choose a reason for hiding this comment

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

상위인 BlackjackGame의 initCards 메서드 안에서 participant.drawCard(cardDistributor.distribute())를 하지 않는 이유가 있을까요??

Copy link
Author

Choose a reason for hiding this comment

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

큰 이유는 없고 있는 메소드를 재활용했습니다!
지금 보니 drawCard()가 로직이 변경되면 같이 변경될 수 있어서 직접 호출하는게 더 낫겠네요

Copy link
Author

Choose a reason for hiding this comment

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

클래스 타입을 비교하지 않으려고 로직을 수정하다보니 List에서 Participant를 찾으려는 상황이 발생했습니다. 현재 Suspicious call to 'List.indexOf' 라고 노란 전구가 뜨고 있는데 너무 불편하네요 개선 방법에 대해 힌트를 주실 수 있을까요?😂

요 부분에 대한 답변도 부탁드립니다!!😄
@lowoon

Copy link

@lowoon lowoon Mar 19, 2022

Choose a reason for hiding this comment

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

위의 답변으로 해결이 된거라고 생각을 했었는데요 😅
participant를 알고 있다면 participant의 drawCard를 호출하면 될거고, 전체가 drawCard를 하는 것이 필요하면 participants로 participant를 넘기지 않고 cardDistributor를 넘겨서 내부에서 하나씩 전달하도록 할 수 있지 않을까요??
cardDistributor를 넘기고 싶지 않다면

    // BlackJackGame
    public void drawCard() {
        Supplier<Card> supplier = cardDistributor::distribute;
        participants.drawCard(supplier);
    }

    // Participants
    public void drawCard(Supplier<Card> supplier) {
        dealer.drawCard(supplier.get());

        for (Player player : players) {
            player.drawCard(supplier.get());
        }
    }

처럼 할 수도 있을거 같아요. 참고만 해주시면 될거 같아요.

List에서 특정 객체의 위치를 찾는 메서드는 사용하지 않는 거 같아요. 특정 위치가 필요한 상황이라면 객체지향에 맞게 구현이 된 것인지 고민해보는 것 같습니다~

Copy link
Author

@yeon-06 yeon-06 Mar 19, 2022

Choose a reason for hiding this comment

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

악 리팩토링할 때 기존 코드를 과감히 버려야하는데 구질구질 놓지 못하는 바람에..
넓게 보지를 못했네요 이해했습니다ㅠ_ㅠ
불필요한 구조를 갖고 있었군요...

@DisplayName("플레이어 승리")
void test(String comment, Player player, Dealer dealer) {
@DisplayName("수익 구하기")
void bettingAmount(String comment, Player player, Dealer dealer, long expect) {
Copy link
Author

Choose a reason for hiding this comment

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

테스트 이름을 지정하기 위해 첫번째 파라미터로 String comment를 받고 있는데요
사용하지 않는 변수가 있다고 노란 전구가 뜨네요 ㅠ.ㅠ
원하는 이름을 테스트마다 지정하는 다른 방법은 없을까요??

Copy link

@lowoon lowoon Mar 17, 2022

Choose a reason for hiding this comment

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

저도 해본적이 없었는데요. 찾은 방법이 있긴 한데 불편하네요;;
이거는 연로그가 적용하고 싶으면 적용해보면 될 거 같아요.

public class TestInfo {
    private final Player player;
    private final Dealer dealer;
    private final long expect;

    public TestInfo(Player player, Dealer dealer, long expect) {
        this.player = player;
        this.dealer = dealer;
        this.expect = expect;
    }

    public Player getPlayer() {
        return player;
    }

    public Dealer getDealer() {
        return dealer;
    }

    public long getExpect() {
        return expect;
    }
}
    @ParameterizedTest(name = "{0}")
    @MethodSource("provideParameters")
    @DisplayName("수익 구하기")
    void bettingAmount(TestInfo testInfo) {
        // given
        Player player = testInfo.getPlayer();
        Dealer dealer = testInfo.getDealer();
        List<Player> players = List.of(player);
        Participants participants = new Participants(players, dealer);
        GameResult gameResult = new GameResult(participants);

        // then
        assertThat(gameResult.getBettingResult(player)).isEqualTo(testInfo.getExpect());
    }

    private static Stream<Arguments> provideParameters() {
        Name playerName = new Name("yeonLog1");
        BettingAmount bettingAmount = new BettingAmount(1000L);

        return Stream.of(
                Arguments.arguments(
                        Named.of("An important file", new TestInfo(
                        new Player(playerName, getCards(Number.QUEEN, Number.NINE, Number.JACK), bettingAmount),
                        new Dealer(getCards(Number.QUEEN, Number.ACE)),
                        -1000L))
                )
        );
    )

findPlayer((Player) participant).drawCard(card);
return;
private List<Player> initPlayers(List<Name> names, List<BettingAmount> bettingAmounts) {
if (names.size() != bettingAmounts.size()) {
Copy link
Author

Choose a reason for hiding this comment

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

설계상 두 리스트가 다를 일이 없는데 테스트 코드에서 실수할까봐 안전하게 사이즈를 비교해주었습니다
안전성을 위한 처리이니 필요하다고 생각이 되면서도
코드를 추가하게 된 계기가 테스트 코드 때문이니 불필요한건 아닐까? 라는 생각도 드네요
로운은 어떻게 생각하시나요??

Copy link

Choose a reason for hiding this comment

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

저는 이 부분을 보면서 왜 2개의 List를 사용하지 라는 생각이 들었어요.
Map의 형태로 넘겨주면 되지 않나요? key를 name으로 하고 BettingAmount를 value로 가진??

@yeon-06 yeon-06 changed the title [2단계 - 블랙잭] 연로그(권시연) 미션 제출합니다. [2단계 - 블랙잭(베팅)] 연로그(권시연) 미션 제출합니다. Mar 17, 2022
private void playPlayersTurn(BlackjackGame blackjackGame, List<Player> players) {
for (Player player : players) {
private void playPlayersTurn(BlackjackGame blackjackGame, Participants participants) {
for (Player player : participants.getPlayers()) {
Copy link

Choose a reason for hiding this comment

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

participants에서 players를 가져와서 돌고 있는데요.
participants에 물어볼 수는 없을까요??

Copy link
Author

Choose a reason for hiding this comment

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

실행 조건에 InputView와 엮인 부분이 있기 때문에 뷰와 모델을 분리하려고 ConsoleGame에서 실행시키게 했습니다ㅠㅠ

Copy link

Choose a reason for hiding this comment

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

네 inputView와 엮여 있기때문에 힘든 부분이 있는데요.
한가지 방법으로 Consumer를 사용하는 것이 있습니다. 참고만 하시면 될 거 같아요~


import blackjack.domain.card.Cards;

public class ParticipantGameInfo {
Copy link

Choose a reason for hiding this comment

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

코드가 길어지는 것 때문에 분리보다는 역할이랑 책임을 더 고려하는 것 같아요.
이 객체를 보면서 가지고 있는 필드들을 cards가 가지고 있을수도 있겠는데 라는 생각을 했거든요.

연로그는 cards와 GameResult의 역할과 책임을 어떻게 정했을까요??

Copy link

@lowoon lowoon left a comment

Choose a reason for hiding this comment

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

반가워요 연로그! 2단계도 잘 부탁해요 😄
질문에 답변달았습니다~
몇 가지 코멘트 남겼으니 확인해주세요ㅎㅎ
궁금한 점이 생기면 언제든지 DM 혹은 댓글남주세요 ^-^

@lowoon
Copy link

lowoon commented Mar 17, 2022

dto를 현재 프로그램에서는 도입할 필요가 없을 수 있습니다. 하지만 도입해야하는 필요성을 느꼈다면 도입해도 된다고 생각합니다.
오버 프로그래밍이라고 생각하는 기준도 개발자마다 다르고, 도메인을 넘기는 것으로 충분하다고 말하는 이유는 레벨1에서 도메인의 역할과 책임에 조금 더 집중하기 위해서랍니다. 도메인의 역할과 책임에 많은 고민을 하면서 dto를 사용하는 이유를 알고 사용의 필요성을 느꼈다면 사용해도 괜찮다고 생각합니다~

저도 만약 한다면 2번으로 했을것 같아요.
suit+number는 항상 같이 다니는 정보이기 때문에 분리할 필요성이 없고, 도메인에서 조합해서 보내주기에는 dto용 메서드를 만드는 것이기 때문에 사용하지 않을것 같습니다.

Copy link

@lowoon lowoon left a comment

Choose a reason for hiding this comment

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

고생하셨어요. 연로그! 리뷰 반영 잘 해주셨어요! 😄
답변 달았고, 몇 가지 코멘트 남겼으니 확인해주세요ㅎㅎ
궁금한 점은 언제든지 DM이나 코멘트 남겨주세요 :)


import blackjack.domain.card.Cards;

public class ParticipantGameInfo {
Copy link

@lowoon lowoon Mar 19, 2022

Choose a reason for hiding this comment

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

개인적인 생각인데요.
위의 책임이라면 이 객체의 메서드들이 GameResult로 넘어가고 해당 클래스가 사라져도 될거 같다는 생각이 들었습니다.

@@ -17,23 +21,33 @@ public Participants(final List<Player> players, final Dealer dealer) {
validatePlayers(players);
}

public Participants(Map<Name, BettingAmount> participantInfos) {
Copy link

@lowoon lowoon Mar 19, 2022

Choose a reason for hiding this comment

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

이전에 만들었던 생성자도 남아있는데요. 테스트에서만 쓰이던데 있을 이유가 있나요??

Copy link
Author

Choose a reason for hiding this comment

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

테스트를 편하게 만들기 위해서..인데 지금 다시보니 테스트 코드를 이에 맞춰서 수정하면 될 일 같네요😅
수정해서 반영하겠습니다~

ParticipantGameInfo dealerGameInfo = new ParticipantGameInfo(dealer.getCards());

if (playerGameInfo.isBlackJack() && !dealerGameInfo.isBlackJack()) {
return player.getBettingAmount() / DIVISION_NUMBER_FOR_WINNER * MULTIPLE_NUMBER_FOR_WINNER;
Copy link

@lowoon lowoon Mar 19, 2022

Choose a reason for hiding this comment

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

1.5를 상수로 두어서 곱하지 않은 이유가 있을까요??
이 부분에서 if구문이 많아지는 것 같은 느낌이 들면 이전의 MatchResult를 활용해서 관리할 수도 있을 거 같아요~

}

@Override
public boolean isFinished() {
return cards.getStatus() != Status.NONE;
return cards.sum() >= GameResult.BLACKJACK_VALUE;
Copy link

Choose a reason for hiding this comment

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

sum으로만 계산을 하고 있는데요.
a, k로 blackJack인 경우에도 끝났다라고 판단을 하나요??

@yeon-06
Copy link
Author

yeon-06 commented Mar 20, 2022

안녕하세요 로운😊
주말에도 고생이 많으십니다ㅠㅠ

질문 사항은 DM으로 다 답변 받았으니 생략하겠습니다!
변경 사항은 아래와 같습니다

  • 불필요한 클래스(ParticipantGame)삭제
  • 이익을 구하는 부분 MatchResult로 분리
  • Participants의 불필요한 생성자 삭제
    • 이로 인한 테스트 코드 수정
  • 카드의 합계를 구하는 sum()에 대한 규칙은 Cards의 책임으로 위임

Copy link

@lowoon lowoon left a comment

Choose a reason for hiding this comment

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

안녕하세요 연로그!😄
간단한 코멘트만 남겼으니 확인 부탁드려요.
이제 그만 블랙잭을 마무리해도 될거 같아요ㅎㅎ
미션은 끝났지만 궁금한점 있으면 DM이나 코멘트로 남겨주세요!

1, 2단계 블랙잭 미션 수고하셨습니다!

public long getDealerProfit() {
return bettingResult.values()
.stream()
.mapToLong(value -> -value)
Copy link

Choose a reason for hiding this comment

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

Suggested change
.mapToLong(value -> -value)
.mapToLong(Long::reverse)

를 할 수도 있을거 같아요~

|| (status1.isNone() && status2.isNone() && cards1.sum() > cards2.sum());
}

private static boolean compareBlackjack(Cards cards1, Cards cards2) {
Copy link

Choose a reason for hiding this comment

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

매개변수들이 다 cards1, cards2인데요.
기준과 비교대상으로 나타낼 수도 있을거 같고, delaer와 player를 비교하기 때문에 dealer와 player를 표시할 수 있으면 조금 더 명확할 수 있을거 같아요.

MatchResult(String value) {
this.value = value;
public static long calculateProfit(Player player, Dealer dealer) {
return (long) (player.getBettingAmount() * findProfit(player.getCards(), dealer.getCards()).profit);
Copy link

Choose a reason for hiding this comment

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

한 줄에 너무 많은 로직이 포함되어 있는데요.
로직을 한줄씩 풀어서 쓰는 것이 더 명확할 수 있습니다~

}

public String getValue() {
return value;
private static MatchResult findProfit(Cards cards1, Cards cards2) {
Copy link

Choose a reason for hiding this comment

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

findProfit이 아닌 findMatchResult아닌가요??

import blackjack.domain.participant.Player;
import java.util.Arrays;
import java.util.function.BiPredicate;

public enum MatchResult {
Copy link

Choose a reason for hiding this comment

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

MatchResult가 결과를 도출하는 역할을 하는데요.
중요로직을 담당하는데 테스트가 없는거 같아요~

public enum Status {
BLACKJACK((count, sum) -> count == GameResult.BLACKJACK_COUNT && sum == GameResult.BLACKJACK_VALUE),
BUST((count, sum) -> sum > GameResult.BLACKJACK_VALUE),
NONE((count, sum) -> sum < GameResult.BLACKJACK_VALUE
Copy link

Choose a reason for hiding this comment

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

count와 sum이 cards의 정보이기 때문에 cards만 넘기도록 할 수도 있을거 같아요~

this.bettingAmount = bettingAmount;
}

public Player(Name name, Cards cards, BettingAmount bettingAmount) {
Copy link

@lowoon lowoon Mar 20, 2022

Choose a reason for hiding this comment

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

이 부분도 테스트에서만 사용을 하는데요.
개인적으로 테스트만을 위한 코드가 production 코드에 만들어지는 것은 좋지 않다고 생각합니다~
테스트만을 위한 코드로 인해 production 코드가 영향을 받으면 안된다고 생각하거든요.
기존에 있는 로직을 활용해서 테스트할 수 있는 방법을 생각하고, 방법이 생각이 안난다면 메서드를 수정하여 테스트 가능한 방법이 없는지 찾는거 같아요~

@lowoon lowoon merged commit 4d5ac2b into woowacourse:yeon-06 Mar 20, 2022
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.

None yet

2 participants