[1단계 - 블랙잭] 밍트(김명지) 미션 제출합니다.

[Starlight258]

안녕하세요, 밍트입니다.
반갑습니다! 😊

체크 리스트

• 미션의 필수 요구사항을 모두 구현했나요?
• Gradle test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?
• 애플리케이션이 정상적으로 실행되나요?
• 프롤로그에 셀프 체크를 작성했나요?
  • 작성한 프롤로그 글

객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?

1~5점 중에서 선택해주세요.

• 1 (전혀 충족하지 못함)
• 2
• 3 (보통)
• 4
• 5 (완벽하게 충족)

선택한 점수의 이유를 적어주세요.

"3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다." 라는 요구사항을 지키기 위해서 파라미터가 많아질 때마다 객체를 쪼갰습니다.
그렇게 되니 객체의 깊이가 깊어지는 문제가 있었습니다. (Participants(Dealer, Players) ->Players(List) -> Player)

좀 더 적합한 방법이 있지 않을까 고민이 되어 4점 선택했습니다.

어떤 부분에 집중하여 리뷰해야 할까요?

설계가 잘 되었는지, 개선할 부분이 있는지 리뷰해주시면 감사드리겠습니다.

소중한 피드백 감사합니다 🙇‍♀️

[Starlight258]

@Rok93 리뷰 감사합니다! 덕분에 많이 배웠어요~ 🙇‍♀️

피드백 반영하여 수정하였습니다!
궁금한 점 간단히 남겼는데, 답변 주시면 감사드리겠습니다 😃

추가 피드백 있으면 주시면 감사드리겠습니다!

[Rok93]

밍트 피드백 반영 잘해주셨어요. 👍👍
1단계는 이정도면 충분한 것 같아 이만 머지하도록하겠습니다. 😃
추가적으로 코멘트 남겨두었는데, 2단계 진행하시면서 같이 확인 부탁드릴게요. 😉

[Rok93]

이 부분은 고민 포인트라 주석으로 남겨두신걸까요? (혹시 사용하지 않는 코드라면 과감히 제거해주세요. 😃)

합성 사용시 메서드 중복 문제
Participants는 Dealer와 Players를 가지고 있습니다.
Dealer와 Players는 Gamer의 공통 메서드(spreadOneCard)를 호출합니다.
그런데 해당 행위를 하는 객체가 Dealer인지 Players인지, 나아가서 Player라면 Players에서 어떤 Player인지 알아야합니다. 따라서 위와 같이 Dealer와 Player마다 동일한 행위에 대해 메서드를 생성했습니다.
이렇게 합성 사용시에 매번 메서드가 객체별로 중복되는 문제가 발생하였는데, 좀 더 개선할 수 있는 방법이 있을까요?

이게 왜 합성의 문제라고하시는지는 잘 모르겠어요... 🤔
합성의 문제라고 생각하신다는 것은 다른 방법으로는 상속을 고민해볼 수 있는데 해당 문제를 상속으로 풀 수 있는 상황은 아닌 것으로 보이거든요. 🙄

저는 굳이 spreadOneCard 라는 메서드를 따로 만들필요가 없다고 생각해요.
각 하위 구현체가 각각 receiveCards 를 호출해주면 되지 않을까요?

AS IS

    public void spreadOneCardToPlayer(final int index, final Card card) {
        final Player player = players.getPlayer(index);
        spreadOneCard(player, card);
    }

    public void spreadOneCardToDealer(final Card card) {
        spreadOneCard(dealer, card);
    }

TO BE

    public void spreadOneCardToPlayer(final int index, final Card card) {
        final Player player = players.getPlayer(index);
        player.receiveCards(new Hand(List.of(card)));.
    }

    public void spreadOneCardToDealer(final Card card) {
        dealer.receiveCards(new Hand(List.of(card)));.
    }

[Starlight258]

Player와 Dealer가 공통적으로 부모 클래스의 메서드를 호출했기 때문에 spreadOneCard라는 메서드를 만들었습니다.
그런데 생각해보면 spreadOneCard 메서드는 하는 역할은 없고 단순히 메서드 호출만 줄이고 있네요. 복잡해지기도 하고요.
각각 호출하는 것이 더 나을 것 같아요!

[Rok93]

먼저 정적 팩토리 메서드 네이밍에 대해서도 고민해보시면 좋을 것 같아요. 😃

꼭 "저 네이밍을 지켜야해!" 는 아니지만, 어느정도 네이밍 규약의 틀에 맞춰서 네이밍하는게 좋다고 생각해요. 😃

