[1단계 - 블랙잭 게임 실행] 홍실(홍혁준) 미션 제출합니다.

[hong-sile]

안녕하세요! 제이미! 이번 리뷰이인 홍실이에요!

궁금한 부분에 대한 질문과 제가 코드를 작성한 의도는 코드에 직접 코멘트 남겼어요!

이번 미션에서 시간이 많이 모자라서 미쳐 고려하지 못한 부분들이 많습니다.
오늘 밤내로 리팩터링 할 수 있는 내용은 리팩터링하고 코멘트를 남기도록 하겠습니다! 감사합니다.

[hong-sile]

이 부분은 indent 1을 지킨다는 조건을 맞추기 위해
기존 continuous를 파라미터로 받고 repeat에서 try-catch 처리하였습니다.

(마음에 안들긴 한데 더 좋은 방법이 생각이 안 났어요... 더 나은 방법이 생각나면 바로 리팩터링 할 예정입니다.)

[hong-sile]

수정하였습니다. drawInput 부분만 에러 발생할 수 있으니, 해당 부분만 trycatch로 감싸서 처리하였습니다.

[hong-sile]

현재 모든 도메인 객체에 메시지를 보내는 방식으로 구현하니 BlackJackGame이 하는 역할이 단순 메서드를 호출하는 역할밖에 하지 않습니다.

그래서, BlackJackGame을 지우려했으나 step2 미션에서 추상화하기 좋은 것 같아서,
step2 미션 코드 작성 후 그 때에도 단순 메서드를 호출하는 역할을 하면 삭제하도록 하겠습니다.

[jamie9504]

1. Controller에서 일부 BlackjackGame이 가져야할 역할을 가지고 있는 게 아닐지 고민해보면 좋을 것 같습니다.
2. Users를 파라미터 2개로 묶어놨는데, 이 부분을 묶어야 할 필요가 있었을지 고민해봐도 좋을 것 같습니다.
3. step1 미션을 할 땐, step2 미션을 고려하지 않고 미션을 진행해야 다음 step으로 넘어갈 때 변경점이 많고 거기서 배우는 점들도 많다고 생각하는데, 이 부분은 진행정도와 취향에 따라 선택하면 될 것 같아요.

[hong-sile]

1. Controller에서 일부 BlackjackGame이 가져야할 역할을 가지고 있는 게 아닐지 고민해보면 좋을 것 같습니다.

지금 제가 생각하기에 Controller에서 BlackjackGame으로 넣을 수 있는 기능은 다음과 같습니다.

1. isContinuous -> 모두 blackjackGame의 메서드로 로직이 구성됨. blackjackGame에서 통합해서 반환할 수 있음
2. y 또는 n에 따라 카드를 뽑고 안 뽑고가 결정되는 로직 -> 나중에서야 확인했는데, 마지막에 n을 입력해도 카드를 한 장 더 뽑더라고요... blackjackGame에서 y또는 n이 들어있는 Enum(DrawOrStay)에 따라, 뽑을지 말지를 결정하는 로직을 넣었습니다.

blackjackGame에서 도메인과 관련된 모든 분기를 처리하고, Controller에서는 단순히 입력과 출력을 매핑해줘야한다는 의견에 적극 동의합니다.

하지만 입력된 값(y,n)에 따라 이후에
"현재 플레이어에 대한 y,n을 받을지, 다음 플레이어에 대한 y,n을 받을지"가 결정되기에 controller에서 어느정도 로직이 들어갈 수 밖에 없다고 생각해요.

음... 이건 적용하진 않았지만 고민한 로직인데, InputView의 y,n을 입력받는 메서드랑, 그 출력하는 메서드를 함수형 인터페이스로 넘길까도 생각해봤어요 (inputView.read -> Supplier, outputView.print -> Consumer)

실제로 이런식으로 하면 controller에서는 로직이 엄청 간단해지고, 입출력함수들도 함수형 인터페이스로 포장되기 때문에, blackjackGame에서 모든 로직을 파악할 수 있습니다. 그리고, Controller에서는 단순히 blackjackGame의 메서드만 호출하게 되겠죠

