[🚀 사이클2 - 미션 (블랙잭 베팅)] 이프 미션 제출합니다.

[jihwankim128]

서론

안녕하세요 제나! 사이클 1에 이어 사이클 2 진행 완료했습니다.
사이클 2를 진행하면서 코드에 정말 많은 변화가 발생했네요 🤣

우선 리뷰 요구 사항을 작성하기 전, 제가 의도했던 바를 간략하게나마 전달하려고 합니다!
사이클 2에서 베팅 기능이 추가되면서 클라이언트가 미베팅 블랙잭 원할 수도 있다는 전제를 두고 리팩토링을 진행했습니다.

여기서 추가된 기능은 블랙잭 확인 기능 추가, 베팅 기능 추가 정도이고, 그 외에는 모두 리팩토링을 진행했습니다!
제가 원한 것은 GameMode(BlackJackApplication 에서 수정)만 변경하면 미베팅 기능으로 전환되는 것입니다.
현재 희망사항은 충족이 됐으나 구조 상 만족하지 못하는 부분이 많이 느껴져서 피드백을 받고 싶습니다 🔥🔥
(시간이 부족해서 더 개선하지 못하고, 우선은 PR을 남깁니다... Step1에서 밀린 것 때문에 Step2에 차질이 생겼네요 ㅠ)

그리고 사이클 2인 만큼 냉정하게 피드백해주시면 제 성장에 정말 큰 도움이 될 것 같아요!

체크 리스트

• 미션의 필수 요구사항을 모두 구현했나요?
• Gradle test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?
• 애플리케이션이 정상적으로 실행되나요?

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

1. Controller 분리 구조에 대한 피드백

Betting, NonBetting은 입력받는 형식과 출력하는 형식부터 달랐기 때문에 각 각 Reader와 ResultReporter로 구분 했습니다.
BettingPlayerReader에서는 BettingPlayer를 생성하도록 했고, NonBettingPlayerReader에서는 NonBettingPlayer를 생성하도록 했습니다.

이렇게 설계한 이유는 이미 구현된 2가지 모드에서 모드 변경이 쉽도록, GameMode에서 toBettingMode(), toNonBettingMode()를 통해 간단히 게임을 변경하기 위해 설계했습니다.

    public static GameMode toBettingMode() {
        return new GameMode(new BettingPlayerReader(), new BettingResultReporter());
    }

    public static GameMode toNonBettingMode() {
        return new GameMode(new NonBettingPlayerReader(), new NonBettingResultReporter());
    }

2. ISP 원칙 미준수에서 발생한 궁금증

미베팅, 베팅에 대한 플레이어를 구분하려다 보니 NonBettingPlayer, BettingPlayer로 구분하게 됐습니다.
그리고 Player라는 interface로 추상화해서 관리했는데, 이러니 SOLID 중 ISP에서 벗어난 것 같습니다.

public interface Player {

    ...
    default Profit calculateProfit(ResultStatus resultStatus) {
        return ZERO;
    }
}

NonBettingPlayer에서는 calculateProfit() 메서드가 필요없는데, PlayerResult에서 공통적으로 처리하려다보니 default method를 생성하게 됐습니다.

이것을 해결하기 위해 어떻게 해야 될까?

오늘 일정을 마친 후 지금까지, 이 질문에 계속해서 생각해봤는데 제너릭 혹은 제너릭 확장 Players<T>, Players<T extends Player>를 활용해 볼 수 있을 것 같았습니다. 다만 Controller에서부터 Players를 사용하니까 Controller 또한 Generic을 의존하게 되는데, 이건 구조적 문제인지... 혹은 더 나은 방법이 있는지 굉장히 궁금해졌습니다.

3. Enum에서 출력 연관 정보 제거

기존 Enum에서는 출력에 대한 정보를 들고 있었습니다.

public enum CardShape {
    DIAMOND("다이아몬드"),
    HEART("하트"),
    CLOVER("클로버"),
    SPADE("스페이드");

    private final String shape;
...
}

여기서 저는 model 패키지에서는 view에 대한 의존성이 없어야 MVC에 맞는 설계라고 생각이 들었습니다.
그래서 shape 변수를 없애고, View에서 직접 Enum에 대한 출력 결과를 매핑하도록 했는데요...

public class EnumMapper {
    public static final Map<CardShape, String> CARD_SHAPE_MAPPER = Map.of(
            CardShape.HEART, "하트",
            CardShape.SPADE, "스페이드",
            CardShape.CLOVER, "클로버",
            CardShape.DIAMOND, "다이아몬드"
    );
}

최종적으로 View는 같은 계층에 있는 EnumMapper를 통해 정보를 출력하도록 수정했습니다.

