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단계 - 블랙잭] 연로그(권시연) 미션 제출합니다. #220

Merged
merged 54 commits into from
Mar 15, 2022

Conversation

yeon-06
Copy link

@yeon-06 yeon-06 commented Mar 10, 2022

안녕하세요 로운!!
요번 주도 벌써 다 지나갔네요 고생 많으셨습니다!
이번 과제 잘 부탁드려요😊

이번 미션을 진행하면서 고민했던 점에 대해도 추가로 남겨둡니다!

공통으로 사용하는 테스트 메소드

편의를 위해 Cards를 생성해주는 getCards() 메소드를 생성했습니다.
요 메소드를 테스트 코드 여기저기서 사용해서 사용하는 곳마다 생성해주었습니다
원래는 public으로 만들었는데 혹시나 메소드 내부 로직이 변경되면 다른 테스트 코드에 영향을 줄 수 있다고 생각했습니다
서로 다른 패키지에 존재하는 경우 테스트 메소드를 public화 해서 사용하는건 어떻게 생각하시나요?

Abstract Class vs Interface

처음에는 수정의 자유도가 비교적 높은 Interface를 택했었는데
공통된 상태와 공통된 메소드 내용이 많아 Abstract Class로 변경했습니다.
java에서 어떤 식으로 활용하나 찾아보기 위해 Map, AbstractMap, HashMap으로 찾아보았는데요
HashMap은 Map을 implements하는 동시에 AbstractMap을 extends 하고 있었고 왜 그런가에 대해서 찾아보았습니다
그러다 stackoverflow 글을 보게 되었는데 '내부 구현 로직을 세세히 볼 필요 없이 interface를 통해 메소드 목록을 즉시 확인할 수 있어 명시적이다.' 라고 이해했습니다.
하지만 전 오버 프로그래밍이 아닌가 라는 의문이 드네요😂
명시적이다라는 의미를 잘 이해하지 못한 것 같은데 로운은 이 문제에 대해 어떻게 생각하시나요?

렌트카 미션에서 interface 사용

블랙잭 미션은 아니지만 렌트카에서 interface 사용하라는 조건이 있었습니다.
interface를 사용해서 구현해보았으나 abstract class보다 이점이라고 느껴지는 점이 없어서 롤백했습니다.
혹시 어떤 맥락에서 interface를 사용하려고 한 것인지 힌트를 주실 수 있을까요?ㅜ.ㅜ

dto의 사용

이전 미션에서는 생성자, getter에서 불변 처리를 해줘도 충분했는데 이번에는 구조가 복잡해지면서 불변 처리를 하는게 헷갈리더라고요
불변 처리 역할을 위임하기 위해 dto를 생성하는 것은 어떻게 생각하시나요?