하지만 view의 메서드들이 비록 함수형 인터페이스라곤 하나, domain내부에 코드가 전달되는게 괜찮을지? 몰라서 고민이 되네요.
view가 바뀐다 하더라도, 전혀 domain의 코드를 바꿀 필요도 없긴 하지만(그래서 의존하는게 아니라고 생각했습니다.)
너무 생소한 방법이다보니? 어떠한 사이드이펙트가 발생할지 몰라서 실제로 코드를 짜서 사용은 해봤지만 커밋은 하지 않았어요.
이렇게 함수형 입출력함수를 함수형 인터페이스로 받아, blackjackGame에서 처리해도 문제가 없을까요?

2. Users를 파라미터 2개로 묶어놨는데, 이 부분을 묶어야 할 필요가 있었을지 고민해봐도 좋을 것 같습니다.

이 부분을 아무리 고민해도 답이 안 나오네요. Users를 만들 때, player 이름에 대한 List와 각 user(player+dealer)의 카드 풀을 초기화 되기 위해서 Deck이 또 필요하다고 생각합니다.
혹시 어떤 식으로 쪼갤 수 있을까요..?

3. step1 미션을 할 땐, step2 미션을 고려하지 않고 미션을 진행해야 다음 step으로 넘어갈 때 변경점이 많고 거기서 배우는 점들도 많다고 생각하는데, 이 부분은 진행정도와 취향에 따라 선택하면 될 것 같아요.

저도 이 말에 동의를 해요. 저도 step1을 작성할 때 step2를 고려하지 않고, 코드를 작성합니다. 그래야 실패도 해보고 실패에서 배울게 많다고 생각하거든요.
책에서 아무리 안 좋은 설계라 이야기해도 직접 본인이 설계를 하다 실패를 겪는 것에서 좀 더 많이 배울 수 있다고 생각해요.
추가적으로 step2를 고려하지 않아야, 일반적으로 코드가 변경되는 상황(실무에서 갑자기 기능이 추가되는 경우)을 겪을 수 있다고 생각하거든요. 만약에 step2를 보고 코드를 작성한다면, 그 step2에 최적화된 코드로 작성하게 될 것 같아서
저는 step1을 짤 땐 step2를 고려하지 않고 짜긴 합니다.

[jamie9504]

1. Controller에서 일부 BlackjackGame이 가져야할 역할을 가지고 있는 게 아닐지 고민해보면 좋을 것 같습니다.

지금 제가 생각하기에 Controller에서 BlackjackGame으로 넣을 수 있는 기능은 다음과 같습니다.
isContinuous -> 모두 blackjackGame의 메서드로 로직이 구성됨. blackjackGame에서 통합해서 반환할 수 있음
y 또는 n에 따라 카드를 뽑고 안 뽑고가 결정되는 로직 -> 나중에서야 확인했는데, 마지막에 n을 입력해도 카드를 한 장 더 뽑더라고요... blackjackGame에서 y또는 n이 들어있는 Enum(DrawOrStay)에 따라, 뽑을지 말지를 결정하는 로직을 넣었습니다.
blackjackGame에서 도메인과 관련된 모든 분기를 처리하고, Controller에서는 단순히 입력과 출력을 매핑해줘야한다는 의견에 적극 동의합니다.
하지만 입력된 값(y,n)에 따라 이후에
"현재 플레이어에 대한 y,n을 받을지, 다음 플레이어에 대한 y,n을 받을지"가 결정되기에 controller에서 어느정도 로직이 들어갈 수 밖에 없다고 생각해요.
음... 이건 적용하진 않았지만 고민한 로직인데, InputView의 y,n을 입력받는 메서드랑, 그 출력하는 메서드를 함수형 인터페이스로 넘길까도 생각해봤어요 (inputView.read -> Supplier, outputView.print -> Consumer)
실제로 이런식으로 하면 controller에서는 로직이 엄청 간단해지고, 입출력함수들도 함수형 인터페이스로 포장되기 때문에, blackjackGame에서 모든 로직을 파악할 수 있습니다. 그리고, Controller에서는 단순히 blackjackGame의 메서드만 호출하게 되겠죠
하지만 view의 메서드들이 비록 함수형 인터페이스라곤 하나, domain내부에 코드가 전달되는게 괜찮을지? 몰라서 고민이 되네요.
view가 바뀐다 하더라도, 전혀 domain의 코드를 바꿀 필요도 없긴 하지만(그래서 의존하는게 아니라고 생각했습니다.)
너무 생소한 방법이다보니? 어떠한 사이드이펙트가 발생할지 몰라서 실제로 코드를 짜서 사용은 해봤지만 커밋은 하지 않았어요.
이렇게 함수형 입출력함수를 함수형 인터페이스로 받아, blackjackGame에서 처리해도 문제가 없을까요?

