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

[2단계 - 블랙잭 구현] 다니(이다은) 미션 제출합니다. #211

Merged
merged 24 commits into from
Mar 16, 2021

Conversation

da-nyee
Copy link

@da-nyee da-nyee commented Mar 13, 2021

안녕하세요 미립! 다니입니다 🙌

이번 주에 제이슨이 상태 패턴 강의를 해주셔서 이번 단계에서 적용해보려고 했는데, 생각보다 더 어려워서 우선 2단계 기능 구현만 추가해서 제출합니다! 상태 패턴으로 Player에 있는 decide 로직의 조건문을 최대한 없애려고 했는데.. 쉽지 않네요 😭

이번 미션을 진행하면서 궁금한 점들이 몇 개 생겼어요! 질문 드려도 괜찮을까요?!

궁금한 점 1.

앞에서 언급한 것처럼 Player/decide() 로직을 더 좋게 수정할 수 있는 방법이 있을까요?
현재는 if 문이 계속 나열된 형태인데요.. else만 없지 전혀 좋은 방법은 아니라고 생각합니다 ㅠㅠ
수많은 조건문을 지우기 위해서는 역시 상태 패턴이 답일까요...?!

궁금한 점 2.

이전 단계에서 마지막으로 피드백 남겨 주신 게임은 Player 가지고 있는데 외부에서 주입 받을 필요가 있을까요? 내용을 반영하려고 노력했는데, 게임 실행 중간에 뷰가 호출되어야 해서 BlackjackController/askToPlayersForHit()를 게임으로 옮기기 어렵더라구요 💦 이 코멘트를 반영하고 싶은데, 혹시 힌트 주실 수 있을까요??

궁금한 점 3.

저는 디자인 패턴이 유독 더 어려운 것 같아요 😥 아직 코드를 작성하면서 어떤 패턴을 적용하는 게 좋은 건지 감도 잘 안 잡히구요.. 리뷰어님은 디자인 패턴을 어떻게 공부하셨나요?? 여러 코드를 작성하다가 자연스럽게 습득하게 되신 건가요??

제 질문은 여기까지 입니다!

지금은 코드가 많이 지저분하지만 코드 리뷰 받고 리팩토링 하면서 개선해 나가겠습니다!

이번에도 귀한 시간 내주셔서 감사합니다 🙇‍♀️✨

@da-nyee
Copy link
Author

da-nyee commented Mar 13, 2021

📚 다니의 학습 로그

[Java] flatMap - 2

내용

  • map은 각각 별도의 스트림을 생성해서 처리하는 반면, flatMap은 하나의 스트림을 생성해서 처리한다.
  • flatMap을 사용하면 중첩 루프를 없앨 수 있다.

링크

Copy link

@seok-2-o seok-2-o left a comment

Choose a reason for hiding this comment

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

안녕하세요 다니.

말씀하신 것처럼 지금 배당금을 계산하는 로직은 한눈에 알아보기 어려운 것 같아요.
이러한 상황에서 유한 상태 머신을 이용하면 가독성 높고 유연한 설계할 수 있을 것 같아요.

조금 어려운 길일 수 있지만, 지금 코드를 그대로 두고 유한 상태머신을 이용한 블랙잭을 구현해보고 비교해보면 어떨까요?

게임이 현재 차례의 플레이어를 기억 하고 있다면 외부에서 플레이어를 가지고 있지 않을 수 있을 것 같아요.
(player.isFinish() 를 제공하고 탐색, 혹은 인덱스를 이용하는 방법 등이 있겠네요.)

디자인패턴은 단기간에 익히기 어려운 부분인 것 같습니다. 오히려 어떤 패턴을 의식하다보면 더 쉬운 길을 나두고 어려운 코드를 작성하게 되기도 하죠 😭 이부분은 역시 다양한 케이스를 연습해보는 수 밖에 없을 것 같습니다. 첫 질문에 답변 드린 것처럼 동일한 문제를 다양한 패턴으로 구현해보고 비교해보는것이 많이 도움이 될 것이라고 생각합니다.

Comment on lines 12 to 13
List<String> names = setUpNames();
List<Double> moneyGroup = setUpMoneyGroup(names);

Choose a reason for hiding this comment

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

둘을 따로 받지 말고 한번에 받아서 처리 하는건 어떻게 생각하시나요.

public static Players getPlayers() {
  List<String> names  = ...
  for(String name : names) {
        Money money = ...
        players.add(new Player(name, money);
  }
}

Comment on lines 16 to 19
List<Card> cards = Arrays.stream(Shape.values())
.flatMap(shape -> Arrays.stream(Value.values())
.map(value -> new Card(shape, value)))
.collect(toList());
Copy link

@seok-2-o seok-2-o Mar 14, 2021

Choose a reason for hiding this comment

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

👍

로또에서 배운 캐싱을 적용해볼 수 있지 않을까요?

.map(value -> new Card(shape, value))
.collect(toList());
}