private void playPlayerTurn(BlackJackGame blackJackGame, Participant player) {
Command command = Command.HIT;

while (command != Command.STAY && !player.isFinished()) {

Choose a reason for hiding this comment

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

  1. while 조건을 좀 더 단순하게 작성해봅시다.
  2. enum의 값을 꺼내서 같은 타입의 enum끼리 비교하지 말고 enum 내부에서 처리해보세요.


while (command != Command.STAY && !player.isFinished()) {
command = inputCommand(player);
blackJackGame.drawCardByCommand(player, command);

Choose a reason for hiding this comment

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

Suggested change
blackJackGame.drawCardByCommand(player, command);
blackJackGame.drawCard(player, command);

파라미터 값으로 전달하는데 메서드명에 또 쓸 필요는 없겠네요.

@@ -0,0 +1,34 @@
package domain.card;

public enum Denomination {

Choose a reason for hiding this comment

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

Denomination이 뭔가요?
블랙잭에서 사용하는 용어가 맞나요?

Copy link
Author

Choose a reason for hiding this comment

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

트럼프 카드의 숫자 또는 알파벳을 denomination이라고 표현하기 때문에 이름을 지었습니다!
다시 찾아봤는데 블랙잭에서는 카드의 숫자/알파벳보다는 배팅한 돈(chip)에서 더 자주 사용하는 단어 같네요

@lowoon
Copy link

lowoon commented Mar 11, 2022

질문하셨던 부분 답변을 드리면

서로 다른 패키지에 존재하더라도 같은 객체를 필요로하는 테스트들이 많아요.
그래서 매번 그 객체들을 생성하기보단 한곳에서 생성해서 반환해주는 메서드를 만들고, 그 메서드를 공통으로 쓰는 경우가 많습니다ㅎㅎ
제 개인적으로는 TestUtil이라는 형태로 만들어서 사용하고 있어요~

'명시적이다'라는 것에 대해서는 제 개인적인 생각을 말씀드릴 수밖에 없는데요;; 😅
interface는 명세서(약속)의 역할도 해서 무엇을 제공하는지 알려주기 때문에 명시적이다라고 하는것 같은데요.
이 부분은 abstract class와 interface에 대해 조금 더 알아보시면 더 좋을거 같아요!
알아보시고 저한테도 공유해주세요ㅎㅎ 그때 조금 더 얘기해봐요.

그러면 연로그가 생각하는 abstract class의 이점은 무엇이었는지 알 수 있을까요??
그래야 이야기를 해볼 수 있을거 같아요ㅎㅎ

새로운 객체로 감싸서 반환을 할 수도 있고 불변으로 반환을 할 수도 있었을텐데 getter에서 불변으로 반환한 이유가 무엇인가요??
이 이유가 dto를 사용하는 이유와 일맥상통 한다면 사용해도 좋다고 생각합니다!
dto를 사용하는 이유는 무엇일까요??

Copy link

@lowoon lowoon left a comment

Choose a reason for hiding this comment

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

반가워요 연로그! 리뷰어 로운입니다! 😄
피드백 몇 가지 남겨봤으니 확인 부탁드려요.
궁금한 점이 생기면 언제든지 DM 혹은 댓글남주세요 ^-^

for (int i = 0; i < INIT_CARD_COUNT; i++) {
cards.add(cardDistributor.distribute());
}
return new Cards(cards);
Copy link

Choose a reason for hiding this comment

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

기본로직이 참여자가 카드를 가져가는데 이곳에서만 카드를 2개 받아서 참여자에게 넘겨주는 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

참여자가 생성한 후에 drawCard()를 호출하기보다는 참여자의 생성자 호출과 동시에 Cards를 초기화시키고 싶어서 사용했습니다!

로운의 의견을 듣고 보니 어떤 방법이 더 좋을지에 대한 고민이 되네요🤔
생성자 이후에 값 초기화를 위해 호출해야하는 메소드가 존재한다면 다른 개발자가 개발할 때 헷갈려하진 않을까요?

Copy link

Choose a reason for hiding this comment

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

제가 생각했던 방향을 말씀드리면,
참여자를 생성하는 것은 이름으로도 생성을 할 수 있고, 한장씩 draw하는 메서드도 있다면 참여자를 먼저 이름으로 생성하고,
drawInitialCards에서 participants.drawCard를 2번 호출하면 되지 않을까 생각을 했어요.
실제 흐름으로는 플레이어를 먼저 만들고 blackJack을 시작할 수도 있지 않을까요??
participants의 생성이 cards에 종속되는 듯한 느낌이 들어서 말씀드려 봤습니다~

Copy link
Author

Choose a reason for hiding this comment

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

  1. BlackjackGame 생성자 - Name을 이용해 Player와 Dealer 생성
  2. BlackjackGame의 drawCard() 메서드 2번 호출

위 순서를 말씀하신게 맞으실까요?

실제 흐름으로 생각해보면 일단 참가자가 모집된 후, 그 후에 카드가 배분되니 로운의 말에 설득이 되네요
다만 drawCard를 2번 호출하는 부분에 대해서는 이견이 있는데요
저는 카드를 어떻게 초기화할지에 대한 로직(2장을 뽑는다)은 BlackjackGame의 책임이라고 생각했기 때문에 따로 메소드를 생성했습니다!
이 부분에 대해서는 어떻게 생각하시나요?

Copy link

Choose a reason for hiding this comment

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

네 2장을 뽑는다는 것은 게임에 대한 규칙이기때문에 BlackjackGame의 책임이 맞는데요.
그렇기 때문에 BlackJackGame에서 게임 흐름에 맞게 drawCard를 2번호출하면 되지 않나요??
따로 메서드를 생성했다는 것이

public void initFirstDraw(Participant participant) {
    participant.drawCard(cardDistributor.distribute());
    participant.drawCard(cardDistributor.distribute());
}

의 의미가 맞을까요??

Copy link
Author

Choose a reason for hiding this comment

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

네 그 의미가 맞습니다!!
ConsoleGame에서 drawCard를 2번 호출하도록 하자고 제안하신걸로 착각했네요 😂

@yeon-06
Copy link
Author

yeon-06 commented Mar 14, 2022

  1. Abstract Class vs Interface
  • Abstract Class: 상속을 이용해 메소드를 재사용하고 기능을 확장하는데 용이
  • Interface: 공통적인 메소드를 묶어 해당 메소드들을 구현하도록 명시적으로 나타내줌

의 정도로 인식하고 있습니다!
본 프로젝트에서는 abstract class가 적합하다고 생각되어 사용했는데
왜 쓰지 말라! 는 식의 강조가 많은지는 이해가 잘 안돼요
쓰는 경우가 그렇게 드물까요 ?_?

제가 공부해본걸 정리한 것은 요 글입니다.

  1. 연료 주입에서 Interface

Abstract Class는 흔히 상위 클래스의 변동이 하위 클래스까지 영향을 끼칠 수 있기 때문에 꺼린다고 알고 있는데요.
K5, Sonata, Avante는 Car에 종속되는 현재 경우에는 오히려 그 영향이 필요하다고 생각 했습니다.
Car가 변동됨으로써 하위 클래스에 이슈가 생긴다면 그건 추상 클래스를 도입했기 때문이 아니라 Car의 변동 내역이 잘못된 거라는 생각이 듭니다.

  1. dto 사용

View에 도메인 모델이 아닌 dto를 전달하면 뷰가 변경되어도 부담 없이 필드를 변경할 수 있고 뷰에서 이용하는 로직을 dto에게 책임을 위임하는 등의 역할을 할 수 있기 때문에 사용합니다!
또한 dto를 통해 전달할 경우 뷰에 데이터를 전달하기 전에 도메인 값이 변경되어도 원래 보내려던 값을 안전하게 보낼 수 있습니다.
이 부분의 연장선으로 dto에서 불변 처리를 해주려고 했는데 지금 다시 생각해보니까 dto를 생성해줄때마다 dto에서 불변 처리 로직을 추가할 바에는 안전하게 도메인에서 처리해주는게 맞다는 생각이 드네요😂

전체적인 수정 사항

  • 패키지 및 클래스 알맞은 자리로 이동
  • 클래스, 메소드, 변수 이름 수정
  • 상태를 꺼내서 비교하지 않고 도메인 내에서 비교하도록 수정
  • Player 목록과 Dealer를 보유한 Participants를 생성
    • Player 목록의 인원 제한
  • 테스트 유틸 분리 (1번 질문과 연관)
  • 카드 상태(블랙잭, 버스트 등)마다 일치 여부를 반환하는 메소드들을 생성하지 않고 enum Status를 반환하는 getStatus() 메소드로 통일

@lowoon
Copy link

lowoon commented Mar 15, 2022

2,3 제 개인적인 의견입니다~
저는 추상클래스를 쓰지 말라는 주의가 아닌데요. 어떤 기능이나 기술도 필요하면 쓰고 정의에 맞게 사용하면 된다고 생각합니다.
추상클래스를 쓰지 말라고 하는 이유들은 제가 생각했을 때 추상클래스를 용도에 맞게 잘 사용하지 않는 사람들이 많기 때문이라고 생각해요.
제가 생각하는 추상클래스는 하위클래스와 정확하게 일치할 때 쓴다고 생각합니다.
추상클래스 A와 이를 상속받는 하위클래스 B는 정확하게 의미와 역할이 일치할 텐데요. 여기서 문제는 B에 점점 메서드나 내용, 역할이 추가될 때 문제가 발생하는 것 같아요. A와 B가 같지 않은데 단순히 중복을 없애는 용도로만 사용하게 될 경우, 복잡성이 높아지고 객체지향에 맞지 않는 결과가 나오게 되는 것 같아요. B의 역할이 커지게 되면 이 때 A와 B가 같은지를 고려해야하고 필요하면 분리하여 추상클래스로 묶지 말아야하는데 이러한 부분들이 안지켜지기 때문에 추상클래스를 사용하지 말라는 말이 나오는 것 같습니다~

제 개인적인 경험을 말씀드리면 추상클래스에 코드가 굉장히 많은 경우가 있었어요.
이때 필요한 부분만 abstract 메서드로 빼서 각각에서 구현을 하게한 것이 있었는데요. 이 부분이 로직의 흐름을 파악하기 힘들게 만드는 부분이 되더라고요.
상속클래스쪽으로 가서 로직 확인했다가 다시 추상클래스로 돌아와서 나머지 로직 확인하고 다시 상속클래스쪽으로 넘어가는 형식으로 파악을 하는데 상속클래스가 많고, 추상클래스의 코드가 큰 경우에는 파악하기 힘들었던 경험이 있습니다~

dto에서 불변처리 로직은 저도 필요가 없다고 생각하고요. 단순히 필요한 데이터만 잘 넘겨주면 된다고 생각합니다!
단 나중에 dto를 쓸 때 생각해야하는 점은 view에서 이용하는 로직을 dto에 위임한다고 했는데요. dto에 로직이 들어가도 될까요?
실제 웹이나 앱에서 어떻게 보여줄지 서버에서 고려를 할까요?? 어떠한 형태로 어떻게 보여줄지는 누구의 역할일까요?
이러한 관점에서 보면 dto에 어떤 정보가 들어가야하는지 판단할 수 있지 않을까요??

Copy link

@lowoon lowoon left a comment

Choose a reason for hiding this comment

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

고생하셨어요. 연로그! 이전 리뷰 내용을 잘 반영해주셨습니다 💯
몇가지 코멘트를 달아봤는데요. 코멘트 반영은 2단계에서 부탁드릴게요.
그리고 고민해보면 좋은 점들도 같이 적었으니 한번 생각해봐주세요~
추가적인 궁금증은 이 pr이 머지되어도 댓글 남겨주셔도 좋구 DM도 좋습니다.

}
}

private MatchResult playResult(Player player, Dealer dealer) {
Copy link

Choose a reason for hiding this comment

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

결과를 도출하다라는 의미이 메서드명으로 하는게 좀 더 명확할 거 같아요.


private static final int ACE_ADDITIONAL_VALUE = 10;
protected static final int BLACKJACK_VALUE = 21;
protected static final int BLACKJACK_COUNT = 2;
Copy link

Choose a reason for hiding this comment

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

접근제어자가 왜 protected일까요??

이곳에서 카드 상태와 점수를 도출하고 BlackJack에 대한 기준도 관리하고 있는데요.
연로그는 Cards의 역할과 책임을 어떻게 정했을까요??
카드의 상태와 게임의 규칙과 관련된 것을 Status에서 관리한다면 status에 있는게 맞지 않을까요??
아니면 결과를 도출하기 위해 필요한 값들이니 GameResult에 있는게 맞을 수도 있지 않을까요??
ace가 있는 경우 10을 더할지 말지를 정하는 것은 어디의 역할일까요??

Copy link
Author

Choose a reason for hiding this comment

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

Status에서 사용하기 때문에 protected로 열었습니다!
Enum에는 상단에 pirvate static final의 선언이 불가능한데 21를 넣는건 매직넘버로 느껴져서 Cards로 빼서 사용했습니다
지금 다시 생각해보니 Enum에서 사용하기 위해 불필요한 상수를 넣다니 좋지 않은 생각인 것 같네요😂

카드의 상태에 관련해서는 Status로 옮기는게 맞을 것 같네요!
enum은 상수로 쓰자에 포인트를 두다보니 자꾸 로직을 밖에서 쓰는 경향이 있는 것 같습니다ㅠ.ㅠ

원래는 ACE가 있는 경우 10을 더할지 말지랑 별개로 '카드들의 합계'를 구하는거니 Cards에 있어도 괜찮다고 생각했습니다
하지만 로운의 말을 듣고보니까 게임의 규칙과 연관이 있는거니 GameResult에 있는게 맞을 것 같아요
다음 단계 때 반영하겠습니다 :D

Copy link

Choose a reason for hiding this comment

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

enum 상단에 private static final의 선언이 불가능하다고 했는데요.

public enum Yn {
    Y, N;

    private static final number = 1;

이렇게 사용하면 되지 않나요??

Copy link
Author

Choose a reason for hiding this comment

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

public enum Command {
    private static final String YES = "y";
    private static final String NO = "n";
    
    HIT(YES),
    STAY(NO);
}
public enum Command {
    HIT(YES),
    STAY(NO);

    private static final String YES = "y";
    private static final String NO = "n";
}

이런 식의 사용이 불가능하다는 의미였습니다!

}

public void drawCard(Participant participant, Card card) {
if (participant.getClass() == Player.class) {
Copy link

Choose a reason for hiding this comment

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

class를 확인하는 것은 isinstanceof를 사용하는 것과 의미가 같지 않을까요??

@lowoon lowoon merged commit a85027d into woowacourse:yeon-06 Mar 15, 2022
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.

4 participants