음.. 제가 말했던 의미는 현재 아래와 같이 isDraw인지 isContinuous할 수 있는지를 컨트롤러에서 판단해서 Game에서 play를 하고 있는데, 이렇게 되면 Controller를 확인하지 않으면 Game만 봤을 때는 언제 play를 할 수 있는지라던가, play를 해도 되는지라던가의 정보가 없다고 생각해서 위와 같이 질문을 남겼었는데요. 이 부분을 다시 생각해보시면 좋을 것 같아요 😄

        DrawOrStay drawOrStay = DrawOrStay.DRAW;
        while (drawOrStay.isDraw() && blackJackGame.isContinuous(playerName)) {
            drawOrStay = repeatUntilReadValidateDrawInput(playerName);
            blackJackGame.playPlayer(playerName, drawOrStay);
            final CardGroup playerCardGroup = blackJackGame.getCardGroupBy(playerName);
            outputView.printUserNameAndCardGroup(playerName.getValue(), ViewRenderer.renderCardGroup(playerCardGroup));
        }

2. Users를 파라미터 2개로 묶어놨는데, 이 부분을 묶어야 할 필요가 있었을지 고민해봐도 좋을 것 같습니다.

이 부분을 아무리 고민해도 답이 안 나오네요. Users를 만들 때, player 이름에 대한 List와 각 user(player+dealer)의 카드 풀을 초기화 되기 위해서 Deck이 또 필요하다고 생각합니다.
혹시 어떤 식으로 쪼갤 수 있을까요..?

User가 초기화될 때 Deck이 필요한지 여부에 대해서 고민해보았어요.
게임으로 치자면, 플레이어는 자신의 이름을 대고 게임을 실행하고, 첫 턴에 카드를 2장 받는거니까요
처음부터 Deck을 받아야하는 걸까? 라는 생각을 해봤습니다 😄

3. step1 미션을 할 땐, step2 미션을 고려하지 않고 미션을 진행해야 다음 step으로 넘어갈 때 변경점이 많고 거기서 배우는 점들도 많다고 생각하는데, 이 부분은 진행정도와 취향에 따라 선택하면 될 것 같아요.

저도 이 말에 동의를 해요. 저도 step1을 작성할 때 step2를 고려하지 않고, 코드를 작성합니다. 그래야 실패도 해보고 실패에서 배울게 많다고 생각하거든요.
책에서 아무리 안 좋은 설계라 이야기해도 직접 본인이 설계를 하다 실패를 겪는 것에서 좀 더 많이 배울 수 있다고 생각해요.
추가적으로 step2를 고려하지 않아야, 일반적으로 코드가 변경되는 상황(실무에서 갑자기 기능이 추가되는 경우)을 겪을 수 있다고 생각하거든요. 만약에 step2를 보고 코드를 작성한다면, 그 step2에 최적화된 코드로 작성하게 될 것 같아서
저는 step1을 짤 땐 step2를 고려하지 않고 짜긴 합니다.

👍

[hong-sile]

좀 더 가독성이 좋게 반환값을 중간에 분리하였습니다.

[hong-sile]

수정하였습니다. drawInput 부분만 에러 발생할 수 있으니, 해당 부분만 trycatch로 감싸서 처리하였습니다.

[hong-sile]

제출하고 코드를 다시 보고 있는데, 이 map의 반환 타입이 Map<String,CardGroup> 보다 Map<Name, CardGroup>이 적당한 것 같군요.
이 메서드 말고 다른 메서드들도 key를 String이 아닌 Name으로 리팩터링 해보도록 하겠습니다.

[jamie9504]

홍실, 안녕하세요. 리뷰어 제이미입니다 😄

리뷰 조금 남겨두었는데, 변경하시고 싶은 부분 변경하신 후 리뷰 요청 다시 주세요.