private void shuffle(List<Card> cards) {

Choose a reason for hiding this comment

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

메소드로 분리했을때 장점이 어떤 것들이 있나요? 🤔

import java.util.Objects;

public class Money {
private final double money;

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.

플레이어가 블랙잭인 경우에 베팅 금액의 1.5배를 얻으니까 소수점도 신경써야 할 것 같아서 double로 지정했습니다!
double보다는 int가 나을까요??

Choose a reason for hiding this comment

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

아뇨 이유가 있다면 ! 그대로 두어도 무방합니다.

@da-nyee
Copy link
Author

da-nyee commented Mar 15, 2021

안녕하세요 미립! 다니입니다 🙌

남겨주신 코멘트들 바탕으로 리팩토링 진행했어요!

변경한 내용 1.

드디어..!!! 상태 패턴을 사용했습니다...!!!! 강의도 다시 보고 여러 코드를 참고하면서 적용했어요!! 상태 패턴을 활용하고 싶어서 며칠 고민한 거 같은데,, 맞게 한 건지는 모르겠지만 약간 뿌듯합니다..!! 상태 패턴 덕분에 Player에 있던 수많은 if 문들을 없앨 수 있었어요! 어려웠지만 이런 방식으로도 코드를 짤 수 있구나를 느낄 수 있던 시간이었습니다!

변경한 내용 2.

게임이 현재 차례의 플레이어를 기억하도록 수정해서 외부에서 플레이어를 따로 주입 받지 않게 만들었어요!

변경한 내용 3.

컨트롤러에서 플레이어의 이름, 베팅 금액을 따로 받지 않고 한번에 입력 받게 수정했어요!

변경한 내용 4.

로또에서 배운 캐싱을 적용했습니다! 제가 지난 리뷰 때 잘못 이해하고 있었더라구요..! 저는 캐싱 자체를 하면 안 된다고 이해했는데, 다시 생각해보니까 Deck은 캐싱을 하면 안 되지만 Cards는 캐싱을 할 수 있는 거였어요! 그래서 이번에는 전체 카드를 캐싱하도록 코드를 수정했습니다!

변경한 내용 5.

shuffle()를 따로 메소드로 분리하지 않았어요! 메소드로 분리했을 때 장점이 전혀 없더라구요..! 그래서 생성자에서 덱에 전체 카드를 넣기 전에 전체 카드를 먼저 shuffle()하도록 수정했습니다!

이번에도 코드 리뷰 기다리고 있겠습니다~!

항상 귀한 시간 내주셔서 감사합니다 🙇‍♀️❣

Copy link

@seok-2-o seok-2-o left a comment

Choose a reason for hiding this comment

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

안녕하세요. 다니!
수많은 피드백을 반영하느라 정말 고생이 많으셨어요 💯
좋은 개발자로 상장하는데 조금이나마 도움이 되었으면 좋겠네요 🙇

소소한 코멘트 몇가지를 마지막으로 이번 미션은 여기서 마무리하도록 하겠습니다 :)

다니와 함께한 시간이 너무 즐거웠네요. 다니에게도 부디 그러하길 바라봅니다.

다음 미션도 재밌게 진행하기를 바랍니다.
이번 미션이 아니더라도 궁금한점 어려운점 있으면 편하게 DM 주세요.

감사합니다.

@@ -6,19 +6,20 @@
import static java.util.stream.Collectors.*;

public class Deck {
private final Deque<Card> deck;
private static final List<Card> CARDS;

Choose a reason for hiding this comment

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

Card 를 캐싱한값은 Card 가 가지고 있는게 좋을지 Deck 이 가지고 있는게 좋을지 고민해보면 좋을것 같아요 🤔

import blackjack.domain.user.Dealer;
import blackjack.domain.user.Money;

public abstract class Finished extends Started {

Choose a reason for hiding this comment

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

상태를 묶어서 상태를 만들어주셨네요 👍

.anyMatch(Player::isHit);
}

public Player currentPlayer() {

Choose a reason for hiding this comment

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

👍
매번 탐색을 하게 되긴하지만
지금처럼 플레이어의 수가 한정되어 있는 (최대 7명 인가요?) 경우에는 가독성 측면에서 좋은 선택인 것 같아요.

@seok-2-o seok-2-o merged commit a7af9c6 into woowacourse:da-nyee Mar 16, 2021
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.

None yet

2 participants