[2단계 - 블랙잭 베팅] 카피(김상혁) 미션 제출합니다. #679

tkdgur0906 · 2024-03-12T07:59:48Z

안녕하세요 웨지🍟
블랙잭 미션을 진행한 카피입니다!

오늘 피드백 강의에서 게임 내 규칙을 객체로 추상화하여 설계하는 것에 대해 배웠습니다.
하지만 바로 반영하기에는 무리가 있어 원래 제가 설계한 대로 마무리하여 아쉬운 점이 보이는 것 같습니다!

궁금한점

처음에 구현할 때는 Participant 클래스를 상속받는 Player와 Dealer를 만들어 블랙잭에 참여하게 하였습니다.
하지만 구현하던 중 두 클래스가 필요한가에 대한 의문이 생겼고, Player와 Dealer를 삭제하고 기존에 만들어놓은 PlayerCards와 DealerCards가 블랙잭 게임에 참여하는 구조로 설계했습니다.
Player와 Dealer 처럼 직관적으로 의미하게 하는 것이 좋지 않나라는 생각도 들지만, 객체의 세계는 현실세계와는 차이가 있다라는 말을 들어보니 상관없지 않나?라는 생각도 합니다.
웨지는 어떻게 생각하시는지 궁금합니다!
구현하면서 클래스의 구조를 여러번 바꾸게 되었는데, 이에 소요되는 시간이 꽤나 컸습니다.
tdd를 통해 작은 것부터 구현해가는 방식을 채택하고 있는데 이번에 시행착오를 겪으면서 구현전에 먼저 전체적인 구조를 어느정도 잡고 가야하는 것인가?라는 생각이 들었습니다. 너무 추상적인 것 같긴 하지만 웨지의 의견을 듣고싶습니다!
원시값을 필드로 가지고 있는 객체가 있을 때(BetAmount 같이) 해당 객체의 필드 값을 반환할 일이 생긴다면 원시값을 반환하는게 좋을지 new BetAmount를 반환하는게 좋을지 생각해보았습니다.
평소에는 별 생각없이 원시값을 반환하곤 했었는데 외부에서 객체 내부의 변수 타입을 알도록 하는게 맞는가에 대해 고민이 듭니다.

질문 읽어주셔서 감사합니다!

…into step2

sihyung92

안녕하세요 카피~~ 리뷰어 웨지입니다.

바로 Q&A!
Q1. 객체와 현실세계의 상관관계
A. 넵 코멘트에도 남겼듯이, 전 은유가 코드에 끼어드는 걸 좋아하지 않습니다. 그런데 말씀해주신 PlayerCards는 실제로 하고 있는 역할을 전부 담고 있지 못합니다. 유저의 카드 목록에 이름이 있나요?
객체지향은 "그곳에 있을법한 코드가 그곳에" 있는 것을 추구합니다. 카피의 팀원은 카피가 휴가간동안 입력금 로직을 수정해달라는 요청을 받았을 때, 일단 패키지를 펼쳐놓고 객체명을 훑어볼 텐데요.
"Player"가 있었으면 쉽게 찾았겠지만 지금 같은 구조라면 코드를 처음부터 읽어봐야 찾을 수 있습니다. PlayerCards가 입력금을 가지고 있다는게 쉽게 연상되지 않기 때문입니다.
저는 짜놓고 따로 인수인계 안 해도 휴가갈 수 있으면(아무나 내 코드를 고칠 수 있으면) 그게 최고의 코드라고 생각합니다.

Q2. 클래스 구조변경 시 TDD의 어려움
A2. 넵 테스트가 있으면 리팩토링 때 수정되는 코드량이 2배이므로 힘든게 맞습니다. 전 TDD의 가치는 코드를 쌓아올려가며 빠르게 피드백을 받고 테스트가 용이한 코드로 자연스러운 리팩토링이 일어나는데 있다고 보고, 이미 완성된 코드의 아키텍처를 변경할 때는 오히려 방해하는 순간도 있다고 생각합니다.

요거는 정답이 없으므로 경험을 쌓아가시면서 "좋았던 때"와 "나빴던 때"를 구분해 적절하게 적용하시길 추천합니다.

Q3. 원시값 랩핑 객체의 불변값
저는 비즈니스적인 처리에선 계속 랩핑 객체(그리고 불변으로)로 반환하는 게 좋고, 최종적으로 노출되는 순간만 (View에 나가야한다든지) 내부 타입을 반환하는게 맞다고 생각합니다.

감정이 메마른 야간에 리뷰를 해서 리뷰가 좀 딱딱할 수 있는데 전 사실 기분이 좋습니다~~ 🚀🚀
어떤 질문이든 좋으니 궁금한게 생기시면 편하게 DM 주세요. 화이팅입니다.

sihyung92 · 2024-03-12T14:01:13Z

src/main/java/manager/Casino.java