이번 반영 후에 2단계로 넘어가면 될 것 같습니다.

[jamie9504]

1. Draw 여부를 가지는 친구인데 이 친구의 이름엔 왜 Input이 붙었나요?
2. 이 친구는 Controller에서만 쓰고 있는데, 그러면 BlackjackGame을 가져다 쓰려고 할 때, Draw인 경우에만 카드를 뽑는다는 건 어떻게 알 수 있을까요? Controller 로직까지 파악을 해야 할까요?

[jamie9504]

앗, 제가 의도를 명확히 표현하지 못했네요!

위에는 CardShape를 cardShape로 선언했고 아래는 CardNumber를 number로 선언해서,
Card 도메인이라서 생략을 할 것이라면 일괄 생략을, 생략을 하지 않을 것이라면 일괄 미생략으로 가야하지 않을까 생각했어요.

[jamie9504]

유저의 수를 제한하면 현재 프로그램에서는 문제 없이 동작하겠지만, 추후 다른 개발자가 해당 도메인을 사용할 때 동일한 문제가 있을 수 있겠죠?
이 Deck이라는 도메인에서 NPE가 발생하도록 두는게 좋은 방향일까요?

[jamie9504]

List<User> users의 User는 CardGroup을 가지고 있고, 이는 List<Card> cards에 카드를 추가할 수 있는데요.

그렇게 되면 copyOf로 호출했을 때, 카피한 객체의 user의 cardGroup에 cards를 추가하게 되면 원본에도 추가가 될까요?

[jamie9504]

3개 이하로 유지하는 거면 Game에서 Deck, Dealer, Players를 가질 수 있는게 아닌가요?

[jamie9504]

println을 직접 호출하는 것과 메서드를 분리한 것의 차이점이 있을까요?

[hong-sile]

개행을 위해 System.out.println을 호출한 것보다, 메서드를 분리하고 이름으로 표현하는게 더 읽기 편하다고 생각해서 메서드를 분리하였습니다.

[jamie9504]

System.out.println()으로 개행을 충분히 의미한다고 생각했지만,
홍실의 의도가 그런 의미라면 그럴 수 있겠네요 👍

[jamie9504]

필립, 홍실, 딜러 등의 반복되는 이름은 상수로 빼도 되지 않을까요?
변경지점을 하나로 줄일 수 있을 것 같아요!

[hong-sile]

아주 좋은 방법인 것 같아요. 다 분리하였습니다.

[jamie9504]

요 부분은 취향 차이이니 참고만 해주세요/
주석은 편하도록 정보를 전달해주고 있지만, 코드 수정시 체크를 하지 못할 수도 있습니다.

테스트 메서드 내에서 필립, 홍실, 딜러의 점수를 계산해서 보여주는 로직이 있어도 좋지 않을까요?
ex) assertThat(blackJackGame.getScore("필립")).isEqualTo(21)

[hong-sile]

와우 이건 생각 못한 아이디어네요. assertThat으로 유효성 검증만 할 생각만 했지.
assserThat으로 정보를 표시하다니! 앞으로 종종 테스트 코드에 적용하도록 하겠습니다.

[jamie9504]

홍실, 안녕하세요. 리뷰어 제이미입니다 😄

피드백을 조금 남겨두었습니다.

2단계 진행해주시면 될 것 같고, 읽어보시고 반영하고 싶으신 부분은 2단계 때 반영해주세요.
추가 코멘트는 2단계 리뷰에 달아주셔야 확인이 가능합니다.

이만 Approve하겠습니다!

[jamie9504]

Optional로 감싼 부분이 리팩토링 중 없어졌나보군요!
Deck에서 빈 Deck을 호출할 때 IllegalStateException을 던지면, NPE가 발생할 일이 없어지죠 👏 👏 👏

비어있을 때 IllegalStateException을 처리하는 것은, Deck의 역할과 책임이고
이걸 가져다 쓰는 경우 IllegalStateException을 발생시킬 수 있다면 try-catch를, 발생할 일이 없다면 별도의 처리를 하지 않아도 된다고 생각합니다 😄

사실상 Deck에서 더 뽑을 카드가 없다면 예외가 발생해야 맞는 상황인거니까요!

