[1단계 - 블랙잭 게임 실행] 성하(김성훈) 미션 제출합니다. #411
Conversation
불필요한 import 제거 & static import
|
안녕하세요 조앤! 하지만 바꾼 구조도 그렇게 맘에 들진 않는 것 같아요 ㅠㅠ 😭 너무 구조를 다 바꾼 것 같아서 조앤에게 죄송합니다.. 구조를 다 바꾸다보니 리팩토링이 많아져서 이번 리팩토링 부분을 간략하게 요약하면 다음과 같습니다! 이렇게 진행하다 보니 거의 다 바뀐 것 같습니다! 😭 리팩토링 시에 추가로 궁금했던 부분들을 |
| this.participants = new Participants(players, dealer); | ||
| } | ||
|
|
||
| public List<String> getPlayerNames() { |
There was a problem hiding this comment.
컨트롤러에서 playerNames를 가져오기 위해 BlackJackGame의 getPlayerNames()를 호출할 때, 흐름이 다음과 같습니다!
BlackJackGame.getPlayerNames() -> pariticipants.getPlayerNames() -> players.getPlayerNames()이렇게 되는데 중간에 participants.getPlayerNames()가
단순히 전달만 해주는 역할만 하고 있어서 찝찝합니다.
이렇게 설계해도 괜찮은 걸까요?
There was a problem hiding this comment.
BlackJackGame에서 playerNames를 가져오기 위해 Participants를 거쳐서 Players에게 받아올 때
바로 받지 않고 뭔가 getter -> getter로 단순히 정보를 가져온다는 느낌이 들어서 그런 것 같아요.
오히려 BlackJackGame이 메시지를 객체에게 잘 전달해서 PlayerNames를 잘 받고 있는 걸까요?
|
|
||
| public class BlackJackController { | ||
|
|
||
| public void run() { |
There was a problem hiding this comment.
컨트롤러가 적절하게 구현됐는지 모르겠습니다!
run() 메소드 안에서 blakcJackGame을 넘겨주기만 하고
넘겨준 blackJackGame을 사용해서 흐름을 진행시키고 있는데
blackJackGame을 계속 인자로 넘겨줘도 괜찮은 건지 모르겠습니다!
There was a problem hiding this comment.
BlackjackGame을 Controller의 멤버가 아닌 지역변수로 선언해주신 이유가 무엇인가요?
There was a problem hiding this comment.
게임을 실행하는 run() 메소드 안에서만 BlackJackGame을 사용해서 해당 run() 메소드 안에서만
지역 변수로 놨던 것 같아요!
또 컨트롤러를 생성할 때 BlackJackGame이 초기화되는 것보다 실행한다는 의미의 run()을 실행했을 때
BlackJackGame이 시작되어 게임이 시작된다는 것이 의미상으로 더 좋았던 것 같아요!
There was a problem hiding this comment.
오홍ㅎㅎ 그런 이유가 있었군요! 설득력이 있는 것 같아요ㅎㅎ 제 생각엔 어차피 Controller.run()을 항상 실행해주기도 하고, 현재의 블랙잭 컨트롤러는 블랙잭 게임의 실행을 관장하기 블랙잭 게임에서 멤버로 갖고 있어도 무방할 것 같다는 생각이 들었어요.
추가로 이것과는 별개로 아래도 다음과 같이 수정해볼 수 있을 것 같아요.
// Result는 수도코드로 봐주세용
public void run() {
BlackJackGame blackJackGame = generateBlackJackGame();
Result result = blackJackGame.play();
printResult(gameResult);
}
지금 담겨있는 모든 스텝들은 컨트롤러의 로직이 아닌 블랙잭 게임의 비즈니스 로직 같아서요ㅎㅎ 블랙잭 게임 내부로 숨길 수 있을 것 같아요. 다음 단계에 반영해주세요~
| public void run() { | ||
| BlackJackGame blackJackGame = generateBlackJackGame(); | ||
| printParticipantInitCardsStep(blackJackGame, blackJackGame.getPlayerNames()); | ||
| playerDrawCardStep(blackJackGame, blackJackGame.getPlayers()); |
There was a problem hiding this comment.
Controller에서는 프로그램 진행 흐름만 관리하고,
BlackJackGame에서 나머지 도메인들을 관리하는 클래스로 선언하게 되었는데,
Controller에서 blackJackGame.getPlayers()로 Players를 받고
players.getPlayers()로 players의 기능을 호출해도
Controller에서 도메인을 모르게 설계한 건지 궁금합니다!
There was a problem hiding this comment.
컨트롤러에서 도메인을 모르는지 확인할 수 있는 방법이 어떤게 있을까요? 한가지 방법으로는 연관된 도메인 내부를 변경했을 때 컨트롤러에 영향을 미치는지를 확인해볼 수 있겠죠, ㅎㅎ 지금은 우선 컨트롤러에서 도메인은 모르도록 구현해주신 것 같아요. 이제 컨트롤러에 남아있는 비즈니스 로직을 도메인으로 옮기기만 하면 더 좋을 것 같아요. 이건 다른 코멘트로 남겨둘게요~
|
|
||
| public class InitGameSetter { | ||
|
|
||
| public static Deck generateDeck() { |
There was a problem hiding this comment.
저는 Deck을 관련 없는 외부에서 접근하면 안된다고 생각해서
static으로 선언하지 않았습니다.
그래서 DeckGenerator를 만들고, InitGameSetter에서 DeckGenerator를 생성해서 Deck을 주고 있는데,
결국 InitGameSetter의 generateDeck()을 static으로 사용하고 있습니다!
이렇게 되면 Deck을 static으로 선언한게 되는 걸까요?..
리팩토링 해야할까요?!
There was a problem hiding this comment.
넵!ㅎㅎ
스태틱으로 선언해야하는 이유가 뭘까요? 👀
제 생각에 현재 DeckGenerator는 불필요한 것 같아요. 덱 제너레이터 안에 속해있는 모든 로직들은 덱 내부에서 수행해도 무방하다고 생각해요. 성하는 어떻게 생각하시나요?
추가로 Deck의 역할이 뭘까요? 성하는 Deck을 어떻게 정의내리셨나요?
package domain.card;
import java.util.*;
public class Deck {
public final Queue<Card> cards;
public Deck() {
this.cards = new LinkedList<>(generateCards());
}
public Deck(final List<Card> cards) {
this.cards = new LinkedList<>(cards);
}
public Card drawCard() {
return cards.poll();
}
public List<Card> getCards() {
return List.copyOf(cards);
}
private List<Card> generateCards() {
List<Card> cards = new ArrayList<>();
for (Suit suit : Suit.values()) {
addCardBySuit(cards, suit);
}
shuffle(cards);
return cards;
}
private void addCardBySuit(List<Card> cards, Suit suit) {
for (Denomination denomination : Denomination.values()) {
cards.add(new Card(suit, denomination));
}
}
private void shuffle(List<Card> cards) {
Collections.shuffle(cards);
}
}
There was a problem hiding this comment.
우와!! 코드까지 직접 친절하게 보여주셔서 감사합니다!! 😃
저는 덱을 실제 게임에 사용되는 덱으로 생각했어요!
그래서 덱에 자기 자신이 카드를 생성해서 넣어주는 행위가 조금 어색하다고 느껴졌어요!
그래서 게임 쪽에서 카드 리스트를 만들고 덱은 생성될 때 바로 카드 리스트를 받아서 생성된다는 느낌이 더 와닿았던 것 같아요!
static을 사용했던 이유는 단순하게 BlackJackGame 필드로 선언하려고 하다보니 2개가 넘어서
InitGameSetter에서 static으로 덱을 생성해주고 있는데,
생각해보니 BlackJackGame 생성 시에 new DeckGenerator()로 덱 제너레이터를 생성하고 deck을 바로
this.deck = deckGenerator.generate()로 초기화해주면 되지 않을까 싶어요!
이렇게 리팩토링 진행해도 괜찮을까요??
| @ValueSource(ints = {1, 2}) | ||
| @DisplayName("참가자에게 올바르게 카드를 준다.") | ||
| void shouldSuccessDistributeCard(int num) { | ||
| BlackJackGame blackJackGame = new BlackJackGame(); |
There was a problem hiding this comment.
아! 테스트를 삭제했는데 커밋을 실수로 안했네요!! 죄송해요 조앤.. 아 죄송 금지
커밋하겠습니다!! 😎
|
|
||
| public class BlackJackController { | ||
|
|
||
| public void run() { |
There was a problem hiding this comment.
BlackjackGame을 Controller의 멤버가 아닌 지역변수로 선언해주신 이유가 무엇인가요?
| if (receiveOrNot.equals("y")) { | ||
| BlackJackGame.distributeCard(player, 1); | ||
| return true; | ||
| // TODO : indent 1로 줄이기 |
There was a problem hiding this comment.
잘 적어두셨네요ㅎㅎ indent 1로 줄여주세요~ 요구사항은 꼭 지키며 구현하는 연습을 해보아요!
| public void run() { | ||
| BlackJackGame blackJackGame = generateBlackJackGame(); | ||
| printParticipantInitCardsStep(blackJackGame, blackJackGame.getPlayerNames()); | ||
| playerDrawCardStep(blackJackGame, blackJackGame.getPlayers()); |
There was a problem hiding this comment.
컨트롤러에서 도메인을 모르는지 확인할 수 있는 방법이 어떤게 있을까요? 한가지 방법으로는 연관된 도메인 내부를 변경했을 때 컨트롤러에 영향을 미치는지를 확인해볼 수 있겠죠, ㅎㅎ 지금은 우선 컨트롤러에서 도메인은 모르도록 구현해주신 것 같아요. 이제 컨트롤러에 남아있는 비즈니스 로직을 도메인으로 옮기기만 하면 더 좋을 것 같아요. 이건 다른 코멘트로 남겨둘게요~
| private void drawCard(BlackJackGame blackJackGame, Player player) { | ||
| while (player.checkCardsCondition()) { | ||
| String playerName = player.getName(); | ||
| OutputView.printInputReceiveYesOrNotMessage(playerName); | ||
| String receiveOrNot = InputView.inputReceiveOrNot(); | ||
| if (receiveOrNot.equals("y")) { | ||
| blackJackGame.distributeCard(player); | ||
| ResultView.printParticipantResult(playerName, blackJackGame.findCardNamesByParticipantName(playerName)); | ||
| } | ||
| if (receiveOrNot.equals("n")) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
만약 drawCard를 수정해야한다고 했을 때, 가장 먼저 어떤 클래스를 보게 될까요? 저라면 BlackJackGame 클래스일 것 같아요. 왜나면 card를 draw하는 것은 블랙잭 게임에 있어서 핵심 로직이기 때문이에요. 컨트롤러는 전반적인 흐름만을 관리하고 도메인의 핵심 로직은 구현해주신 도메인 내부로 숨겨보는 건 어떨까요?ㅎㅎ
즉, drawCard와 아래 dealerDrawCardStep 을 blackjackGame 내부로 옮겨보시면 좋을 것 같아요ㅎㅎ
지금은 컨트롤러에서 순서만 관장하고 있는 것이 아닌 핵심 로직까지 다루고 있어요!
There was a problem hiding this comment.
AdditionalDrawStatus라는 상태 객체를 둬서,
BlackJackGame에게 메세지를 던지는 식으로 리팩토링 했습니다!!! 😃
| private static final int ACE_VALUE = 1; | ||
| private static final int ANOTHER_ACE_VALUE = 11; |
There was a problem hiding this comment.
이 값은 Denomination 클래스에서 스태틱하게 갖고 있어도 될 것 같아요.
There was a problem hiding this comment.
이 생각은 하지 못했는데 Denomination이 가지고 있는게 역할에 더 맞는 것 같아요!
감사합니다!! 😃
| private List<Integer> getAceSituationValues(List<Integer> totalValues, Card card) { | ||
| List<Integer> commonSituationValues = new ArrayList<>(); | ||
| List<Integer> aceSituationValues = new ArrayList<>(); | ||
| private List<Integer> getAnotherAceValueSum(List<Integer> totalValues) { |
There was a problem hiding this comment.
와 ACE를 11로 계산하는 거 도전해보셨군요!! 👍 👍 멋져용ㅎㅎ
지금 전반적으로 계산하는 로직들이 굉장히 유사한데 쪼~~금씩 달라서 메서드로 구분되어있는데, 어떻게 하면 더 깔끔하게 만들어볼 수 있을까요?!!? 왠지 하실 수 있을 것 같아서 코멘트 남겨봅니다ㅎㅎㅎ
There was a problem hiding this comment.
조앤 덕분에 중복되는 코드들을 잘 발견해서 리팩토링했어요!
확실히 처음 중복이 많았던 코드보다 훨씬 가독성이 좋아진 것 같아요! 😃
감사합니다!!
| public Dealer findDealerByDealerName(String dealerName) { | ||
| if (dealerName.equals(Constants.DEALER_NAME)) { | ||
| return dealer; | ||
| } | ||
| throw new IllegalArgumentException(NOT_MATCH_DEALER_NAME); | ||
| } |
There was a problem hiding this comment.
컨트롤러에서 상수 딜러 이름을 넘겨주고 있어서 발생할 수 없을 것 같아요!
추가로 생각해봤는데 딜러 이름으로 딜러를 반환하는 것이 아니라,
BlackJackGame에서 바로 딜러를 반환해도 될 것 같아서 딜러 이름으로 딜러를 반환하는 기능을 삭제했습니다!
| ResultCount playerResultCount = results.get(player); | ||
| ResultCount dealerResultCount = results.get(dealer); | ||
| int playerCardValueSum = player.getCardValueSum(); | ||
| int dealerCardValueSum = dealer.getCardValueSum(); |
There was a problem hiding this comment.
동일한 부분이 dealer와 player로만 구분되어 반복되는데 어떻게 없앨 수 있을까요? 아래 모두 다 딜러&플레이어가 두벌로 진행되어요!
There was a problem hiding this comment.
이 부분은 어떻게 줄여야할지 모르겠어요 ㅠㅠ
승부를 내는 책임을 Dealer에게 옮겨야 없앨 수 있을 것 같은데,
저는 승부를 내는 책임을 가진 'ResultCalculator'를 만든거라서 지금 이 구조를 유지하고 어떻게 바꿔야할지 생각이 안나네요 ㅠㅠ
There was a problem hiding this comment.
우선 손패를 의미하는 Hand에서 score를 알 수 있도록 하고, ResultCalculator에서 각 플레이어 및 딜러의 hand를 불러와서 서로 비교하도록 할 수도 있을 것 같아요! 이 부분에 대한 리팩터링은 2단계를 진행하면서도 할 수 있으니 그 때 한번 해보시길 바랄게요! 혹 제가 말한 방법 말고 성하가 고민하셔서 다른 방법으로 적용해주셔도 재밌을 것 같아요!!ㅎㅎ
그리고 ResultCalculator라는 중간 역할을 수행하는 유틸성 객체 없이 GameResult 혹은 HandCard에게 ResultCalculator의 역할을 부여할 수도 있을 것 같아요~
| @Test | ||
| @DisplayName("생성한 덱은 52장이다.") | ||
| void generateDeck() { | ||
| DeckGenerator deckGenerator = new DeckGenerator(); | ||
| Deck deck = deckGenerator.generate(); | ||
| List<Card> cards = deck.getCards(); | ||
|
|
||
| Assertions.assertThat(cards.size()).isEqualTo(52); | ||
| } |
|
안녕하세요 조앤!!! 처음부터 설계를 잘했으면 좋았을 것 같은데 아쉽네요 ㅠㅠ 이번 리팩토링에서 주요 리팩토링은 다음과 같습니다! 항상 리뷰해주셔서 감사합니다~! 😃 |
|
|
||
| public class BlackJackController { | ||
|
|
||
| public void run() { |
There was a problem hiding this comment.
오홍ㅎㅎ 그런 이유가 있었군요! 설득력이 있는 것 같아요ㅎㅎ 제 생각엔 어차피 Controller.run()을 항상 실행해주기도 하고, 현재의 블랙잭 컨트롤러는 블랙잭 게임의 실행을 관장하기 블랙잭 게임에서 멤버로 갖고 있어도 무방할 것 같다는 생각이 들었어요.
추가로 이것과는 별개로 아래도 다음과 같이 수정해볼 수 있을 것 같아요.
// Result는 수도코드로 봐주세용
public void run() {
BlackJackGame blackJackGame = generateBlackJackGame();
Result result = blackJackGame.play();
printResult(gameResult);
}
지금 담겨있는 모든 스텝들은 컨트롤러의 로직이 아닌 블랙잭 게임의 비즈니스 로직 같아서요ㅎㅎ 블랙잭 게임 내부로 숨길 수 있을 것 같아요. 다음 단계에 반영해주세요~
| DRAW, | ||
| PASS; | ||
|
|
||
| public static boolean isDrawable(AdditionalDrawStatus additionalDrawStatus) { |
| while (player.checkCardsCondition()) { | ||
| private void drawCardOrPass(BlackJackGame blackJackGame, Player player) { | ||
| AdditionalDrawStatus additionalDrawStatus = AdditionalDrawStatus.DRAW; | ||
| while (AdditionalDrawStatus.isDrawable(additionalDrawStatus) && blackJackGame.canPlayerDrawCard(player)) { |
| } | ||
|
|
||
| public boolean canDealerDrawCard() { | ||
| return participants.canDealerDrawCard(); | ||
| public AdditionalDrawStatus distributePlayerCardOrPass(Player player, String receiveOrNot) { |
There was a problem hiding this comment.
이러한 로직은 전부 블랙잭 게임 안에 있는거 맞는 것 같아요! 👍 👍 컨트롤러에 남아있는 블랙잭 게임의 역할들도 다음 단계에서 모두 옮겨보아요~
| ResultCount playerResultCount = results.get(player); | ||
| ResultCount dealerResultCount = results.get(dealer); | ||
| int playerCardValueSum = player.getCardValueSum(); | ||
| int dealerCardValueSum = dealer.getCardValueSum(); |
There was a problem hiding this comment.
우선 손패를 의미하는 Hand에서 score를 알 수 있도록 하고, ResultCalculator에서 각 플레이어 및 딜러의 hand를 불러와서 서로 비교하도록 할 수도 있을 것 같아요! 이 부분에 대한 리팩터링은 2단계를 진행하면서도 할 수 있으니 그 때 한번 해보시길 바랄게요! 혹 제가 말한 방법 말고 성하가 고민하셔서 다른 방법으로 적용해주셔도 재밌을 것 같아요!!ㅎㅎ
그리고 ResultCalculator라는 중간 역할을 수행하는 유틸성 객체 없이 GameResult 혹은 HandCard에게 ResultCalculator의 역할을 부여할 수도 있을 것 같아요~
| import java.util.List; | ||
| import java.util.Stack; | ||
|
|
||
| public class DeckGenerator { |
There was a problem hiding this comment.
ResultCaculator와 마찬가지로 DeckGenerator도 Deck으로 역할을 위임할 수 있을 것 같아요~
안녕하세요~ 백엔드 5기 크루 성하입니다!
몇 가지 질문들이 있어서 질문 남깁니다!
추상적인 질문들은 PR에 남기고, 코드 질문들은 코멘트로 해당 코드에 남기도록 하겠습니다!
🎯 테스트 커밋 메시지 질문
원래 테스트 커밋 메시지 컨벤션이 테스트 코드가 리팩토링 되어도
test로 고정인 걸로 알고 있습니다!그런데 의미는 '테스트 코드 리팩토링'이라서
refactor를 사용하고 싶은데,컨벤션을 어겨도 되는 걸까요? 조앤은 어떻게 생각하시는지 궁금합니다!