Skip to content
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

[1단계 - 블랙잭 게임 실행] 저문(유정훈) 미션 제출합니다. #472

Merged
merged 43 commits into from
Mar 7, 2023

Conversation

jeomxon
Copy link

@jeomxon jeomxon commented Mar 3, 2023

안녕하세요 검프 :)
백엔드 5기 크루 저문입니다!
이번 블랙잭 게임에 대한 리뷰 잘 부탁드리겠습니다.🙇🏻‍♂️🙇🏻‍♂️

페어인 라온과 많은 이야기를 하면서 구현을 하였기에 좋은 생각을 많이 들을 수 있어서 좋았습니다.
반면 미션의 난이도도 어려워졌고, 많은 대화를 통해 시간을 쓴 만큼 구현에도 시간이 많이 소요됐던 것 같습니다.
구현에 급급했던지 테스트와 리팩터링이 생각처럼 되지 않아서 아쉬움이 남습니다ㅠㅠ
아직 많이 부족한만큼 많은 리뷰를 통해서도 성장하고 싶은 마음이 큰 것 같습니다.
가감없이 리뷰 해주시면 성실히 반영하도록 하겠습니다!!😃

이번 미션을 진행하는 과정에서도 질문이 있어 여기에 남기겠습니다.


1. Card를 만드는데 있어서 52개를 상수화하는 것 vs 카드를 만들어놓지 않고 뽑을 때마다 만들기

카드를 만드는 과정에서 페어인 라온과 계속해서 고민했던 부분이었습니다.
카드 52개를 전부 상수화하여 미리 카드 객체를 만들어준다면 카드를 뽑을 때마다 객체를 생성해야할 필요가 없고,
만들어 놓을 때마다 뽑게된다면 뽑을 때마다 객체를 생성해야하지만 모든 카드들에 대한 객체를 생성하지 않아도 됩니다.
이런 관점에서 봤을 때 어떤 방식이 더 좋은지 결정을 할 수 없었기에 실제 카드처럼 생각하여 전자를 택하였습니다.
검프의 의견은 어떠실지 궁금합니다!

2. 복잡하게 얽힌 로직이 발생했을 때 생각해야할 것

메인 로직에 대해 구현을 하다보니 BlackJackGameControllerBlackJackGame에 큰 로직이 생겼습니다.
이를 분리하기 위해서 여러 개의 메서드를 만들게 되었는데, 그에 따른 가독성 저하가 발생한 것 같습니다.
설계가 처음부터 잘못된 것인지, 로직을 더 깔끔하게 분리할 수 있는 좋은 방법이 있는지 궁금합니다!
뭔가 억지로 메서드를 분리하는 것 같고, 메서드 명을 정하기도 힘들었던 것 같습니다ㅠㅠ

혹시나 추가적으로 질문이 생긴다면 말씀드리도록 하겠습니다!
리뷰 감사드립니다 :)

Copy link

@Livenow14 Livenow14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 저문! 구현 잘 해주셨어요. 몇가지 간단한 피드백 남겼으니 확인해주세요.
진행하시다가 어려우시면 DM, 커멘트 부담없이 달아주세요 ㅎㅎ

질문에 대한 답변 남깁니다.

  1. Card를 만드는데 있어서 52개를 상수화하는 것 vs 카드를 만들어놓지 않고 뽑을 때마다 만들기

저도 전자를 택하는 편입니다. 도메인내에 그의미가 명확하게 드러나는게 좋거든요.
추가적으로 의미를 줄 수 잇는 카드가 필요한 경우(조커같은..?), 정적 팩토리 메서드를 통해, 추가적인 인스턴스를 만들 수 있게 합니다 :)

  1. 복잡하게 얽힌 로직이 발생했을 때 생각해야할 것

저라면, 우선 메서드 분리를 진행하지 않고, 하나의 메서드에 몰아넣습니다.
그다음 해당 로직을 테스트하기위한 테스트를 작성해두고, 눈에보이는 것들을 리팩터링 해봐요.
리팩터링 마다 테스트를 진행하고 -> 테스트가 통과한다면 -> 커밋을 하고 그다음 리팩터링을 진행해요.

너무 처음부터 분리하다보면, 오히려 변경을 하지 못하게 되더라고요. 도메인 지식이 충분하지 않을 땐, 해당 방법을 통해 이해도를 높이는 것도 방법입니다.!

전반적인 피드백은 남긴다면,

pr 후 추가 커밋 or 수정은 금지입니다!!

pr후 수정시, 바로 Merge가 리뷰의 규칙이라, 다음부터는 꼭 신경써주세요 :)

테스트 코드를 조금더 작성해주세요.

현재 요구사항을 전체로 테스트해볼 수 없다고 느껴져요. 구현에 집중하기 보다는 테스트에 조금더 집중해보셨으면 좋겠어요..!

