[🚀 사이클2 - 미션 (블랙잭 게임 실행)] 정콩이 미션 제출합니다.#1126
[🚀 사이클2 - 미션 (블랙잭 게임 실행)] 정콩이 미션 제출합니다.#1126xxbeann wants to merge 54 commits intowoowacourse:xxbeannfrom
Conversation
players일급 컬렉션을 도입해 도메인 객체 스스로 검증하게 구현했습니다. input view 에서 도메인 규칙을 모르게끔 리팩토링하였고, gameManager에 흩어져 있던 검증로직을 관련 도메인으로 끌어왔습니다.
- inputView 테스트를 위해 기존에 쓰던 inputStream을 제거하고, validator를 분리 이를 테스트 했습니다. - 기존 exception의 메시지를 테스트 하던 방식을 예외 타입 검증으로 변경했습니다. 추후 커스텀 예외를 통해 메시지 타입까지 비교할 예정입니다. - 기존에 있던 DisplayName과 영어 메서드명을 한글 메서드명으로 변경했습니다.
- Players 일급 컬렉션을 생성한 후 순회하는 로직을 구현하기 위해 일급 컬렉션 안에 forEach 순회 메서드를 만들었습니다. 이는, player 리스트의 forEach를 위임한 역할입니다. - 처음에는 getPlayers를 통해 순회를 해결하려 했으나, 그럴 경우 캡슐화가 약해져 외부가 Players내부 표현 방식에 의존한다는 걸 알았습니다. 외부가 players 내부 구현을 알 경우 변경에 취약하고, 책임이 새어나가 확장에 불리하다 판단해 forEach를 도입했습니다. - 실제로 리팩터링 과정 중 눈에 띄게 가독성이 향상됨을 체감했습니다.
- Cards를 일급 컬렉션으로 생성후, 이 Cards를 ParticipantCards와 Deck이 필드로 갖게 했습니다. 기존 저는 card는 단일 1장, cards는 card의 복수형, Deck은 cards의 특수한 상황(52장 + shuffle)으로 생각했습니다. cards가 일급 컬렉션의 의도였으나 changeAvailableAceCount필드를 추가하게 되면서 player가 들고 있는 Cards로 일급 컬렉션이 전락했습니다. 영이의 말대로 Deck과 Cards가 각각 카드 리스트를 가지는 일급 컬렉션의 구조를 생각해 보았으나, 저는 순수하게 카드 리스트를 가지는 일급 컬렉션 cards를 고려했고. 이를 다시 Deck과 ParticipantCards에서 사용되게 했습니다. 기존에 있던 Cards를 ParticipantCards로 메서드 명을 바꾸면서 최대한 전체 구조에 수정을 줄이고자 했습니다. 실제로 순수한 일급 컬렉션 Cards를 도입해보니 ParticipantCards와 Cards의 책임이 어느정도 분리됨을 확인할 수 있었습니다. 다만, 영이의 설계와 제가 한 설계중 뭐가 더 나은 방향인지 궁금합니다! 제가 처음에 ParticipantCards에 getSize등의 메서드 들이 있었다가, 안써서 지웠는데, 굳이 꼽자면 나중에 확장성이 있겠다...정도...?
- BettingManager 도입으로 참가자별 배팅 금액 관리 - BettingAmount에 0원/승리 배팅 금액 계산 로직 추가 - Participant의 이름 반환 타입을 Name 객체로 변경 - 배팅 금액 관련 단위 테스트 추가
- Participant에 블랙잭 판정 로직 추가 - ParticipantCards에 카드 개수 조회 기능 추가 - BettingManager에 블랙잭 승리 정산 기능 추가 - BettingAmount에 1.5배 계산 로직 추가 - 블랙잭 및 배팅 정산 관련 테스트 보강
- BettingAmount와 BettingManager에서 정산 관련 메서드 제거 - GameResult에 BLACKJACK 결과 추가 - Player에 블랙잭 결과 판정 로직 반영 - 수익 계산용 CalculateProfit, Revenue 도메인 추가 준비
- CalculateProfit과 GameResultManager로 참가자 수익 계산 로직 추가 - BettingAmount와 Revenue를 값 객체로 보강 - RevenueManager 도입으로 수익 집계 기반 마련 - 테스트 패키지를 도메인 구조에 맞게 재배치 - CalculateProfit 테스트 추가 및 기존 배팅 테스트 정리
- BlackJackGameController에서 BettingManager 초기화 로직 추가 - GameManager의 결과 조회 책임을 GameResultManager로 이동 - GameResultManager에 플레이어/딜러 기반 결과 계산 로직 추가 - GameResultManager 테스트 추가 및 기존 GameManager 테스트 정리
- ParticipantRevenueDto 추가 - 컨트롤러에서 도메인 수익 결과를 DTO로 변환하도록 변경 - GameResultManager에서 참가자 및 딜러 수익 집계 로직 추가 - OutputView에 최종 수익 출력 기능 추가 - Dealer 이름을 고정값으로 관리하도록 정리
src/main/java/view/InputView.java
Outdated
| System.out.println(); | ||
| System.out.println(player + "의 배팅 금액은?"); | ||
| Scanner sc = new Scanner(System.in); | ||
| Integer input = sc.nextInt(); |
There was a problem hiding this comment.
askBettingAmount()에서 입력을 처음에는 String으로 받아 문자열 검증 후 int로 변환하는 방향을 생각했습니다.
다만 Scanner.nextInt()를 사용하면 숫자가 아닌 입력에 대해 기본적인 검증 효과를 바로 얻을 수 있어서,
구현 복잡도를 줄이기 위해 현재는 nextInt()를 사용하는 방식으로 변경했습니다.
이 경우 입력 계층에서 별도의 문자열 검증 로직을 두지 않아도 된다는 장점이 있다고 봤는데,
이런 상황에서 String -> 검증 -> 변환 흐름을 유지하는 편이 더 낫다고 보시는지,
아니면 nextInt()를 활용해 입력 단계의 복잡도를 줄이는 방향도 괜찮다고 보시는지 의견이 궁금합니다.
There was a problem hiding this comment.
nextInt를 사용하고 숫자가 아닌 문자를 입력했을때
어떤 에러메세지가 보이던가요??
사용자가 쉽게 이해하기 좋은 메세지가 내려오는지를 판단해보면 좋을것 같습니다
There was a problem hiding this comment.
pobi의 배팅 금액은?
Exception in thread "main" java.util.InputMismatchException
at java.base/java.util.Scanner.throwFor(Scanner.java:947)
at java.base/java.util.Scanner.next(Scanner.java:1602)
at java.base/java.util.Scanner.nextInt(Scanner.java:2267)
at java.base/java.util.Scanner.nextInt(Scanner.java:2221)
at view.InputView.askBettingAmount(InputView.java:30)
at controller.BlackJackGameController.lambda$initBettingManager$0(BlackJackGameController.java:52)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
at domain.participant.Players.forEach(Players.java:18)
at controller.BlackJackGameController.initBettingManager(BlackJackGameController.java:51)
at controller.BlackJackGameController.run(BlackJackGameController.java:36)
at Application.main(Application.java:6)
> Task :Application.main() FAILED
확인해보니 현재는 숫자가 아닌 값을 입력했을 때 InputMismatchException이 발생합니다.
현재 askBettingAmount()는 nextInt()를 사용하고 있어서 정수 입력만 허용합니다.
즉 지금 구조에서는 소수 금액 입력은 허용되지 않습니다.
제 생각에는 한국 화폐 기준의 정수 금액만 우선 고려하고 있어서, 이 부분이 당장 큰 문제는 아니라고 생각했습니다.
한편 내부 돈 계산은 BigDecimal을 사용하고 있어서,
예를 들어 홀수 금액 배팅 후 블랙잭이 된 경우처럼 소수 수익이 발생하는 상황은 계산할 수 있도록 두었습니다.
즉 현재는 "입력은 정수", "계산 결과는 소수 가능"한 구조로 이해하고 있습니다.
There was a problem hiding this comment.
넵 이 에러를 개발자가 아닌 일반 사용자들이 쉽게 이해할 수 있을까요?
실제 앱이나 웹사이트에서 이런 에러를 봤을때 어떤 느낌이 들었었나요?
여기서 바로 적용해보는 걸 요청드리는건 아니지만
보통은 정의된 에러가 아닌 경우가 발생하는것을 최대한 피해야 합니다
지금 단계에서는
- string을 받고 숫자인지를 체크하여 원하는 에러와 메세지를 함께 내려주는 방법
- nextInt를 사용하더라도 try catch를 통해서 원하는 에러와 메세지를 함께 내려주는 방법
두가지 방법을통해서 사용자가 왜 에러가 발생하는지 명확하게 이해할 수 있도록 해줘야할것 같아요.
혹은 콘솔로 하는 게임이기 때문에 try catch하고 system.out.println 에 메세지만 잘 적어줘도 사용성이 증가할것 같네요
choijy1705
left a comment
choijy1705
left a comment
There was a problem hiding this comment.
안녕하세요 정콩이
현재 핵심 요구사항이 TODO로 적혀있고 구현이 안되어 있는것 같습니다
몇가지 코멘트를 남기긴했는데 같이 고민해보시면서
핵심 요구사항을 구현하여 TODO를 없애고 다시 리뷰요청 주시면 좋을것 같습니다!
| private void distributeCardToDealer(Dealer dealer) { | ||
| distributeInitialCards(dealer); | ||
| } |
There was a problem hiding this comment.
이것도 굳이 메서드 분리를 할필요가 있을지 고민해보면 좋을것 같습니다
There was a problem hiding this comment.
저는 그래도 Dealer에게 초기 카드를 분배한다는 의도를 더 담고 싶어서 distributeCardToPlayers와 동일하게 한 번 더 감쌌던 건데, 굳이 안 그래도 의미가 잘 전달이 되나요?? 지금은 아래와 같이 리팩토링한 상태입니다!
public void distributeInitialCards() {
distributeInitialCards(dealer);
distributeCardToPlayers(players);
}
private void distributeCardToPlayers(Players players) {
players.forEach(this::distributeInitialCards);
}
private void distributeInitialCards(Participant participant) {
drawCardTo(participant);
drawCardTo(participant);
}
|
영이 안녕하세요! ✨ |
choijy1705
left a comment
choijy1705
left a comment
There was a problem hiding this comment.
정콩이 빠르게 반영해주셨네요
질문 주신내용과 추가적인 코멘트 남겼으니
확인해보시고 한번더 피드백 반영부탁드려요!
| package view; | ||
|
|
||
| public class InputValidator { | ||
| public static void validateContinueResponse(String input) { | ||
| if (!input.matches("[yn]")) { | ||
| throw new IllegalArgumentException("응답은 y와 n만 허용됩니다."); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
흠 저도 깊게 고민해보지 못한 포인트네요
제 관점에서는 view는 사실 프론트의 영역이고 백엔드는 도메인 테스트에 집중하자가 포인트였습니다.
보통은 System.setIn() 과 같은 코드까지 짜면서 테스트를 하는 케이스를 본적이 없어서 익숙하지 않음에 리뷰 드렸습니다.
위와 같은 문제도 충분히 발생할수 있을것 같네요! 학습하신 내용으로도 충분히 이런 테스트는 하지 않는 방향이 좋을것 같습니다!
src/main/java/view/InputView.java
Outdated
| System.out.println(); | ||
| System.out.println(player + "의 배팅 금액은?"); | ||
| Scanner sc = new Scanner(System.in); | ||
| Integer input = sc.nextInt(); |
There was a problem hiding this comment.
nextInt를 사용하고 숫자가 아닌 문자를 입력했을때
어떤 에러메세지가 보이던가요??
사용자가 쉽게 이해하기 좋은 메세지가 내려오는지를 판단해보면 좋을것 같습니다
| void 참가자가_승리하면_수익은_배팅금액만큼_증가한다() { | ||
| Name name = new Name("pobi"); | ||
| Player player = new Player(name); | ||
| Map<Name, BettingAmount> amountMap = new HashMap<>(); | ||
| amountMap.put(player.getName(), new BettingAmount(1000)); | ||
| BettingAmounts bettingAmounts = new BettingAmounts(amountMap); | ||
| CalculateProfit calculateProfit = new CalculateProfit(bettingAmounts); | ||
| int expected = 1000; | ||
| assertEquals(expected, calculateProfit.calculate(name, GameResult.WIN).getMoney()); | ||
| } |
There was a problem hiding this comment.
작은 단위에서 테스트가 된것들은
상위 레벨에서는 과감하게 테스트 하지 않아도 된다고 생각합니다.
추가적으로요구사항이 메서드의 길이를 10줄 이하로 하라는것 때문에 개행없이 이렇게 처리하신걸까요??
가독성이 많이 떨어지는것 같습니다!
10줄이하는 가독성을 올리기 위해서인데 거기에 매몰되어 오히려 가독성을 해치는게 아닐지 고민해보면 좋을것 같습니다
또 테스트에서 중복이 많은 부분들을 어떻게 처리해 코드를 줄일수 있을지도 고민해보면 좋겠네요
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class NameTest { | ||
| // TODO: 나중에 커스텀 예외로 변경. 현재는 예외가 터지는 것만 검증됨. |
There was a problem hiding this comment.
커스텀 예외는 정말 특수한 예외인경우에 대해서만 만드는 편인것 같아요.
프론트에서 별도로 Exception 타입을 보고 처리가 필요한 경우요
실제 대부부의 회사들은 그냥 input으로 잘못들어온건 프론트에서 별다른 처리 없이(여기서는 view) 사용자에게 메세지만 잘 전달되면 되기 때문에 하나의 에러로 간주해서 처리할것 같습니다!
- BettingAmount와 Revenue의 money 타입을 BigDecimal로 변경 - CalculateProfit의 수익 계산을 BigDecimal 기반으로 리팩터링 - 블랙잭 배당 및 손실 계산에서 캐스팅/부동소수점 의존 제거
- 컨트롤러의 배팅 금액 생성 로직을 BigDecimal 기반으로 수정 - GameResultManager의 딜러 수익 합산을 BigDecimal 연산으로 변경 - ParticipantRevenueDto의 수익 타입을 BigDecimal로 변경 - 배팅/수익 관련 테스트를 BigDecimal compareTo 방식으로 정리
There was a problem hiding this comment.
영이 안녕하세요 🙇🏻♀️
조금 오래걸렸네요 ㅜㅜ
추가 리뷰 반영 및 리팩터링을 진행했습니다!! 주요 수정사항은 아래를 확인해주세요~
(선택미션은 제가 추후에 부족한 부분 학습 계획과 리팩토링 할 부분 추후 공유 드리겠습니다!!)
1. Players로 컬렉션 관련 책임 이동
- 플레이어 이름 목록 조회 메서드 추가
- 플레이어별 게임 결과 계산 메서드 추가
- 플레이어별 수익 계산 메서드 추가
GameResultManager에서 직접 순회하던 일부 로직을Players로 위임
2. 수익 계산 구조 정리
BettingAmount,Revenue의 금액 타입을BigDecimal로 변경CalculateProfit의 수익 계산을BigDecimal기반으로 리팩터링- 딜러 수익 합산도
BigDecimal연산으로 변경 ParticipantRevenueDto의 수익 타입을BigDecimal에 맞게 수정- 블랙잭 수익 계산 테스트를 보강하고,
BigDecimal비교는compareTo방식으로 정리
3. 수익 배율 정책 이동
- 기존
CalculateProfit내부 분기 로직에 있던 수익 배율 정책을GameResultenum으로 이동 - 결과 타입이 수익 배율을 직접 가지도록 변경해 계산 로직을 단순화
4. 카드 컬렉션 구조 단순화
Cards클래스를 제거Deck이 직접 카드 리스트를 관리하도록 변경ParticipantCards도 카드 목록 기반으로 점수를 계산하도록 리팩터링- 에이스 개수 캐시 상태(
changeAvailableAceCount) 제거
5. 입력 및 검증 관련 정리
InputView에서Scanner를 메서드마다 생성하지 않고 재사용하도록 변경- 중복 이름 검사는 자료구조(
Set)를 활용하는 방식으로 리팩터링
6. 테스트 코드 정리
- 반복되는 준비 코드를 메서드로 분리
- 블랙잭 수익 계산 케이스를 추가
- 관련 리팩터링에 맞춰 테스트 코드도 함께 정리
7. 기타
- 불필요한 주석 제거
| package view; | ||
|
|
||
| public class InputValidator { | ||
| public static void validateContinueResponse(String input) { | ||
| if (!input.matches("[yn]")) { | ||
| throw new IllegalArgumentException("응답은 y와 n만 허용됩니다."); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
아하, 말씀 듣고 보니 view를 프론트 영역으로 봐야 한다는 관점을 잠시 놓치고 있었던 것 같습니다.
저는 "제가 작성한 코드는 처음부터 끝까지 다 검증해봐야 한다"는 생각이 강해서,
InputView까지 테스트하는 흐름도 크게 어색하게 느끼지 못했던 것 같습니다.
특히 입력값은 도메인뿐 아니라 프론트 단에서 먼저 차단하는 것도 중요하다고 생각해서 더 그랬던 것 같습니다.
다만 현재 과제 맥락에서는 view 테스트보다는 도메인과 검증 로직에 집중하는 편이 더 적절하겠다는 점이 이해됐습니다.
말씀해주신 방향과 System.setIn() 관련 위험 요소까지 고려해서,
앞으로는 입력 스트림 기반 테스트는 지양하는 쪽으로 가져가보겠습니다.
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class NameTest { | ||
| // TODO: 나중에 커스텀 예외로 변경. 현재는 예외가 터지는 것만 검증됨. |
There was a problem hiding this comment.
아하, 말씀해주신 내용을 보니 커스텀 예외는 단순히 의미 구분을 위해 늘리기보다는,
프론트에서 예외 타입별로 별도 처리가 필요한 경우에 더 많이 쓰는 것 같다는 점이 이해됐습니다!!
저는 이전 사이드 프로젝트에서는 커스텀 예외를 써보면서
어떤 종류의 오류인지 빠르게 구분할 수 있어 디버깅이 편하다고 느꼈던 적이 있습니다.
아래 처럼 썼던 경험이 있어서요..!
public enum CommonErrorType implements ErrorType{
METHOD_ARGUMENT_NOT_VALID_EXCEPTION("COMMON400_001", HttpStatus.BAD_REQUEST, "RequestDTO 유효성 검사 미통과"),
ILLEGAL_ARGUMENT_EXCEPTION("COMMON400_002", HttpStatus.BAD_REQUEST, "Illegal argument exception 발생 "),
MISSING_PATH_VARIABLE_EXCEPTION("COMMON400_003", HttpStatus.BAD_REQUEST, "경로 변수(PathVariable)가 누락됐습니다."),
MISSING_REQUEST_PARAM_EXCEPTION("COMMON400_004", HttpStatus.BAD_REQUEST, "쿼리 스트링이 누락됐습니다."),
UN_EXPECTED_EXCEPTION("COMMON500_001", HttpStatus.INTERNAL_SERVER_ERROR, "예기치 못한 에러가 발생했습니다."),
REDIS_NOT_READY_EXCEPTION("COMMON500_002", HttpStatus.INTERNAL_SERVER_ERROR, "Redis의 연결 상태가 Ready가 아닙니다");
그래서 실무에서는 이런 디버깅 편의만으로 커스텀 예외를 늘리기보다는,
말씀해주신 것처럼 실제 분기 처리가 필요한 특수한 경우에만 제한적으로 사용하는 편이라고 이해하면 될지 궁금합니다!!
| void 참가자가_승리하면_수익은_배팅금액만큼_증가한다() { | ||
| Name name = new Name("pobi"); | ||
| Player player = new Player(name); | ||
| Map<Name, BettingAmount> amountMap = new HashMap<>(); | ||
| amountMap.put(player.getName(), new BettingAmount(1000)); | ||
| BettingAmounts bettingAmounts = new BettingAmounts(amountMap); | ||
| CalculateProfit calculateProfit = new CalculateProfit(bettingAmounts); | ||
| int expected = 1000; | ||
| assertEquals(expected, calculateProfit.calculate(name, GameResult.WIN).getMoney()); | ||
| } |
There was a problem hiding this comment.
아하 그럼 작은 단위에서 테스트가 된다면, 얇은 통합 테스트(?)는 수행하지 않아도 된다는 의미일까요??
추가적으로요구사항이 메서드의 길이를 10줄 이하로 하라는것 때문에 개행없이 이렇게 처리하신걸까요??
이 부분은 의식한 건 아닌데, 가독성 개선을 해보겠습니다!
- Players에 이름 목록 조회 메서드 추가 - Players에 게임 결과 계산 및 수익 계산 메서드 추가 - GameResultManager의 플레이어 순회 로직을 Players로 위임 - CalculateProfit의 불필요한 내부 계산 메서드 제거 - 테스트에서 공통 배팅 금액 생성 로직을 Players 메서드로 정리
- Cards 클래스 제거 - Deck이 직접 카드 리스트를 관리하도록 변경 - ParticipantCards가 카드 목록 기반으로 점수를 계산하도록 리팩터링 - 에이스 개수 캐시 상태 제거 - 관련 테스트 코드 정리
xxbeann
left a comment
xxbeann
left a comment
There was a problem hiding this comment.
getPlayers()처럼 내부 컬렉션 표현을 직접 노출하는 것보다는 forEach가 낫다고 생각했지만,
말씀해주신 것처럼 Consumer<Player>를 열어두면 결국 외부가 각 Player에 대해 어떤 행위를 할지 전부 결정하게 되어 Players가 도메인 객체라기보다 순회 통로처럼 보일 수 있다는 점도 이해됐습니다.
다만 현재 구조에서는 입력/출력/게임 진행 흐름처럼 Players 안으로 옮기기 어려운 순회도 남아 있어서, 이 경우 forEach를 완전히 제거하기보다는 의미 있는 도메인 메서드로 대체할 수 있는 부분만 점진적으로 줄여가는 방향으로 이해하면 되는지 궁금합니다.
정리하자면 뭔가 무분별한 getter를 줄이듯이, 약간 무분별한 forEach를 줄이자...라는 맥락으로 이해하면 될까요?🤔
| private void printParticipantCards(Dealer dealer, Players players) { | ||
| printCards(toParticipantCardsDto(dealer)); | ||
| players.forEach(player -> printCards(toParticipantCardsDto(player))); | ||
| } | ||
|
|
||
| private void printFinalScores(Players players) { | ||
| players.forEach(player -> printFinalCards(toParticipantCardsDto(player))); | ||
| } | ||
|
|
There was a problem hiding this comment.
Players.forEach(...)를 단순 순회 위임으로 두는 것은 아쉽다는 점은 이해했습니다!
다만 이 부분은 플레이어를 순회하면서 ParticipantCardsDto로 변환하고 출력까지 이어지는 흐름이라,
이를 그대로 Players 안으로 옮기면 도메인이 DTO나 출력 책임을 알게 되는 구조가 될 것 같아 고민이 됩니다.
이런 경우에는 Players에 도메인 메서드를 추가하기보다,
컨트롤러에서 DTO 목록으로 변환하는 별도 메서드로 분리하는 편이 더 자연스러운지 궁금합니다.
즉 계층을 유지하면서도 forEach 의존을 줄이려면 어떤 방식이 더 적절한지 궁금합니다 🤔
There was a problem hiding this comment.
위의 리뷰 바탕으로 player와 bettingAmount의 관계에서 도메인 설계를 고민해보고 고민해보면 좋을것 같습니다.
forEach가 왜 필요한지도 이해하지 못하겠어요.
forEach의 메서드로 view 로직을 전달하면 도메인과 view가 잘 분리되었다고 생각하신걸까요?
| players.forEach(player -> { | ||
| int amount = InputView.askBettingAmount(player.getName().getName()); | ||
| bettingAmounts.put(player.getName(), new BettingAmount(BigDecimal.valueOf(amount))); | ||
| }); |
There was a problem hiding this comment.
이 부분도 forEach를 줄여보려 했는데,
InputView를 통해 입력을 받고 BettingAmount로 매핑하는 흐름이라 Players 안으로 옮기면
도메인이 입력 계층과 강하게 결합되는 구조가 될 것 같아 망설여졌습니다.
이 경우에는 Players의 도메인 메서드로 옮기기보다,
컨트롤러에서 입력값을 수집하고 도메인 객체로 조립하는 책임으로 두는 것이 더 자연스러운지 궁금합니다.
There was a problem hiding this comment.
여기서 for 문으로 players를 순회하는것이랑 지금 forEach랑 어떤 차이가 있나요?
player가 bettingAmount를 가지고 있지 않고 controller 메서드 안의 지역변수로 관리되고 있는게 좋은 방향일까요?
player 와 bettingAmount 부터 한번 고민해보시면 좋을것 같네요!
| private void playGame(Players players, GameManager gameManager) { | ||
| players.forEach(player -> playGameWithPlayer(player, gameManager)); | ||
| playGameWithDealer(gameManager); |
There was a problem hiding this comment.
이 부분도 forEach를 Players 안으로 옮길 수 있을지 고민했는데,
플레이어 턴을 진행하는 흐름 자체가 GameManager, 사용자 입력, 컨트롤러 오케스트레이션 책임과 연결되어 있어
Players의 도메인 행위로 보기는 조금 어렵다고 느꼈습니다.
이런 경우에는 forEach를 그대로 두더라도
도메인 책임을 침범하지 않는 선에서 컨트롤러의 흐름 제어로 남겨두는 편이 더 적절한지 의견을 듣고 싶습니다.
| public BettingAmounts createBettingAmounts(BigDecimal amount) { | ||
| Map<Name, BettingAmount> bettingAmounts = new HashMap<>(); | ||
| players.forEach(player -> { | ||
| bettingAmounts.put(player.getName(), new BettingAmount(amount)); | ||
| }); | ||
| return new BettingAmounts(bettingAmounts); | ||
| } |
There was a problem hiding this comment.
이 메서드는 테스트에서 반복되는 준비 코드를 줄이려는 의도로 추가했는데,
실제 프로덕션 흐름에서는 모든 플레이어가 동일한 금액을 배팅하는 상황이 일반적이지 않아서
도메인 메서드라기보다 테스트 편의 메서드에 더 가까운 것 같기도 합니다.
이런 경우에는 프로덕션 Players 안에 두기보다
테스트 픽스처나 테스트 헬퍼 쪽으로 옮기는 편이 더 적절한지도 의견을 듣고 싶습니다.
| public Map<String, GameResult> judgeResultsAgainst(Dealer dealer) { | ||
| Map<String, GameResult> gameResult = new HashMap<>(); | ||
| players.forEach(player -> { | ||
| GameResult result = player.judgeResult(dealer); | ||
| gameResult.put(player.getName().getName(), result); | ||
| }); | ||
| return gameResult; | ||
| } | ||
|
|
||
| public LinkedHashMap<Name, Revenue> calculateProfitsAgainst(Dealer dealer, CalculateProfit calculateProfit) { | ||
| LinkedHashMap<Name, Revenue> finalRevenues = new LinkedHashMap<>(); | ||
|
|
||
| players.forEach(player -> { | ||
| GameResult result = player.judgeResult(dealer); | ||
| Revenue revenue = calculateProfit.calculate(player.getName(), result); | ||
| finalRevenues.put(player.getName(), revenue); | ||
| }); | ||
| return finalRevenues; |
There was a problem hiding this comment.
이 메서드는 테스트에서 반복되는 준비 코드를 줄이려는 의도로 추가했는데,
실제 프로덕션 흐름에서는 모든 플레이어가 동일한 금액을 배팅하는 상황이 일반적이지 않아서
도메인 메서드라기보다 테스트 편의 메서드에 더 가까운 것 같기도 합니다.
이런 경우에는 프로덕션 Players 안에 두기보다
테스트 픽스처나 테스트 헬퍼 쪽으로 옮기는 편이 더 적절한지도 의견을 듣고 싶습니다.
| private record TestFixture(Player pobi, Player crong, Players players, Dealer dealer) { | ||
| } |
There was a problem hiding this comment.
이렇게 TestFixture로 record를 두는 건, 실무에서도 많이 적용하는 방식일까요??
choijy1705
left a comment
choijy1705
left a comment
There was a problem hiding this comment.
정콩이 많은 고민들을 해보신것 같습니다.
forEach에 너무 매몰되어 있어서 쉽게 짤 수 있는 코드를 너무 어렵게 짜고 있는게 아닐지 고민해보시면 좋을것 같습니다.
고민해보시고 피드백 반영후 다시 리뷰요청 주세요!
| players.forEach(player -> { | ||
| int amount = InputView.askBettingAmount(player.getName().getName()); | ||
| bettingAmounts.put(player.getName(), new BettingAmount(BigDecimal.valueOf(amount))); | ||
| }); |
There was a problem hiding this comment.
여기서 for 문으로 players를 순회하는것이랑 지금 forEach랑 어떤 차이가 있나요?
player가 bettingAmount를 가지고 있지 않고 controller 메서드 안의 지역변수로 관리되고 있는게 좋은 방향일까요?
player 와 bettingAmount 부터 한번 고민해보시면 좋을것 같네요!
| private void printParticipantCards(Dealer dealer, Players players) { | ||
| printCards(toParticipantCardsDto(dealer)); | ||
| players.forEach(player -> printCards(toParticipantCardsDto(player))); | ||
| } | ||
|
|
||
| private void printFinalScores(Players players) { | ||
| players.forEach(player -> printFinalCards(toParticipantCardsDto(player))); | ||
| } | ||
|
|
There was a problem hiding this comment.
위의 리뷰 바탕으로 player와 bettingAmount의 관계에서 도메인 설계를 고민해보고 고민해보면 좋을것 같습니다.
forEach가 왜 필요한지도 이해하지 못하겠어요.
forEach의 메서드로 view 로직을 전달하면 도메인과 view가 잘 분리되었다고 생각하신걸까요?
| private ParticipantCardsDto toParticipantCardsDto(Participant participant) { | ||
| return new ParticipantCardsDto( | ||
| participant.getName().getName(), | ||
| participant.getCardsInfo(), | ||
| participant.getScore() | ||
| ); | ||
| } |
There was a problem hiding this comment.
dto 안에 있는게 좋지 않을까요?
dto객체가 많아지면 controller가 비대해질것 같아요🤔
| List<String> playersName = new ArrayList<>(); | ||
| players.forEach(player -> { | ||
| playersName.add(player.getName().getName()); | ||
| }); | ||
| return playersName; |
There was a problem hiding this comment.
forEach에 뭔가 꽂히신것 같은데....
return players.stream()
.map(player -> player.getName()) // 메서드 체이닝도 하지 않도록 해주세요
.toList(); 로 풀수 있는걸 더 어렵게 가는게 아닐까요?
| public void forEach(Consumer<Player> action) { | ||
| players.forEach(action); | ||
| } |
There was a problem hiding this comment.
저는 개발하면서
이런 구현을 한번도 해본적 없고 동료의 코드를 리뷰하면서도 본적이 없었던것 같아요
밑에서도 예시를 보여드렸듯이
충분히 stream 으로 풀 수 있는것들과 도메인 설계가 좀 이상해지면서 이렇게 되는게 아닐지 고민해보시면 좋을것 같습니다!
밑의 질문들에 대해서는 따로 답변을 달지 않을께요 한번 리펙토링 해서 forEach를 없애 보는걸 제안드립니다!
| package view; | ||
|
|
||
| public class InputValidator { | ||
| public static void validateContinueResponse(String input) { | ||
| if (!input.matches("[yn]")) { | ||
| throw new IllegalArgumentException("응답은 y와 n만 허용됩니다."); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
InputView에 그냥 있으면 될 코드를 굳이 클래스를 만든게 아닐까요?
| void 참가자가_승리하면_수익은_배팅금액만큼_증가한다() { | ||
| Name name = new Name("pobi"); | ||
| Player player = new Player(name); | ||
| Map<Name, BettingAmount> amountMap = new HashMap<>(); | ||
| amountMap.put(player.getName(), new BettingAmount(1000)); | ||
| BettingAmounts bettingAmounts = new BettingAmounts(amountMap); | ||
| CalculateProfit calculateProfit = new CalculateProfit(bettingAmounts); | ||
| int expected = 1000; | ||
| assertEquals(expected, calculateProfit.calculate(name, GameResult.WIN).getMoney()); | ||
| } |
There was a problem hiding this comment.
Player 를 예시로들면 Player의 로직들만 테스트하면되지
player.Name의 validation 까지 테스트 할 필요없다는 의미로 생각하시면 될것 같습니다.
통합테스트 에서만 할 수 있는것은 통합테스트 진행하셔야할것 같아요
| private record TestFixture(Player pobi, Player crong, Players players, Dealer dealer) { | ||
| } |
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class NameTest { | ||
| // TODO: 나중에 커스텀 예외로 변경. 현재는 예외가 터지는 것만 검증됨. |
There was a problem hiding this comment.
디버깅은 웬만하면 error message로 하실거에요
프론트 혹은 다른팀에서 분기처리가 필요한 경우에 대해 에러 타입정의가 필요할때 커스텀 예외를 쓰고 내부적으로는 딱히 커스텀예외를 만들지는 않는것 같아요!
2 participants
INTRO
안녕하세요 영이! 이번 리뷰도 잘 부탁드립니다 🙇🏻♀️
부족한만큼 많이 배워가겠습니다!
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?개요
이번 PR에서는 블랙잭 게임에 배팅 금액, 블랙잭 판정, 최종 수익 계산/출력 기능을 추가했습니다.
기능 구현 과정에서 도메인 책임이 흩어지지 않도록 패키지 구조와 객체 책임도 함께 리팩터링했습니다.
특히 이번 사이클에서는 불변 객체 개념을 적용해보려 했습니다.
배팅 금액과 수익은 상태를 변경하는 대신 새로운 값을 생성하는 방향으로 모델링했고,
이를
BettingAmount,Revenue에 반영했습니다.변경 배경
기존에는 게임 진행, 결과 계산, DTO 변환, 입력 검증 등의 책임이 여러 객체에 분산되어 있었습니다.
이번 변경에서는 다음 방향을 기준으로 구조를 정리했습니다.
주요 변경 사항
1. 배팅 금액 입력 및 검증 흐름 추가
InputView가 직접 도메인 규칙을 알지 않도록 정리2. 참가자/카드 관련 도메인 구조 정리
Players일급 컬렉션 도입Players내부에서 플레이어 수, 중복 이름 검증 수행forEach위임 메서드 추가Cards/ParticipantCards책임 분리Participant,GameManager의 DTO 의존 제거 (이는 아직 의존 방향이 완전히 해결되지 않았습니다.)3. 패키지 분리
도메인 객체를 역할에 따라 아래와 같이 재구성했습니다.
domain.carddomain.participantdomain.gamedomain.betting이를 통해 카드, 참가자, 게임 결과, 배팅/수익 계산 책임을 분리했습니다.
4. 블랙잭 판정 및 결과 확장
Participant에 블랙잭 판정 로직 추가GameResult에BLACKJACK결과 추가GameManager에서GameResultManager로 이동5. 배팅/수익 도메인 추가
최종 수익 계산을 위해 관련 도메인을 도입했습니다.
BettingAmount: 배팅 금액 VOBettingAmounts: 참가자별 배팅 금액을 관리하는 일급 컬렉션Revenue: 최종 수익 VORevenues: 참가자별 최종 수익을 관리하는 일급 컬렉션CalculateProfit:GameResult와BettingAmount를 바탕으로 수익 계산GameResultManager: 참가자 결과 및 수익 집계 담당배팅 금액 자체와 정산/수익 계산 책임을 분리하려고 했습니다.
6. 최종 수익 출력 기능 추가
ParticipantRevenueDto로 변환OutputView에서 참가자 및 딜러의 최종 수익 출력7. 테스트 정리 및 보강
Players,ParticipantCards,Player,Dealer등 도메인 테스트 정리BettingAmountTest,CalculateProfitTest,GameResultManagerTest추가참고
기능 구현과 함께 책임 분리를 병행한 PR이라 변경 범위가 다소 큽니다.
리뷰 시에는 아래 순서로 봐주시면 편합니다.
어떤 부분에 집중하여 리뷰해야 할까요?
GameResultManager가 게임 결과 계산과 수익 집계를 함께 가지는 현재 책임 범위가 적절한지BettingAmounts,Revenue등 배팅/수익 도메인 분리가 자연스러운지