[2단계 - 블랙잭 베팅] 리브(김민주) 미션 제출합니다. #695
Conversation
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public record NameProfit(String name, int profit) { |
There was a problem hiding this comment.
장부나 정산서 처럼 금액을 핸들링한다는 느낌의 네이밍이면 좋을 것 같아요 ㅎㅎ
There was a problem hiding this comment.
ProfitStatement로 변경하였습니다. 좋은 의견 감사합니다!😊
| private void validatePositiveMoney(final int money) { | ||
| if (money <= 0) { | ||
| throw new IllegalArgumentException("0원 이하의 금액을 베팅할 수 없습니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Money 가 유효한 범위인지는 원시값 포장을 해두었으니 돈이 생성될 때 검증하는게 더 안전하지 않을까요?
Bets 가 아닌 다른 곳에서 Money 를 생성하게되면 음수가 생성될 수도 있을 것 같아요~ 🤔
There was a problem hiding this comment.
패배했을 경우 -1 이기 때문에 열어두셨군요.
그렇다면 배팅 머니와 / 정산 머니를 나눠도 좋을 것 같아요.
There was a problem hiding this comment.
베팅 머니와, 정산 머니를 나누어 BetMoney, Profit으로 만들어주고,
BetMoney에서 유효성 검증을 하도록 변경하였습니다. 😆
| private Money calculatePlayerProfit(final Map<Player, ResultCommand> result, final Player player) { | ||
| final double multiplier = result.get(player).getRate(); | ||
| return bets.get(player).multiply(multiplier); | ||
| } | ||
|
|
There was a problem hiding this comment.
시그니처에 Map 을 넘겨주지 않고 rate 를 바로 넘겨주면 좋을 것 같아요. 계산이 필요한 플레이어 외에 다른 플레이어의 정보를 꺼내 올 수 있는 여지를 막도록요! ㅎㅎ
| () -> new EnumMap<>(ResultCommand.class), | ||
| reducing(0, i -> 1, Integer::sum) | ||
| )); | ||
| private ResultCommand calculateResult(final Player player) { |
There was a problem hiding this comment.
Q: 상태패턴을 꼭 써야하는가?
디자인 패턴이란? 반복되는 문제를 해결하기 위한 방법 중 자주쓰이는 것들을 추려서 디자인 패턴이라 명명하고 쓰인다고 생각해요~
상태에 따른 다른 행위를 할 때는 상태를 명시해두고, 그 상태일 때 해야하는 행위에 대해 나타내게되면
상태와 행위에 관계에 대해서 직관적으로 표현되고 무엇보다 관리가 쉽다는 장점이 있어요.
그런데 디자인 패턴을 적용하기 전에는 무조건 해결하려는 어떤 문제 가 정의되어 있어야해요.
지금 누군가는 이 로직을 보고 조건들을 파악하기 어렵다 라거나 조건 분기가 너무 불편하다 이런 생각을 할수도 있지만
때로는 나열된 조건문들이 더 직관적이기도 하니까요!
결론 : 리브가 보기에 문제라고 느끼지 않으면 디자인 패턴 적용하지 않아도 된다. 이미 상태패턴을 구현해보았는데도 이점이라고 느껴지지 않는다면 더더욱이요~! ㅎㅎ
There was a problem hiding this comment.
상세한 설명 감사합니다!
if문의 분기를 줄일 수 있다는 게 상태패턴의 장점 중 한가지라고 생각합니다.
하지만 한편으로는 현재 코드를 완전히 뒤집어 엎을만큼은 아니라고 생각하기도 합니다.ㅎㅎ
현재 규모에서는 상태 패턴으로 변경하는 데 소모되는 비용이 더 클 것 같아요.
상태 패턴이 엄청 매력적이어서 꼭꼭 써보고싶지만요...🥹
|
|
||
| public Participant() { | ||
| this.hand = new Hand(); | ||
| } | ||
|
|
||
| public void receiveInitialCards(final List<Card> cards) { | ||
| hand.add(cards); | ||
| for (Card card : cards) { | ||
| hand.add(card); |
| bets = new Bets(); | ||
| player = new Player("리브"); |
There was a problem hiding this comment.
반복되는 테스트 객체 생성은 TestFixture 로 만들어 볼까요?
There was a problem hiding this comment.
테스트를 만들 때 반복되는 테스트 객체 생성에 @BeforeEach를 사용 했었는데요,
수달의 리뷰를 보고 TestFixture에 대해서 더 공부하며 새로운 방법도 알게 되었습니다.
바로 별도 Fixture 클래스에 static 메서드들을 사용하는 방법입니다.
아예 별도 파일로 Fixture 클래스를 분리하기도 하는 것 같지만 지금은 클래스가 크지 않아서 각 테스트 코드 하단에 정의하였습니다.
그 외에도 private 메서드를 만들어서 사용하는 방법도 있지만, 테스트 코드 메서드와 같은 레벨에 정의하게 되기 때문에 가독성에 좋지 않아 보였습니다.
처음에는 그렇게 복잡하지도 않은데 Fixture 클래스까지 만들어야 하나?라는 고민도 했어요.
그런데 직접 해보고 나니 나름대로 장점을 느낄 수 있었습니다.
먼저 @BeforeEach는 각 테스트 메서드를 읽을 때 가독성이 좋지는 않다고 느꼈었어요.
상단에 테스트 코드를 실행하기 위해서 거쳐야할 과정들이 정의되기 때문에 테스트 과정에 꼭 필요한 Given이 생략되고 When과 Then만 나와서 테스트 코드 하나만을 보고는 바로 해당 메서드를 사용하는 과정이 한 눈에 읽히지 않는 것 같다고 생각했어요.
그러면서도 테스트를 하면서 객체를 어떻게 만드는지(new 생성자를 사용하는지, 정적 팩토리 메서드를 사용하는지)는 중요하지 않은 부분일 수도 있는데 그 부분을 미리 정의해 둘 수 있어서 좋았어요.
사실 지금은 한 파일에 테스트 코드의 양이 많지는 않아서 @BeforeEach도 나쁘지는 않겠다는 생각은 들지만 새로운 방법을 배울 수 있어서 매우 좋았습니다 🥰
There was a problem hiding this comment.
직접 구현해보고, 장단점을 느껴보는게 이 미션에서 가져갈 수 있는 재밌는 점들인데요~
리브는 재밌게 미션을 하고 있는 것 같아서 보기 좋습니다 😘
|
|
||
| @ParameterizedTest | ||
| @CsvSource(value = {"WIN,10_000", "LOSE,-10_000", "DRAW,10_000", "BLACK_JACK_WIN,15_000"}) | ||
| @DisplayName("플레이어의 수익을 계산한다.") |
There was a problem hiding this comment.
DisplayName에 행위 ~ 결과에 대한 명시가 있으면 좋을 것 같아요.
지금은 테스트 메서드명을 한글로 해석한 것 같은 느낌을 받았어요!
There was a problem hiding this comment.
테스트는 문서라고도 표현되는데요~
ParameterizedTest 테스트로 간편하게 하는 것도 좋지만
해당 테스트에서는 승리했을 때, 패배했을 때 무승부였을 때에 대한 명세를 자세히 적어줘도 좋을 것 같아요.
There was a problem hiding this comment.
테스트를 각각 분리해주고 걸맞는 DisplayName와 메서드 명도 달아주었습니다!
|
|
||
| @ParameterizedTest | ||
| @CsvSource(value = {"WIN,-10_000", "LOSE,10_000", "DRAW,-10_000", "BLACK_JACK_WIN,-15_000"}) | ||
| @DisplayName("딜러의 수익을 계산한다.") |
There was a problem hiding this comment.
딜러의 수익 계산 방식과 플레이어의 수익 계산에 가장 큰 차이는 무엇인가요?
테스트로 명시해두면 이해하기 훨씬 수월할 것 같아요~
There was a problem hiding this comment.
딜러 수익 계산 방법도 각각 개별 메서드로 분리하여 설명을 덧붙여 보았습니다!
| void checkNotBust() { | ||
| void canHit() { |
There was a problem hiding this comment.
이것도 회귀방지가 안된 케이스 같은데요~ 테스트 메서드 명을 운영코드의 메서드명과 일치시키니
리네임을 하면 테스트도 수정되어야하는 상황이 발생해요.
저는 테스트명으로 설명이되면 DisplayName 을 사용하지 않아도 된다고 생각해요.
영어로 표현하는게 힘들면 한글로 테스트명을 작성해도 좋고요!
이거 왜이렇게 불편하지? 이런 의식적인 생각들이 더 효율적이고 멋진 코드를 만드는 것 같아요!👍🏻
There was a problem hiding this comment.
사실 DisplayName이있어서 테스트 코드 메서드 이름에 신경을 많이 쓰지 않아도 될 것 같다라는 생각이 있었던 것 같습니다..😅
이번에는 제 코드를 보는 사람 중에 영어만 사용하는 외국인도 있다! 생각하면서 메소드 명을 구체적으로 작성해 보았습니다!
There was a problem hiding this comment.
영어만 사용하는 외국인도 있다! ㅋㅋㅋㅋ 너무 귀여운 설명 같아요 ~
| @DisplayName("전달 받은 금액으로 Money를 생성한다.") | ||
| void createMoney() { | ||
| int money = 1_000; | ||
| assertThat(new Money(money).getMoney()).isEqualTo(money); |
There was a problem hiding this comment.
-
int를 비교할 때,
a==b를 사용한다. -
Integer를 비교할 때,
equals 를 사용한다.
Money 객체에서 여기서 객체 값을 비교하기 위해서 equals & hash code 를 재정의하신줄 알았는데요~
이렇게 값을 꺼내서 쓰면 재정의하신 의미가 없어지는 것 같습니다!
There was a problem hiding this comment.
아래와 같이 변경해 보았습니다!
@Test
@DisplayName("전달 받은 금액으로 Money를 생성한다.")
void createMoney() {
int money = 1_000;
assertThat(new BetMoney(money)).isEqualTo(new BetMoney(1_000));
}사실 new 생성자로 객체가 잘 생성되는 건 매우 당연한 일이라서 필요 없는 테스트 코드가 아닐까라는 생각도 들었습니다...🥹
…shouldHit으로 변경하여 의미 명확화
|
안녕하세요 수달! 😆 제 닉네임은 제 본명의 한자 중 하나인 살 주(住, live)에서 따왔어요. 😆 상태 패턴에 대해서 수달의 생각을 구체적으로 알려주셔서 감사합니다! 그리고 그동안 테스트 코드를 잘 작성하고 있는 것인지도 궁금했는데 수달의 리뷰 덕분에 많이 배울 수 있었어요. 벌써 주말이 왔네요! 😆 |
안녕하세요, 수달☺️
리브입니다.
블랙잭 미션의 첫 번째 피드백에서 상태 패턴으로 구현하는 걸 보여주셨습니다.
저도 2단계를 구현하면서 적용해보려 시도 하였지만, 막상 만들고 보니 관리해야 할 클래스는 많아지는데 비해 활용성이 높지 않아서 다시 제거하였습니다. 😅
애초에 구현 초기부터 상태 패턴을 고려하지 않고 구현했기 때문에 발생한 문제일지도 모르겠지만, 현재로서는 상태패턴을 적용하지 않는 것이 오히려 관리하기 편하다고 생각되었어요.
혹시 수달은 블랙잭 미션에서 디자인 패턴을 적용하는 것에 대해 어떻게 생각하시나요❓