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단계 - 블랙잭 게임 실행] 우르(김현우) 미션 제출합니다. #391

Merged
merged 60 commits into from
Mar 7, 2023

Conversation

java-saeng
Copy link

안녕하세요 범블비
이번 리뷰를 받게된 5기 우르라고 해요~~

먼저 전체적으로 도메인끼리의 의존성을 파악하기 위해서 클래스 다이어그램 하나 올려보겠습니다
image

제가 최근에 '객체지향의 사실과 오해'라는 책을 3번 읽다가 실패해서 이번에 4번째 트라이에 성공했습니다.

그래서 그런지 아래와 같은 생각들을 항상 하는 것 같아요.

  1. 객체지향적인 설계란 무엇일까?
  2. 객체지향적인 설게는 어떻게 하는걸까?
  3. 어떻게 잘할 수 있을까?

물론 아직까지 저의 객체지향은 찾지 못했습니다,,

객체지향의 사실과 오해 7장

지금 미션을 하면서 위와 같은 질문들에 대한 훈련을 하고 있다고 생각합니다만 제 나름대로 판단이 잘 되지 않기 때문에 리뷰를 받는다고 생각해요.

그래서 저는 범블비의 객체지향이란 무엇이며, 이번 미션에는 충분히 객체지향적인지 알고 싶어요.

이번 리뷰 감사히 잘 받겠습니다 ~~ 좋은 주말 보내세요 :)

shin-mallang and others added 30 commits February 28, 2023 15:02
- DealerCardArea, ParticipantCardArea 제거
- null 이거나 hit 상태일 경우 hit를 할 수 있음
- 그래서 null 과 hit 상태를 모두 나타내기 위해 연산을 풀어서 작성
- hit 상태를 State에게 직접 물어봄
Copy link

@ddaaac ddaaac left a comment

Choose a reason for hiding this comment

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

안녕하세요 우르 👋

전체적으로 구현 잘 해주셨습니다.

몇 가지 코멘트를 남겼으니 확인해주세요~


그래서 저는 범블비의 객체지향이란 무엇이며, 이번 미션에는 충분히 객체지향적인지 알고 싶어요.

조금 많이 추상적인 질문이긴 하네요 ㅎㅎ
간단하게 생각하면 객체에게 적절한 책임과 역할을 부여하는 것이라고 얘기하고 싶어요.
개인적으로 객체지향이 어떤건지 생각하는 것도 중요하지만 왜 객체지향을 해야하는가 역시 중요하다고 생각합니다.
이런 방법론들은 결국에는 읽기 좋은 코드, 유지보수하기 쉬운 코드를 작성하는 데 목적이 있다고 생각해서 지나치게 방법론에 매몰되지 않고 어떤 코드가 읽기 좋은가 이런 부분에 좀 더 초점을 두면 좋을 것 같아요.
개인적으로 우르 코드는 읽는 데 크게 어려움이 없었습니다. 잘 학습하고 계신 것 같으니 앞으로도 이런 고민들 많이 하시면서 구현하시면 좋을 것 같네요 :)

Comment on lines 114 to 121
return new Dealer(new CardArea(cardDeck.draw(), cardDeck.draw()));
}

