[🚀 사이클2 - 미션 (블랙잭 게임 실행)] 코코 미션 제출합니다.#1116
[🚀 사이클2 - 미션 (블랙잭 게임 실행)] 코코 미션 제출합니다.#1116yejinkimis wants to merge 25 commits intowoowacourse:yejinkimisfrom
Conversation
| public Dealer(String name) { | ||
| super(name); | ||
| public static final String DEALER_NAME = "딜러"; | ||
| public static final int DEALER_REFERENCE_POINT = 16; |
There was a problem hiding this comment.
- 상수의 응집도
저번 피드백을 바탕으로 Constants 클래스에 모여있던 상수를 각 상수가 실제로 사용되는 도메인과 뷰 내부로 이동시켜 응집도를 높였습니다. 다만, 추후에 여러 클래스에서 공통으로 쓰이는 구분자 등의 값을 매번 중복해서 정의하는 것이 맞는지, 그때가서 공용 관리 상수로 빼는 게 맞는 지 궁금합니다!!
예를 들어서 "," 와 같은 단순 구분자가 여러 곳에서 중복되는 경우를 생각해보면, 처음에는 Util로 공통 관리하는 것이 효율적이라 생각했으나, 그 상수가 각 도메인마다 쓰이는 의미가 다르고 추후 변경 요구사항이 발생했을 때 서로 영향을 주지 않아야 한다고 생각하면 중복해서 선언을 해야할 것 같기도 합니다. 이런 경우는 어떻게 상수를 관리하면 좋을까요?
넵 각 도메인 로직에서 쓰이는 상수 값들을 잘 분배해주셨네요 👍
"," 와 같은 구분자는 도메인 로직이 맞을까요~? 입력받는 문자열의 구분자가 어떤 것인지는 도메인의 관심사로 보기 어려운 것 같아요. 오히려 view 영역의 관심사로 보이고 이런 경우는 view 단에 선언되는 것이 더 맞지 않나 생각이 듭니다!
| @@ -58,11 +63,7 @@ public List<Gambler> getGamblers() { | |||
| return gamblers; | |||
| } | |||
| import java.util.Map; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class Betting { |
There was a problem hiding this comment.
- Gambler와 BettingAmount의 관계
"베팅하는 행위" 를 따로 표현하고 싶어서 Betting클래스를 따로 뒀습니다. 그런데 Gambler도 베팅한 금액을 알아야 하는 게 맞다고 생각되어서 Gambler에 베팅한 금액을 생성 시 인자로 넣어 관리하게 되었습니다.
지금은 Betting 클래스에서 이름에 따른 베팅 금액을 입력받고 임시로 들고있다가 베팅이 다 끝나면 Gambler객체를 만드는 방식을 사용하고 있습니다. 근데 이렇게 하면 Gambler가 생기기 전까지 임시로 Map과 같은 곳에 저장해둬야 합니다.
객체를 생성하기 위해 외부에서 모든 데이터를 수집한 뒤 주입하는 방식(완성된 객체 생성)과 객체를 먼저 생성하고 이후 상태를 채워나가는 방식 사이에서 어떤 것이 더 견고한 로직인지 궁금합니다!
좋은 고민이네요 👍 저는 가능하면 "객체를 생성하기 위해 외부에서 모든 데이터를 수집한 뒤 주입하는 방식" 을 선호하긴 하는데요, 그 이유는 객체를 생성한 후 상태변경을 최소화 하기 위함입니다! 완성된 객체를 생성했기 떄문에 Gambler 의 bettingAmount 을 불변으로 선언할 수 있었죠 ㅎㅎ
다만 저는 오히려 Betting을 굳이 도메인 객체로 빼서 둘 필요가 있나?가 고민이 되네요!, 현재 해당 클래스가 하고 있는 중복 이름 검증은 오히려 Gamblers 일급 컬렉션에서 진행 가능해보이고, Map<String, BettingAmount> 별로 겜블러를 만드는 책임은 컨트롤러나 혹은 도메인이 아닌 별도 팩터리 클래스로 두어도 괜찮아보이긴 합니다!
| public Gamblers(Map<String, BettingAmount> gamblerNameAndBettingInfo) { | ||
| this.gamblers = gamblerNameAndBettingInfo.entrySet() | ||
| .stream() | ||
| .map(entry -> new Gambler(entry.getKey(), entry.getValue())) | ||
| .collect(Collectors.toList()); | ||
| } |
There was a problem hiding this comment.
- 테스트 용이성을 위한 생성자 오버라이딩
테스트 코드 작성을 위해 실제 로직에서는 사용하지 않는 생성자를 추가하는 경우가 있었습니다. 예를 들어 가공된 Map을 인자로 받는 생성자에 넣을 Map을 만드는 것이 너무 복잡하여서 그 테스트에서만 필요한 정보를 List 로 만들어 인자로 넣고싶어 지는 경우가 있었습니다.
테스트만을 위해 프로덕션 코드를 수정하는, 제 경우에는 테스트 용이성만을 위해 생성자를 추가하는 행위에 대해 리뷰어님은 어떻게 생각하시는지 궁금합니다!
코코가 생각하는 생성자의 책임은 무엇인가요~? 현재 map 을 받는 생성자는 Gambler객체를 생성하고, 컬렉션을 구성하는 등 복잡한 로직이 들어가있어요. 그런데 Gamblers의 본질적인 역할은 이미 존재하는 Gambler들을 묶어서 관리하는 것이지, Gambler를 직접 만들어내는 것은 아니라고 생각되어요.
말씀해주신 애용은 Gamblers가 어떤 형태로 생성되어야 자연스러운지를 테스트가 알려준 거라고 생각이 들기도 하네요! 오히려 List<Gambler>를 받는 생성자만 남기고, Map으로부터 Gambler들을 만드는 책임은 Gamblers를 사용하는 쪽이나 별도의 정적 팩토리 메서드로 분리하는 방향이 더 깔끔하지 않을까 싶어요 ㅎㅎ
일반적으로 테스트만을 위해 프로덕션 코드를 수정하는 건 지양하는 게 맞지만, 테스트가 불편하다는 건 종종 설계가 보내는 신호이기도 합니다! 이번 경우처럼 테스트를 위해 추가한 생성자가 객체의 역할에 더 부합한다면, 그건 테스트 덕분에 더 나은 설계를 발견한 거라고 봐도 좋을 것 같아요 :)
|
|
||
| outputView.printInitialDeal(names); | ||
| private Game initializeGame(Map<String, BettingAmount> gamblerNameAndBettingInfo) { | ||
| Game game = new Game(Dealer.DEALER_NAME, gamblerNameAndBettingInfo, |
There was a problem hiding this comment.
딜러가 알고 있는 DEALER_NAME 을 외부에서 알려줄 필요가 있을까요~?
|
|
||
| public class BettingAmount { | ||
|
|
||
| private final int bettingAmount; |
There was a problem hiding this comment.
int, double, BigInteger 각각에 대해 알아보고 돈과 관련된 연산에는 어떤 자료구조가 적합한지 고민해보면 좋겠네요 ㅎㅎ
| int totalProfit = participantProfits.values().stream() | ||
| .mapToInt(Profit::getProfit) | ||
| .sum(); | ||
| return new Profit(totalProfit * REVERSE_SIGN); |
There was a problem hiding this comment.
Profit 의 값을 외부에서 꺼내서 연산하지 않고 Profit 스스로 연산한 값을 반환하게 할 수 있지 않을까요~?
| class GameResultTest { | ||
|
|
||
| @Test | ||
| @DisplayName("게임 최종 결과 딜러, 겜블러 동시에 블랙잭") |
There was a problem hiding this comment.
여기도 given when then 절로 정리되면 더 가독성이 좋을 것 같아요 :)
| @Test | ||
| @DisplayName("승패 결과 저장 검증") | ||
| void 승패_결과_저장_검증() { | ||
| @DisplayName("딜러, 참여자들의 수익을 정산") |
There was a problem hiding this comment.
현재 한가지 케이스만 테스트하고 있는데 중요한 로직이니 여러 테스트 케이스를 보강해 볼 수 있을까요~?
| }); | ||
| } | ||
|
|
||
| } No newline at end of file |
There was a problem hiding this comment.
개행을 항상 신경쓰시죠!
그리고 인텔리제이에서 해당 레벨에서 Cmd + Option + L 을 할 경우 전체 포맷팅이 적용됩니다~!
2 participants
안녕하세요! 코코입니다! 🙇🏻♀️ 너무 늦게 리뷰요청은 드리게 되어 죄송합니다.
기능 추가에 시간을 많이 써서 저번에 피드백 받은 부분은 완벽히 적용시키지는 못 했습니다.
우선 리뷰요청 먼저 드리고 계속해서 리팩토링 할 예정입니다...! 정말 감사합니다 🥹
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?어떤 부분에 집중하여 리뷰해야 할까요?
1. 상수의 응집도
저번 피드백을 바탕으로 Constants 클래스에 모여있던 상수를 각 상수가 실제로 사용되는 도메인과 뷰 내부로 이동시켜 응집도를 높였습니다. 다만, 추후에 여러 클래스에서 공통으로 쓰이는 구분자 등의 값을 매번 중복해서 정의하는 것이 맞는지, 그때가서 공용 관리 상수로 빼는 게 맞는 지 궁금합니다!!
예를 들어서 "," 와 같은 단순 구분자가 여러 곳에서 중복되는 경우를 생각해보면, 처음에는 Util로 공통 관리하는 것이 효율적이라 생각했으나, 그 상수가 각 도메인마다 쓰이는 의미가 다르고 추후 변경 요구사항이 발생했을 때 서로 영향을 주지 않아야 한다고 생각하면 중복해서 선언을 해야할 것 같기도 합니다. 이런 경우는 어떻게 상수를 관리하면 좋을까요?
2. Gambler와 BettingAmount의 관계
"베팅하는 행위" 를 따로 표현하고 싶어서 Betting클래스를 따로 뒀습니다. 그런데 Gambler도 베팅한 금액을 알아야 하는 게 맞다고 생각되어서 Gambler에 베팅한 금액을 생성 시 인자로 넣어 관리하게 되었습니다.
지금은 Betting 클래스에서 이름에 따른 베팅 금액을 입력받고 임시로 들고있다가 베팅이 다 끝나면 Gambler객체를 만드는 방식을 사용하고 있습니다. 근데 이렇게 하면 Gambler가 생기기 전까지 임시로 Map과 같은 곳에 저장해둬야 합니다.
객체를 생성하기 위해 외부에서 모든 데이터를 수집한 뒤 주입하는 방식(완성된 객체 생성)과 객체를 먼저 생성하고 이후 상태를 채워나가는 방식 사이에서 어떤 것이 더 견고한 로직인지 궁금합니다!
3. 테스트 용이성을 위한 생성자 오버라이딩
테스트 코드 작성을 위해 실제 로직에서는 사용하지 않는 생성자를 추가하는 경우가 있었습니다. 예를 들어 가공된 Map을 인자로 받는 생성자에 넣을 Map을 만드는 것이 너무 복잡하여서 그 테스트에서만 필요한 정보를 List 로 만들어 인자로 넣고싶어 지는 경우가 있었습니다.
테스트만을 위해 프로덕션 코드를 수정하는, 제 경우에는 테스트 용이성만을 위해 생성자를 추가하는 행위에 대해 리뷰어님은 어떻게 생각하시는지 궁금합니다!
감사합니다☺️