-
Notifications
You must be signed in to change notification settings - Fork 389
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단계 - 블랙잭 베팅] 우르(김현우) 미션 제출합니다. #512
Conversation
- 테스트 코드 다시 작성
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 우르 👋
2단계 구현도 잘 해주셨네요.
몇 가지 코멘트를 남겼으니 확인해주세요~
@@ -8,7 +8,8 @@ public class Score { | |||
|
|||
private static final Score UPPER_LIMIT_SCORE = new Score(21); | |||
|
|||
private static final Score REMAIN_SCORE_ACE = new Score(10); | |||
private static final Score SOFT_HAND_SCORE = new Score(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Score는 점수를 담는 값 객체,, 그래서 한정지으면 안된다??
보통 Score라는 객체를 만들 때 모든 점수값을 담을 수 있는 범용적인 Score 객체로 만들지는 않습니다.
이 객체 같은 경우 좀 더 분명하게 이름을 짓는다면 BlackjackScore 정도 되겠죠.
다만, 지금은 블랙잭을 다루는 어플리케이션이기 때문에 굳이 이렇게 지을 필요는 없는 것 같아요.
대부분의 리팩토링은 정말 필요할 때 해도 됩니다. 나중에 다른 게임이 추가되고 이 객체의 이름이 헷갈린다는 생각이 들면 그 때 해도 될거에요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇군요,, 저는 Score나 Money 와 같은 객체들은 범용적으로 사용한다는 생각을 당연하게 하고 있었습니다
그래서 위와 같은 고민이 생겼구요,,
범블비 말씀대로 범용적은 Score를 만드는게 일반적이지 않다면 필요할 때 리팩토링하는 식으로 진행하는게 합리적으로 보입니다~~
public void firstMatchWith(final Dealer dealer) { | ||
if (isBlackjack() && dealer.isBlackjack()) { | ||
betResultState = new BreakEvenState(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Participants의 배팅 결과 상태는 메서드 파라미터로 받을까? 상태로 가지고 있는게 맞을까?
참가자는 money만 갖고 있고, 결과를 계산하는 객체를 따로 만들 수 있지 않을까요?
void는 어떻게 테스트 하는가?
참가자가 결과를 저장하는 것이 아닌 계산한다면 리턴 타입도 생길테니 이 문제도 자연스럽게 해결될 거 구요.
null object 패턴?
아마 초기 상태를 지정해줄 필요도 없어질테니 필요한 상태만 갖게 될 것 같네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (isBlackjack() && dealer.isBlackjack()) { | ||
betResultState = new BreakEvenState(); | ||
} | ||
if (isBlackjack() && !dealer.isBlackjack()) { | ||
betResultState = new WinState(); | ||
} | ||
if (!isBlackjack() && dealer.isBlackjack()) { | ||
betResultState = new LoseState(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if 문이 많으면 의심해봐라
interface Condition {
Boolean canApply(Player player, Dealer dealer);
BetResultState state();
}
class PlayerBust implements Condition {
@Override
public Boolean canApply(Player player, Dealer dealer) {
return player.isBust();
}
...
}
class ConditionFinder {
private final List<Condition> conditions = List.of(
new PlayerBust(),
...
);
public BetResultState findStateOf(Player player, Dealer dealer) {
conditions.stream()
.filter(it -> it.canApply(player, dealer))
...
}
}
이런식으로 가능은 한데(이름 같은 건 대충 지었으니 넘어가주세요 ㅎㅎ), if문 쓰는 것과 어떤 부분이 다른지 고민해보시면 좋을 것 같아요.
전 지금 정도의 if문이면 그냥 if문을 사용하는게 훨씬 낫다고 생각하는 편입니다. 조건이 많으면 당연히 if 문이 많은 게 정상이겠죠.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
범블비 말씀대로 해보니 같은 내용 리뷰
에서도 말씀해주셨던 내용이 해결이 되네요,,
다만 if 문의 분기가 많을수록 그 클래스가 많아지네요,,
저의 코드 만족도는 높지만, 제 코드를 보여주고 설명이 필요할 것 같은 느낌이 들어요,,, ㅋㅋㅋㅋ
제가 느낀 if 문과의 차이는 책임이 Participant -> BetResultFinder 로 변경이 됐고, if 문의 분기들이 하나하나 클래스로 만들어졌습니다. 클래스가 많아지지만, 그 이름을 명확히 해서 클래스들만 봐도 그 분기들을 정확히 파악할 수 있는 장점이 있는 것 같습니다. 저는 너무 적은 클래스보다는 너무 많은 클래스가 더 괜찮다고 생각해서요
범블비 덕분에 좋은 방법 하나 얻고 갑니다 !! 감사합니다
return !(hasNotBetState()); | ||
} | ||
|
||
public void firstMatchWith(final Dealer dealer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void는 어떻게 테스트 하는가?
앞 답변과 별개로 void를 정말 테스트하고 싶다면 상태 검증 vs 행위 검증에 대해 찾아보시면 좋을 것 같아요. 지금은 너무 깊게 학습할 필요없이 가볍게 읽어보고, 나중에 필요하실 때 다시 찾아서 적용해보시면 좋을 것 같네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존에 verify 를 사용해서 테스트를 해본 적이 있는데 이게 상태 검증, 행위 검증으로 나뉘는지는 몰랐네요,, 범블비 덕분에 새로운 개념을 알게 됐습니다 !!
말씀대로 저도 if 분기에 걸리면 무조건 return 을 하기 때문에 verify 메서드를 통해서 해당 메서드가 호출됐는지 안됐는지 확인해보려고 했는데,, 아직 mockito 를 사용하는 건 미션에 조금 벗어나는 것 같아서 사용하지 못했습니다,,
|
||
public interface BetResultState { | ||
|
||
BetResultState NULL = money -> Money.MIN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null object 패턴?
만약 참가자 내부에 state를 저장한다고 결정했으면 지금 방식도 괜찮아보입니다.
대신 이 방법을 쓸 때 저는 정말 내가 null을 나타내고 싶은지에 대해 생각하는 편이에요.
null이 아닌 특정 상태를 나타내고 싶은데 그냥 null을 사용하는 건지, 정말 null인 상태를 나타내기 위해 null을 사용하는 건지 생각해보면 좋을 것 같네요.
그런 의미에서 아마 NULL에서 calculateBetOutcomeOf을 호출하면 예외가 발생하게 하는 것도 괜찮아보이네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 이제 null 을 나타내고 싶은가보다는 아무 상태가 아님을 나타내고 싶어서 표현했던 것 같습니다.
그리고 말씀대로 NULL 이나 아무 상태가 아닌 클래스를 하나 만들고, 거기에 연산을 사용하면 예외가 발생하게 하는 것도 괜찮은 방법인 듯 합니다.
다만 범블비가 "null 을 나타내고 싶은지"라는 말씀이 정확히 와닿지가 않아서요,, 혹시 범블비가 null 을 나타내고 싶은 상황이 있었다면 그 상황이 어떤 상황인지 왜 그렇게 사용하셨는지 말씀해주실 수 있으실까요!!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코멘트에 답변하는 걸 까먹었네요 😅
null은 무엇인가가 "없는" 것을 표현하기 위한 거죠. 그래서 결과가 "없다는" 걸 표현하기에 괜찮다고 생각했어요.
조금 반대로 null을 사용하기에 적합하지 않은 예시를 들어보자면 null에 의미를 부여하는 경우가 있어요. 예를 들면 boolean을 true/false 대신 true/null과 같은 식으로 나타내는 경우인데요, 코딩을 하다보면 편의를 위해 null을 사용하고 거기에 의미를 부여하고 싶을 때가 있을거에요. 그런 경우를 주의했으면 좋겠어서 남긴 코멘트입니다.
public interface BetResultState { | ||
|
||
BetResultState NULL = money -> Money.MIN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여담이지만 여기 money -> Money.MIN;
처럼 함수형으로 사용하실 거라면 @Funtionalnterface
를 붙여서 함수형 인터페이스라는 사실을 명시해주면 좋습니다.
An informative annotation type used to indicate that an interface type declaration is intended to be a functional interface as defined by the Java Language Specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아,, @FunctionalInterface
해주는게 좋아 보이네요,, 잊지 않겠습니다!!
} | ||
|
||
public abstract List<Card> faceUpFirstDeal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
faceUpFirstDeal 할 때, Hand 반환 List 반환?
아마 양쪽 다 크게 문제가 없어보여서 이런 고민을 하셨을 것 같은데요, 그런 경우 대부분 둘 중 어느 방법을 사용해도 상관없습니다.
대신 요구사항이 추가되면서 이 메서드를 재활용해야 되는 등의 경우가 생길 수 있을텐데요, 그러면 이 메서드가 어떤 타입을 반환해야하는지 좀 더 분명해지기도 합니다. 지금은 실제 필요한 타입이 List이니 이를 활용하다가 나중에 Hand가 필요하면 그 때가서 리팩토링해도 괜찮을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다,, 범블비 말씀대로 양쪽 다 크게 문제가 없어보였습니다,, 이럴 때는 그냥 쿨하게 생각해서 넘어가고, 다음에 필요할 때 다시 리팩토링하는게 좋아보이네요
src/main/java/domain/Score.java
Outdated
public boolean isBlackjack() { | ||
return this.equals(BLACKJACK); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
객체의 책임을 표현하는 메서드들은 관용적인 getter 등 보다 위에 위치하게 해주세요.
src/main/java/domain/hand/Hand.java
Outdated
public boolean isBlackjack() { | ||
return calculate().isBlackjack(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
카드 3장으로 21점일 경우도 블랙잭일까요?
src/main/java/domain/hand/Hand.java
Outdated
if (isSoftHand()) { | ||
score = score.plusSoftHand(); | ||
} | ||
|
||
return score; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (isSoftHand()) { | |
score = score.plusSoftHand(); | |
} | |
return score; | |
if (isSoftHand()) { | |
score = score.plusSoftHand(); | |
} | |
return score; |
if (isSoftHand()) { | |
score = score.plusSoftHand(); | |
} | |
return score; | |
if (isSoftHand()) { | |
return score.plusSoftHand(); | |
} | |
return score; |
이렇게도 쓸 수 있겠네요.
- 그래서 참여자가 블랙잭이 아니고, 합이 21이거나 hit 할 수 있는 상황 + 딜러가 버스트면 참여자가 이김
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 우르 👋
리뷰 반영 잘 해주셨어요.
머지해도 좋을 것 같긴 한데, 조금 더 확인해보고 싶은 부분이 있네요.
반영 후에 요청 주세요!
|
||
@Override | ||
public Boolean canApply(final Participant participant, final Dealer dealer) { | ||
return dealer.isBust() && (participant.isSumTwentyOne() || participant.canHit()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return dealer.isBust() && (participant.isSumTwentyOne() || participant.canHit()); | |
return dealer.isBust() && !participant.isBust()); |
|
||
@Override | ||
public Boolean canApply(final Participant participant, final Dealer dealer) { | ||
return participant.isBlackjack() && dealer.isBlackjack(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
점수가 같을 때도 체크해야하지 않을까요?
public BetResultState state() { | ||
return new BreakEvenState(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수익을 구해야하는데 비겼을 때와 이겼을 때의 값이 같은 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 이겼을 때와 졌을 때 모두 참여자 입장에서는 돈을 돌려받아서 본전이라는 상태를 반환하도록 했습니다,,
그래서 무승부, 블랙잭이 아닌 경우로 이겼을 경우에는 참여자가 받은 돈 그대로 돌려받는 본전(BreakEven), 블랙잭으로 이긴 경우(Win), 아예 졌을 경우를(Lose)로 생각했습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 기억하기로 블랙잭 규칙이 수익을 기준으로 블랙잭 - 1.5배, 승리 - 1배, 무승부 - 0배, 패배 - -1배였던 것 같아요.
승리할 때와 무승부일 때 받는 돈이 같은 건 조금 어색하지 않나요?
@FunctionalInterface | ||
public interface BetResultState { | ||
|
||
Money calculateBetOutcomeOf(final Money money); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
함수형으로 쓰는 곳이 없다면 @FunctionalInterface
도 필요 없을 것 같네요 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하,, 함수형으로 쓰기 위해서 @FunctionInterface를 명시해주는군요,,
그냥 인터페이스에서 추상 메서드가 단 하나일 경우에 그냥 해주는 줄 알았습니다 !!
나중에 BetResultState 를 람다식으로 쓸 때, "아 맞다 @FunctionInterface 붙여줘야지"라고 생각하는게 쉽지 않을 것 같아서,
만약 추상 메서드가 하나이면 그냥 붙여놓고, 나중에 추상 메서드가 추가되면 그때 해당 어노테이션을 빼주는건 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추상 메서드가 하나인 인터페이스여도 함수형인 인터페이스가 있고, 단순히 추상 메서드가 하나인 인터페이스가 있을 거에요.
그래서 @FunctionalInterface
는 함수형 인터페이스로 나타내고 싶을 때 사용하면 될 것 같네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 우르 👋
구현 잘 해주셨습니다.
이번 미션 여기서 머지할게요! 고생하셨어요~
public BetResultState state() { | ||
return new BreakEvenState(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 기억하기로 블랙잭 규칙이 수익을 기준으로 블랙잭 - 1.5배, 승리 - 1배, 무승부 - 0배, 패배 - -1배였던 것 같아요.
승리할 때와 무승부일 때 받는 돈이 같은 건 조금 어색하지 않나요?
@FunctionalInterface | ||
public interface BetResultState { | ||
|
||
Money calculateBetOutcomeOf(final Money money); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추상 메서드가 하나인 인터페이스여도 함수형인 인터페이스가 있고, 단순히 추상 메서드가 하나인 인터페이스가 있을 거에요.
그래서 @FunctionalInterface
는 함수형 인터페이스로 나타내고 싶을 때 사용하면 될 것 같네요.
|
||
public interface BetResultState { | ||
|
||
BetResultState NULL = money -> Money.MIN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코멘트에 답변하는 걸 까먹었네요 😅
null은 무엇인가가 "없는" 것을 표현하기 위한 거죠. 그래서 결과가 "없다는" 걸 표현하기에 괜찮다고 생각했어요.
조금 반대로 null을 사용하기에 적합하지 않은 예시를 들어보자면 null에 의미를 부여하는 경우가 있어요. 예를 들면 boolean을 true/false 대신 true/null과 같은 식으로 나타내는 경우인데요, 코딩을 하다보면 편의를 위해 null을 사용하고 거기에 의미를 부여하고 싶을 때가 있을거에요. 그런 경우를 주의했으면 좋겠어서 남긴 코멘트입니다.
안녕하세요 범블비 !!
저번 1단계 미션 때 생각은 많이 했는데 그때그때 작성하지 않아서 그런지 pr 제출할 때 적어보려니까 무슨 생각을 했는지 기억이 안나더라구요..
그래서 이번에는 미션을 진행하면서 생각했던 부분들을 기록해두고, pr 제출 전에 참고하면서 질문할 것들을 정리했어요,, 그래서 질문이 좀 많습니다,, 미리 답변 감사드립니다:)
질문에 대해서는 텍스트랑 노션 링크 둘 다 올려놓겠습니다 !! 어떤게 편하실지 몰라서 다 준비해왔습니다.
이번 리뷰도 잘 부탁드려요 !! 감사합니다~~
다이어그램
질문
Score는 점수를 담는 값 객체,, 그래서 한정지으면 안된다??
노션 링크
Score 값 객체를 사용하면서 궁금한 점이 생겼습니다 !!
아래의 내용이 제 의견이어서 이에 대한 범블비 고견을 듣고 싶습니다!!
블랙잭 게임에서 처음 받은 두 장의 카드 합이 21이면 블랙잭(또는 natural)이라고 하는데, 이를 확인하기 위해서 Score VO에 isBlackjack 이라는 메서드를 둘까? 라는 고민이다.
왜냐하면 지금은 볼륨이 작은 애플리케이션이기 때문에 Score VO는 blackjack에 한정될 수 밖에 없다. 하지만 VO는 공통으로 쓰일 수 있기 때문에 볼륨이 큰 애플리케이션이라면 isBlackjack 이라는 메서드가 적절할까에 대한 생각이다. 왜냐하면 이 Score는 blackjack 에 종속적인 느낌이 들어서이기 때문이다,,
Participants의 배팅 결과 상태는 메서드 파라미터로 받을까? 상태로 가지고 있는게 맞을까?
노션 링크
이번에 배팅금액을 결정하는 방법을 인터페이스로 표현해서 나타내보았습니다.
그래서 배팅금액상태를 Participant가 가지게 설계를 했는데요. 굳이 상태를 가져야할까? 라는 생각이 들었어요. 왜냐하면 Participant가 상태를 가지고 있다면 유연성이 떨이지고, 상태를 변경할 때마다 setter 메서드를 통해서 변경해야하니까요.
그래서 인스턴스 변수로 두지 않고 메서드 파라미터로 상태를 전달하면 유연성이 높아지지 않을까? 라는 생각을 했습니다.
하지만 이 방법은 제가 적용하기에 어려웠습니다. 왜냐하면 배팅결과가 2번으로 나뉘기 때문에 처음에 결정된 배팅 결과를 “저장”할 수가 없기 때문입니다. 저장하려면 CardTable에 Map<Participant, State> 필요하더라구요..
그래서 그냥 Participant 가 상태를 가지고 있을까,, 라고 제 자신과 합의를 보면서 코드를 작성했던 것 같아요.
제 말 중에 이해가 안되시면 편하게 말씀해주세요!!
만약 범블비가 제 글을 보시고 떠오르는 아이디어가 있으시면 듣고 싶습니다!!
if 문이 많으면 의심해봐라
노션 링크
if 분기가 많은 이유는 첫 두장이 블랙잭인 경우와 아닌 경우로 나뉘게 됩니다.
그런데 if 문 분기가 많으면 Strategy Pattern 을 통해서 해결할 수 있다고 하는데,, 저는 전혀 이해가 되지 않아서.. 혹시 이러한 분기를 줄인다(?), 이쁘게 작성하는 방법이 없을까해서 범블비에게 여쭤보고 싶습니다!
void는 어떻게 테스트 하는가?
노션 링크
Participant 의 상태를 변경하는 if 문들입니다.
상태만 변경하기 때문에 반환형은 void 로 해서 테스트하기가 어렵습니다(상태를 보여줄 일이 없기 때문에)
그래서 해당 메서드를 사용하는 메서드로 테스트하는데 이 방법이 괜찮은지 의문이 들더라구요.
해당 메서드를 단위 테스트 하는데, 이 메서드를 사용하는 메서드를 테스트한다. 라는 느낌으로 보여서 의문이 들었습니다.
범블비는 어떻게 생각하시는지 궁금합니다 !
null object 패턴?
노션 링크
처음에는 betResultState 가 없기 때문에 null 로 했었습니다. 하지만 null 을 직접적으로 사용하는 것은 위험성이 있다고 판단해서 첫 장에서 승부가 나지 않았다 라는 의미인 TieState를 사용했습니다.
하지만 이 부분도 의미가 좀 다른 것 같아서 구글링을 하는 중에 null object 패턴을 보았습니다. 글을 읽어봐도 제대로 이해가 되진 않았지만 TieState, 직접적인 null 사용보다는 괜찮다는 생각을 해서 사용해보았습니다.
범블비가 보시기에는 변경 후 코드가 합당해보이는지 궁금합니다 !
faceUpFirstDeal 할 때, Hand 반환 List 반환?
노션 링크
범블비가 말씀해주신 부분도 타당하다고 생각이 들어서 코드에 적용해보았습니다.
그러나 카드를 반환해줄 때 List로 반환할까? Hand로 반환할까에 대한 고민이 생겼습니다.
아래는 제 의견이고 범블비의 고견을 듣고 싶습니다!!
왜 Hand를 반환해야하나?
Hand의 정의가 Card들을 관리하는 일급 컬렉션이기 때문에 Cards를 반환하는건 Hand로 하는게 적합하다.
왜 List를 반환해야하나?
해당 메서드는 Dealer와 Participant에 따라 보여지는게 달라진다. 그래서 보여지기 위한 메서드이기 때문에 굳이 Hand를 반환해야하나? 그냥 List 반환해서 캡슐화도 지킬 수 있고, 더욱 객체지향적으로 보이는 것 같다.