고생많으셨습니다.

README.md Outdated
Comment on lines 9 to 24
## 실행 프로세스

블랙잭 게임을 변형한 프로그램을 구현한다. 블랙잭 게임은 딜러와 플레이어 중 카드의 합이 21 또는 21에 가장 가까운 숫자를 가지는 쪽이 이기는 게임이다.

카드의 숫자 계산은 카드 숫자를 기본으로 하며, 예외로 Ace는 1 또는 11로 계산할 수 있으며, King, Queen, Jack은 각각 10으로 계산한다.
게임을 시작하면 플레이어는 두 장의 카드를 지급 받으며, 두 장의 카드 숫자를 합쳐 21을 초과하지 않으면서 21에 가깝게 만들면 이긴다. 21을 넘지 않을 경우 원한다면 얼마든지 카드를 계속 뽑을 수 있다.
딜러는 처음에 받은 2장의 합계가 16이하이면 반드시 1장의 카드를 추가로 받아야 하고, 17점 이상이면 추가로 받을 수 없다.
게임을 완료한 후 각 플레이어별로 승패를 출력한다.

1. 게임에 참여할 사람의 이름 입력
2. 딜러와 플레이어에게 2장씩 카드를 나눠줌
3. 각 플레이어에게 카드 더 받을건지 물어봄
3-1. 플레이어가 "n"를 입력할 때까지 반복
4. 딜러의 카드가 17 이상이 될 때까지 카드를 더 받음
5. 딜러와 플레이어 카드 출력
6. 최종 승패 출력

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용자 스토리가 잘 정의되어 있네요 :)

실행프로세스 -> 사용자 스토리로 명명해보는건 어떨까요/.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 이전에 따로 적어보진 않았는데, 페어의 의견을 받아서 함께 작성해보았습니다!
사용자 스토리로 변경하겠습니다!!


public class Application {
public static void main(String[] args) {
BlackJackGameController blackJackGameController = new BlackJackGameController();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

controller에는 따로 인자가 없나 보네요..?
(밑에서 확인해보겠습니다 )

Comment on lines 19 to 20
final BlackJackGame blackJackGame = generateBlackJackGame();
final ShufflingMachine shufflingMachine = new ShufflingMachine();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BlackJackGame과 ShufflingMachine은 controller의 인자로 둬도 충분할 것 같아요!!

전체 메서드에서 해당 객체를 공유하고있어서요 :)
파라미터를 줄 일 수 잇을 것 같습니다.

Copy link
Author

@jeomxon jeomxon Mar 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그런 방법도 있네요 전혀 생각도 못했습니다..!
그런데 BlackJackGame을 생성자 주입을 통해서 만들게 되면
현재 BlackJackGame이 이름을 입력받아서 Players객체를 생성하는 책임을 가지고 있기 때문에 인자로 두는 것이 어려울 것이라고 판단됩니다.
Players객체를 Controller에서 생성하여 BlackJackGame에 생성자 주입을 통해 만드는 방법을 사용하여 바꾸는 방식을 사용하게되면 Main메서드에서 입력을 받게되는 경우가 생길 것 같습니다.
혹시 제가 생각이 짧아 구현을 하지 못했을 가능성이 높은데,
방법이 있다면 다시 시도해보도록 하겠습니다.

일단 ShufflingMachine만 인자로 넘겼습니다!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

빈 Players가 존재할 수 잇을 것 같다는 생각이들어요 :)

Game에 Player들이 참여한다고 생각해보면 어떨까요?
참여 규칙은, 이름이 있어야 하는 것이구요. (이름이 입력되면 Game에 플레이어로 추가된다. )

Comment on lines 22 to 23
blackJackGame.handOutInitCards(shufflingMachine);
final Dealer dealer = blackJackGame.getDealer();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handOutInitCards...?
Init을 빼고 handOutCards 도 괜찮을 것 같아요.

아래 handOutCardToXXX 등이 있는데, 하나로 추상화 해도 괜찮지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네이밍은 말씀해주신게 더 좋은 것 같아서 변경했습니다!
그러나 Dealer와 Player를 나눈 이유는 둘의 로직이 엄연히 다르다고 생각해서였습니다.
Dealer와 Player의 카드를 뽑는 기준이 다르다는 것을 예시로 들 수 있습니다.
혹시 제가 생각하지 못한 좋은 방법이 있다면 다시 생각해보도록 하겠습니다!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메서드 오버로딩 규칙에 의해서, 네이밍을 동일하게 가져가도 되지 않나요?!
파라미터만 다르게 설정하면 될 것 같아서요.

추가적으로, Player, Dealer는 같은 부모를 가지고 있는데, 로직이 엄연히 다르다는게 조금 납득이 되지 않아요!
동일한 책임을 가지고 있기에, 동일한 행동을 수행한다고 기대돼서요.