+import java.util.stream.Collectors;
+
+import static view.InputView.*;
+import static view.OutputView.*;

 public class Casino {


domain 패키지와 view 패키지는 MVC의 역할에 따른 이름 분리인데 manager 패키지는 무엇을 manage 하나요?
controller를 manager라고 이름붙이는 데서 오는 이점이 있나요?

또 카지노는 contoller 역할을 하는데 왜 이름은 카지노인가요?

현실의 은유는 팀 단위 코드에서는 안 좋은 영향을 줄 때가 많습니다.
좋은 코드는 10명 중 7명이 읽고 납득할만한 코드입니다. 그런데 은유가 많아질수록 많은 사람의 동의를 얻기 점점 어려워집니다.
디자인 패턴이 좋은 이유 중 하나는 'MVC' 라고만 말해도 여러 개발자가 동일한 멘탈 모델을 떠올리기 때문입니다.
조금더 객체가 하는 역할이 분명히 드러나는 네이밍으로 리팩토링 해보셔요.

확실히 현실의 것에 대해 너무 의존하게 된다면 보는 사람으로 하여금 그 의미를 한번에 파악하기 어렵게 만드는 것 같습니다!

다만 디자인 패턴이라는 것은 어떤 반복적인 상황을 해결하기 위한 방법이라고 생각합니다. 하지만 블랙잭 게임에서는 MVC 패턴을 적용할만한 반복적인 패턴이 보이지 않는다고 생각합니다.
그래서 게임 자체를 관리한다는 의미에서 manage 패키지를 만들었는데, 'Casino'를 'BlackjackGameManager'로 변경하는 거에 대해서는 어떻게 생각하시나요?

사실 controller 역할을 한다는 것은 알고 있지만, MVC 패턴을 알고 있다고 해서 필요성을 느끼지 않는데 사용하기는 꺼려지는 부분이 있어 manage로 이름을 붙였습니다.
controller의 역할을 하는 클래스가 여러개 생긴다면 MVC 패턴을 도입하는게 자연스럽지 않을까라는 생각을 했습니다😂
이미 domain과 view를 사용했기 때문에 MVC를 어느정도 적용한것이 아닐까? 라는 고민도 했지만, domain과 view는 MVC를 몰라도 생각할 수 있을 만한 네이밍이라는 생각도 듭니다.

네넵 말씀하시는 의견 동의합니다 ㅎ
안다고 적용할 필요가 없고, (특히 우테코에서) 지식의 저주에 걸리는 개발자도 많습니다. 손으로 익은 것 보다 머리로 아는 게 많아서 괜한 걱정으로 코드를 못 치시는 분들이 많죠.

디자인 패턴도 특정 문제상황을 해결하기 위한 굿 프렉티스에 불과하고, 모든 상황에 알맞은 패턴이 없는것도 맞습니다.

그러나 본문에도 말씀드렸듯이 디자인 패턴의 강점 중 하나는 여러 개발자의 멘탈모델을 쉽게 공유할 수 있다는 부분에 있습니다.
실제로 현업에 가보시면 manager라는 낯선 모델링 때문에 매번 설명마다 30초씩 시간을 더 쓰실거에요.
그리고 실제로 manager가 domain과 view의 연결 역할 말고 다른 역할을 부여받은 게 있나요?
manager 패키지에 대한 설명을 다 들은 개발자는 속으로 그냥 '이름만 다른 controller네' 하고 머릿속에 정리해둡니다.
혹은 본인이 이해한게 맞은지 되물을거에요. "그러면 이거 MVC의 controller와 비슷한 거라고 이해하면 될까요?"

혼자 개발할 때와 달리 팀 차원의 업무에선 정보전달의 효율성이 중요하기 때문에 네이밍 등은 컨벤션과 남들이 이해하기 쉬운 모델로 구성해야한다는 맥락이 생긴다는 점 전달드립니다 ㅎ

일단 그렇구나 해두시고 코드 작성은 스스로 옳다고 느끼는 방향 (manager 유지)로 해주셔요

sihyung92 · 2024-03-12T14:06:13Z

src/main/java/domain/card/Cards.java

@@ -6,6 +6,8 @@
 public class Cards {


File Changed에 Card가 없어 여기 대신 답니다.

<Card 리뷰>

toString()을 로직으로 쓰지 않기를 권유합니다.
toString()은 Object에서부터 내려오는 메서드이어서 매우 쉽게 재정의되고, 버저닝에 매우 취약합니다.
나는 toString을 "cardNumber.getSign() + shape.getShape();"로 유지되길 바랬지만 누군가 필드를 하나 추가하고 습관적으로 toString을 지우고 IDE에서 재정의하면 즉시 view 단에서 버그가 발생합니다.

toString의 재정의만 봐서는 이 코드가 어디서 사용될지 유추하기가 매우 까다롭습니다.
개인적으로는 asString() 처럼, 이 코드를 String화 하여 다른 곳에 사용한다는 의미를 내포하는 네이밍을 선호합니다.

그리고 Equals와 Hashcode를 재정의해주세요. enum 두 개 필드의 조합인 Card는 표시가 같다면 같은 인스턴스로 간주되어야 합니다. 역할을 가진 도메인 "객체" 들에게는 eqauls, hashcode, toString을 필수적으로 재정의해주세요.

File Changed에 Deck이 없어 여기 대신 답니다.

<Deck 리뷰>
createDeck() 메서드에서 =>

create Deck에 매직넘버가 있어서 가독성이 떨어집니다. 1, 14, 0, 4 는 무엇을 의미하나요?
flatmap에서 IntStream을 반환하는 방식도 depth만 없을뿐, 2중 포문과 다르지 않다고 생각합니다.
매직넘버와 Stream 가독성을 개선해주세요.

draw()는 0장이 되면 반드시 오류가 발생하는데, 버그도 수정해주셔요.

또 테스트 하기 어려운 랜덤을 가능한 적게 넣는 방법도 좋을거 같아요.
Deck을 만들 때 한번 셔플해두고, 1개씩 순차적으로 응답하는 방법은 어떠신가요?
DrawStrategy 처럼 불필요한 인터페이스를 삽입할 필요가 없어질거 같습니다.

File Changed에 Card가 없어 여기 대신 답니다.

<Card 리뷰>

toString()을 로직으로 쓰지 않기를 권유합니다. toString()은 Object에서부터 내려오는 메서드이어서 매우 쉽게 재정의되고, 버저닝에 매우 취약합니다. 나는 toString을 "cardNumber.getSign() + shape.getShape();"로 유지되길 바랬지만 누군가 필드를 하나 추가하고 습관적으로 toString을 지우고 IDE에서 재정의하면 즉시 view 단에서 버그가 발생합니다.

toString의 재정의만 봐서는 이 코드가 어디서 사용될지 유추하기가 매우 까다롭습니다. 개인적으로는 asString() 처럼, 이 코드를 String화 하여 다른 곳에 사용한다는 의미를 내포하는 네이밍을 선호합니다.

그리고 Equals와 Hashcode를 재정의해주세요. enum 두 개 필드의 조합인 Card는 표시가 같다면 같은 인스턴스로 간주되어야 합니다. 역할을 가진 도메인 "객체" 들에게는 eqauls, hashcode, toString을 필수적으로 재정의해주세요.

toString을 로직으로 사용하지 말라는 의견에 대해서 동의합니다!
toString의 용도는 디버깅을 한정하여 사용하는 것이 좋을 것 같습니다.

만약 도메인의 정보에 대해서 string으로 변환하는 로직이 필요하다면 asString() 처럼 따로 정의해서 사용해보도록 하겠습니다!

equals와 hashcode를 정의하는 것에 대해 필요성을 몰랐는데 학습해보니 equals가 정의되지 않으면 동등성 비교가 안되기 때문에 재정의해줘야 하는 것 같습니다!
hash를 사용하는 Collection에서 hashcode, equals 리턴값의 비교를 통해 동등성을 확인하는데 hashcode를 재정의하지 않은 경우 Object의 hashcode를 사용하여 객체마다 다르게 나오기 때문에 hashcode도 같이 재정의해야 한다고 이해했습니다!

File Changed에 Deck이 없어 여기 대신 답니다.

<Deck 리뷰> createDeck() 메서드에서 =>

create Deck에 매직넘버가 있어서 가독성이 떨어집니다. 1, 14, 0, 4 는 무엇을 의미하나요? flatmap에서 IntStream을 반환하는 방식도 depth만 없을뿐, 2중 포문과 다르지 않다고 생각합니다. 매직넘버와 Stream 가독성을 개선해주세요.

draw()는 0장이 되면 반드시 오류가 발생하는데, 버그도 수정해주셔요.

또 테스트 하기 어려운 랜덤을 가능한 적게 넣는 방법도 좋을거 같아요. Deck을 만들 때 한번 셔플해두고, 1개씩 순차적으로 응답하는 방법은 어떠신가요? DrawStrategy 처럼 불필요한 인터페이스를 삽입할 필요가 없어질거 같습니다.

stream으로 한 방식이 이중 for문과 동일하다고 말씀하신 것에 대해 동의합니다!
그래서 stream으로 생성 하는 방식 대신 이중 for문으로 생성하는 방식에서 메서드 분리를 통해 depth를 줄이도록 변경하였습니다.

draw()할 때 덱이 비어있을 경우 발생하는 오류에 대해서 예외를 던지도록 했습니다!
명시적인 예외를 던지는 것으로 현재는 충분하다고 생각하여 따로 예외를 처리하지는 않았습니다.

랜덤으로 덱을 만들어야한다는 것에 매몰되어 덱을 섞는다는 방식을 생각하지 못한 것 같습니다😂
DrawStrategy를 삭제하고 Collections.shuffle()을 통해 생성된 덱을 섞고 카드를 뽑는 방식으로 변경하였습니다.

equals & map ->
네네 맞아요 ㅎ hashCode는 가장 자주 사용되는 컬렉션 중 HashSet, HashMap (그리고 hash 알고리즘을 사용하는 기타등등)에서 활용되므로 재정의하는 것이 중요합니다

src/main/java/view/InputView.java

src/main/java/domain/Rule.java

sihyung92 · 2024-03-12T14:41:21Z

src/main/java/domain/BetAmount.java

+
+import domain.result.Status;
+
+public class BetAmount {


VO 여서 equals, hashcode, (가급적) toString을 구현해주시면 좋습니다.

3개의 메서드는 앞으로 신경써줘야겠네요!

equals, hashcode, toString을 domain 패키지 내부의 거의 모든 클래스에 추가하게 되었습니다.

제 생각엔 도메인 적인 의미를 가진 모든 클래스에 추가해주는게 맞다는 생각이 드는데,
해당 메서드를 굳이 오버라이딩할 필요가 없는 경우가 있을지 궁금합니다!

개인적으로 생각하기엔 없습니다! 도메인 객체에선 무조건 추가해줘야됩니다 ㅎㅋ

개발에선 무조건이 잘 없는데 이건 무조건입니다

클라이언트가 소스코드 용량을 64kb로 제한했다 이런거 아니면요
그런데 자바로 짜라고 해놓고 소스코드 용량 제한하면 불공정 거래로 경찰에 신고해야 됩니다

src/main/java/domain/BetAmount.java

src/main/java/domain/card/Cards.java

README.md

sihyung92

안녕하세요 카피~~ 리뷰어 웨지입니다!
기존에 달아주신 코멘트에 대한 답변과 추가 리뷰를 남겼으니 확인해주셔용~
감사합니다!

sihyung92 · 2024-03-15T06:18:52Z

src/main/java/domain/Rule.java

+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class Rule {


<위에건 지난 코멘트에 대한 답변이고 이건 새 리뷰입니당!>
여전히 Rule이라는 객체명이 주는 정보가 너무 부족합니다.
저는 코드 리뷰를 할 때 카피가 휴가가면 제가 이 코드를 대신 고쳐야된다고 생각하고 봅니다.

즉 코드를 작성하는 사람은 내 기능을 다른 사람이 수정한다는 관점에서 작성해야 하고, 이를 달성하지 못하면 휴가를 못 갑니다. (퇴사 할 때도 들들 볶입니다)

누가 카피 없으니까 저보고 수익금 반환로직을 고쳐달라고 했다고 가정해볼게요.

인텔리제이로 브랜치를 check out합니다

왼쪽 패키지를 전부 펼쳐놓고 슥 훑어볼텐데, Rule에선 연상되는 것이 없으므로 그냥 지나갑니다.

Incomes가 있으므로 들어가봅니다.

<Name, Income> 이 있으므로 이 구조만 보고 "아 유저당 수익금 ㅇㅋ" 합니다. 그런데 수익금 계산로직은 없으므로 Incomes를 의존하는 코드를 찾아봅니다.

Rule이 있고, Rule로 들어가서 로직 확인하고 수정합니다.

Incomes라는 구조체를 만들어주시고, Name과 Income을 원시값 랩핑한 덕분에 4 -> 5으로 가는데 들어간 인지적 부하가 매우 줄었습니다. (👍👍)
그러나 애초에 Rule라는 객체명이 좀더 수익금 계산이란 역할을 암시하는데 적절했다면, 2에서 끝났겠죠.

개발자의 작업이란 읽는데 80%, 쓰는데 20%의 시간을 쓰는 작업입니다. 읽는 시간을 줄여줄 수 없을지 고민하면 좋은 코드가 된다고 생각합니다.

sihyung92 · 2024-03-15T06:51:11Z

src/main/java/domain/result/Incomes.java

+
+public class Incomes {
+
+    private final Map<Name, Income> incomes;


코드를 보다보니 Incomes를 도메인 객체로 여기고 계시다면 Name 대신에 Player이 들어가는 게 더 적절하다는 의견입니다

Suggested change

private final Map<Name, Income> incomes;

private final Map<Player, Income> incomes;

객체 생성패턴이 Player를 통해서만 생성되고 있기 때문이고요, 의미론적으로도 Income은 이름에 부과되는 게 아니라 유저에게 부과되는 것이기 때문입니다.

그러면 Player가 Income을 반환할 수 있는 로직이 있으니 애초에 Map 형태로 데이터를 보관할 필요도 없겠지요. (순서를 보장하려고 LinkedHashMap을 쓸 필요도 없고요)

Suggested change

private final Map<Name, Income> incomes;

private final List<Player> players;

이러다보면 적절한 객체명도 달라지고, 패키지의 위치도 달라질겁니다.

이거는 코드의 용도에 따라서 다르게 볼 수도 있는데요, 지금의 Incomes 객체는 사실 Dto에 가깝다고 느껴집니다. (OutputView에 넘기기 위한 용도)

그래서 리뷰는 아래 두 방향 중 선택하시거나, 의견을 들려주세요.

Incomes의 필드를 List로 바꾸는 방향

Incomes는 Dto로 간주하고 적절한 패키징, 객체명 리팩토링을 하는 방향

src/main/java/domain/BetAmount.java

src/main/java/domain/card/CardNumber.java

src/main/java/domain/card/Shape.java

sihyung92 · 2024-03-15T06:58:55Z

src/main/java/domain/participant/Dealer.java

+    }
+
+    public String getFirstCard() {
+        return cards.get(0).asString();


Card를 String으로 바꾸어 제공하는건 view 로직이에요. Card를 반환하고 그걸 어떻게 활용할지 결정하는 건 client 코드에 맡기는게 나아보입니다.

도메인이 반환하는 것은 Card이고 그걸 활용하는 것은 View의 역할이라는 것에 동의합니다!

추가적인 궁금증인데, Card 리스트인 cards에서 get(0)을 통해 첫번째 카드를 반환하는데,
인덱스에 의존하는 로직 같은데 괜찮나? 라는 생각이 듭니다.

이에 관해서
인덱스에 의존하는 것이 맞다! get은 안쓰는게 좋다! 혹은
이 정도는 괜찮다! 그 대신 다른 예외처리를 해줘야 할 것 같다! 인지
웨지의 생각이 궁금합니다!

객체는 겉으로 드러내는 것들이 중요하고,
내부적인 로직은 그 다음이라고 생각합니다 (캡슐화)

외부 객체와 계약한건 getFirstCard라는 메서드명, 인자는 없음, String을 반환함 입니다.
인덱스를 활용하건 다른 무언가를 사용하건 큰 상관은 없다고 봅니다.
또 지금은 인덱스더라도 몇 번의 버전을 거치고나면 전혀 다른 로직으로 변모할 수 도 있습니다.
예외처리 같은 경우엔 발생할 여지가 있다면 처리해주는게 좋고요. (OutOfBoundException)

sihyung92 · 2024-03-15T06:59:52Z

src/main/java/view/InputView.java


-    private Scanner scanner = new Scanner(System.in);
+    private static Scanner scanner = new Scanner(System.in);


final 처리가 가능함을 인텔리제이가 알려주네요 ㅎ
제약은 가능한 강하게 걸어두는 것이 좋은 코딩 습관입니다 (접근제어자, final 등등)

제약을 강하게 두는 것에 대해 필요성을 느끼고 있는 중입니다!

네오의 강의를 들으면서 주의깊게 본 것이 class, 메서드 내의 변수 선언할 때 final을 붙이는 것입니다.
ex) final String s = "s";
코드를 통해 의미를 전달하는다는 측면에서 좋은 방식이라고 생각하는데,
웨지는 실제 코드를 짜면서 class와 메서드 내의 변수에 final을 하나하나 붙여주는 편인가요?

전 안 붙입니다~~
붙일거면 다 붙이던지 안 붙일거면 다 안 붙여야 된다고 생각하는데 놓치기 쉬운 습관이고 팀원들에게 컨벤션으로 강요하기 어려워서 선호하지 않습니다.

sihyung92 · 2024-03-15T07:00:51Z

src/main/java/view/InputView.java

        String opinion = scanner.nextLine();
-        return POSITIVE.equals(opinion);
+        if(POSITIVE.equals(opinion)){


이부분 포맷팅이 안 되어 있습니다.
commit 전 전체 코드 포맷팅, 사용하지 않는 import 제거를 돌려보는 습관이 있으면 좋습니다.

아이고 신경을 쓴다고 썼는데 매번 이런 실수가 나오네요😥
커밋 전에 포맷팅하는 습관을 들이도록 해봐야겠습니다!

sihyung92

안녕하세요 카피~ 리뷰어 웨지입니다.
지난번 코멘트에 달린 답변이 없는 줄 알고 살짝 슬펐는데,
github UI의 문제였더라구요. 정성스럽게 답변해주신 내용 잘 봤습니다 🙇‍♂️
다양하게 고민을 하는 시간을 가지고 계신거 같습니다.

질문해주신 내용에 대해선 �이번 리뷰가 답변이 되었을거라 생각합니다.
반영은 자유롭게 해주시고, 질문도 편히 주세요

sihyung92 · 2024-03-16T01:22:31Z

src/main/java/domain/participant/Dealer.java

+    }
+
+    public String getFirstCard() {
+        return cards.get(0).asString();


객체는 겉으로 드러내는 것들이 중요하고,
내부적인 로직은 그 다음이라고 생각합니다 (캡슐화)

외부 객체와 계약한건 getFirstCard라는 메서드명, 인자는 없음, String을 반환함 입니다.
인덱스를 활용하건 다른 무언가를 사용하건 큰 상관은 없다고 봅니다.
또 지금은 인덱스더라도 몇 번의 버전을 거치고나면 전혀 다른 로직으로 변모할 수 도 있습니다.
예외처리 같은 경우엔 발생할 여지가 있다면 처리해주는게 좋고요. (OutOfBoundException)

sihyung92 · 2024-03-16T02:08:35Z

src/main/java/domain/result/Incomes.java

+        return players.stream()
+                .collect(Collectors.toMap(
+                        player -> player,
+                        player -> player.determineIncome(dealer.decideStatus(player)),


코드를 보다보니 이 부분에서 위화감을 느꼈는데요,
player -> dealer -> player 를 부르는 부분입니다.

객체에서 양방참조는 하나의 객체가 해야할 일을 둘로 나뉘어 하고있다는 신호인데,
status를 사이에 끼고 양방 참조가 되는 방식입니다.

그래서 이걸 어떻게 해소할 수 있을까.. 저도 어려워서 30분 정도 끙끙 거리면서 고민을 해봤고, 아래처럼 결론을 내렸습니다.

Incomes가 직접 배당금을 몇배로 불려야하는지 알야아 하는게 합당하다고 생각함

현재는 BetAmount가 계산식을 가지고 있음

그래서 BetAmount에게 Status를 넘기기 위해 순환참조가 생김

위 작업을 하기 위해 몇 가지 리팩토링이 필요함

BetAmount는 단순히 계산이 가능한 multiply, plus, minus, getValue를 남김 (외부 도메인 의존 제거하고 순수한 값객체로 만듬)

player는 BetAmount의 전달자 역할만 함. 즉 getBetAmount()만 남기고 determineIncome() 메서드를 제거함

Income에서 Status, BetAmount를 받아 승무패에 따른 계산이 가능한 메서드를 생성함(기존 betAmount의 determineIncome()과 동일한 역할), 혹은 IncomeCalculator (가제)를 새롭게 만들어도 좋을거같습니다.

위처럼 변경하면 순환참조가 없어지고 배팅금을 계산하는 로직이 Income (혹은 IncomeCalculator) 객체 로 모이게 됩니다. 기존엔 딜러 / 플레이어가 승패를 결정하고 액수를 결정하는 두 가지 역할을 수행하는 부분을 다른 객체로 위임해서 해소해보고자 했는데요.

너무 작업해야할 방향을 강하게 지시하는 리뷰인거 같아 반영 여부는 카피에게 맡기겠습니다.
아래 두 가지만 기억해주셨으면 합니다.

순환참조는 리팩토링의 신호이다. (한 객체가 할 일을 나누어 하고 있는 것이다.)

위 아이디어의 방향성 -> 순환참조의 원인 고민하기, 객체 한 쪽으로 역할 몰아보기, 적절한 객체 탐색하기

순환참조에 대해서 듣기만 했었는데 제 코드에서 직접 보니 이해가 되고 바로 수정하고 싶어지네요😅

우선 작성해주신 방법으로 반영해보았습니다.
BetAmount 가 가지고 있던 수익 결정에 대한 역할을 Incomes로 이동하였습니다!

player -> dealer -> player 를 부르면서 순환참조가 일어난 부분이
Incomes -> dealer -> player의 구조로 바뀌는 것을 확인하였습니다.

이번에 수정을 하면서 어떤 객체가 역할을 맡아야하는지에 대한 기준을 세우는데 인사이트를 얻은 것 같습니다.

sihyung92 · 2024-03-16T02:09:09Z

src/main/java/domain/result/Incomes.java

+    public Map<Player, Income> playersIncomes(Dealer dealer) {
+        return players.stream()
+                .collect(Collectors.toMap(
+                        player -> player,


자기 자신을 반환하는 경우를 위해 Funtion에선 identity()라는 정적 메서드를 제공합니다.

Suggested change

player -> player,

Funtion.identity(),

player라는 것을 명시적으로 보여주는 게 좀 더 가독성이 있다고 생각해서 Function.identity()를 사용하지 않았는데,
정적 메서드를 쓰는 것을 추천하시는 이유가 있을까요?

List의 of 메서드, Objects의 equals 메서드 등등처럼 가급적 SDK에서 제공하는 API를 사용하자는 방향입니다.
of / equals 같은 정적 메서드를 사용할 때 처럼 명백한 이득은 없지만요 ㅎ

sihyung92 · 2024-03-16T02:10:17Z

src/test/java/domain/card/DealerTest.java

+
+        dealer.receive(new Card(2, Shape.CLUB));
+
+        Assertions.assertThat(dealer.decideStatus(player)).isEqualTo(Status.WIN);


Assertions 를 static import할 때도 있고 안 할 때도 있네요. 하는 쪽으로 통일해볼까요?

tkdgur0906 · 2024-03-17T14:21:11Z

수정을 하면서 궁금한 것이 생겨서 질문 드립니다!

필드를 1개 가지고 있는 값 객체인 경우 (ex : BetAmount) 필드의 이름을 지을 때 int betAmount or int value 중 어느 것을 선호하시나요? 그리고 getter의 이름도 필드에 따라가는편이신가요?(ex: getBetAmount() or getValue())
value가 명확하지 않은 것 같다는 크루의 의견도 들은바가 있어, 작성하는 사람의 컨벤션 정도의 느낌인지 궁금합니다!
현재 Player, Dealer가 Card를 상속받고 있는데, 아무래도 Participant를 상속받는 구조가 일반적이고 직관적이라 생각합니다.
의미론적인 부분도 고려해볼 사항이라고 생각하는데, Card를 상속받는 부분에 대해서는 어떻게 생각하시나요?
순환참조를 해결하면서 수익을 계산하는 determineIncome()을 Incomes로 옮겼는데, 이 메서드가 사용되는 곳은 Incomes 내부의 dealerIncome(), playersIncomes() 두 곳 뿐입니다.
따라서 private으로 외부의 호출을 막는 것이 좋다고 생각하는데, 기능적으로 중요한 메서드이기 때문에 단위 테스트도 필요하다고 생각합니다.
현재는 단위 테스트를 위해 public으로 열어두었는데, 웨지는 이런 상황에서 어떤 구조를 택할 것인지 궁금합니다!

sihyung92

안녕하세요 카피! 리뷰어 웨지입니다.
리뷰 반영 잘 해주셨고, 이번엔 질문에만 답변 드리는 방식으로 리뷰를 남겼습니다.
반영해보고 싶은 부분이 있을까 싶어 request change를 보내니 확인해주셔용~~

sihyung92 · 2024-03-17T22:44:07Z

src/main/java/domain/participant/BetAmount.java

+
+public class BetAmount {
+
+    private final int betAmount;


Q1. value 사용?
A. 저는 로컬 변수로 활용할 때 betAmount.getBetAmount() 같은 호출이 어색해서 value 등을 활용하는 편입니다

sihyung92 · 2024-03-17T22:48:43Z

src/main/java/domain/card/Cards.java


-public class Cards {
+public abstract class Cards {


Q2. Participant로 변경
A. 상속은 is-a 관계를 충족해야 한다고들 하는데요, (Dealer is a Cards?) 그런 맥락에서 보면 Cards 보다는 Participants가 어울릴거에요.
객체지향의 is-a 관계에 대해 찾아보시고, 지켜지지 않았을 때 어떤 일이 발생할 수 있을지 상상해보셔요.

is-a 관계를 정의할 때는 A는 B이다라는 is 관계가 명확하게 사용하는 것이 좋을 것 같습니다!
플레이어는 카드다 라는 정의는 읽는 사람으로 하여금 혼동을 줄 수 있을 것이라 생각합니다!

네넵!
개인적으론 is-a 관계가 충족되지 않으면 잠재적인 오류 가능성이 발생한다고 생각합니다.
당장 필요하지는 않더라도 향후에 상속을 통해 기능이 추가될 수 있죠,
이 때 다형성을 활용한 코드에 새롭게 상속된 객체가 활용될 수 있는데 기존 코드의 역할 (is-a 였다면 똑같이 수행했을텐데, 그렇지 않다면 오동작 할 수 있음)을 다하지 못하면 오류가 발생할 것이기 때문에 상속에선 is-a 관계를 충족해주는 게 중요합니다.

sihyung92 · 2024-03-17T22:53:37Z

src/main/java/domain/result/Incomes.java

+    public Income determineIncome(Status status, BetAmount betAmount) {
+        if (status == Status.WIN) {
+            return new Income(betAmount.getBetAmount());
+        }
+        if (status == Status.LOSE) {
+            return new Income(-betAmount.getBetAmount());
+        }
+        if (status == Status.WIN_BLACKJACK) {
+            return new Income((int) Math.round(1.5 * betAmount.getBetAmount()));
+        }
+        return new Income(0);
+    }


Q3. 테스트를 위한 접근제어자 변경
A. public API의 제공은 '영원히 유지보수해야됨'임을 고려해주세요. 단위 테스트가 어려운 코드는 분리의 신호이고, 단위 테스트가 용이하게 만들다보면 객체지향이 추구하는 방향과 맞아지기도 합니다.

현업에선 저희가 오픈소스 개발자가 아니라 특정 회사 안에서만 관리되는 코드를 만지기 때문에 접근제어자가 ☝️ 바로 위에서 말씀드린 것처럼 중요하진 않습니다 (나 말고 재활용 할 사람이 잘 없음)
그러나 단위 테스트가 용이하게 만들다보면 객체지향이 추구하는 방향과 맞아짐 이라는 제언은 유효하기 때문에 public으로 전환하는 것보다 객체 분리도 고려해보셔요.

해당 메서드를 IncomeCalculator라는 클래스가 가지도록 수정했습니다!
신경쓰였던 접근제어자 문제 뿐만 아니라 단위테스트도 훨씬 용이해지는 것을 확인할 수 있었습니다.
SRP에 대해서 저만의 기준이 잡혀가는 것 같아 기분이 좋네요 ㅎㅎ 감사합니다!

sihyung92

안녕하세요 카피~~ 리뷰어 웨지입니다!
2단계 잘 구현해주셔서 여기서 머지할게요 ^_^
2단계 때 만났지만 좋은 경험으로 가져가셨으면 좋겠네요.
앞으로도 화이팅입니당!

tkdgur0906 added 24 commits March 11, 2024 16:06

docs(README) : 요구사항 문서 추가 작성

e6f0b2b

feat(BetAmount) : 게임 결과에 따라 수익을 결정하는 기능

f0868b5

feat(InputView) : 플레이어의 배팅 금액 입력받는 기능 구현

2854793

feat(Incomes) : 딜러의 최종 수익 반환하는 기능

08f0209

feat(Rule) : Rule이 ScoreBoard 대신 Incomes를 가지도록 변경

99b5d2c

refactor(ScoreBoard) : Scoreboard 제거

1232f1b

refactor(BetAmount, PlayerCards) : 메소드 명 변경

f53e32d

feat(OutputView) : 최종 수익 출력

0b03744

feat(Casino) : 최종 수익 출력 로직 반영

eb6905f

refactor(View) : 출력 포맷팅

aa94321

refactor(View) : View 메서드 static 처리

50957c6

refactor(Rule) : 상태 판별 조건 메서드 분리

262c714

refactor(View) : View 출력 형식에 맞게 수정

1be8a56

refactor(Cards) : 매직넘버 상수 처리

823b938

refactor(Cards) : 미사용 코드 제거

1a30dad

refactor(OutputView) : 메서드 분리

191b424

feat(Rule) : 플레이어가 블랙잭일 경우 블랙잭 상태 반환

ff88bf3

refactor(Rule) : 메서드 분리

3eec762

refactor(Casino) : 메서드 분리

670639f

docs(README) : 구현 기능 목록 체크

5ce502e

docs(README) : 구현 기능 목록 체크

ca79356

refactor(ALL) : 패키지 구조 분리

4e2ba03

Merge branch 'step2' of https://github.com/tkdgur0906/java-blackjack …

e89c190

…into step2

fix(Rule) : 상태 판별 로직 burst도 정상적으로 작동하도록 수정

016f4ae

sihyung92 requested changes Mar 12, 2024

View reviewed changes

tkdgur0906 added 5 commits March 13, 2024 11:33

refactor(BlackjackGameManager) : Casino 클래스를 BlackjackGameManager로 이름 변경

2d32103

refactor : toString 대신 asString으로 클래스의 string 반환

3480e80

feat : 도메인 객체에 equals, hashcode, toString 재정의

2c89fa0

refactor(Deck) : deck 생성 방식 for문 사용

79785bf

refactor(Deck) : 덱을 랜덤 생성 대신 생성 후 섞는 방식으로 변경

8027037

tkdgur0906 added 4 commits March 14, 2024 15:48

refactor(Cards) : 점수 계산 메서드 이름 명확하게 수정

82958f1

refactor(BlackjackGameManager) : 불필요 코드 삭제

f5422c7

docs(README) : 구현 기능 목록에 예외 케이스 정의

38b4d0f

refactor(PlayerCards, DealerCards) : 클래스 이름 Player, Dealer로 명확하게 변경

fe35a94

sihyung92 requested changes Mar 15, 2024

View reviewed changes

tkdgur0906 added 10 commits March 15, 2024 16:58

refactor(ALL) : 전체 코드 포맷팅

bab57ac

refactor(InputView) : final을 통해 제약 부여

6d7399f

refactor(Dealer) : 첫번째 카드 반환 시 객체 반환

56f12c3

refactor(Shape) : 미사용 로직 제거

381db67

refactor(CardNumber) : 파라미터 명 의미에 맞게 수정

f94863e

refactor(BetAmount, Name) : 클래스 패키지 이동

bb2573f

feat(Dealer) : Dealer가 플레이어의 게임 결과를 반환하도록 기능 추가

dcfb7f6

feat(Incomes) : Incomes가 모든 플레이어의 수익 반환하는 기능 추가

37eb921

refactor(BlackjackGameManager, OutputView) : 변경된 기능에 맞게 메서드 시그니처 수정

b5fe28b

feat(Incomes) : 플레이어들의 수익 반환할 때 플레이어 순서 보장하도록 수정

343a2d6

sihyung92 requested changes Mar 16, 2024

View reviewed changes

tkdgur0906 added 3 commits March 17, 2024 22:17

style : static import

c7a6457

feat : 딜러의 첫번째 카드 조회 시 존재하지 않는 경우 예외처리

c86f59e

refactor : 수익 결정하는 책임 BetAmount에서 Incomes로 전달

1a7c208

sihyung92 requested changes Mar 17, 2024

View reviewed changes

tkdgur0906 added 6 commits March 18, 2024 12:15

refactor : 자기 자신 반환하는 람다 SDK API 사용

a3686f1

refactor : 클래스 명과 동일한 멤버 변수명 value로 수정

6e705a5

refactor : Cards 클래스의 이름을 Participant로 변경

afa7f99

refactor : 수익 결정하는 메서드 클래스 분리

cec92ba

refactor : IncomeCalculator 유틸 클래스로 변경

415fa64

refactor : 패키지 분리

602f7d8

sihyung92 approved these changes Mar 18, 2024

View reviewed changes

sihyung92 merged commit e3231d1 into woowacourse:tkdgur0906 Mar 18, 2024

tkdgur0906 changed the title ~~[2단계 - 블랙잭 베팅] 카피 미션 제출합니다.~~ [2단계 - 블랙잭 베팅] 카피(김상혁) 미션 제출합니다. Sep 16, 2024


		public class Incomes {

		private final Map<Name, Income> incomes;

	private final Map<Name, Income> incomes;
	private final Map<Player, Income> incomes;

	private final Map<Name, Income> incomes;
	private final List<Player> players;


		private Scanner scanner = new Scanner(System.in);
		private static Scanner scanner = new Scanner(System.in);


		dealer.receive(new Card(2, Shape.CLUB));

		Assertions.assertThat(dealer.decideStatus(player)).isEqualTo(Status.WIN);

[2단계 - 블랙잭 베팅] 카피(김상혁) 미션 제출합니다. #679

[2단계 - 블랙잭 베팅] 카피(김상혁) 미션 제출합니다. #679

Conversation

tkdgur0906 commented Mar 12, 2024 • edited Loading

sihyung92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdgur0906 Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdgur0906 Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

sihyung92 Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

sihyung92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdgur0906 Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sihyung92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sihyung92 Mar 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdgur0906 commented Mar 17, 2024

sihyung92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdgur0906 Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

sihyung92 left a comment

Choose a reason for hiding this comment

tkdgur0906 commented Mar 12, 2024 •

edited

Loading

tkdgur0906 Mar 13, 2024 •

edited

Loading

tkdgur0906 Mar 14, 2024 •

edited

Loading

sihyung92 Mar 15, 2024 •

edited

Loading

tkdgur0906 Mar 15, 2024 •

edited

Loading

sihyung92 Mar 17, 2024 •

edited

Loading

tkdgur0906 Mar 18, 2024 •

edited

Loading