그런데 이런 방법론, 모델링 개념들을 100% 적용하는 것이 좋은가?

결과를 마주하니까 기존 Enum에서 간단하게 처리할 수 있는 것을, View에서 클래스 하나를 더 만들면서 '굳이?' 라는 생각도 들더라구요... ㅋㅋㅋ 그래도 제나는 어떻게 생각하시는지 궁금해서 한 번 적용해봤습니다...

여쭙고 싶은 내용을 마지막에 작성해버렸는데, 제나의 경험을 토대로 Model에서 View에 대한 의존이 없는 구조가 그래도 좋다 vs Model에 View에 대한 최소한의 의존은 오히려 좋을 수 있다에 대해 어떻게 생각하시는지 알 수 있을까요?

4. 그 외

그 외 전체적으로나 자유로운 피드백을 주시면 정말 감사하겠습니다..!

[yenawee]

현재 타입 계층 구조를 보면:

Participant (abstract class)
├── Dealer
├── BettingPlayer implements Player
└── NonBettingPlayer implements Player

Player (interface)
├── BettingPlayer
└── NonBettingPlayer

Player , Participant 간 중복 오버로드가 발생하는데요.

drawOneCard(), printCardByPlayerWithScore() 이 두 메서드는 완전히 동일한 코드입니다.
calculateProfit() 하나를 제외하면 Player 인터페이스는 Participant를 그대로 복사한 것으로 보여요.

오버로드된 메서드가 동일한 동작을 하고 있다면, 타입 계층 설계를 다시 고려해볼 필요성이 보여요.

[jihwankim128]

리뷰 감사합니다! 관련된 작업을 수정했습니다!

해당 문제를 고민하면서 구조적인 개선이 들어갔습니다! 그러니 자연스럽게 ISP 문제도 해결되더라구요!

Participant (abstract class)
├── Dealer
└── Player (class)
    └── BettingPlayer (extends Player)

PlayerResult -> BettingCalculator (베팅 관련 로직 분리)

ResultReporter (interface)
├── BettingResultReporter    ← BettingCalculator 사용
└── NonBettingResultReporter

GameMode
├── PlayerReader (interface)
│   ├── BettingPlayerReader
│   └── NonBettingPlayerReader
└── ResultReporter (interface)
    ├── BettingResultReporter
    └── NonBettingResultReporter

기존에는 미베팅/베팅 플레이어 간 공통적으로 처리하기 위해 Interface를 분리했지만, 개선 후 abstract로도 충분했다는게 느껴지네요 😅 그리고 베팅 기능이 추가되면서 기존 PlayerResult에서 베팅 관련 로직을 추가하려고 했는데... 추가된 기능으로 인해 새로운 책임과 관심사가 발생한 것으로 아예 새 클래스(BettingCalculator)를 만드는게 훨씬 깔끔해지는 것 같아요 !

우여곡절 끝에 개선이 됐고 아직도 부족한게 많겠지만 이전처럼 ISP를 위반하던 문제는 해결완료 됐습니다! 감사합니다 👍 👍

[yenawee]

안녕하세요 이프!
2단계 잘 구현해주셨네요 👏 해주신 질문들도 다 깊은 고민속에서 나온 것이라는게 느껴졌어요.
코멘트 확인부탁드려요~!

1. README 에 추가된 기능들 정리해주셨으면 좋았을 거 같아요! 이번 PR 에 추가된 기능들이 많기도 하고, 특히 NonBetting 모드가 추가되었는데 코드 리뷰 코멘트나 코드만 읽고는 이 기능이 지금 작동하는지 안하는지 파악하는데 시간이 좀 걸려서 리드미에 정리되어 있었다면 좀 더 용이했을 거 같아요.

2. PlayerReader와 ResultReporter 두 전략을 묶어서 모드별로 조합하는 방식은 OCP 를 잘 따른다고 생각됩니다. 새로운 모드가 추가되어도 기존 코드를 변경할 필요가 없으니까요! 하지만 현재 GameMode 에는 View Logic, Domain Logic 이 섞여있다고 봐요. '어떤 Reader, report'를 사용할 것이냐'만을 정하는 것으로 GameMode 책임의 경계를 좁혀보는 것은 어떨까요?

3. NonBettingPlayer에게 calculateProfit()이 필요했던 이유는 PlayerResult가 두 모드를 같은 타입으로 다루려 하기 때문입니다.
   이프 의견대로 제너릭을 사용하면 문제를 해결할 수 있지만 컨트롤러, GameMode 등이 제네릭에 의존하게 되서 복잡성이 증가할 수 있는 단점이 있죠. 아니면 Player 인터페이스 calculateProfit 를 제거하고 모드별 Result 타입을 분리할 수도 있지만 코드 양이 많아지죠. 어떤 방법을 택하던지 트레이드 오프는 있다고 생각해요.