private List<Participant> dealParticipantsCards(final CardDeck cardDeck) {
return InputView.readParticipantsName()
.stream()
.map(Name::new)
.map(name -> new Participant(name, new CardArea(cardDeck.draw(), cardDeck.draw())))
Copy link

Choose a reason for hiding this comment

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

처음 시작할 때 두 장을 뽑는다는 건 도메인 룰인 것 같아요. 모델 내부로 로직을 이동시키는 건 어떨까요?

Copy link
Author

@java-saeng java-saeng Mar 5, 2023

Choose a reason for hiding this comment

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

말씀대로 두 장을 뽑는다는 것은 도메인 규칙이라서 CardTable 이라는 객체를 통해 카드 두장을 뽑아서 Dealer와 Participant 에게 전달하는 식으로 작성했습니다.

도메인 룰에 따라서 CardTable 에 참여자 각 1장 - 딜러 1장 - 참여자 각 1장 - 딜러 1장으로 수정했습니다

Comment on lines 28 to 33
while (aceCount > 0) {
if (totalValue <= 11) {
totalValue += 10;
}
aceCount--;
}
Copy link

Choose a reason for hiding this comment

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

  • 이런 값들은 상수로 관리하면 좋을 것 같아요
  • 블랙잭은 최대 점수가 21이기 때문에 ace 갯수를 셀 필요가 없습니다. 단순히 ace가 존재하면 조건으로 체크하면 로직을 조금 더 간단하게 표현할 수 있을 것 같아요.

Copy link
Author

@java-saeng java-saeng Mar 6, 2023

Choose a reason for hiding this comment

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

이런 값들은 상수로 관리하면 좋을 것 같아요

아,, 이 부분은 꼭 했어야했는데ㅠㅠ 다음부터는 정신차리고 하겠습니다 !!

블랙잭은 최대 점수가 21이기 때문에 ace 갯수를 셀 필요가 없습니다. 단순히 ace가 존재하면 조건으로 체크하면 로직을 조금 더 간단하게 표현할 수 있을 것 같아요.

아래 이미지는 제가 작성한 테스트 케이스 들인데요.
image

범블비가 말씀해주신 힌트를 보고 계속 생각을 해봤는데 ace가 존재하는 조건으로 체크하면
1. ace가 존재, 1점 처리
2. ace가 존재, 11점 처리
3. ace가 두개 존재, 1개는 1점, 1개는 11점 처리
4. ace가 두개 존재, 1개는 1점, 1개는 1점 처리
5. ace가 두개 초과할 땐 모두 1점 처리

되는 조건이 있어서 더 복잡하게 되지 않을까 생각이 드는데, 혹시 범블비가 생각하신 부분에 대해 힌트를 좀 더 받을 수 있을까요!!?

오늘 블랙잭 피드백 강의를 듣고 범블비의 말씀을 깨달아서 수정했습니다~~

커밋 링크

cards.add(card);
}

public int calculate() {
Copy link

Choose a reason for hiding this comment

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

int CardArea#calculate란 시그니처를 보면 어떤 값을 반환하는지 확실하지 않아보여요. calculateScore 등으로 이름을 바꾸거나 Score라는 값 객체 타입을 반환하면 이를 표현해줄수도 있겠네요.

Copy link
Author

@java-saeng java-saeng Mar 6, 2023

Choose a reason for hiding this comment

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

Score 라는 값 객체를 사용해서 표현해봤습니다. int 로 나타내는 것보다 타입으로 명시적으로 보여주는게 다른 사람이 코드를 보았을 때도 무엇을 계산하는지 파악하는 것보다 점수를 반환하기 때문에 점수를 계산하는구나라고 바로 알 수 있을 것 같습니다 ~~

2차 미션 때는 돈이 나오기 때문에 int money, int score 보다는 Money, Score 로 표현하는게 더 좋다고 생각했습니다!!

return calculate() < 21;
}

public boolean isBurst() {
Copy link

Choose a reason for hiding this comment

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

저도 처음 구현할 때는 burst인줄 알았는데 bust 더라구요. 😄
블랙잭 룰을 한 번 자세히 읽어봐도 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

오 감사합니다!! 규칙 자세히 읽고 다른 로직도 한번 수정해보겠습니다 ~~

}