[jamie9504]

1. Controller에서 일부 BlackjackGame이 가져야할 역할을 가지고 있는 게 아닐지 고민해보면 좋을 것 같습니다.

지금 제가 생각하기에 Controller에서 BlackjackGame으로 넣을 수 있는 기능은 다음과 같습니다.
isContinuous -> 모두 blackjackGame의 메서드로 로직이 구성됨. blackjackGame에서 통합해서 반환할 수 있음
y 또는 n에 따라 카드를 뽑고 안 뽑고가 결정되는 로직 -> 나중에서야 확인했는데, 마지막에 n을 입력해도 카드를 한 장 더 뽑더라고요... blackjackGame에서 y또는 n이 들어있는 Enum(DrawOrStay)에 따라, 뽑을지 말지를 결정하는 로직을 넣었습니다.
blackjackGame에서 도메인과 관련된 모든 분기를 처리하고, Controller에서는 단순히 입력과 출력을 매핑해줘야한다는 의견에 적극 동의합니다.
하지만 입력된 값(y,n)에 따라 이후에
"현재 플레이어에 대한 y,n을 받을지, 다음 플레이어에 대한 y,n을 받을지"가 결정되기에 controller에서 어느정도 로직이 들어갈 수 밖에 없다고 생각해요.
음... 이건 적용하진 않았지만 고민한 로직인데, InputView의 y,n을 입력받는 메서드랑, 그 출력하는 메서드를 함수형 인터페이스로 넘길까도 생각해봤어요 (inputView.read -> Supplier, outputView.print -> Consumer)
실제로 이런식으로 하면 controller에서는 로직이 엄청 간단해지고, 입출력함수들도 함수형 인터페이스로 포장되기 때문에, blackjackGame에서 모든 로직을 파악할 수 있습니다. 그리고, Controller에서는 단순히 blackjackGame의 메서드만 호출하게 되겠죠
하지만 view의 메서드들이 비록 함수형 인터페이스라곤 하나, domain내부에 코드가 전달되는게 괜찮을지? 몰라서 고민이 되네요.
view가 바뀐다 하더라도, 전혀 domain의 코드를 바꿀 필요도 없긴 하지만(그래서 의존하는게 아니라고 생각했습니다.)
너무 생소한 방법이다보니? 어떠한 사이드이펙트가 발생할지 몰라서 실제로 코드를 짜서 사용은 해봤지만 커밋은 하지 않았어요.
이렇게 함수형 입출력함수를 함수형 인터페이스로 받아, blackjackGame에서 처리해도 문제가 없을까요?

음.. 제가 말했던 의미는 현재 아래와 같이 isDraw인지 isContinuous할 수 있는지를 컨트롤러에서 판단해서 Game에서 play를 하고 있는데, 이렇게 되면 Controller를 확인하지 않으면 Game만 봤을 때는 언제 play를 할 수 있는지라던가, play를 해도 되는지라던가의 정보가 없다고 생각해서 위와 같이 질문을 남겼었는데요. 이 부분을 다시 생각해보시면 좋을 것 같아요 😄

        DrawOrStay drawOrStay = DrawOrStay.DRAW;
        while (drawOrStay.isDraw() && blackJackGame.isContinuous(playerName)) {
            drawOrStay = repeatUntilReadValidateDrawInput(playerName);
            blackJackGame.playPlayer(playerName, drawOrStay);
            final CardGroup playerCardGroup = blackJackGame.getCardGroupBy(playerName);
            outputView.printUserNameAndCardGroup(playerName.getValue(), ViewRenderer.renderCardGroup(playerCardGroup));
        }

2. Users를 파라미터 2개로 묶어놨는데, 이 부분을 묶어야 할 필요가 있었을지 고민해봐도 좋을 것 같습니다.

이 부분을 아무리 고민해도 답이 안 나오네요. Users를 만들 때, player 이름에 대한 List와 각 user(player+dealer)의 카드 풀을 초기화 되기 위해서 Deck이 또 필요하다고 생각합니다.
혹시 어떤 식으로 쪼갤 수 있을까요..?

