-
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단계 - 블랙잭 베팅] 히스타(오승현) 미션 제출합니다. #871
base: hacanna42
Are you sure you want to change the base?
Conversation
- 더 이상 MVC 패턴을 사용하지 않음 - OutputView에서 버스트 상수를 Player가 아니라 Score에서 가져오도록 수정 - 패키지 구조 변경
- 너무 큰 수를 입력하면 오버플로우 가능성이 있기에, 5억 이하로 제한했습니다.
- 베팅 기능 구현 끝
- 베팅 음향 효과 - 최종 수익 결과 음향 효과 - Prompt(질문) 음향 효과
- 최종 게임 결과 (승리, 패배, 블랙잭 승)등을 표시하도록 추가 - 이전 코드를 재활용했지만, 최종 수익과 별개로 게임 결과도 있으면 좋겠다고 판단.
- 딜러의 첫번째 2장 드로우에서, 블랙잭의 룰에 맞게 마지막 카드는 가렸습니다.
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단계 미션도 잘 구현해주셨네요.
몇가지 커멘트 남겼는데 확인 부탁드립니다.
package controller; | ||
package object.view; |
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.
Answer 이라는 Enum은 왜 view 단의 클래스라고 생각하셨을까요?
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.
- 유저의 답변을 표현하는 열거형 상수이기 때문입니다.
- 패키지를 나누는 기준은 레이어가 아니라, 보기 편하게 집단을 분리한 것입니다.
inputView.askDrawOneMore()
의 답변을 표현하는 열거형 상수라 입/출력에 밀접한 관련이 있다 생각해 view 패키지에 넣었습니다.
public void betMoney(int amount) { | ||
betMoney += amount; | ||
validateWallet(); | ||
} |
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.
검증이 보통 값 할당보다 우선이지만, 해당 경우에서는 betMoney() 메서드를 여러번 사용해서 제한을 초과하는 경우도 있을 것 같아서 이후에 검증했습니다.
boolean ableToDraw(final int score); | ||
void applyGameRecord(GameResult result); | ||
void bet(int amount); | ||
boolean areYouDealer(); |
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.
boolean에 대한 getter는 관습적으로 is를 prefix로 씁니다.
https://stackoverflow.com/questions/5322648/for-a-boolean-field-what-is-the-naming-convention-for-its-getter-setter
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.
boolean에 대한 getter는 is가 대부분이 사용하는 컨벤션이고, 가독성도 좋음을 인정합니다.
하지만 participant.areYouDealer()
도 충분히 가독성이 좋고, 객체 지향 설계를 학습하는 관점에서 메시지를 전달한다는 것이 잘 드러나서 채택했었는데요. isDealer()도 가독성이 충분히 좋고, 웬만하면 컨벤션을 따르는 것이 옳음에 동의합니다.
@Override
public boolean ableToDraw(final int score) {
return score < STAY_THRESHOLD;
}
추가적으로, 그러면 위의 경우에도 isAbleToDraw()
바꾸는 것이 불필요한 오해를 막는 것에 도움이 되겠네요.
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.
모두 is 컨벤션으로 수정했습니다 :)
private void updateGameResultBlackJack(Participant winner, Participant loser) { | ||
winner.applyGameRecord(GameResult.BLACKJACK_WIN); | ||
loser.applyGameRecord(GameResult.LOSE); | ||
} | ||
|
||
private void updateGameResult(Participant winner, Participant loser) { | ||
winner.applyGameRecord(GameResult.WIN); | ||
loser.applyGameRecord(GameResult.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.
인자의 순서에 의존해서 메서드를 작성해주셨네요.
차라리 별도로 승패를 관리하는 메서드를 만들어서 재사용하는게 직관적이지 않나요?
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.
완전히 동의합니다. 코드 중복을 줄이는 것에 너무 집착했어요.
쓰면서도 헷갈렸는데, 아서가 말씀해주신 방향으로 고치는 것이 좋겠네요 !
private void updateBattleResultBetween(Participant dealer, Participant participant, Score dealerScore) { | ||
if (participant.areYouDealer()) { | ||
return; | ||
} | ||
|
||
Score playerScore = getScoreOf(participant); | ||
|
||
if (playerScore.isBlackJack() && !dealerScore.isBlackJack()) { | ||
updateGameResultBlackJack(participant, dealer); | ||
return; | ||
} | ||
|
||
if (playerScore.isBust()) { | ||
updateGameResult(dealer, participant); | ||
return; | ||
} | ||
|
||
if (dealerScore.isBust()) { | ||
updateGameResult(participant, dealer); | ||
return; | ||
} | ||
|
||
if (playerScore.getScore() > dealerScore.getScore()) { | ||
updateGameResult(participant, dealer); | ||
return; | ||
} | ||
|
||
if (playerScore.getScore() < dealerScore.getScore()) { | ||
updateGameResult(dealer, participant); | ||
return; | ||
} |
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.
네. BlackJackBoard
에게 아직도 역할이 집중되어 있는 것 같습니다.
해당 사항에 대해서는 고민해보겠습니다 :) 감사합니다 !!
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.
GameResult 열거형 상수에서 Score을 받고 결과를 반환하도록 하는게 훨씬 깔끔하네요.
이것을 생각 못했다니. 피드백 정말 감사드립니다 아서 :)
if (playerScore.isBlackJack() && !dealerScore.isBlackJack()) { | ||
updateGameResultBlackJack(participant, dealer); | ||
return; | ||
} | ||
|
||
if (playerScore.isBust()) { | ||
updateGameResult(dealer, participant); | ||
return; | ||
} | ||
|
||
if (dealerScore.isBust()) { | ||
updateGameResult(participant, dealer); | ||
return; | ||
} | ||
|
||
if (playerScore.getScore() > dealerScore.getScore()) { | ||
updateGameResult(participant, dealer); | ||
return; | ||
} | ||
|
||
if (playerScore.getScore() < dealerScore.getScore()) { | ||
updateGameResult(dealer, participant); | ||
return; | ||
} |
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.
오.. 맞는 것 같습니다.
승리/패배 등 여부를 분리해서 먼저 판단하고, 그것을 통해 update 분기 처리를 하는 것이 좀 더 좋을 것 같다는 말씀이시죠?
동의합니다. 수정해보겠습니다!
String playerNickname = player.getNickname(); | ||
List<String> playerCardNames = blackJackBoard.getCardDeckOf(player) | ||
.getCards().stream() | ||
.map(card -> card.getName() + card.getCardSymbol()) |
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.
card에서 이름을 만들어주는 로직도 계속 중복되어져 나오는데 어디서 관리해주면 좋겠네요.
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.
이걸 놓쳤네요. 어차피 CardDeck은 Card를 알고 있는데, 왜 굳이 꺼내서 이름을 만들어줬을까요.
좋은 피드백 감사합니다 아서 :)
public enum GameResult { | ||
BLACKJACK_WIN("블랙잭", 2.5), | ||
WIN("승", 2.0), | ||
LOSE("패", 0.0), | ||
DRAW("무", 1.0) | ||
; |
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 enum GameResult { | |
BLACKJACK_WIN("블랙잭", 2.5), | |
WIN("승", 2.0), | |
LOSE("패", 0.0), | |
DRAW("무", 1.0) | |
; | |
public enum GameResult { | |
BLACKJACK_WIN("블랙잭", 2.5), | |
WIN("승", 2.0), | |
LOSE("패", 0.0), | |
DRAW("무", 1.0), | |
; |
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 class BlackJackManager { | ||
private final InputView inputView; |
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.
Manager 클래스가 좀 하는 일이 많네요.
게임의 흐름도 관리하고, DTO 변환도 해주고, view단의 형식까지 맞춰주고 있어요.
분리 해볼 수 있을 까요?
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.
해당 부분에 대해 많이 생각해봤는데요.
현재 코드에서 DTO
는 다른 객체를 받고, View
에서 쉽게 쓸 수 있게 형식을 맞춰주는 일을 하고 있는것이잖아요?
근데 대체 왜 BlackJackManager
가 중간에 껴들어서 반절정도 형식을 맞춰주고 나머지는 DTO
에게 넘기지? 라는 생각이 들었어요. 해줄거면 아예 다 해주던가, 이럴거면 DTO
를 왜 만들었지? 정말 애매한 설계라는 생각이 들었습니다.
이렇게 되면 만약 DTO
로 변환될 객체가 변경되면, Manager
도 변경될 것 같아요. 괜히 변경의 전파를 유도하는 잘못된 설계라고 생각합니다.
왜 이런 코드를 작성하게 됐을까 생각해보니, 제가 학습하면서 흘려 들었던 여러가지 문장들 (DTO
에 대한 것들.. MVC
에 대한 것들..) 이 무의식에 영향을 받아 나만의 생각 없이 저것이 옳다고 믿고 작성한 것 같아요.
이렇게 제가 평소에 듣는 것들이, 특히 네이밍에 연관되어서 (DTO
, Model
, Controller
등) 무의식적으로 적용되는 것 같아요.
DTO
를 객체로써 보지 못하고, Data Transfer Object
로 보니까 단순 데이터를 전달하는 용도로만 생각하게 되니 객체 역할 분배을 깊게 신경쓰지 못한 것 같아요.
아서가 피드백해주신 부분은 수정할 것이고,
이제는 DTO
를 Response
객체로 부르려고 합니다.
제 생각이 혹시 잘못되었다면 말씀해주시고, 좋은 피드백 감사드립니다 아서 !! :)
public void winBetRate(GameResult gameResult) { | ||
double betRate = gameResult.getBetRate(); | ||
earnMoney(betMoney * betRate); | ||
} |
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.
winBetRate, 배팅률(배당)을 얻다(win) 의미로 사용한 것인데, applyBetRate() 가 더 적절할 것 같습니다.
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.
.applyBetRateBy(GameResult gameResult) 로 메서드 시그니처 수정했습니다.
- card.getName()이 카드 이름(숫자 + 심볼)을 리턴하도록 수정했습니다.
- .winBetRate() 를 .applyBetRateBy() 로 수정 - 사용되지 않던 메서드 삭제
- BlackJackManager 에게 의도된 역할은 "게임 흐름 제어" 였지만, 실제로는 View를 위한 일부 데이터를 가공하고 있었습니다. 따라서, 해당 책임을 모두 Response 객체로 옮겼습니다.
- 카드덱이 비었을때의 오류 처리 추가 - 플레이어가 입력되지 않았을때의 오류 처리 추가 - 플레이어가 현재 카드덱 개수(52개)로 감당 불가능한 수만큼 입력될 시의 오류 처리 추가 - 클라이언트의 플레이어 반점 구분 입력에서, 띄어쓰기를 무시하도록 추가
프로그램 작동 영상 (소리있음)
_2._.mp4
체크 리스트
test
를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?https://prolog.techcourse.co.kr/studylogs/4031
객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?
1~5점 중에서 선택해주세요.
선택한 점수의 이유를 적어주세요.
대부분의 객체지향 체조 원칙을 지켰지만,
4점을 선택했습니다.
🃏 블랙잭 미션 Step2 PR
안녕하세요, 아서!
step2에서는 정말 다양한 관점에서 고민을 많이 했는데요.
🎯 이번 미션에서 중점으로 둔 점
1️⃣ 실세계와 객체세계의 간극
step1에서는 실세계와 객체지향 설계 사이의 간극을 인지하지 못하고, 역할 분배가 골고루 되지 않았었습니다.
따라서 step2에서는 요구사항 구현에 앞서 역할을 분배하려고 노력하였고, 기존
GameBoard
에 있던 과도한 역할을 덜어보려고 노력했습니다.💡 배운 점:
2️⃣ DTO를 왜 사용하는가?
DTO를 사용하지 말라는 피드백을 많이 들었는데요.
이에 대해 고민해보니, DTO가 오버 엔지니어링이라는 의견이 주된 이유였습니다.
하지만 DTO의 정적 팩토리 메서드가 객체 정보를 받아 분해하면, View의 로직을 덜 수 있어 코드가 깔끔해질 것이라고 생각했습니다.
DTO가 있는데 컨트롤러 등에서 조립해준다면 오히려 View의 로직이 복잡해져 가독성이 떨어질 것 같았습니다.
결론적으로, DTO는 현재 요구사항에서 굳이 필요 없는 오버 엔지니어링일 수 있지만, View에서 객체 정보를 분해하고 파싱하는 역할을 덜어줄 수 있기에, 있어서 나쁠 건 없다고 판단했습니다.
따라서 DTO를 유지하되, 네이밍을
Response
로 변경하였습니다.3️⃣ MVC 패턴을 왜 사용하는가?
우테코 프리코스 1주차 미션을 풀 때는 디자인 패턴 없이 순수한 객체들의 협력으로 설계했었는데요.
어느 순간부터 MVC 디자인 패턴을 당연하게 적용하는 저를 발견할 수 있었습니다.
OutputView
,InputView
를 생성Application.main()
을 간결하게 유지하고Controller.start()
로 시작이런 것들이 너무 당연하게 여겨졌고, 그래서 이번 미션에서는 이 당연했던 것들을 의심해보는 시간을 가졌습니다.
🎯 결론적으로, 이번 미션에서 MVC 패턴을 적용할 이유가 전혀 없었습니다.
하지만 오랜 시간 MVC 패턴을 익혀왔기에 사고방식이 자연스럽게 MVC적으로 흘러가는 걸 목격했습니다.
그래서 최상위 패키지명을
object
로 변경하여,OutputView
,InputView
가 View가 아닌 단순한 입/출력 객체임을 계속 의식하도록 했습니다.4️⃣ 상속의 단점
step1에서는
GameBoard
에서 다형성을 활용한 것이 유연하고 좋다고 느껴졌습니다.하지만 step2에서 베팅 기능이 추가되면서,
Player
와Dealer
에게 각각 다른 요구사항이 생겼습니다.Dealer
는 베팅을 하지 않기 때문에 별도의 베팅 관련 메서드가 필요 없음GameBoard
에서 다형성을 유지하려다 보니 다운캐스팅이 발생💡 다운캐스팅이 발생했다는 것은?
🎯 결론적으로, 상속은 서브타이핑(다형성)을 이용하고자 할 때 사용해야 하며, 그 외에는 합성을 사용하는 것이 더 유연한 선택이라는 점을 깨달았습니다.
✅ 마무리
이번 미션에서는
✔️ 실세계와 객체세계의 간극을 인지하고,
✔️ DTO와 MVC 패턴을 무조건 적용하는 것이 아니라 이유를 고민하며,
✔️ 상속과 합성의 차이를 직접 경험해보았습니다.
리뷰해주셔서 감사합니다! 😊
피드백 언제든 환영합니다!