public static void showStateOf(final Dealer dealer) {
final Card dealerFistCard = dealer.cardArea().firstCard();
Copy link

Choose a reason for hiding this comment

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

"딜러는 첫 번째 카드만 보여줘야한다" 역시 도메인 룰인 것 같아요.
View에서 처리되기보다는 Participant와 Dealer의 차이로 코드에 명시돼야하지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

오... 맞습니다 범블비

"딜러의 cardArea에서 첫번째 카드를 뽑는다" 보다는

"딜러는 첫번째 카드를 보여준다" 가 더욱 자연스러워 보여서 첫번째 카드만을 보여주는 것은 딜러에서 행동하는 것으로 수정했습니다!!

다만 여기서 생각해야할 부분은 위의 로직을 추가하면서 Dealer에 Card라는 의존성이 추가가 됐습니다.

Card 라는 의존성이 추가 됐지만, Dealer와 Card가 아예 연관없는 의존성이 아니기도 하고, 코드가 훨씬 잘 읽히기 때문에 괜찮다고 생각했습니다 !!


@Override
public boolean canHit() {
return cardArea.calculate() <= DEALER_LIMIT_SCORE;
Copy link

Choose a reason for hiding this comment

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

Suggested change
return cardArea.calculate() <= DEALER_LIMIT_SCORE;
return score() <= DEALER_LIMIT_SCORE;

로도 가능할 것 같아요. 그러면 cardArea의 접근제어자도 변경할 수 있어보이구요.

Copy link
Author

Choose a reason for hiding this comment

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

이미 Player 에 score 라는 메서드가 존재했었네요,,

말씀대로 score 로 수정하는게 좋아보입니다 !! 다만 cardArea의 접근 제어자를 변경하는건 어려워보입니다..ㅠㅠ

 // Dealer 클래스

public Card faceUpFirstCard() {
    return cardArea.firstCard();
}

첫 번째 카드만 보여주는 메서드입니다.

그래서 이 부분을 Player 로 빼고 cardArea의 접근 제어자를 변경해볼까? 라는 생각을 했었는데, 해당 규칙은 Dealer의 고유한 규칙이라고 생각이 들어서 여기에 놔두는게 적합하지 않을까 라는 생각을 했습니다 !

+리뷰를 보고 Name의 접근제어자를 변경해야겠다 생각이 들어서 protected -> private 로 변경했습니다

Copy link

Choose a reason for hiding this comment

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

image

에서 "딜러"만 "딜러카드"로 표현한다면

이 부분도 추상 메서드로 뺼 수 있지 않을까 하는 생각이 들긴하네요.
추상 메서드로 표현되는만큼 딜러와 참가자 간에 이런 차이가 있구나 하고 명확히 표현할 수도 있겠구요.

Comment on lines 92 to 103
private void hitForParticipant(final CardDeck cardDeck, final Participant participant) {
while (participant.canHit()) {
participant.changeState(inputHitOrStay(participant));
determineHitForParticipant(cardDeck, participant);
}
}

private void determineHitForParticipant(final CardDeck cardDeck, final Participant participant) {
if (participant.wantHit()) {
participant.hit(cardDeck.draw());
}
OutputView.showStateOf(participant);
Copy link

Choose a reason for hiding this comment

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

want라는 개념을 뽑아내주셨네요 👍
다만, player의 상태를 바꿀 필요 없이

while (participant.canHit() && inputHitOrStay() == YES) {
...

이런 식으로 구현해도 되지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

범블비 말씀을 듣고 굳이 State 가 필요없지 않을까라는 생각을 했어요.

제가 봤을 땐,

  1. canHit 가 true 라고 가정
  2. y 면 무조건 hit
  3. n 이면 stay

그래서 canHit 로 hit 할 수 있는지 없는지 판단하고, y와 n을 통해서 카드를 draw 할지 말지 결정할 수 있기 때문에 State 가 굳이 필요없다라고 판단했습니다!!

Comment on lines 11 to 25
public static ParticipantResult matchBetween(final Participant participant, final Dealer dealer) {
if (participant.isBurst()) {
return ParticipantResult.LOSER;
}
if (dealer.isBurst()) {
return ParticipantResult.WINNER;
}
if (participant.score() > dealer.score()) {
return ParticipantResult.WINNER;
}
if (participant.score() == dealer.score()) {
return ParticipantResult.DRAWER;
}
return ParticipantResult.LOSER;
}
Copy link

Choose a reason for hiding this comment

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

전체적으로 블랙잭 룰을 잘 파악해주신 것 같네요.

이렇게 static을 사용한 코드와 dealer.matchWith(participant)와 같은 코드에는 어떤 차이가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

범블비가 주신 질문의 의도를 제대로 파악하지 못한 것 같아서, 제 나름대로의 답변을 먼저 드리겠습니다!!
만약에 의도하신 부분이 아니시라면 조금 더 자세하게 말씀해주실 수 있으실까요?

  1. 상태에 의존적인가?
  • static method는 클래스의 인스턴스와는 관련이 없기 때문에 객체의 상태에 의존적이지 않도록 행해지는 메서드입니다.
  • instance method는 그 반대로 클래스의 인스턴스와 관련이 있어서 상태에 의존적입니다.
  1. 누구의 책임인가?
    범블비가 말씀하신 dealer.matchWith(participant) 와 제가 작성한 Participant.matchBetween(participant, dealer)을 보면 점수 matching을 하는 것이 dealer에게 책임이 있고, 저는 누구의 책임이 아닌 그냥 계산 에만 집중하는 듯해 보입니다. 많이 쓰는 Math.sqrt 와 같이요.

그래서 만약 책임에 대해서 말씀하신 것 같아서 생각을 해봤습니다.
보시기에 노션이 좋을 것 같아서 링크 첨부해보겠습니다 !!
노션 링크

Copy link

Choose a reason for hiding this comment

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

잘 고민해주셨네요 💯

  1. static 메서드가 객체지향에서 좋은 방법일까? 하는 이 스레드를 읽어보시면 좋을 것 같아요. 당연히 static 메서드를 사용해야할 때도 있겠지만 수정한 코드처럼 객체로 나타낼 수도 있겠죠.
  2. 개인적으로 블랙잭을 구현할 때, 결과는 "딜러"와 "참가자"의 1:1 대결로 구해진다는 걸 파악하는 게 중요하다고 생각했습니다. dealer.matchWith(participant)와 같은 인터페이스가 딜러에게 있다면(딜러의 책임으로 나타낸다면) 이를 좀 더 명확하게 표현할 수 있지 않을까 생각했었네요.

Comment on lines 107 to 126
// 10 + [11] = 21
final CardArea cardArea1 = new CardArea(new Card(CardShape.CLOVER, TEN), new Card(CardShape.CLOVER, ACE));

// 10 + 10 + [1] = 21
final CardArea cardArea2 = new CardArea(new Card(CardShape.CLOVER, JACK), new Card(CardShape.CLOVER, TEN));
cardArea2.addCard(new Card(CardShape.CLOVER, ACE));

// [11] + 9 + [1] = 21
final CardArea cardArea3 = new CardArea(new Card(CardShape.CLOVER, ACE), new Card(CardShape.CLOVER, NINE));
cardArea3.addCard(new Card(CardShape.CLOVER, ACE));

// [11] + 6 + 3 = 20
final CardArea cardArea4 = new CardArea(new Card(CardShape.CLOVER, SIX), new Card(CardShape.CLOVER, THREE));
cardArea4.addCard(new Card(CardShape.CLOVER, ACE));

// [11] + 10 = 21
final CardArea cardArea5 = new CardArea(new Card(CardShape.CLOVER, ACE), new Card(CardShape.CLOVER, TEN));

// 10 + [1] + 7 = 18
final CardArea cardArea6 = new CardArea(new Card(CardShape.CLOVER, TEN), new Card(CardShape.CLOVER, ACE));
Copy link

Choose a reason for hiding this comment

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

꼼꼼한 케이스와 적절한 주석이네요 💯

// when & then
assertEquals(cardValue.value(), value);
}
}
Copy link

Choose a reason for hiding this comment

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

POSIX new line에 대해서 알아볼까요?

인텔리제이에서는 옵션으로 항상 개행을 붙여줄 수 있습니다 :)

Copy link
Author

Choose a reason for hiding this comment

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

전 미션에서 리뷰를 받고 설정을 했는데도,, 가끔 코드 수정하면서 삭제를 해버렸나봅니다,,ㅠㅠ 바로 수정하겠습니다

세심한 리뷰 감사합니다 !!

- Name은 하위 클래스에서 사용하지 않기 때문에 접근제어자 protected -> private 변경
- OutputView에서
"딜러의 cardArea에서 첫 번재 카드를 뽑는다"보다는 "딜러는 첫번째 카드를 보여준다" 가 자연스러워 보임
- 도메인 룰은 Participant 1장 -> Dealer 1장 -> Participant 1 -> Dealer 1장 임
- 현재 코드는 Participant 2장 한번에 주고 -> Dealer 2장 한번에 주기 때문에 도메인 룰 위반
- 카드 나눠주는 행위도 도메인 룰이기 때문에 CardTable에 위임
- CardArea 생성자를 삭제하고 나서의 코드 수정
@java-saeng
Copy link
Author

안녕하세요 범블비 !!

범블비 리뷰 덕분에 많은 생각을 하게 됐습니다,,

처음 시작할 때 두 장을 뽑는다는 건 도메인 룰인 것 같아요. 모델 내부로 로직을 이동시키는 건 어떨까요?

해당 리뷰에서부터 생각을 시작하게 됐는데, 도메인 룰 자체가 참여자 각 1장 - 딜러 1장 - 참여자 각 1장 - 딜러 1장이라는 것을 알게 되서 관련 코드를 모두 수정했습니다.
그리고 어느 객체에 책임을 할당하는게 적절할까에 대해서 생각을 하고, 그에 대한 트레이드 오프도 저 나름 대로 생각을 해봤습니다.

github comments보다는 노션이 보기 편하실 것 같아서 제가 해당 리뷰를 받고 정리한 제 생각을 노션에 적어보았습니다!!

감사합니다 :)