[Rok93]

두 가지 방향이 있을 것 같아요. 😃
주생성자를 public 으로 열어두거나 혹은 fixture 함수가 주생성자를 대체할 수 있도록 만들거나 둘 중 하나인 것 같아요. 🤔

저는 보통 주 생성자는 굳이 private으로 막지 않는 편이라 이런 고민 포인트는 잘 안가지는 편이긴한데 고민해보면 위의 두 가지 선택지가 생각나는 것 같아요. 😉

[Rok93]

오... 클린코드를 읽어보셨군요? 저도 일부분만 읽어봣지만, 지금 딱 얘기하려던게 수직거리의 이야기가 맞습니다 😉👍

[Rok93]

일단 지금의 맥락과 상관없이 enum에 도메인 로직이 들어가는 것에는 좋다고 생각해요. 👍
enum 도 저는 객체라고 생각하기 때문에 도메인 로직을 가지는 것이 당연하다고 생각해요. 😉

혹시 enum의 승패 결정 로직까지 Dealer 도메인으로 옮기는 것까지 권장하시나요? 현재는 enum에 도메인 로직이 들어가도 된다고 생각해서 두었는데, 로키의 생각이 궁금해요!

음... 지금의 컨텍스트에서는 저는 Dealer에게 승패를 결정하는 역할을 위임하는게 좋지 않을까 싶어요.
먼저 ResultStatus가 ResultStatus를 판단하게되면 결과가 누구를 기준으로 판단되는건지 조금 모호해진다고 생각해요.
score는 dealer의 것인지 player의 것인지 구분할 수가 없을 것 같아요. 🤔

물론 ResultStatus.calculateResultStatus 메서드 파라미터로 Dealer와 Player를 넘겨주는 방법도 있지만, 사실 이 부분도 결과가 누구를 기준으로 나오는지가 자연스럽지 않다고 생각해요.

반대로 dealer.calculateResultStatus(player) 와 같은 메서드 시그니처를 가지게된다면, 딜러가 플레이어에 대해서 ResultStatus를 결정해준다고 자연스럽게 읽히지 않을까 생각이 드네요. 순서에 대한 실수도 방지할 수도 있구요. 😃

[Rok93]

비기는 경우를 push라고 하는군요???
오호... 해당 용어는 저도 잘 몰랐는데 덕분에 알아갑니다. 😉👍

[Rok93]

인터페이스는 "행동(behavior)"을 정의하는 역할 이기 때문에 말씀하신 케이스도 인터페이스의 취지와는 맞지 않다고 생각해요.

차라리 이런 경우에는 별도의 상수를 별도 클래스에서 관리해서 하위 구현체에서 공통으로 쓰는게 낫지 않을까 싶어요. 🤔

[Rok93]

아무래도 지금의 요구사항에서는 말씀하신 포인트가 모든것을 완벽하게 만족하며 진행하기가 어려운 것 같아요. 🥲

일단 1,2,3 번의 방식을 말씀해주셨는데, 저라도 3번을 선택할 것 같아요. 😃
2번은 동일한 이유이고, 1번은 말씀하신 이유도있지만, 불필요한 상태가 추가된다고 생각해서에요.

저 역시 3번의 방식을 택할 것 같지만, 최대한 도메인 로직을 컨트롤러에서 덜 수행하도록 변경해볼 것 같아요. (이 방식은 예시이고 꼭 이런 방식을 취하지 않으셔도 됩니다!)

(depth는 무시하고 대략적인 코드로 작성할게요 🙄)

private void spreadPlayersExtraCards(final BlackjackGame blackjackGame) {
    while(blackjackGame.canGetMoreCard()) {
        Player player = blackjackGame.drawablePlayer(); // 현재 카드를 뽑는 턴인 플레이어 
        if(isMoreCard(gamer)) {
            player.receiveCard();
        }
    }
}

생각해보니 이 방식도 고민 포인트가 생기는 것 같긴하네요. 🤔
이렇게 더 개선하기가 어렵다면 지금의 방식도 괜찮다고 생각해요. 😃
(이 부분을 모두 만족시키기는 조금 어렵다고 생각해서 최대한 고민해보시고 어렵다면 지금처럼 진행해주셔도 될 것 같아요. 😉)

[Starlight258]

@Rok93 리뷰 감사합니다! 피드백 꼼꼼하게 해주셔서 정말 많이 배웠습니다.
피드백 주신 부분은 step2에 함께 반영하여 pr 올리겠습니다.
감사합니다 🙇‍♀️