private void handOutCard(final BlackJackGame blackJackGame, final Participant participant)

로 설정할 수 없다면, 같은 부모를 가지면 안될 것 같아요.

++ handOutCardToPlayers, handOutCardToDealer 가 순서종속성을 가진다고도 판달할 수 있을 것 같아요.

Comment on lines 18 to 34
public void run() {
final BlackJackGame blackJackGame = generateBlackJackGame();
final ShufflingMachine shufflingMachine = new ShufflingMachine();

blackJackGame.handOutInitCards(shufflingMachine);
final Dealer dealer = blackJackGame.getDealer();
final Players players = blackJackGame.getPlayers();

OutputView.printInitCard(players.getPlayers(), dealer.getFirstCard());

handOutCardToPlayers(blackJackGame, shufflingMachine, players);
handOutCardToDealer(blackJackGame, shufflingMachine, dealer);

blackJackGame.findWinner();
OutputView.printCardsWithSum(players.getPlayers(), dealer);
OutputView.printFinalResult(players.getPlayers(), dealer.getResults());
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run() 메서드에 많은 책임이 들어있어요.
게임을 실행하기 위한 게임 준비 -> 실행 -> 결과 출력 등을 나타내면 좋은데,
한눈에 보이지 않아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메서드 분리를 통해서 변경해보았습니다!

return false;
}

private boolean judgeResultWhenPlayerIsBust(final Player player) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

judgeResultWhenPlayerIsBust -> isBustPlayer

Comment on lines 93 to 106
private void setUpResultWhenPush(final Player player) {
dealer.setResults(Result.PUSH);
player.setResult(Result.PUSH);
}

private void setUpResultWhenDealerWin(final Player player) {
dealer.setResults(Result.WIN);
player.setResult(Result.LOSE);
}

private void setUpResultWhenPlayerWin(final Player player) {
dealer.setResults(Result.LOSE);
player.setResult(Result.WIN);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

외부에서 set을 하는 과정이 꼭 필요할까요..?
게임이 진행되며 상태가 자연적으로 변경되면, 각 객체들의 상태가 reulst를 대변해줄 것 같아서요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GameResult객체를 만들어서 풀어보았습니다!
물어볼 때마다 친절히 도움주셔서 감사합니다!


public class Players {

private static final String COMMA = ",";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

한번밖에 쓰이지 않는 상수화의 경우, 메서드 내에서 그 의미를 드러낼 수 있도록 해주세요.

Comment on lines 17 to 19
public Players(final String input) {
this.players = makePlayers(input);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

생성자에 로직을 넣지 말아주세요..!
생성자가 무거워지는 단점이 있어, 사용하기 어려운 객체가 됩니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그런 단점이 존재하군요!
따로 메서드를 만들지 말고, 로직을 생성자에 그대로 넣으라는 말씀일까요?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

외부에서, Collection players를 만들어서 넣어줄 수는 없을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 split하는 로직과 validate를 해주는 로직이 둘 다 존재해야하기 때문에 위와 같은 방법을 사용했습니다.
특히 split하는 역할은 도메인에 있어야한다고 생각이 들었습니다.
따라서 생성자를 무겁지 않게 하면서, 위와 같은 로직을 사용하려면 정적 팩토리 메서드를 사용하면 될 것 같다는 생각을 했습니다!
제 생각의 방향이 잘못되었다면 지적해주시면 바로 수정하겠습니다!

Comment on lines 44 to 46
Card card1 = new Card(rank1, Suit.CLOVER);
Card card2 = new Card(rank2, Suit.SPADE);
Card card3 = new Card(rank3, Suit.SPADE);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트 인스턴스에 1,2,3 등 넘버링은 지양해주세요.
테스트의 목적이 흐려지게 됩니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 지적 감사합니다!
수정하였습니다.

@jeomxon
Copy link
Author

jeomxon commented Mar 7, 2023

안녕하세요 검프 :)
질문을 할 때마다 친절하게 답변해주셔서 감사드립니다🙇🏻‍♂️
말씀해주신 코멘트 반영하여 리팩터링 진행해보았습니다.
많이 부족하지만 주신 코멘트는 최대한 반영하려고 고민하고 노력했습니다!!
생각이 짧아 어려움을 겪다보니 생각보다 시간이 오래걸린 것 같네요...

감사합니다.

Copy link

@Livenow14 Livenow14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 저문! 구현잘해주셨어요.

아무래도 객체지향적 사고를 요하는 피드백을 많이 남겼었는데, 거기서 많이 어려움을 느끼시는 것 같아요..!
시간 나실때, "객체지향의 사실과 오해"를 한번 편안하게 읽어보시길 추천드려요 (정리x, 공부x)

성급하다고 얻을 수 있는것이 아니기때문에, 천천히 여유를 가지길 바랍니다 :)
교육 때, 객체지향을 이해하는 것 하나만 목표로 잡아도 정말 큰 수확이거든요..

다음미션에 피드백 반영해도 될 것 같아서, 이번 미션은 여기서 마무리 할게요!!
수고하셨습니다 :)