노션 링크

@java-saeng java-saeng requested a review from ddaaac March 6, 2023 09:46
Copy link

@ddaaac ddaaac left a comment

Choose a reason for hiding this comment

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

안녕하세요 우르 👋

리뷰 반영 너무 잘 해주셨습니다.

다음 단계 진행해주세요!

Comment on lines +32 to +36
deal(cardTable, players);

printStateAfterDeal(participants, dealer);
hittingPlayer(cardTable, participants, dealer);
printStateAfterHit(participants, dealer);
Copy link

Choose a reason for hiding this comment

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

Suggested change
deal(cardTable, players);
printStateAfterDeal(participants, dealer);
hittingPlayer(cardTable, participants, dealer);
printStateAfterHit(participants, dealer);
deal(cardTable, players);
printStateAfterDeal(participants, dealer);
hittingPlayer(cardTable, participants, dealer);
printStateAfterHit(participants, dealer);

요렇게 개행을 잡으면 의미 단위로 묶을 수 있을 것 같아요.

Comment on lines +39 to +47
public Score plusTenIfNotBurst() {
final Score plusScore = this.plus(REMAIN_SCORE_ACE);

if (plusScore.isLessEqualThan(UPPER_LIMIT_SCORE)) {
return this.plus(REMAIN_SCORE_ACE);
}

return this;
}
Copy link

