[2단계 - 블랙잭 베팅] 리비(이근희) 미션 제출합니다. #709
Conversation
|
안녕하세요 썬 ☀️ 2단계 리팩토링 진행해보았습니다. 여러 질문 주세요 썬! 이번에도 잘 부탁드립니다 🙇🏻♂️ |
| @@ -0,0 +1,24 @@ | |||
| package blackjack.domain.bet; | |||
|
|
|||
| public class BetAmout { | |||
There was a problem hiding this comment.
| public class BetAmout { | |
| public class BetAmount { |
| BLACKJACK_WIN(1.5, (dealer, player) -> dealer.hasNoBlackJackHand() && player.hasBlackJackHand()), | ||
| PLAYER_LOSE(-1.0, (dealer, player) -> player.isBusted() || dealer.hasScoreAbove(player)), | ||
| PLAYER_WIN(1.0, (dealer, player) -> (player.isNotBusted() && (dealer.isBusted() || player.hasScoreAbove(dealer)))), | ||
| PUSH(0.0, (dealer, player) -> player.isNotBusted() && dealer.isNotBusted() && player.hasSameScore(dealer)); | ||
|
|
There was a problem hiding this comment.
predicate를 활용하셨네요 👍 이전 if문으로 분기하는 것에서 현재로 변경하신 이유가 궁금합니다~
또한, 리비가 보시기엔 가독성 측면에서 어떤 코드가 더 좋은가요?
There was a problem hiding this comment.
가독성 측면에서는 비슷하다고 판단합니다! 조건식이 거의 동일한 것 같아서요 🤔
제가 위의 코드처럼 바꾼 이유는 채점 로직 분리를 통해 Judge 클래스를 파생되게 만드는 것보다 어떤 결과가 나올지 판정 로직을 GameResult 도메인 안에 응집시키고 싶었기 때문입니다.
다만 다음과 같은 고민이 있기는 합니다.
플레이어가 승리하는 판정 로직을 변경해주세요라는 변경 요구사항이 들어오면 다른 개발자들이 위화감없이 GameResult라는 enum을 찾아올 수 있을지가 고민이에요!
관련해서 썬의 의견도 들어보고 싶습니다 🙌
| return Arrays.stream(values()) | ||
| .filter(gameResult -> gameResult.biPredicate.test(dealer, player)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new IllegalArgumentException("[INTERNAL ERROR] 게임 결과를 판정할 수 없습니다")); |
There was a problem hiding this comment.
사소하지만 internal error라면 IllegalArgumentException일까요?
There was a problem hiding this comment.
파라미터가 적절하지 않다는 의미에서 IllegalArgumentException을 생각하기는 했어요 🤔
다만 NoSuchElement등 다른 예외도 생각이 나기는 하네요
표준 예외를 사용한다고 했을 때 어떤 예외를 고르는지 팁이 있을까요!?
골라본 경험이 많지 않다보니 이것도 될 것 같고 저것도 될 것 같고 많이 헷갈립니다 🤔
| @@ -19,37 +19,34 @@ public static Hand createHandFrom(CardDeck cardDeck) { | |||
| return new Hand(cardDeck.popCards(INITIAL_HAND_SIZE)); | |||
| } | |||
|
|
|||
| public Score calculateScore() { | |||
| int sum = calculateCardSummation(); | |||
| if (hasAce() && (sum + ACE_WEIGHT) <= BUST_THRESHOLD) { | |||
There was a problem hiding this comment.
| if (hasAce() && (sum + ACE_WEIGHT) <= BUST_THRESHOLD) { | |
| if (hasAce() && isNotBust()) { |
이런식으로 메서드 추상화 수준을 맞춰주면 코드 가독성이 더 좋습니다
| public static GameResult of(Dealer dealer, Player player) { | ||
| return Arrays.stream(values()) | ||
| .filter(gameResult -> gameResult.biPredicate.test(dealer, player)) | ||
| .findFirst() |
There was a problem hiding this comment.
순서에 상관이 없다면 findAny를 사용할 수 있습니다
각각에 대한 차이를 알아보아요~
There was a problem hiding this comment.
스트림 요소들을 직렬로 처리할 때는 문제가 없지만 병렬로 처리하는 경우에 차이가 있을 수 있다는 것을 학습할 수 있었습니다 👊
|
|
||
| GameResult(double profitLeverage) { | ||
| GameResult(double profitLeverage, BiPredicate<Dealer, Player> biPredicate) { |
There was a problem hiding this comment.
열거형 생성자는 기본적으로 접근제어자가 private인걸로 학습했습니다!
public, protected로 설정해보았는데 not allowed here라고 하면서 빨간줄이 뜨더라고요 🤔
불필요한 생성을 막기 위한 맥락으로 이해했습니다 👍
| this.biPredicate = biPredicate; | ||
| } | ||
|
|
||
| public static GameResult of(Dealer dealer, Player player) { |
There was a problem hiding this comment.
생성자가 아닙니다. 다만 GameResult.of(dealer, player)와 같은 코드가 딜러와 플레이어의 게임 결과처럼 평문적으로 읽힌다고 생각해서 네이밍을 위처럼 진행해보았어요.
썬의 말씀 듣고 추가적인 생각해보았습니다 🤔
그 결과 다른 개발자가 본다면 정적 팩토리 메서드의 네이밍 패턴을 사용하고 있기에 헷갈릴 수도 있겠다는 생각을 하게 되었어요.
고쳐보는게 낫다고 판단하게 되었습니다. 반영해볼게요 💪🏻
|
|
||
| PlayerProfits playerProfits = playerBets.calculateProfitResult(dealer); | ||
|
|
||
| assertThat(GameResult.of(dealer, player2)).isEqualTo(GameResult.PLAYER_LOSE); |
There was a problem hiding this comment.
앗.. 테스트가 통과하지 않아서 디버깅 용도로 제가 작성한 테스트인데 삭제하지 않았네요 😓
| @DisplayName("특정 플레이어의 수익을 조회할 수 있다") | ||
| @Test | ||
| void testFindProfitOfPlayer() { | ||
| Player player1 = TestPlayerCreator.of("썬", 1, 2, 3, 4); |
| Player player1 = TestPlayerCreator.of("썬", 1, 2, 3, 4); | ||
| Player player2 = TestPlayerCreator.of("리비", 3, 4); | ||
|
|
||
| Map<Player, Profit> playerProfitMap = new HashMap<>(); |
There was a problem hiding this comment.
기본적으로 변수명에는 자료구조를 사용하지 않습니다~
클린코드의 의미 있는 이름에 대해 학습해보아요
| Map<Player, Profit> playerProfitMap = new HashMap<>(); | |
| Map<Player, Profit> playerProfits = new HashMap<>(); |
There was a problem hiding this comment.
특정 자료형에 종속적인 변수명은 의미를 드러내지 못하고 확장사항에 잘 견디지 못하는 것을 학습할 수 있었습니다
반영해보겠습니다 💪🏻
|
안녕하세요 썬 ☀️ 미션 진행에 대해서 다음의 것들에 대해 썬과 의견 나눠보고 싶어요 🙌 🙋🏻♂️ 연관관계 방향에 대해서 고민해보았습니다저는 플레이어가 베팅금액을 가지지 않도록 설계했어요 🤔 처음에는 플레이어 도메인에 속성으로써 추가하려고 했는데 이렇게 한두개씩 플레이어에 추가하다가는 후에 Player가 가지게 되는 속성이 굉장히 많아질수도 있다고 생각하게 되었습니다. 현업의 경우를 저 혼자 멋대로 상상해보았는데요 현업의 도메인들은 지금의 미션과는 비교도 안될 정도로 많은 연관관계를 가지고 있을 것 같아요. 학습할만한 키워드 짧게 던져만주셔도 도움 많이 될 것 같습니다 💪🏻 🙋🏻♂️ 도메인은 어떤 속성을 가지고 어떤 속성을 외부로 덜어내야 하는가위 질문과 비슷한 맥락입니다. 플레이어가 플레이어 이름을 속성으로 가지고 있는 것은 제게는 매우 합리적입니다. 이렇게 치면 플레이어 관련된 모든 속성이 플레이어에 엮여야 하는데 이는 결국 클래스를 비대하게 만들고 시스템 유지보수의 취약점이 될 수 있을 것 같다는 생각입니다. 어디까지를 플레이어가 알고 어디까지가 외부로 덜어내져야 하는 것일까요 .. 절대적 기준이 없음을 알지만 썬의 의견이 도움이 될 수 있을 것 같습니다 🙇🏻♂️ (지금 생각해보니 🙋🏻♂️ 검증 책임에 대해 크루들과 토의하고 결론을 내려보았습니다크루들끼리 토의하고 스스로 결론 내려보았습니다. 사고 과정과 도출한 결론이 합리적인지 썬께서 봐주시면 너무 좋을 것 같아요 😃 시작은 이렇습니다. 그런데 문득 이런 생각이 들었어요
이런 생각 이후로 Players 도메인이 순수해보이지 않았습니다. (자신의 역할이외의 룰까지 알고 있어서 순수하지 않다라는 생각..) 간단히 넘어갈 수도 있는 문제였지만 저는 그때부터 다른 도메인들도 의심스러워지기 시작했어요 🫠
이러한 고민을 크루들과 얘기해보았고 우선 다음과 같이 스스로 결론을 내보았습니다.
위 관점에서 보면 저의 결론인데 어떻게 생각하시나요 👀 🙋🏻♂️ 특정 기능의 책임 위치를 정하는 요령을 생각해보았습니다.특정 기능을 어느 도메인의 책임으로 둘 지 스스로 판단하기 힘들었습니다. 1,2단계 모두 특히 이 과정에서 자꾸 행위를 추상화하려고 하다가 객체지향의 특성을 많이 놓친 경험도 있습니다. 크루 중에 승부를 내는 기능을 Dealer에 위치시킨 크루가 있었고 이 크루와 자세히 얘기해볼 수 있는 기회가 있었습니다.
크루들과 얘기를 한 다음에 저는 설득되어 다음과 같은 결론을 내리게 되었어요
앞으로 위치를 정의할 때 필요한 속성을 가지고 있는 객체, 그리고 실제로 상호작용이 이루어지는 객체에게 물어보도록 할까 합니다. |
|
| @@ -3,12 +3,13 @@ | |||
| import blackjack.domain.player.Dealer; | |||
| import blackjack.domain.player.Player; | |||
| import java.util.Arrays; | |||
| import java.util.NoSuchElementException; | |||
| import java.util.function.BiPredicate; | |||
|
|
|||
| public enum GameResult { | |||
There was a problem hiding this comment.
넵 현재도 충분히 좋은 방법인 것 같아요 👍
다만, 이렇게 될 경우 enum의 순서가 로직에 영향을 미치고 있는데, 이를 인지 못한 누군가가 결과를 추가/변경할 때 실수할 여지가 있을 것 같아요. 모든 케이스를 추가해볼 수도 있겠네요 🙂
추가) 딜러 - (A, 10) 블랙잭, 플레이어 - (5, 6, 10) 단순 21 에 대한 로직이 누락되었습니다!
저라면 좀 더 변경에 유연한 아래와 같은 if문을 사용할 것 같긴 한데,
코드에 정답이 없고 개인의 판단에 따라 달라지는 부분이라고 생각합니다~!
public Result judge(Dealer dealer, Player player) {
if (player.isBusted() || dealer.isBusted()) {
judgeBust()
}
if (player.isBlackjack() || dealer.isBlackjack()) {
judgeBlackjack()
}
return judgeByScore()
}| .findFirst() | ||
| .orElseThrow(() -> new IllegalArgumentException("[INTERNAL ERROR] 게임 결과를 판정할 수 없습니다")); | ||
| .findAny() | ||
| .orElseThrow(() -> new NoSuchElementException("[INTERNAL ERROR] 게임 결과를 판정할 수 없습니다")); |
There was a problem hiding this comment.
NoSuchElementException을 의도한건 맞습니다!
자주 사용하는 표준 예외에 대해서는 이펙티브 자바 아이템 72. 표준예외를 사용하라를 참고해보면 좋을 것 같아요.
근데 저도 항상 기억하고 있지는 않아서 한번씩 다시 찾아보는 편입니다 🤣
| @@ -21,11 +22,11 @@ public enum GameResult { | |||
| this.biPredicate = biPredicate; | |||
| } | |||
|
|
|||
| public static GameResult of(Dealer dealer, Player player) { | |||
| public static GameResult judge(Dealer dealer, Player player) { | |||
| this.cards = cards; | ||
| } | ||
|
|
||
| CardDeck(List<Card> cards) { | ||
| this((Queue<Card>) new LinkedList<>(cards)); |
There was a problem hiding this comment.
| this((Queue<Card>) new LinkedList<>(cards)); | |
| this(new ArrayDeque<>(cards)); |
요렇게 해볼 수 있겠네요 🙂
강제 타입 변환은 추천드리지 않습니다
안녕하세요 썬 ☀️
2단계 PR 요청 드립니다.
2단계에서는 어떤 객체가 어떤 속성을 가지고 있고 어떤 기능을 제공해야 하는지를 중점적으로 고민해봤습니다.
제가 제일 약한 부분이더군요... 쉽사리 답을 찾지 못하고 여기저기 책임을 퍼뜨린 것 같습니다.
2단계도 잘 살펴봐주시면 열심히 학습해보겠습니다
감사합니다 😊