User가 초기화될 때 Deck이 필요한지 여부에 대해서 고민해보았어요.
게임으로 치자면, 플레이어는 자신의 이름을 대고 게임을 실행하고, 첫 턴에 카드를 2장 받는거니까요
처음부터 Deck을 받아야하는 걸까? 라는 생각을 해봤습니다 😄

3. step1 미션을 할 땐, step2 미션을 고려하지 않고 미션을 진행해야 다음 step으로 넘어갈 때 변경점이 많고 거기서 배우는 점들도 많다고 생각하는데, 이 부분은 진행정도와 취향에 따라 선택하면 될 것 같아요.

저도 이 말에 동의를 해요. 저도 step1을 작성할 때 step2를 고려하지 않고, 코드를 작성합니다. 그래야 실패도 해보고 실패에서 배울게 많다고 생각하거든요.
책에서 아무리 안 좋은 설계라 이야기해도 직접 본인이 설계를 하다 실패를 겪는 것에서 좀 더 많이 배울 수 있다고 생각해요.
추가적으로 step2를 고려하지 않아야, 일반적으로 코드가 변경되는 상황(실무에서 갑자기 기능이 추가되는 경우)을 겪을 수 있다고 생각하거든요. 만약에 step2를 보고 코드를 작성한다면, 그 step2에 최적화된 코드로 작성하게 될 것 같아서
저는 step1을 짤 땐 step2를 고려하지 않고 짜긴 합니다.

👍

[jamie9504]

프로그래밍 요구 사항
모든 기능을 TDD로 구현해 단위 테스트가 존재해야 한다. 단, UI(System.out, System.in) 로직은 제외
핵심 로직을 구현하는 코드와 UI를 담당하는 로직을 구분한다.
UI 로직을 InputView, ResultView와 같은 클래스를 추가해 분리한다.

이 부분을 확인했을 때, View쪽은 테스트를 짜지 않아도 된다고 생각했었는데, 제 생각과 다를까요?

[jamie9504]

이 부분 단순하게 생각했었는데요.

User에서 CardGroup을 가지고 있고, CardGroup이 Score를 만드는 책임을 가지고 있는데
반대로 Score에서 CardGroup을 받아서 자신을 생성할 책임이 있다면?
User에서 Score를 이용해서 사용할 수 있지 않을까~ 싶었던 부분입니다.

// AS-IS

public abstract class User {
    private final CardGroup cardGroup;

    final public boolean isBust() {
        return cardGroup.isBustScore();
    }

    final public boolean isBlackJackScore() {
        return cardGroup.isBlackJackScore();
    }
}

public class CardGroup {
    public Score getScore() {
        return Score.calculateScore(getTotalValue(), getAceCount());
    }

    public boolean isBustScore() {
        return getScore().isBust();
    }

    public boolean isBlackJackScore() {
        return getScore().isBlackJackScore();
    }
}

// TO-DO

public abstract class User {
    private final CardGroup cardGroup;

    final public boolean isBust() {
//        return Score.isBust(cardGroup);
        return new Score(cardGroup).isBust();
    }

    final public boolean isBlackJackScore() {
//        return Score.isBlackJack(cardGroup);
        return new Score(cardGroup).isBlackJack();
    }
}

public class Score {
    public Score(final CardGroup cardGroup) {
        // use Code
    }
}

[jamie9504]

초기에 2장을 뽑는다는 것은 게임의 룰인데 CardGroup이 알아야 하는지 고민이네요!

[jamie9504]

단일 모듈 프로그래밍이면 그럴 일이 없긴 할 것 같아요.

public으로 상수를 쓰는 것에 대한 반대 의견은 없습니다.
단지 public으로 상수를 그대로 써서 비교하는 것과,
private으로 감춘 후 딜러 내에서 뭔가 메서드를 만든다던가 해서 비교하는 책임을 지어주는 것 중 어떤 것이 좋을까 라는 고민을 해봤으면 좋겠어서 남겼던 코멘트였어요 😄

[jamie9504]

이 부분 위 코멘트와 동일한데, 처음에 2장을 나눠주는 건 게임의 역할이 아닌가 생각했어요 😄

[jamie9504]

System.out.println()으로 개행을 충분히 의미한다고 생각했지만,
홍실의 의도가 그런 의미라면 그럴 수 있겠네요 👍