Choose a reason for hiding this comment

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

값 객체 Score 구성 잘 해주셨네요 👍

soft vs hard라는 블랙잭 용어를 사용해도 좋을 것 같네요.


개인적으로 값 객체 내부에서는 원시타입을 사용하는 편이 좀 더 코드를 읽기 편하다는 생각이 들긴 하네요.


@Override
public boolean canHit() {
return cardArea.calculate() <= DEALER_LIMIT_SCORE;
Copy link

Choose a reason for hiding this comment

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

image

에서 "딜러"만 "딜러카드"로 표현한다면

이 부분도 추상 메서드로 뺼 수 있지 않을까 하는 생각이 들긴하네요.
추상 메서드로 표현되는만큼 딜러와 참가자 간에 이런 차이가 있구나 하고 명확히 표현할 수도 있겠구요.

Comment on lines 11 to 25
public static ParticipantResult matchBetween(final Participant participant, final Dealer dealer) {
if (participant.isBurst()) {
return ParticipantResult.LOSER;
}
if (dealer.isBurst()) {
return ParticipantResult.WINNER;
}
if (participant.score() > dealer.score()) {
return ParticipantResult.WINNER;
}
if (participant.score() == dealer.score()) {
return ParticipantResult.DRAWER;
}
return ParticipantResult.LOSER;
}
Copy link

Choose a reason for hiding this comment

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

잘 고민해주셨네요 💯

  1. static 메서드가 객체지향에서 좋은 방법일까? 하는 이 스레드를 읽어보시면 좋을 것 같아요. 당연히 static 메서드를 사용해야할 때도 있겠지만 수정한 코드처럼 객체로 나타낼 수도 있겠죠.
  2. 개인적으로 블랙잭을 구현할 때, 결과는 "딜러"와 "참가자"의 1:1 대결로 구해진다는 걸 파악하는 게 중요하다고 생각했습니다. dealer.matchWith(participant)와 같은 인터페이스가 딜러에게 있다면(딜러의 책임으로 나타낸다면) 이를 좀 더 명확하게 표현할 수 있지 않을까 생각했었네요.

@ddaaac ddaaac merged commit 10779cd into woowacourse:java-saeng 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