final Dealer dealer = blackJackGame.getDealer();
final Players players = blackJackGame.getPlayers();

final Map<Player, ResultType> playerResult = playBlackJackGame(blackJackGame, dealer, players);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BlackJackGame에서 dealer, players를 빼내서 프로세스를 진행하는것보다, BlackJackGame에서 관리할 순 없는걸까요?!

private BlackJackGame generateBlackJackGame() {
Optional<BlackJackGame> blackJackGame;
do {
blackJackGame = checkNames();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkName인데, Return값이 있는게 어색하게 느껴져요.
내부 구현을 봤을 땐, create & get의 느낌이 더 강한것 같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네이밍 수정하였습니다!

Comment on lines +74 to +78
private void handOutCardToPlayers(final BlackJackGame blackJackGame, final Players players) {
for (final Player player : players.getPlayers()) {
handOutCardToEachPlayer(blackJackGame, player);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

depth 2를 피하기위함이라면, dpeth2 까지는 괜찮아보여요.
또한, players를 꺼내서 다른 게산로직에 넣는게 어색해보여요.

Comment on lines +21 to +24
public BlackJackGame(final Dealer dealer, final String inputNames) {
this.dealer = dealer;
this.players = new Players(inputNames);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

외부에서, Players도 만들어서 넣어줄 수 있을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 방법인 것 같네요!
수정하였습니다.

private static final int NUMBER_OF_BLACKJACK_CARD = 2;

private final Set<Card> cards;
private int numberOfElevenAce = 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set cards; 에서 numberOfElevenAce을 충분히 가져올 수 있을 것 같아요.
해당속성은 제거되도 좋을 것 같네요 :)


import java.util.Scanner;

public class InputView {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InputView가 Util성이 아닌, 하나의 객체로 관리되는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

View의 클래스들 같은 경우에는 상태를 가지지 않는 클래스라고 판단하여 따로 객체를 만들지 않았습니다.
또한 객체로 관리를 하게된다면 Controller가 View를 가지게 된다고 생각하여 util성으로 만들었습니다!
이에 대한 검프의 의견이 궁금합니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실상 그런의도로 관리되려면, 도메인 정보가 Util성 클래스에 들어가면 안될 것 같아요.

Util성 클래스의 성격에 맞게 "어디서든지" 쓰일 수 있게 하려면, 어떤 도메인 정보로 요청하던 책임을 수행할 수 있게해야할 것 같아요.

InputView.readWantMoreCard(),
OutputView.printFirstDealerCards()

은 몇몇 도메인에 특화되어있는 것 같아서요.

또한, 상태를 가지지 않으신다고 하셨는데, InputView의 경우 InputStream을 가지는 Scanner를, OutputView의 경우, StringBuilder, PrintStream인 System.out 을 가지고 있네요.

과도한 설계이긴 하겠지만, 작성해주신 도메인은 Scanner와, PrintStream을 통한 출력이 가능한 도메인일까요?

힌트, Java API인 Util성 클래스 Collections 의 구현을 한번 보시면, 인사이트를 얻을 수 있을 것 같습니다 :)

import java.util.Set;
import java.util.stream.Collectors;

public class OutputView {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OutputView도 마찬가지..!

(Util성 클래스는 어디서도 쓸 수 있기 때문에, 응집도가 떨어져요.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위의 코멘트에 의견을 남겼습니다!

// then
assertThat(dealerResult.get(ResultType.LOSE)).isEqualTo(2);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개행이 있네요 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개행이 없어서 추가하였습니다.
인텔리제이에 좋은 설정이 있다는걸 이번 계기로 알게 됐습니다 :)

Comment on lines +20 to +25
// assertThat(values.remove(0)).isEqualTo("first"); // 첫 번째 값을 삭제한다.
// assertThat(values.size()).isEqualTo(2); // 값이 삭제 됐는지 확인한다.

// TODO values에 담긴 모든 값을 출력한다.
// values.stream()
// .forEach(System.out::println);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주석 제거 부탁드려요 :)

@@ -0,0 +1,27 @@
package techcourse.jcf.mission;

public interface SimpleList {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리스트를 재구현 할 이유가 있었을까요..?!

바퀴를 재발명하는 느낌이네요...

Copy link
Author

@jeomxon jeomxon Mar 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리스트 미니 미션 구현을 위해 작성했습니다!

@Livenow14 Livenow14 merged commit 56fbe96 into woowacourse:jeomxon Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants