[또링] 블랙잭 2차 미션 코드리뷰 요청합니다. #68
Conversation
첫주차 페어 종료를 위한 PR
…클래스의 checkResultType으로 이동
두번째 페어 종료를 위한 PR
src/main/java/GameController.java
Outdated
| OutputView.printGameResult(new Profits(gameResult)); | ||
| } | ||
|
|
||
| private void bet(PlayerNames playerNames, Deck deck) { |
There was a problem hiding this comment.
베팅은 딜러 -> 플레이어에게 해주는 행위처럼 보이는데요. 아래와 같이 행위의 주체인 딜러에게 메서드를 위임시켜주는건 어떨까요?
Players players = new Players(new PlayerNames(InputView.inputPlayerNames()));
Dealer dealer = new Dealer(new Deck());
players = dealer.betting(players);There was a problem hiding this comment.
안녕하세요 Havi님! 주신 피드백을 읽고 bet()에 대해 많은 고민을 하다 질문드립니다!
저는 bet이라는 행위가 "플레이어가 얼마의 금액을 걸지" 를 나타내는 행위이기 때문에 플레이어의 행위라고 생각이 들었고, Player에 위임을 해야겠다고 생각했습니다.
하지만, bet이라는 행위는 사용자로부터 금액을 입력받아 BettingMoney 객체를 생성해주는 일을 하는데 player로 로직을 옮기다 보니 도메인과 view의 분리가 어려워졌습니다. (사실 이를 위해 도메인의 메서드에 Function을 전달해주는 방법도 시도해보았는데, 아무래도 인자를 값이 아닌 함수로 받는 것이 낯설게 느껴져서 다시 수정한 상태입니다! 이러한 방법에 대해서도 어떻게 생각하시는지 궁금합니다! - 1780df5 커밋입니다!)
그래서 현재는 사용자로부터 입력을 받아 player의 bet()을 통해 생성된 BettingMoney를 Map에 저장하고 있는데이도 저도 아닌 것 같은 생각이 듭니다. (bettingMoney에 대해 생각하다보니 굳이 player가 들고있을 필요가 없다고 생각하여 필드를 삭제하였습니다)이런경우에 Havi님은 어떤식으로 구현하실지 궁금해서 여쭤봅니다. 아무래도 글로 제 의견을 전달하려다보니 조금 길어졌네요 😥 천천히 답변 주시면 또 고민해보겠습니다!
There was a problem hiding this comment.
아 제가 착각했었네요ㅎㅎ;; 베팅은 플레이어가 맞습니다.
그리고 커밋한 부분도 좋습니다. 함수형 인터페이스로 확장성을 열어두고 프린트, 인풋값을 받는 방식이라 더 좋네요 💯
마지막 답변도 좋은 선택하신것 같습니다!
| if (this.isBlackJack() && !dealer.isBlackJack()) { | ||
| return ResultType.BLACKJACK; | ||
| } | ||
| if (this.isBurst() && !dealer.isBurst()) { | ||
| return ResultType.LOSE; | ||
| } | ||
| if (dealer.isBurst() && !this.isBurst()) { | ||
| return ResultType.WIN; | ||
| } | ||
| if (this.score() > dealer.score()) { | ||
| return ResultType.WIN; | ||
| } | ||
| if (this.score() < dealer.score()) { | ||
| return ResultType.LOSE; | ||
| } | ||
| return ResultType.DRAW; |
There was a problem hiding this comment.
코드가 이쁘지 않네요ㅠ
rule이 늘어날 수록 단순한 반복문이 늘어나는 형태이기 때문에 개선이 필요해 보입니다. 전략 패턴, enum 형식, 기타 방법으로 개선해 줄 수 있을까요?ㅎ
There was a problem hiding this comment.
저도 구현하면서 마음에 들지 않는다고 생각했는데, 어떻게 개선해야할지 몰라서 우선 if를 사용하여 구현하였습니다! 힌트로 주신 전략패턴과 enum 중 무엇을 사용해야할까 고민해보다 이미 ResultType이라는 enum이 존재해서 이걸 이용하는 방향으로 수정하였습니다.
ResultType에 각 상수들을 판별하는 함수를 추가하였는데, 각각의 상황이 여러가지의 조건에 의해 일어나는 로직이라 return시에 많은 코드가 존재합니다T_T 이 부분이 아직도 깔끔하지 않다고 느껴지는데 혹시 더 좋은 방법이 있을까요?
There was a problem hiding this comment.
조건 자체가 복잡해서 이보다 간결하게 할 방법이 딱히 생각나지 않네요ㅠㅠ
| import domains.user.money.ProfitMoney; | ||
| import domains.user.name.PlayerName; | ||
|
|
||
| class ProfitsTest { |
There was a problem hiding this comment.
테스트 코드가 깔끔하네요ㅎㅎ
수익률에 대해서 비겼을 시나 다양한 케이스에 대해 조금 더 추가 되면 좋아보입니다.
| @@ -5,20 +5,13 @@ | |||
| public class User { | |||
| Hands hands; | |||
| private boolean burst = false; | |||
| boolean blackJack = false; | |||
There was a problem hiding this comment.
안녕하세요! 이제 보게 되어 코멘트 남깁니다 😥 같은 패키지 내에서의 접근이라 아무 생각 없이 default로 두었는데, 생각해보니 protected가 맞겠네요! 좋은 지적 감사합니다! 의식적으로 접근지정자를 사용해야겠네요! ㅎㅎ
안녕하세요! 이번에도 잘 부탁드립니다😀 많은 부분 지적해주시면 열심히 고민해보겠습니다!