[2단계 - 블랙잭 베팅] 망쵸(김주환) 미션 제출합니다.#16
[2단계 - 블랙잭 베팅] 망쵸(김주환) 미션 제출합니다.#163Juhwan wants to merge 28 commits intowoowacourse-6th-code-review-study:mainfrom
Conversation
* docs: 기능 요구 사항 작성 Co-authored-by: jeongjiho0731@gmail.com * feat: 플레이어에게 카드를 분배하는 기능 구현 Co-authored-by: J-I-H-O<jeongjiho0731@gmail.com> * feat: 각 플레이어에게 2장씩 카드를 분배하는 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: 가진 패의 합계를 구하는 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: 플레이어가 죽었는지 여부를 반환하는 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: 카드 덱에서 무작위 카드를 뽑는 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: 딜러와 단일 플레이어의 승패를 판단하는 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: 딜러와 여러 플레이어의 승패를 판단하는 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * refactor: 플레이어에 픽스처 적용 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * refactor: Dealer 클래스 분리 및 픽스쳐 수정 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * refactor: 픽스쳐 내 중복 제거 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * docs: 게임 승패 결정 로직 작성 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: 승패 판단 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * chore: 불필요한 클래스 삭제 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * docs: 뷰에 대한 기능 요구사항 추가 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: 딜러의 승패 횟수 계산 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: InputView 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: Player 이름 길이를 검증하는 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: 딜러와 플레이어 전원에게 초기에 분배한 카드 출력하는 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: 플레이어가 보유한 모든 카드를 출력하는 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: 딜러가 추가 카드를 발급 받았는지 여부 출력하는 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: 보유한 모든 카드의 합을 출력하는 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: 최종 승패를 출력하는 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: controller에서 사용자에게 처음 카드 2장씩 부여하는 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * fix: 플레이어 카드를 출력하는 구문 수정 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: Controller 전체 기능 구현 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * refactor: 딜러, 플레이어 생성자에 매개변수 최소화 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * refactor: 딜러 클래스의 메서드명 수정 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * refactor: 도메인 용어로 변경 (draw -> hit) Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * refactor: 패키지 분리 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * refactor: view, controller 리팩터링 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * refactor: 도메인 로직 리팩터링 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * feat: 에이스 관련 처리 기능 추가 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * docs: 구현한 사항 체크 Co-authored-by: MangCho <13selfesteem91@naver.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> * chore: 잘못된 변수명 수정 * fix: 카드 계산 로직 변경으로 테스트 결과 수정 * refactor: Deck 리팩터링 - 정적 팩토리 메서드 추가 - 접근 제어자를 적절하게 수정 * test: Deck에 52장의 카드가 있는지 검증하는 테스트 * refactor: Hand 클래스 수정 - 합계를 구하는 로직 수정 - 접근 제어자 수정 * test: Hand 클래스 테스트 * refactor: 접근 제어자 수정 * test: DealerTest 추가 * refactor: 메서드 분리 * refactor: Judge의 역할을 Dealer로 옮김 * refactor: 게임 로직 설정 정보 이동 * refactor: 출력 메시지를 Outview로 위임 * refactor: CardGameResult를 record로 변경 * refactor: OutputView에서 메시지를 생성하는 로직을 MessageResolver로 분리 * style: 불필요한 공백 제거 * refactor: 덱 생성 로직에 스트림 적용 * refactor: 불필요한 위임 메서드 삭제 * test: 게임 결과에서 플레이어가 순서대로 반환되는지 테스트 * refactor: CardGame의 역할을 Deck으로 이동 * refactor: 승패 결과에 대한 결과 문자열을 뷰로 이동 * refactor: BUST 조건에 대한 다른 클래스의 의존성 제거 * refactor: 게임 결과를 생성하는 로직을 CardGameResult로 이동 --------- Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>
| import java.util.List; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class BlackjackController { |
There was a problem hiding this comment.
Controller란, 어떤 요청이 들어왔을 때 그 요청을 처리할 수 있는 model 에게 그 요청의 처리를 위임하고, 처리 결과를 View에 전달하는 것을 말합니다. 하지만, 우리 미션에서는 요청을 표현하는 것이 없습니다. 그저 프로그램을 실행하면 알아서 동작을 하기 때문에 Controller라는 이름은 부적절하다고 생각합니다!
There was a problem hiding this comment.
로빈의 controller 정의를 통해 새로운 인사이트를 얻습니다 👍
현재 미션에서 적용하는 MVC 패턴에서 Controller의 역할은 Model과 View를 이어주는 다리 역할이라고 생각해요!
그래도 Controller 라는 네이밍이 적절하지 않을 수도 있겠네요~
BlackjackGame 같은 네이밍을 한다면 로빈 말대로 알아서 동작하는 프로그램과 같은 의미를 담을 수 있겠어요 ✊
| int minusValue() { | ||
| return -value; | ||
| } |
There was a problem hiding this comment.
메서드 이름이 조금 부적절한 것 같습니다. minus는 뺀다는 의미니까요
There was a problem hiding this comment.
좋은 지적 감사해요 👍
부적절하다는 코멘트에 공감하고, 리팩터링 과정에서 해당 메서드는 삭제할 수 있어서 지웠어요~
| if (WIN.equals(status)) { | ||
| return new Profit(money.value()); | ||
| } | ||
| if (LOSE.equals(status)) { | ||
| return new Profit(money.minusValue()); | ||
| } | ||
| if (PUSH.equals(status)) { | ||
| return new Profit(0); | ||
| } | ||
| if (BLACKJACK.equals(status)) { | ||
| return new Profit(money.multipleValue(BLACKJACK_PROFIT_RATE)); | ||
| } |
There was a problem hiding this comment.
WinningStatus를 잘 이용한다면, if문을 제거할 수 있을 것 같습니다!
| public ProfitDetails calculateProfit(final CardGameResult result) { | ||
| final Map<Name, Profit> profitDetails = new LinkedHashMap<>(); | ||
|
|
||
| for (final var nameAndStatus : result.totalResult().entrySet()) { | ||
| final Name name = extractName(nameAndStatus); | ||
| final WinningStatus status = extractStatus(nameAndStatus); | ||
| final Profit profit = Profit.of(findBettingMoney(name), status); | ||
| profitDetails.put(name, profit); | ||
| } | ||
|
|
||
| return new ProfitDetails(profitDetails); | ||
| } |
There was a problem hiding this comment.
1단계 미션에서 구현한 각 플레이어의 승패무 통계 관련 코드를 재사용 하기 위해 지금과 같은 구조로 수익을 계산하는 건가요? 기존 코드를 삭제하고 컴팩트하게 구현했다면 조금 더 이해하기 쉬운 코드가 될 수 있었을 것 같아요.
There was a problem hiding this comment.
오 저는 오히려 잘 이용했다고 생각했는데 역시 사람마다 생각이 다르군요
There was a problem hiding this comment.
로빈~ 정확하시네요 👍
제가 이번 미션을 하면서 의미를 뒀던 부분은 step1 코드를 최소한으로 건드리고 step2를 구현할 수 있는가에요!
나름 목표를 달성했다고 생각했는데, 언급해 주신 부분은 놓친 부분이었고 리팩터링을 통해 해결해야 했어요~
지금은 수정했습니다 😀
seokmyungham
left a comment
There was a problem hiding this comment.
망쵸 수고했어~ 👍
step2는 크게 구현할 게 많지 않았던 것 같아
악명높은 체스 들어가기 전 마지막 점검시간이랄까..
다음 미션도 화이팅해보자고!! ㅋㅋ 😄
| public static WinningStatus doesPlayerWin(final Dealer dealer, final Player player) { | ||
| if (player.isBlackjack()) { | ||
| return WinningStatus.BLACKJACK; | ||
| } |
There was a problem hiding this comment.
플레이어와 딜러 모두 블랙잭일 때 결과가 어떻게 되나요?
관련된 테스트 코드가 없는 것 같아 궁금해서 질문드립니다!
There was a problem hiding this comment.
맞아요.. 꼼꼼한 리뷰 감사합니다 🥹
마지막에 추가한 로직인데 얼른 실행 결과를 보고 싶은 마음에 TDD를 하지 않고 테스트도 작성하지 않았어요.
추가했습니다!!
| public Card getFirstCard() { | ||
| try { | ||
| return hand.getAllCards().get(0); | ||
| } catch (IndexOutOfBoundsException e) { |
There was a problem hiding this comment.
hand.getAllCards().get(0) 대신 hand가 직접 일하도록 할 수도 있을 것 같아요 😄
There was a problem hiding this comment.
고민했던 부분이고 위와 같이 코드를 작성했어요. 이렇게 코멘트를 들으니까 다시 생각해 보게 됐는데, hand에게 책임을 주는 것이 더 좋을 것 같네요!
객체에게 일을 하도록 시키는 것 항상 중요하게 고려하고 있어요 😀
| final String errorMessage = String.format("[ERROR] 이름의 길이는 %s 이상, %s 이하여야 합니다.", | ||
| MINIMUM_LENGTH, MAXIMUM_LENGTH); | ||
| throw new IllegalArgumentException(errorMessage); |
There was a problem hiding this comment.
에러 메시지 문자열 결합 관련 (feat.커찬)
추가로 찾아본 내용
커찬이 직접 학습하고 좋은 내용의 글을 공유해줬는데 망쵸도 보면 좋을 것 같아 남겨용 👍
There was a problem hiding this comment.
좋은 글 공유 고마워요 ❤️
안 그래도 고민한 부분인데요~
저는 가독성을 택했어요! format을 사용하는 것이 훨씬 가독성이 좋다고 생각해요!
아직은 성능을 고려하지 않아도 될 것 같아요
| public Player(final Name name) { | ||
| this(new Hand(), name); | ||
| } | ||
|
|
There was a problem hiding this comment.
public Player(final String name) {
this(new Hand(), new Name(name));
}생성자를 위 처럼 수정하면 테스트를 할 때마다 더 간편할 것 같은데 어떻게 생각하시나요?
There was a problem hiding this comment.
테스트를 위해 코드를 추가하거나 변경하는 것을 안 좋아하는데요~~
확실히 테스트하는 부분에서 어려움이 있었어요 😢
팀으로 한다면 컨벤션을 따를 생각은 있지만, 저 개인이라면 좀더 고민이 필요할 것 같아요!
의견 감사해요 👍
hyxrxn
left a comment
There was a problem hiding this comment.
늦어서 정말 죄송합니다..... 🙇♀️
체스도 화이팅!
| void run() { | ||
| final List<Name> names = inputView.askPlayerNames(); | ||
| final Dealer dealer = new Dealer(); | ||
| final List<Player> players = names.stream().map(Player::new).toList(); | ||
| final CardDeck deck = CardDeck.createShuffledDeck(); | ||
|
|
||
| final BetDetails playersBetDetails = getPlayersBettingMoney(names); | ||
| initializeHand(deck, dealer, players); | ||
| outputView.printInitialHandOfEachPlayer(dealer, players); | ||
|
|
||
| for (final Player player : players) { | ||
| givePlayerMoreCardsIfWanted(deck, player); | ||
| } | ||
| giveDealerMoreCardsIfNeeded(deck, dealer); | ||
|
|
||
| printHandStatusOfEachPlayer(dealer, players); | ||
| final CardGameResult result = CardGameResult.of(dealer, players); | ||
| ProfitDetails profits = playersBetDetails.calculateProfit(result); | ||
| printProfit(profits); | ||
| } |
There was a problem hiding this comment.
함수(또는 메서드)의 길이가 10라인을 넘어가지 않도록 구현한다.
There was a problem hiding this comment.
프로그램의 진입점으로 생성과 로직 실행의 책임을 갖고 있는 메서드니까 괜찮겠지 라는 생각으로 해당 메서드는 요구사항을 벗어나게 작성했어요! 하지만 확실히 흐름이 명확하게 보이지 않네요. 요구사항이 괜히 있는 게 아니라는 생각과 함께 수정했어요!
| } | ||
|
|
||
| private Card draw() { | ||
| if (deck.size() == 0) { |
There was a problem hiding this comment.
isEmpty()를 작성하면 가독성이 올라갈 것 같네요 👍
하지만,
- Deck 내부에서만 사용하는 메서드가 될 것 같아요.
draw()도 private 메서드이고 재사용하지 않아요.- 다른 개발자가
draw()구현에 대해 궁금할 가능성이 낮고 코드를 열어 보지 않을 것 같아요.
위 이유로 메서드로 분리해서 코드 줄수를 늘리는 것보다 분리하지 않는 것이 좋다고 판단했어요!
아토라면 분리할 건가요~?
| import java.util.stream.Stream; | ||
|
|
||
| public class CardDeck { | ||
| private static final int INITIAL_CARD_NUMBER = 2; |
There was a problem hiding this comment.
현실 세계를 떠올려보면 다소 어색한 것 같아요. 이렇게 작성한 의도는 Deck이 아는 것이 응집도 높은 코드가 나오기 때문이에요. 초기 카드를 나눠주는 역할을 Deck이 하니까, 몇 장을 나눠줄지도 Deck이 아는 것이 좋겠죠. 현실과 추상화 세계가 일대일 대응일 필요는 없다고 생각해서 절충한 셈이에요!
아토는 어떤 부분에서 어색하다고 생각하는지 궁금하네요 💭
| if (player.isBlackjack()) { | ||
| return WinningStatus.BLACKJACK; | ||
| } | ||
| if (!player.isAlive()) { | ||
| return WinningStatus.LOSE; | ||
| } | ||
| if (!dealer.isAlive()) { | ||
| return WinningStatus.WIN; | ||
| } | ||
| if (dealer.score() == player.score()) { | ||
| return WinningStatus.PUSH; | ||
| } | ||
| if (dealer.score() < player.score()) { | ||
| return WinningStatus.WIN; | ||
| } | ||
| return WinningStatus.LOSE; |
There was a problem hiding this comment.
저도 if문이 과하다고 생각했어요! 리비 코드를 참고해서 enum의 BiPredicate 타입 필드로 분리해서 해결했어요 😀
| import java.util.List; | ||
|
|
||
| public abstract class Participant { | ||
| static final int BUST_CONDITION = 21; |
There was a problem hiding this comment.
BUST_CONDITION = 21이라는 비즈니스 정책을 하나로 관리하기 위해 고심해서 작성했지만 결국 두 곳에 두게 됐네요 😢
하지만 리팩터링을 통해 현재 코드는 리팩터링 과정에서에서만 관리하도록 변경했습니다!
| hand.add(card); | ||
| } | ||
|
|
||
| public final boolean isAlive() { |
There was a problem hiding this comment.
딜러에게는 이 메서드가 필요 없는 것 같은데 Player의 메서드로 옮기는게 어떨까요?
There was a problem hiding this comment.
꼼꼼한 리뷰 감사해요 😃
리팩터링을 통해 현재 코드에서는 딜러도 해당 메서드를 사용하게 변경됐습니다!
| public int getDealerWinCount() { | ||
| return (int) totalResult.values() | ||
| .stream() | ||
| .filter(playerWinningStatus -> playerWinningStatus.equals(LOSE)) | ||
| .count(); | ||
| } | ||
|
|
||
| public int getDealerLoseCount() { | ||
| return (int) totalResult.values() | ||
| .stream() | ||
| .filter(status -> status.equals(WIN)) | ||
| .count(); | ||
| } |
| return -value; | ||
| } | ||
|
|
||
| int multipleValue(final double rate) { |
There was a problem hiding this comment.
저는 times를 사용했어요
money.times(rate) 이렇게 보이게용
There was a problem hiding this comment.
times 좋은 네이밍이네요! 👍
현재 코드는 money 내부의 메서드를 대부분 삭제하여 값만을 반환하는 VO의 형태를 갖추게 됐어요~
| public ProfitDetails calculateProfit(final CardGameResult result) { | ||
| final Map<Name, Profit> profitDetails = new LinkedHashMap<>(); | ||
|
|
||
| for (final var nameAndStatus : result.totalResult().entrySet()) { | ||
| final Name name = extractName(nameAndStatus); | ||
| final WinningStatus status = extractStatus(nameAndStatus); | ||
| final Profit profit = Profit.of(findBettingMoney(name), status); | ||
| profitDetails.put(name, profit); | ||
| } | ||
|
|
||
| return new ProfitDetails(profitDetails); | ||
| } |
There was a problem hiding this comment.
오 저는 오히려 잘 이용했다고 생각했는데 역시 사람마다 생각이 다르군요
| this(new Hand(), name); | ||
| } | ||
|
|
||
| public boolean isBlackjack() { |
There was a problem hiding this comment.
이건 딜러에게도 필요해보이는데 Participant로 옮기는게 좋을 것 같아요
모두 고생 많으셨어요 😀