-
Notifications
You must be signed in to change notification settings - Fork 467
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단계 - 블랙잭 베팅] 밍트(김명지) 미션 제출합니다. #917
Conversation
- 인터페이스는 행위를 정의하기 위해 사용되어야한다. 상수가 존재하면 인터페이스가 하는 역할에 벗어난다. 관리와 변경도 어렵다. - 덱을 생성하도록 변경하고, 덱에서는 한장씩 뽑을 수 있도록 한다.
- 기존에는 ArrayList로 저장하여 removeFirst를 하였는데, 카드 하나를 뽑을 때마다 모든 원소의 인덱스가 한칸씩 움직여 성능상 좋지 않았다. - 따라서 Deque을 이용하여 pollFrist()를 하면 O(1) 시간복잡도가 걸린다.
- controller가 흐름 처리 및 비즈니스 로직도 처리하고 있으므로, BlackjackGame이라는 클래스명으로 변경한다.
- 응답에 대한 출력은 view의 역할이므로 view 패키지로 이동한다.
- Player는 Gamer의 하위 클래스로 더 많은 역할을 할 수 있으므로 Player 참조하도록 변경한다.
- 참가자들을 나타내는 Participants 객체를 도입하여 참가자 공통 로직 수행
- 명시적으로 상속을 불가능하게 만든다.
- 추가된 요구사항에 따라 기능 목록을 수정한다.
- ACE의 경우 특별하게 처리하고, 그 외는 점수값을 반환한다. - ACE 하나로 인해 모든 카드 점수가 List가 되는 것은 오버엔지니어링이 될 수 있다. Q,K,J가 추가 점수를 가질 확률이 낮기 때문이다.
- 좀 더 유연하고 테스트하기 쉬운 구조로 변경한다.
- BlackjackGame은 셔플된 덱을 생성하기 위해 Deck의 생성자에 shuffleDeckGenerator를 생성하여 주입한다는 사실을 몰라도 생성 가능하다.
- Set을 이용하면 조회시 O(1)의 시간이 걸리므로 리스트보다 성능이 좋다.
- 생성자로 대체 가능하다면 대체한다.
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.
밍트 상태패턴을 활용한 방식으로 잘 리팩터링해보셨네요. 👍👍
마지막으로 한번 더 짚고넘어가면 좋을 부분들이보여서 코멘트 남겨두었어요. 😃
확인 부탁드릴게요. 😉
import blackjack.blackjack.state.State; | ||
import blackjack.blackjack.state.finished.Stay; | ||
|
||
public final class DealerRunning extends Running implements State { |
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.
DealerRunning과 PlayerRunning의 경우 (State를 구현한) Running을 상속받았으니, State를 구현해줄 필요는 없을 것 같아요. 👀
public final class DealerRunning extends Running implements State { | |
public final class DealerRunning extends Running { |
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.
맞습니다! Running
은 Started
를 상속받기 때문에 State
인터페이스를 또 다시 구현할 필요가 없습니다! 반영했습니다!
|
||
public static final int BURST_THRESHOLD = 21; | ||
private static final int ACE_SUBTRACT = 10; | ||
private static final int BLACKJACK_SIZE = 2; | ||
private static final int DEALER_THRESHOLD = 16; | ||
|
||
private final List<Card> 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.
hand 객체가 상태로 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.
그렇네요! 클래스명과 필드명을 동일하게 하면 내부 필드를 가리키는지, 객체를 가리키는지 혼란스러울 수 있겠네요! 어색하기도 하고요. 수정했습니다!
return calculateScore() > BURST_THRESHOLD; | ||
} | ||
|
||
public boolean isDealerStay() { |
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.
dealer가 stay 상태인가에 대한 정의는 DealerRunning 객체에서 직접 판단하는게 좋지 않을까 싶어요.
Hand는 Dealer나 Player 둘 다 가지는 상태이다보니 Player나 Dealer의 정책은 Hand 객체 내부에 정의하지 않는 것이 좋다고 생각해요. (Dealer 정책은 Dealer 관련 객체에... Player 정책은 Player 관련 객체에...)
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.
맞습니다! Hand
는 Player
와 Dealer
모두 쓰이므로 Dealer
에서만 사용되는 로직은 Dealer
에 두는 것이 좋습니다! 반영했습니다!
return calculateScore() > DEALER_THRESHOLD; | ||
} | ||
|
||
public Hand subHand(final int startInclusive, final int endExclusive) { |
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.
subHand
는 어떤 행위인건지 설명해주실 수 있을까요?
동작과 네이밍의 의미를 설명해주시면 조금 더 이해가 될 것 같아요. 😃
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.
List
인터페이스의 subList
처럼 카드 일부라는 의미를 표현하고 싶었습니다.
가진 카드의 startInclusive
(시작 포함 인덱스)부터 endExclusive
(끝 제외 인덱스)를 가져오는 메서드입니다.
메서드명이 직관적이지 않은 것 같아서, getPartialHand
로 수정했습니다!
Hand playerHand = deck.drawCardsByCount(SPREAD_CARD_SIZE * players.getSize()); | ||
players.receiveCardsByCount(playerHand, SPREAD_CARD_SIZE); |
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.
players에게 패를 주는 행위는 dealer가 수행할 필요가 없어보이지 않나요? 🤔
players와 deck을 모두 전달 받아서 내부에서 players에게 패를 주는 부분이 조금 어색하게느껴지는 것 같아요.
participants#dealInitialCards
이 행위에서 dealer와 players에게 각각 초기 패를 전달해주면 될 것 같아요.
public void dealInitialCards(final Deck deck) {
dealer.dealInitialCards(deck);
players.dealInitialCards(decK)
}
dealer.dealInitialCards(players, deck);
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
에서 나눠주는 것이 의미상 더 자연스럽겠네요! 굳이 딜러에서 Players
의 카드를 나눠주는 메서드를 호출할 필요가 없네요!
} | ||
|
||
public void stayIfRunning() { | ||
if (state.getStateType() == StateType.RUNNING) { |
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.
각각의 State를 객체화했다면 이 StateType
을 사용할 필요가 없지 않을까 싶어요. 😃
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.
stay()
는 Running
상태일때 Finished
(종료 상태) 중 Stay
(카드를 더이상 받지 않고 종료) 상태로 이동하는 메서드입니다.
따라서 실행 상태가 아닌 종료 상태 (블랙잭, 버스트)인 경우 stay()
를 호출하면 예외를 발생하도록 하였습니다.
위 메서드가 존재하는 이유는 모든 플레이어에 대해 루프를 돌 때, Running
인지 아닌지 구분하여 stay()
를 호출해야 예외가 발생하지 않기 때문입니다.
그런데stay()
메서드는 Running
일때 Stay
로 상태를 변경하고, 그 외에는 아무 작업도 수행하지 않는 것이 좀 더 깔끔하다는 생각이 들었습니다. 호출하는 쪽에서 매번 어떤 상태인지 확인하지 않아도 되니까요!
따라서 아래와 같이 수정했습니다!
Finished.java
@Override
public State stay() {
return this; // 기존에는 예외 발생하도록 했음
}
import blackjack.blackjack.card.Hand; | ||
import blackjack.blackjack.state.State; | ||
|
||
public abstract class Participant { |
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.
모든 메서드가 추상 메서드라면 interface를 선언해도 될 것 같은데, 추상 클래스로 선언하신 이유가 있으신가요?
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 BigDecimal calculateProfit(final BigDecimal bettingAmount, final State dealerState) { |
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.
두번째 메서드 파라미터로 dealer의 state가 온다는 보장이 없을 것 같아요.
또한 현재 Stay 객체(== this) 또한 Player 것이라는 보장이 없는 것 같구요. 🤔
이득을 구하는 구체적인 로직은 player#calculateProfit에 두는게 좋지 않을까요? 🤔 (+ 메서드 파라미터로 dealer의 state를 꺼내서 받는게 아니라, 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.
그렇군요! State
객체를 받으면 매개변수명이 dealerState
여도, Player
의 State
이 들어올 수 있어 버그가 발생할 수 있군요!
refactor: 매개변수 DealerState -> Dealer로 변경
Player
에 수익 계산 로직을 두면, 수익 계산 기준이 플레이어임이 명확해져 모호함이 사라지네요! 감사합니다!!
feat: 수익 계산 로직 State -> Player로 이동
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.
깃헙 콜백 오류?로 LMS에 Request Changed로 변경이 안돼서 다시 Request Change 할게요.
밍트 상태패턴을 활용한 방식으로 잘 리팩터링해보셨네요. 👍👍
마지막으로 한번 더 짚고넘어가면 좋을 부분들이보여서 코멘트 남겨두었어요. 😃
확인 부탁드릴게요. 😉
- Running은 State 인터페이스를 구현한 Started을 상속받기 때문에, 또 다시 State를 구현할 필요가 없다.
- 클래스명과 필드명을 동일하게 하면 내부 필드를 가리키는지, 객체를 가리키는지 혼란스럽기 때문에 수정한다.
- Hand는 플레이어와 딜러 모두 가지므로, 딜러 관련 카드 로직은 DealerRunning으로 이동한다.
- Dealer와 Players를 갖는 Participants가 카드를 나눠주도록 하여 의미상 자연스럽도록 수정한다. - 카드를 나눠주는 크기인 SPREAD_CARD_SIZE 공통 상수도 Participants로 이동한다.
- stay()는 Running 상태일때 Stay() 상태로 바꾸는 메서드이다. - 이미 종료한 경우 stay()를 호출하면 아무 작업도 하지 않도록 한다. - 이를 통해 루프문에서 Running 상태인지, Finished 상태인지 매번 확인하지 않고, Running일때만 Stay()로 변하도록 할 수 있다.
- 공통 필드나 구현 메서드가 존재하지 않으므로 인터페이스로 둔다. - 이를 통해 다중 구현을 가능하게 하여 확장성을 높일 수 있다.
- DealerState는 State 타입이므로 Dealer의 타입임을 특정할 수 없다. - Dealer를 매개변수로 받아 상태 처리를 하면 Dealer의 상태임을 보장할 수 있다.
- State는 Player와 Dealer 모두 쓰이므로 수익 계산 기준이 모호하다. 따라서 Player로 이동한다.
- 초기에 BlackjackGame을 만들때 빈 상태에서 만들기 때문에, 딜러와 플레이어의 initialState()가 제대로 호출되지 않았다. - 블랙잭은 플레이어의 초기 카드 2장이 21인 경우에만 해당되므로, 기존에는 블랙잭이어도 초기 상태가 아니므로 블랙잭으로 판단되지 않았다. - 초기에 빈 카드로 생성하지 않고 deck에서 꺼낸 카드로 초기화되도록 수정한다.
public BigDecimal calculateProfit(final Dealer dealer) { | ||
if (state.getStateType() == StateType.BUST) { | ||
return bettingAmount.multiply(BigDecimal.valueOf(-1)); | ||
} | ||
if (dealer.getStateType() == StateType.BUST) { | ||
return bettingAmount; | ||
} | ||
if (state.getStateType() == StateType.BLACKJACK) { | ||
return bettingAmount.multiply(BLACKJACK_PROFIT_RATE); | ||
} | ||
return compareByScore(bettingAmount, 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.
요구사항이 "함수(또는 메서드)의 길이가 10라인을 넘어가지 않도록 구현한다" 인데, 혹시 메서드 줄은 메서드 정의하는 부분도 포함해서 세는지 궁금합니다!
추가로 로키는 이렇게 if문이 여러개일때 어떤 식으로 메서드를 분리하시나요?
@Rok93 꼼꼼하고 세심하게 피드백해주시고, 개선점을 명확하게 이야기해주셔서 많은 것을 배울 수 있었어요! |
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 (playerScore < dealerScore) { | ||
return bettingAmount.negate(); | ||
} | ||
if (state.getStateType() == StateType.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.
이 부분은 state 객체에게 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보다는 직접 state에게 물어보는 것이 좋겠네요!
} | ||
} | ||
|
||
private BigDecimal compareByScore(final BigDecimal bettingAmount, 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.
(질문) 요구사항이 "함수(또는 메서드)의 길이가 10라인을 넘어가지 않도록 구현한다" 인데, 혹시 메서드 줄은 메서드 정의하는 부분도 포함해서 세는지 궁금합니다!
추가로 로키는 이렇게 if문이 여러개일때 어떤 식으로 메서드를 분리하시나요?
의미있는 단위로 메서드를 추출할 수 있는지 살펴보고 적절한 단위로 메서드를 분리하는 것 같아요. 😃
약간 감각에 의존한 방법이라 딱 어떻게 한다라고 정의하기가 조금 어렵네요. 😅
자주 고민해보는 습관을 들이다보면 점차 이런 부분을 더 잘하시게될거라 생각해요. 🙄
지금의 경우에는 아래의 로직을 (플레이어) 블랙잭 여부에따라 계산한다
같이 의미를 부여해볼 것 같아요.
if (state.getStateType() == StateType.BLACKJACK) {
return bettingAmount.multiply(BLACKJACK_PROFIT_RATE);
}
return bettingAmount;
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.
답변 감사합니다!
@Rok93 감사합니다! 🙇♀️ |
로키 안녕하세요! 밍트입니다! 😊
블랙잭 2단계도 잘 부탁드립니다! 🙇♀️
체크 리스트
test
를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?
1~5점 중에서 선택해주세요.
선택한 점수의 이유를 적어주세요.
블랙잭 요구사항이 도메인과 뷰의 결합이 높아
Massive Controller
가 되는 문제가 있었습니다.특히 플레이어를 순회하며 추가 카드 여부를 물어보는 로직은 도메인 로직임에도 불구하고
Controller
에 존재하게 되었습니다.이를 해결하기 위해
서비스
를 만드려고 하다가, 도메인의 응집도를 높이는 방법은 아니라는 생각으로 인해Controller
를 제거하게 되었습니다. 해당 내용은 아래 글로 정리해보았습니다!MVC 패턴, 서비스가 콘솔 애플리케이션에서 필요한가?
결론적으로
Controller
를 제거하고BlackjackGame
으로 만들어 도메인 로직도 함께 처리하였습니다.이 방법이 객체지향을 잘 지키는 방법은 아니라는 생각이 들어 4점 주었습니다.
로키는 블랙잭 요구사항이
MVC
패턴에 적합하다고 생각하시는지, 그렇지 않다면 이 방식(Controller
를 제거하고BlackjackGame
을 두는 방식)은 어떻게 생각하시는지 궁금해요!어떤 부분에 집중하여 리뷰해야 할까요?
객체지향적으로 설계되었는지 봐주시면 감사드리겠습니다!
질문은 코드 코멘트로 남겼습니다 😄
감사합니다 🙇♀️