4. Model이 View를 몰라야한다는 원칙은 유효하고 "다이아몬드" 와 같은 값이 출력과 연관되 있는 값이긴 하지만 현재 구조에서 enumMapper 를 만든다면 저도 이프와 동일하게 '굳이?' 싶을 거 같아요. 혹시 다국어 지원이나 여러 방향으로 출력이 지원되야 한다면 enumMapper 가 필요하겠지만 현재 "다이아몬드" 값은 굳이 출력에만 국한된 정보라기 보다는 추가 도메인 정보로 봐도 무방할 거 같아요.

[jihwankim128]

@yenawee

1. README 에 추가된 기능들 정리해주셨으면 좋았을 거 같아요! ...

아이고... ㅠㅠ 스텝 2 기능 구현에 대한 README 업데이트가 전혀 없었네요..! 다음부터는 이 부분도 집중해보겠습니다. 감사합니다 😊

2. PlayerReader와 ResultReporter 두 전략을 묶어서 모드별로...

#1104 (comment) 해당 코멘트에서 답변드린 내용이네요!! 책임의 경계를 좁혀보는 것.. 오랜 시간 고민했는데, 핵심 키워드가 이 코멘트에 있었군요! 감사합니다 😊

4. Model이 View를 몰라야한다는 원칙은 유효하고 ...

추가 도메인 정보!! 이 경계를 구분하기가 정말 어려웠던 것 같아요. 제나의 코멘트 덕분에 다국어 지원 혹은 여러 방향 출력과 같이 다양성이 있지 않은 이상 추가 도메인 정보로 봐도 된다는 기준이 생겼어요. 감사합니다 😄😄

[yenawee]

instanceof 사용 지양해볼수 있을까요?

[yenawee]

안녕하세요 이프!
Player 관련 계층이 많이 깔끔해졌어요! 👏

그런데 하나 꼭 짚고 넘어가고 싶은 부분이 있어서 코멘트 남겨요! 확인 부탁드립니다

[jihwankim128]

@yenawee
제나, instanceof를 지양해야 하는 이유에 대해 먼저 알아봤습니다.

instanceof를 사용하는 경우는 상속 및 구현 클래스에 대해 구체적인 정보가 필요할 때 사용하게 됩니다.
그런데 abstract로 추상화가 잘 된 경우 instanceof를 사용할 필요가 없다는 것을 알게 됐습니다.

그런데 지금 instanceof를 사용했다는 것은 추상화가 제대로 되지 않았다는 신호로 받아들여졌습니다.
하지만 구조상 자식 클래스에서 사용하는 1개의 기능을 추상화하자니 ISP나 LSP를 위반하게 되고...
그래서 instanceof를 활용하는 방법밖에 떠오르지 않더군요...

어떻게 해결해야 되나 계속해서 고민했는데, PR에 적혀있던 2번 질문처럼 제네릭을 활용하는게 정답이라고 느껴졌습니다.
그래서 제네릭을 통해 추가 개선을 하면서 '복잡도가 올라간게 아닌가..?'라고 생각이 들었지만, 오히려 구현을 마치고나니 타입 안정성이라는 장점과 GameMode에서 고민하던 문제도 해결됐습니다.

사실 GameMode에서 모드에 따라 다른 구현체로 갈아끼울 수 있다는 걱정이 있었어요..

public class GameMode<T extends Player> {

    private final PlayerReader<T> playerReader;
    private final ResultReporter<T> resultReporter;

    private GameMode(PlayerReader<T> playerReader, ResultReporter<T> resultReporter) {
        this.playerReader = playerReader;
        this.resultReporter = resultReporter;
    }

    public static GameMode<BettingPlayer> toBettingMode() {
        return new GameMode<>(new BettingPlayerReader(), new BettingResultReporter());
    }

    public static GameMode<Player> toNonBettingMode() {
        return new GameMode<>(new NonBettingPlayerReader(), new NonBettingResultReporter());
    }
    ...
}

현재 코드는 제네릭을 사용한 코드입니다.
제네릭을 사용하지 않을 때는 베팅 모드에서 playerReader에 new NonBettingPlayerReader()을 추가할 수 있기때문에, 정적팩토리 메소드로 명확히 표현하고자 했는데... 제네릭을 활용하니까 관련 구현체가 아니면 컴파일 시점에서 주입조차 안되더라구요 ㅎㅎ..

그래서 이 경험을 통해 자식 클래스에서 기능적으로 확장이 필요한 경우 제네릭을 통해 타입 안전성, 다형성 관리를 하면 된다는 개념이 생겼습니다. 감사합니다 ☺️