[2단계 - 블랙잭(베팅)] 성하(김성훈) 미션 제출합니다. #531
Conversation
| } | ||
| } | ||
|
|
||
| private void drawCardOrPass(BlackJackGame blackJackGame, Player player) { | ||
| private void askPlayerAdditionalCardDrawStep(BlackJackGame blackJackGame) { |
There was a problem hiding this comment.
🎯 1단계 리팩토링 - 추가 드로우 Controller -> BlackJackGame에게 위임
원래 1단계에서 추가 드로우 시 BlackjackGame.getPlayers()로 플레이어를 다 받아서
Controller에서 반복문을 돌면서 드로우를 시키는 식으로 했었습니다.
현재 Controller에서는 출력 시 플레이어의 이름만 필요하기 때문에,
blackjackGame.getPlayerNames()를 호출해서 이름을 가져온 후에,
BlackjackGame에게 PlayerName으로 플레이어를 찾은 후, BlackjackGame 내부에서
찾은 플레이어에게 추가 카드 드로우 로직을 실행하도록 리팩토링하였습니다!
|
|
||
| import domain.card.Denomination; | ||
|
|
||
| public class TotalSumValues { |
There was a problem hiding this comment.
🎯 1단계 리팩토링 - TotalSumValues 생성 후 Hand가 가지도록 리팩토링
원래 카드의 최적 합을 계산하는 로직을 Participant가 가지고 있었는데,
Pariticipant가 Hand를 가지도록 하고 해당 Hand가 최적 합 계산 시 필요한 TotalSumValues를 가지도록 했습니다.
이렇게 하니, 각 객체의 역할이 분리가 되어 훨씬 가독성이 좋아진 것 같습니다!
그리고 TotalSumValues를 불변 객체로 만들었는데, 기능을 실행할 때마다 새로운 TotalSumValues를 생성하게 해서
불변이 보장되어 기분이 좋았던 것 같아요!
| .getProfit(); | ||
| } | ||
|
|
||
| public Map<String, Double> getFinalProfitByParticipantForPrint() { |
There was a problem hiding this comment.
🎯 2단계 질문 - 도메인에서 출력용 원시타입 반환
두 가지 고민이 있습니다!
1. ✅Map<String, Double>을 만드는 객체
FinalProfitByParticipant에서 출력용 Map<String, Double>을 만들어 주고 있는데,
해당 역할을 도메인에서 하는게 맞을 지 고민입니다!
현재 해당 메소드를 호출하는 곳은 컨트롤러인데 Map<Participant, FinalProfit>으로 반환하고,
컨트롤러에서 Map<String, Double>으로 바꾸는 것이 맞을까요?
2. Map<String, Double>로 사용하는 것이 괜찮은지
해당 메소드를 사용해서 반환받는 컨트롤러와 View 입장에서는 Key와 Value가 String과 Double이라서
어떤 값을 의미하는지 잘 모를 것 같아요!
오히려 잘 모르기 때문에 View가 도메인에 의존하지 않아서 좋은 걸까요?
There was a problem hiding this comment.
흠.. 이부분은 조금 고민이 되는데용. 우선 1번에 대해서는 어디서 하나 비슷한 것 같구 2번은 사실 이런 경우를 위해 DTO가 존재해요. 레이어간 Data를 전달하는데만 사용되는 클래스인거죠. 그래서 도메인의 값을 View로 넘기는 DTO로 변환해서 넘기고, View에서는 해당 DTO내부 값을 전달받아 출력할 수 있도록 하는..ㅎㅎㅎ 한번 고민해보셔용!
https://hudi.blog/data-transfer-object/ <= 이 글의 레퍼런스로 달린 글들도 한번 봐보세용
| private final int money; | ||
|
|
||
| public BettingMoney(final int money) { | ||
| validate(money); |
There was a problem hiding this comment.
🎯 2단계 질문 - BettingMoney가 숫자가 아닐 때 검증
현재 InputView에서 BettingMoney를 받을 때, String으로 받아오고
해당 String을 Integer.parseInt()로 변환을 거친 후 BettingMoney로 생성하고 있습니다.
저는 Integer.parseInt()에서 예외가 발생하는 것도 BettingMoney에서 관리하도록 하고 싶은데,
String 인자를 받는 생성자를 만들어도 validate를 못하더라고요!
해당 검증을 BettingMoney에서 하는 방법이 없을까요?!
There was a problem hiding this comment.
모가 안된다는 건지 이해가 잘 안되어요!
아래처럼 하고 싶으시단게 맞나요?
public BettingMoney(final String money) {
this.money = validate(money);
}
private int validate(final String inputMoney) {
int money = validateInt(inputMoney);
if (isLessOrEqualZero(money)) {
throw new IllegalArgumentException(Constants.ERROR_PREFIX + LESS_OR_EQUAL_ZERO_ERROR_MESSAGE);
}
return money;
}
private static int validateInt(String inputMoney) {
try {
return Integer.parseInt(inputMoney);
} catch (Exception e) {
throw new IllegalArgumentException("숫자 아님");
}
}
| finalProfitByParticipant.putParticipantFinalProfit(player, new FinalProfit(bettingMoney.getValue())); | ||
| } | ||
|
|
||
| public Double calculateDealerFinalProfit() { |
There was a problem hiding this comment.
🎯 2단계 질문 - 다른 클래스에서 같은 메소드명 사용
현재 BettingManager와 FinalProfitByParticipant에서
동일하게 calculateDealerFinalProfit()을 네이밍으로 사용하고 있습니다!
코드만 봤을 때 메소드 시그니처와 메소드 바디의 메소드 명이 같아서 조금 찝찝한 느낌이 드는데,
서로 객체가 다르기 때문에 괜찮은 걸까요?
아니면 신경써서 네이밍을 다르게 해야할까요?
There was a problem hiding this comment.
BettingManager에서 단순 반환만 해서 별 문제는 없어보여요. 근데 코드 구현 상으로 dealer를 특정한 구현은 아닌 것 같은데 메서드명에 꼭 dealer 들어가는 이유는 무엇인가요??
There was a problem hiding this comment.
메소드 바디의 계산 로직이 딜러의 최종 수익을 구하는 로직이라서 dealer를 메소드명에 명시했습니다!
모든 플레이어의 최종 수익을 합해서 -1을 곱한 값을 반환하는데,
해당 값이 딜러의 최종 수익이라서 메소드명에 딜러를 포함했습니다!
| private void calculatePlayerFinalProfit(Player player, Dealer dealer) { | ||
| Hand playerHand = player.getHand(); | ||
| BettingMoney bettingMoney = bettingMoneyByPlayer.findBettingMoneyByPlayer(player); | ||
| if (playerHand.isBlackjack()) { |
There was a problem hiding this comment.
🎯 2단계 질문 - if문 중복 제거 관련
BettingManager에서 배팅 금액 계산 시 if문을 반복해서 사용하고 있는데,
해당 구조는 '상태 패턴'을 적용하지 않고는 바꾸지 못하는 걸까요?
네오의 피드백 강의에서 상태 패턴을 적용하는 모습을 봤었는데, 잘 모르고 적용했다가
구조가 다 망가질 것 같아서 일단은 조앤의 리뷰를 들어보고 적용할지 말지 결정하려고 합니다!
지금 if문이 많이 쓰인 구조가 맘에 들지 않지만 다른 방법이 떠오르지 않아서 일단 제출하게 되었습니다 ㅠㅠ
There was a problem hiding this comment.
우선 이 메서드는 10라인이 넘어가기 때문에 리팩터링 대상이 되는 건 맞구요ㅎㅎ
상태 패턴을 어떤 경우에 적용하면 좋을까요?
위에서 코멘트 남긴 FinalProft과도 유사한데요, 블랙잭 게임은 여러가지 상태를 갖기 때문에 외부에서 결과값을 가져오기 위해서는 플레이어와 딜러가 어떤 상태인지를 계속 물어봐야해요. 지금의 구현된 방식처럼요. 하지만 만약, 플레이어와 딜러가 State(가칭)를 갖고 있고, 그 State 안에서 State가 블랙잭인지, draw할 수 있는지, 이미 끝났는지, 해당 상태에서 베팅 금액이 n원일 때 최종 profit은 얼마인지를 관리할 수 있겠죠. 그럼 외부에서는 player.getProfit(money)을 호출하고, getProfit 메서드 내부에서는 player가 가진 state에게 state.profit(money)를 호출함으로써 반복되는 if문을 제거할 수 있게 되는 거죠.
State의 예시 인터페이스를 구현해보자면 아래와 같을 거에요.ㅎㅎ
public interface State {
State draw(final Card card);
State stay();
boolean isFinished();
double profit(final Money money);
}
https://johngrib.github.io/wiki/pattern/state/
하지만 말씀하신대로 구조 변경도 많을거라 공수가 꽤 들거예요. 다만 경험해보시면 훨씬 코드가 깔끔해지고 가독성이 좋아질 거예요! 특히 BettingManager, FinalProfitByParticipant 등 몇 가지 클래스들은 다 반복되는 로직들로만 이루어져있는데, 이러한 클래스들이 줄어들 수 있을 거예요. 고민해보시고 선택적으로 적용해주세요!
| this.finalProfitByParticipant = new LinkedHashMap<>(); | ||
| } | ||
|
|
||
| public void putParticipantFinalProfit(Participant participant, FinalProfit finalProfit) { |
There was a problem hiding this comment.
🎯 2단계 질문 - 테스트를 위한 Getter 사용
현재 참가자별 배팅 금액과 참가자별 최종 수익이 있는
BettingMoneyByPlayer와 FinalProfitByParticipant에서
getter를 사용하지 않기 때문에 배팅 금액 저장과, 최종 수익 저장 테스트를 하지 못했습니다,,
원래는 테스트를 위해 getter를 써야한다면 해당 테스트를 포기했었는데,
배팅 금액 저장과 최종 수익 저장은 중요한 핵심 로직이기 때문에 테스트를 해야할 것 같은데
이러한 테스트를 하기 위해서 사용되지 않을 getter를 사용하는게 맞을지 고민입니다!
There was a problem hiding this comment.
getter를 사용하지 않기 때문에 배팅 금액 저장과, 최종 수익 저장 테스트를 하지 못했습니다
-> 배팅 금액 저장 및 최종 수익 저장에 대한 테스트는 자료구조 Map에 대한 테스트를 하는 것과 마찬가지인 것 같아요.
성하가 의도하신 테스트를 수행하시기 위해서는 BettingMoneyByPlayer와 FinalProfitByParticipant에 "어떤 값을 저장"하도록 하는 곳을 테스트하면 될 것 같아요, 즉. 지금은 예를 들어 BettingManager에서 FinalProfit을 계산하여 집어넣어주고 있으니 이 부분을 테스트하시면서 해결할 수 있을 것 같아요.
근데 문제는, 현재 BettingManager에서 핵심 로직을 다루고 있는데 이와 관련된 테스트가 하나도 없다는 점이죠. calculate와 관련된 모든 로직이 해당 클래스에 모두 들어있고, 오픈된 메서드는 calculateParticipantFinalProfit 하나밖에 없는데, 외부에서 테스트하기가 어려운 구조예요. 어떻게 하면 계산된 결과값을 정확히 반환하는지 테스트할 수 있을까요? 해당 PR에 위에서부터 남긴 여러가지 코멘트들에서 답을 얻으실 수 있을거라 생각해요!!!
| dealerDrawCardStep(blackJackGame); | ||
| printParticipantFinalCardsStep(blackJackGame, blackJackGame.getPlayerNames()); | ||
| printFinalFightResultStep(blackJackGame); | ||
| BettingManager bettingManager = new BettingManager(); |
There was a problem hiding this comment.
요구사항이 추가될 때마다 run 메서드의 길이가 점점 늘어나는데, 크게 단계로 나눠서 한번 더 메서드로 분리해주셔도 좋을 것 같아요. 지금도 딱 10라인이네요.
There was a problem hiding this comment.
게임 시작 단계 - 추가 카드 드로우 단계 - 결과 출력 단계
이렇게 3가지 단계로 분리하여 리팩토링하였습니다!! 😃
| return new FinalProfit(this.profit + finalProfit.profit); | ||
| } | ||
|
|
||
| public FinalProfit convertToNegative() { |
There was a problem hiding this comment.
convertToNegative라는 메서드는 FinalProfit 내부에서만 알고 있고 외부에서는 단순히 최종 profit만을 알도록 구현해도 될 것 같아요.
즉, 현재는 외부에서 FinalProfit을 계산할 때 아래처럼 하고 있는데,
// FinalProfitByParticipant.java line 24
return finalProfitByParticipant.values()
.stream()
.reduce(new FinalProfit(INIT_FINAL_PROFIT_VALUE), FinalProfit::add)
.convertToNegative()
.getProfit();
다음처럼 변경해봐도 될 것 같아요. 즉, 외부에서는 getProfit만을 호출하면 FinalProfit 객체에서 최종 profit을 반환해준다고 믿고 사용하는 거죠.
public Double calculateDealerFinalProfit() {
return finalProfitByParticipant.values()
.stream()
.reduce(new FinalProfit(INIT_FINAL_PROFIT_VALUE), FinalProfit::add)
.getProfit();
}
There was a problem hiding this comment.
현재 getProfit()을 딜러의 최종 수익을 계산할 때 밖에 사용하지 않고 있습니다!
그래서 convertToNegative()를 getProfit() 안에 넣게 되면
'딜러의 최종 수익'을 반환하는 의미를 네이밍에 줘야할 것 같은데
FinalProfit의 메소드가 해당 의미를 아는 네이밍을 가지는 것은 적절하지 않은 것 같아서 그대로 놔두려고 하는데, 괜찮을까요?!
There was a problem hiding this comment.
흠.. finalProfit안의 profit은 player에게는 최종수익이고 dealer에겐 최종 수익이 아닌거군요. 사용처에 따라 프로퍼티가 다른 의미를 가지게 된다면 일관적이지 못한 것 같아요. 차라리 외부에서 FinalProfit을 생성할 때 딜러의 경우엔 negative로 변환한 채로 넣어 finalProfit이 가지는 profit은 최종 수익임을 보장할 수 있는 구조가 나을 것 같은데, 어떻게 생각하시나요?
| private void calculatePlayerFinalProfit(Player player, Dealer dealer) { | ||
| Hand playerHand = player.getHand(); | ||
| BettingMoney bettingMoney = bettingMoneyByPlayer.findBettingMoneyByPlayer(player); | ||
| if (playerHand.isBlackjack()) { |
There was a problem hiding this comment.
우선 이 메서드는 10라인이 넘어가기 때문에 리팩터링 대상이 되는 건 맞구요ㅎㅎ
상태 패턴을 어떤 경우에 적용하면 좋을까요?
위에서 코멘트 남긴 FinalProft과도 유사한데요, 블랙잭 게임은 여러가지 상태를 갖기 때문에 외부에서 결과값을 가져오기 위해서는 플레이어와 딜러가 어떤 상태인지를 계속 물어봐야해요. 지금의 구현된 방식처럼요. 하지만 만약, 플레이어와 딜러가 State(가칭)를 갖고 있고, 그 State 안에서 State가 블랙잭인지, draw할 수 있는지, 이미 끝났는지, 해당 상태에서 베팅 금액이 n원일 때 최종 profit은 얼마인지를 관리할 수 있겠죠. 그럼 외부에서는 player.getProfit(money)을 호출하고, getProfit 메서드 내부에서는 player가 가진 state에게 state.profit(money)를 호출함으로써 반복되는 if문을 제거할 수 있게 되는 거죠.
State의 예시 인터페이스를 구현해보자면 아래와 같을 거에요.ㅎㅎ
public interface State {
State draw(final Card card);
State stay();
boolean isFinished();
double profit(final Money money);
}
https://johngrib.github.io/wiki/pattern/state/
하지만 말씀하신대로 구조 변경도 많을거라 공수가 꽤 들거예요. 다만 경험해보시면 훨씬 코드가 깔끔해지고 가독성이 좋아질 거예요! 특히 BettingManager, FinalProfitByParticipant 등 몇 가지 클래스들은 다 반복되는 로직들로만 이루어져있는데, 이러한 클래스들이 줄어들 수 있을 거예요. 고민해보시고 선택적으로 적용해주세요!
| } | ||
| } | ||
|
|
||
| private void calculatePlayerFinalProfit(Player player, Dealer dealer) { |
There was a problem hiding this comment.
아래 맥락과는 별개로 또 말씀드리고 싶은 부분이, 지금 isBlackJack, isBust, isStay에 따라서 계산 로직이 달라지는 건 없이 단순히 finalProfit에 곱해지는 값이 달라지는데요. 또다른 개선 방법은 상태를 나타내는 ENUM을 만들어 그 안에 로직을 넣을 수 있을 것 같아요.
public enum Result {
WIN(money -> money),
LOSE(money -> money * -1),
BLACKJACK(money -> (int) (money * 1.5))
// etc..
;
private IntUnaryOperator operator;
Result(IntUnaryOperator operator) {
this.operator = operator;
}
public int calculate(int money) {
return operator.applyAsInt(money);
}
}
그렇다면 결과로 나온 Result에게 money를 넣어 calculate 만 요청하면 isBlackJack인지... 등등의 여부와 관계없이 결과값이 계산된 최종 결과금액이 나올 수 있을 것 같아요~
BettingManager 내부에서 중복되는 부분이 많아 이런 저런 방법들을 제안드리는거니 성하가 보시고 알맞은 방법을 적용해주세요~
There was a problem hiding this comment.
이런 방법은 생각 못했었는데 감사합니다!!
덕분에 UnaryOperator라는 함수형 인터페이스도 알아가는 것 같아요! 😃
| private void saveBlackjackProfit(Player player, BettingMoney bettingMoney) { | ||
| finalProfitByParticipant.putParticipantFinalProfit(player, new FinalProfit(bettingMoney.getValue() * BLACKJACK_VALUE)); | ||
| } | ||
|
|
||
| private void saveLoseProfit(Player player, BettingMoney bettingMoney) { | ||
| finalProfitByParticipant.putParticipantFinalProfit(player, new FinalProfit(bettingMoney.getValue() * LOSE_VALUE)); | ||
| } |
There was a problem hiding this comment.
곱해지는 Value 제외 중복! 아래 saveWonProfit도 마찬가지예용
| private final int money; | ||
|
|
||
| public BettingMoney(final int money) { | ||
| validate(money); |
There was a problem hiding this comment.
모가 안된다는 건지 이해가 잘 안되어요!
아래처럼 하고 싶으시단게 맞나요?
public BettingMoney(final String money) {
this.money = validate(money);
}
private int validate(final String inputMoney) {
int money = validateInt(inputMoney);
if (isLessOrEqualZero(money)) {
throw new IllegalArgumentException(Constants.ERROR_PREFIX + LESS_OR_EQUAL_ZERO_ERROR_MESSAGE);
}
return money;
}
private static int validateInt(String inputMoney) {
try {
return Integer.parseInt(inputMoney);
} catch (Exception e) {
throw new IllegalArgumentException("숫자 아님");
}
}
| return money <= 0; | ||
| } | ||
|
|
||
| public int getValue() { |
There was a problem hiding this comment.
앗 실수했습니다! 바로 리팩토링했습니다! 감사합니다!!
| private int getCardValueSumMax(TotalSumValues totalValues, List<Integer> handCardValues) { | ||
| int maxSumValue = 0; | ||
| if (handCardValues.contains(Denomination.ACE_VALUE)) { | ||
| maxSumValue = totalValues.getLessThanLimitMaxValue(); | ||
| } | ||
| if (!handCardValues.contains(Denomination.ACE_VALUE)) { | ||
| maxSumValue = totalValues.getMaxValue(); | ||
| } | ||
| return maxSumValue; | ||
| } |
There was a problem hiding this comment.
| private int getCardValueSumMax(TotalSumValues totalValues, List<Integer> handCardValues) { | |
| int maxSumValue = 0; | |
| if (handCardValues.contains(Denomination.ACE_VALUE)) { | |
| maxSumValue = totalValues.getLessThanLimitMaxValue(); | |
| } | |
| if (!handCardValues.contains(Denomination.ACE_VALUE)) { | |
| maxSumValue = totalValues.getMaxValue(); | |
| } | |
| return maxSumValue; | |
| } | |
| private int getCardValueSumMax(TotalSumValues totalValues, List<Integer> handCardValues) { | |
| return handleAce(totalValues, handCardValues); | |
| } | |
| private static int handleAce(TotalSumValues totalValues, List<Integer> handCardValues) { | |
| if (handCardValues.contains(Denomination.ACE_VALUE)) { | |
| return totalValues.getLessThanLimitMaxValue(); | |
| } | |
| return totalValues.getMaxValue(); | |
| } |
There was a problem hiding this comment.
항상 코드까지 직접 예시로 작성해주셔서 감사합니다!! 😃
리팩토링 하도록 하겠습니다!
| this.finalProfitByParticipant = new LinkedHashMap<>(); | ||
| } | ||
|
|
||
| public void putParticipantFinalProfit(Participant participant, FinalProfit finalProfit) { |
There was a problem hiding this comment.
getter를 사용하지 않기 때문에 배팅 금액 저장과, 최종 수익 저장 테스트를 하지 못했습니다
-> 배팅 금액 저장 및 최종 수익 저장에 대한 테스트는 자료구조 Map에 대한 테스트를 하는 것과 마찬가지인 것 같아요.
성하가 의도하신 테스트를 수행하시기 위해서는 BettingMoneyByPlayer와 FinalProfitByParticipant에 "어떤 값을 저장"하도록 하는 곳을 테스트하면 될 것 같아요, 즉. 지금은 예를 들어 BettingManager에서 FinalProfit을 계산하여 집어넣어주고 있으니 이 부분을 테스트하시면서 해결할 수 있을 것 같아요.
근데 문제는, 현재 BettingManager에서 핵심 로직을 다루고 있는데 이와 관련된 테스트가 하나도 없다는 점이죠. calculate와 관련된 모든 로직이 해당 클래스에 모두 들어있고, 오픈된 메서드는 calculateParticipantFinalProfit 하나밖에 없는데, 외부에서 테스트하기가 어려운 구조예요. 어떻게 하면 계산된 결과값을 정확히 반환하는지 테스트할 수 있을까요? 해당 PR에 위에서부터 남긴 여러가지 코멘트들에서 답을 얻으실 수 있을거라 생각해요!!!
| .getProfit(); | ||
| } | ||
|
|
||
| public Map<String, Double> getFinalProfitByParticipantForPrint() { |
There was a problem hiding this comment.
흠.. 이부분은 조금 고민이 되는데용. 우선 1번에 대해서는 어디서 하나 비슷한 것 같구 2번은 사실 이런 경우를 위해 DTO가 존재해요. 레이어간 Data를 전달하는데만 사용되는 클래스인거죠. 그래서 도메인의 값을 View로 넘기는 DTO로 변환해서 넘기고, View에서는 해당 DTO내부 값을 전달받아 출력할 수 있도록 하는..ㅎㅎㅎ 한번 고민해보셔용!
https://hudi.blog/data-transfer-object/ <= 이 글의 레퍼런스로 달린 글들도 한번 봐보세용
|
안녕하세요 조앤!! 상태 패턴은 적용해보려고 했지만, 너무 어려워서 실패했습니다.. 그래도 조앤이 올려주신 참고 링크와 보여주신 코드 예시 덕분에 상태 패턴 지식을 좀 더 쌓게 되었습니다. 아래에 주요 리팩토링과 리팩토링 시 생긴 질문들을 나열해보겠습니다! 🙇♂️ ✅ 주요 리팩토링1. BettingManager의 역할 분리 & 중복되는 로직 제거 & 핵심 로직 테스트리팩토링 전 BettingManager는 다음과 같은 역할을 가지고 있었습니다. 이렇게 BettingManager가 많은 역할을 가지고 있어서 이로 인해 핵심 로직 테스트가 어려웠습니다. 또한 최종 수익을 계산하기 위해서 플레이어와 딜러의 상태를 체크했었는데 조앤이 말씀해주신 방향 덕분에 이렇게 하니 2. DTO 생성하여 ResultView에 전달리팩토링 전에는 ResultView에서 DTO를 만들어서 ResultView에서 DTO를 직접 전달해서 ResultView에서 DTO의 getter로 값을 받아오니, 하지만 해당 DTO로 바로 접근해서 어떤 key, value인지 알 수 있다는 점이 장점이라고 생각했고 🎯 리팩토링 시 생긴 질문1. if문 제거 리팩토링 시 의미가 잘 전달될까?
위의 사진은 조앤이 코멘트 주셨던 if문 제거 방식입니다!! handCardValues에 ACE_VALUE가 포함되지 않으면 의 의미가 명시적으로 전달되지 않는다고 생각되어 if문을 2번 사용했었습니다! 조앤이 추천해서 리팩토링한 방식처럼 handCardValues에 ACE_VALUE가 포함되지 않으면 의 의미를 명시하지 않아도 2. 상태 패턴 적용 생각의 흐름제가 상태 패턴을 이번에 처음 배워서 그런지 지금 다시 블랙잭 게임 설계 때로 돌아간다고 해도 그래서 조앤은 상태 패턴 관련해서 처음부터 설계 시에 '상태로 묶일 수 있겠다!, 상태 패턴을 적용해야겠다!'라는 생각을 하시는지 실제 프로젝트 설계에서 설계 시에 떠오르는 경우가 많은지, 아니면 리팩토링 시에 떠오르는 경우가 많은지 궁금합니다!! 😃 |
There was a problem hiding this comment.
안녕하세요, 성하ㅎㅎ 새벽에 작업을 해주셨군요
첫번째 제출해주신 코드부터 지금까지 굉장히 많은 것들이 변한 것 같아요. 성하는 지금 최종적으로 구현된 코드가 만족스러우실지 궁금하네요ㅎㅎ
많은 리뷰를 주고 받은 것 같아 이번 단계는 우선 여기서 머지할게요, 아직 코멘트 여러개 남겨뒀는데 추가로 반영해보시고 질문 있으시면 DM 주셔도 좋아요. 코멘트는 꼭 살펴보시길 바라요! 고생 많으셨어요~
- 상태패턴과 관련해서 남겨주신 질문에 대한 답변 드려요. 저는 우선 if문이 여러번 반복될 때 고민해보는 편이에요. 다만 이전에 상태패턴을 적용했던 패턴과 유사한 코드가 나온다면, 경험적으로 적용하는 것이 더 낫다고 판단해 바로 적용해보는 경우도 있어요. 즉, 경험이 굉장히 중요하다는 말을 전해드리고 싶어요.ㅎㅎ 블랙잭 미션을 통해 상태패턴을 적용하는 걸 경험해보셨다면, 다음번 미션에서 유사한 구조가 발견될 때 이건 상태패턴을 적용했을 때 좀 더 깔끔해지겠구나~하는 아이디어가 생기실 거예요. 그래서 이래나저래나 우테코 동안 많은 걸 경험해보시길 추천드려요ㅎㅎ
| return new FinalProfit(this.profit + finalProfit.profit); | ||
| } | ||
|
|
||
| public FinalProfit convertToNegative() { |
There was a problem hiding this comment.
흠.. finalProfit안의 profit은 player에게는 최종수익이고 dealer에겐 최종 수익이 아닌거군요. 사용처에 따라 프로퍼티가 다른 의미를 가지게 된다면 일관적이지 못한 것 같아요. 차라리 외부에서 FinalProfit을 생성할 때 딜러의 경우엔 negative로 변환한 채로 넣어 finalProfit이 가지는 profit은 최종 수익임을 보장할 수 있는 구조가 나을 것 같은데, 어떻게 생각하시나요?
| return handleAce(totalValues, handCardValues); | ||
| } | ||
|
|
||
| private int handleAce(TotalSumValues totalValues, List<Integer> handCardValues) { |
There was a problem hiding this comment.
1번째 질문에 대한 답을 달아보자면, 저는 개인적으로 말씀해주신대로 ACE_VALUE가 포함된 경우와 그렇지 않은 경우 두가지로 나뉘기 때문에 괜찮다고 생각해요!
| printResultStep(blackJackGame, bettingManager); | ||
| } | ||
|
|
||
| private void startGameStep(BlackJackGame blackJackGame, BettingManager bettingManager) { |
There was a problem hiding this comment.
startGameStep에서 사용되는 private 메서드들을 바로 하단에 두는게 읽기에 더 편리할 것 같아요~!
| return findPlayer.canHit(); | ||
| } | ||
|
|
||
| public AdditionalDrawStatus distributePlayerCardOrPass(String playerName, String receiveOrNot) { |
There was a problem hiding this comment.
| public AdditionalDrawStatus distributePlayerCardOrPass(String playerName, String receiveOrNot) { | |
| public AdditionalDrawStatus distributePlayerCardOrPass(String playerName, String receiveOrNot) { | |
| Player findPlayer = participants.findPlayerByPlayerName(playerName); | |
| if (receiveOrNot.equals(ADDITIONAL_DRAW_CARD_OK_SIGN)) { | |
| findPlayer.takeCard(deck.drawCard()); | |
| return AdditionalDrawStatus.DRAW; | |
| } | |
| return AdditionalDrawStatus.PASS; | |
| } |
는 어떨까요?
| if ((dealerHand.isBlackjack() || hand.isBust()) || | ||
| (hand.isStay() && dealerHand.isStay() && isLoseWhenStay(dealerHand))) { |
There was a problem hiding this comment.
조건문이 길어지면 메서드로 빼주시는게 어떨까요?
딜러의 패가 블랙잭이거나 패가 버스트거나 혹은 패가 스테이이며 딜러의 패가 스테이이며 딜러의 패가 스테이일 때 지는 경우 라고 읽히는데, 이렇게 조건문이 길어지면 유지보수하기가 굉장히 힘들어요.
| AdditionalDrawStatus additionalDrawStatus = AdditionalDrawStatus.DRAW; | ||
| while (AdditionalDrawStatus.isDrawable(additionalDrawStatus) && blackJackGame.canPlayerDrawCard(player)) { | ||
| String playerName = player.getName(); | ||
| while (AdditionalDrawStatus.isDrawable(additionalDrawStatus) && blackJackGame.canPlayerDrawCard(playerName)) { |
There was a problem hiding this comment.
additionalDrawStatus에게 직접 물어보는게 어떨까요?
| while (AdditionalDrawStatus.isDrawable(additionalDrawStatus) && blackJackGame.canPlayerDrawCard(playerName)) { | |
| while (additionalDrawStatus.isDrawable() && blackJackGame.canPlayerDrawCard(playerName)) { |
|
|
||
| public Players getPlayers() { | ||
| return players; | ||
| public List<Player> getRawPlayers() { |
| printFinalProfit(bettingManager); | ||
| } | ||
|
|
||
| private void printFinalCardResult(BlackJackGame blackJackGame) { |
There was a problem hiding this comment.
dealer 수행 -> 프린트 -> player 수행 -> 프린트가 아닌
participants 들에 대해 final Result 조회 -> 프린트 로 가면 좀 더 깔끔할 것 같아요. 전반적인 로직이 전부 유사한데 딜러, 플레이어 각각 수행하니 중복된 코드들이 발생하고 있어요.
|
|
||
| import java.util.function.DoubleUnaryOperator; | ||
|
|
||
| public enum PlayerBettingResult { |
| public int calculateOptimalCardValueSum() { | ||
| List<Integer> initValues = new ArrayList<>(); | ||
| initValues.add(INIT_VALUE_FOR_ADD); | ||
| TotalSumValues initTotalSumValues = new TotalSumValues(initValues); | ||
| List<Integer> handCardValues = getCardValues(); | ||
| TotalSumValues totalSumValues = initTotalSumValues.addValuesToAllElement(initTotalSumValues, handCardValues); | ||
|
|
||
| return getCardValueSumMax(totalSumValues, handCardValues); | ||
| } |
There was a problem hiding this comment.
init 값을 갖는 리스트를 할당하는 TotalSumValues 생성자를 만들고, 그 안에서 addValuesToallElement를 처리하게 하는건 어떨까요? 리스트나 값을 외부에서 선언, 내부로 그 값을 넘겨 계산하는 로직이 반복되는 곳이 많은 것 같아요. 내부에서 처리할 수 있는 것들은 최대한 객체 내부로 옮겨보는 연습을 해 보시면 좋을 것 같아요.
조앤 안녕하세요! 5기 성하입니다! 😃
이번 2단계 미션은 참가자별 배팅 금액과 참가자별 최종 수익을 어디에 둘지 고민을 많이 했습니다!
처음에는 단순히 요구사항에서
배팅 금액을 딜러에게 받는다라고 나와있어서 딜러에게 넣었었습니다.그러나 딜러도 Participant의 자식 객체로, 참가자이기 때문에 새로운 배팅 관련 역할을 책임지는
BettingManager를 만들어서Controller에서
BlackjackGame과BettingManager를 생성하여 게임을 진행하도록 했습니다!진행 흐름에서, 처음 2장의 카드를 받았을 때 딜러가 블랙잭인 경우
플레이어의 추가 카드 요청 단계를 Skip하고 모든 플레이어를 Lose 처리하여 바로 최종 수익이 출력되도록 했습니다!
🎯 1단계 리팩토링
이번 2단계를 시작하기 전에 1단계를 리팩토링한 부분은 다음과 같습니다.
해당 부분과 관련해서는 어떤 코드를 리팩토링 했는지 관련 코드에 코멘트 남기도록 하겠습니다!
🎯 2단계 진행하면서 생긴 질문
질문들이 다 코드단의 질문들이라서 질문들을 코드에 코멘트 달도록 하겠습니다!! 😃
항상 리뷰해주셔서 감사합니다! 🙇♂️