[🚀 사이클1 - 미션 (블랙잭 게임 실행)] 이삭 미션 제출합니다.#986
Conversation
pkeugine
left a comment
There was a problem hiding this comment.
안녕하세요 이삭~
고민의 흔적이 많이 보이는 PR 이군요! 열심히 하는 모습 보기 좋아요~
질문이 많으니 바로 질문 위주의 답변 남기겠습니다 ㅋㅋㅋ
여기 없으면 코멘트에 답변 또는 관련된 내용이 있을 거예요.
누락되었다면 알려주세요. 추가할게요!
Q4. 테스트코드 추가 시점
하나의 테스트가 통과한 뒤 구현을 진행하다가 새로운 분기나 예외가 보이면, 그 순간 테스트를 더 보강하는 흐름, 자연스러워요.
테스트와 실제 코드는 서로 영향을 주면서 같이 자라는 편이라고 생각합니다.
그런데 TDD 에서는 또 다른 것 같기도 한데, 이삭의 질문 의도가 TDD 관점을 물어본 건가요?
Q6. private 메서드 테스트
private 메서드를 직접 검증하기 어렵다는 느낌이 들 때는, "이 책임이 정말 한 객체 또는 메서드 안에 있어야 하나?"를 먼저 떠올려보면 좋아요.
public 메서드로 충분히 검증이 안 될 정도라면 분리 신호일 가능성이 크더라구요.
모든 private 메서드를 public이나 클래스로 따로 분리하는 것이 맞는지 -> 이건 조금 극단적이라 그렇다고 말하긴 힘든 것 같네요.
오히려 '한 객체에 있는게 맞다'고 생각이 든다면, 'public 메서드를 검증하는 것으로 충분'하다고 생각하고 넘어가도 괜찮을 듯 해요.
참고로 테스트를 위해서 메서드를 public 으로 바꾸는건 반대입니다. (코멘트에도 관련 내용이 있어요.)
Q7. 처음 시작할 테스트
처음에는 "이 객체가 존재해야 하는 이유를 가장 잘 보여주는 시나리오"부터 잡아보면 도움이 돼요.
블랙잭에서는 카드 점수 계산, 에이스 보정, 딜러 hit 규칙 같은 규칙이 그런 출발점이 되기 좋습니다.
지금 당장 만들 수 있는 가장 쉽고 직관적인 기능부터 바로 만들어도 좋을 듯 하고요.
그런 것들을 테스트하거나 만들면서 추가로 할 일이 생기니까요.
리팩터링 및 수정할 부분이 있을 것 같아서 request changes 하겠습니다.
언제든 추가 질문 있다면 코멘트나 DM 주시고,
해피해킹하세요!!!! 🔥 🔥 🔥
src/main/java/Main.java
Outdated
| Result result = new Result(); | ||
| BlackJackService blackJackService = new BlackJackService(result); | ||
| BlackJackController blackJackController = new BlackJackController(blackJackService, result); |
There was a problem hiding this comment.
Main에서 Result를 먼저 만든 뒤 BlackJackService, BlackJackController가 같이 나눠 갖고 있네요.
이렇게 되면 두 객체가 같은 상태를 암묵적으로 공유하게 되는데, 누가 Result를 책임지는지 조금 흐려질 수 있어요.
BlackJackService.calculateResult()가 새 Result를 만들어서 반환하는 쪽이 더 자연스럽지 않을까요?
There was a problem hiding this comment.
저는 한 판의 블랙잭게임만 하니 결과가 전역에 있어도 된다고 생각했었습니다.
그런데 PK께서 주신 의견을 보니 책임도 흐려지고 읽는 입장에서도 혼동이 올 수 있다고 느꼈습니다.
수정완료했습니다!
| import domain.participants.Player; | ||
| import domain.participants.Players; | ||
|
|
||
| public class BlackJackService { |
There was a problem hiding this comment.
그리고 여기서 한 가지 더 궁금했던 건,
이삭이 이 클래스를 BlackJackService라고 이름 붙인 이유예요.
이 클래스가 어떤 역할을 한다고 생각했고, 그래서 BlackJackCalculator나 ResultJudge 같은 이름이 아니라 service라고 표현하신 건지도 같이 듣고 싶어요.
There was a problem hiding this comment.
저는 서비스를 여러도메인을 전달받아서 하나의 기능을 제공하는 계층이라고 생각하고 있었고 따라서 딜러와 플레이어라는 도메인을 넘겨 블랙잭 게임의 결과를 결정하는 기능을 제공하는 클래스라고 판단했습니다. 그래서 Service라는 이름을 붙이게 되었습니다!
또한 당시에는 단순히 Controller로 흐름을 통제하는 레이어드 아키텍쳐를 사용하면서 아키텍처 관례에 맞는 Service라는 네이밍을 사용하는 것이 자연스럽다고 판단했고 구체적인 의미는 메서드 이름을 통해 전달하면 된다고 생각했습니다.
다만 그 당시에는 PK가 말씀해주신 BlackJackCalculator나 ResultJudge와 같은 네이밍은 떠올리지 못했습니다. 지금 다시 생각해보니 해당 클래스의 역할을 더 명확하게 드러낼 수 있는 이름이라고 생각합니다!
PK께서는 보통 어떤 경우에 Service라는 네이밍을 사용하시는지도 궁금합니다.
There was a problem hiding this comment.
답변 잘 남겨주셨네요 :)
말씀하신 것처럼 여러 도메인을 받아 하나의 기능을 수행하게 만든다는 관점에서 Service라고 생각하신 흐름은 이해가 돼요.
레이어드 아키텍처를 접하면 자연스럽게 Controller - Service - Repository 구조를 먼저 떠올리게 되기도 하고요.
다만 제가 이 코멘트를 남긴 이유는, "틀렸다"기보다 이 클래스가 실제로 하는 일을 이름만 보고 바로 알 수 있는지가 궁금했기 때문이에요.
예를 들어 BlackJackService라고 하면 범위가 꽤 넓게 느껴지는데, ResultJudge나 BlackJackCalculator는 "아 결과를 판단하는구나", "계산을 하는구나"가 조금 더 바로 보이거든요.
현재 미션에서는 Service 가 과연 필요할까? 너무 앞서나가는건 아닐까? 하는 생각도 있었고요.
저는 보통 Service라는 이름을,
하나의 도메인 객체, 또는 여러 객체를 엮어서 어떤 유스케이스를 진행시키거나,
repository 같은 외부 구성요소와 연결되면서 흐름을 조율하는 역할일 때 자주 쓰는 편이에요.
반대로 지금처럼 책임이 비교적 또렷하게 한 문장으로 설명되는 경우에는,
Judge, Calculator, Manager처럼 역할이 직접 드러나는 이름을 먼저 떠올려보는 편이고요.
그래서 이번 답변처럼 스스로 정리하신 점, 아주 좋았습니다 👍
저도 그 방향의 고민이 자연스럽다고 생각해요~
| private Result result; | ||
|
|
||
| public BlackJackController(BlackJackService blackJackService, Result result) { | ||
| this.blackJackService = blackJackService; | ||
| this.result = result; | ||
| } |
There was a problem hiding this comment.
지금 result는 처음에는 생성자로 받은 값인데,
run() 안에 들어가면 다시 계산된 결과를 담는 변수처럼 쓰이고 있네요.
읽는 입장에서는 같은 이름인데 역할이 중간에 바뀌는 느낌이 들어서 조금 헷갈릴 수 있어요.
이 경우에는 필드로 들고 있기보다 run() 안에서 결과를 지역변수로 두는 편이 더 자연스럽지 않을까요?
There was a problem hiding this comment.
첫번째로 주신 코멘트와 같은 맥락인 것 같아 제가 첫번째 코멘트에서 수정한 게 PK께서 의도하신게 맞는 지 확인 부탁드리고 싶습니다!
| public void run() { | ||
| CardDeck cardDeck = new CardDeck(); | ||
| Players players = new Players(getPlayer()); | ||
| Dealer dealer = new Dealer(new Hand()); | ||
|
|
||
| initGame(cardDeck, dealer, players); | ||
|
|
||
| for (Player player : players.getPlayers()) { | ||
| progressGame(cardDeck, player); | ||
| } | ||
|
|
||
| dealerHitOrStand(dealer, cardDeck); | ||
| OutputView.scoreStatisticsMessage(dealer, players); | ||
| result = blackJackService.calculateResult(dealer, players); | ||
| OutputView.gameResultMessage(result); | ||
| } |
There was a problem hiding this comment.
run() 안에서 카드덱 생성, 참가자 생성, 초기 배분, 턴 진행, 결과 출력까지 한 번에 이어지고 있네요.
이번 미션은 함수 길이 <= 10라인 요구사항이 있다 보니, 이 메서드도 한 번 더 나눠보면 좋을 것 같아요.
길이만 줄이자는 의미보다는,
지금 흐름을 기준으로 역할을 나누다 보면 요구사항도 더 자연스럽게 맞춰질 것 같거든요 :)
| OutputView.inputPlayerMessage(); | ||
| String input = InputView.input(); | ||
| Validator.validateInput(input); | ||
| List<String> names = Parser.separateBySeparator(input); |
There was a problem hiding this comment.
지금 validateInput(input)는 입력 한 줄 전체가 비어 있는지만 확인하고 있네요.
그래서 pobi,,jason처럼 중간 이름이 비어 있어도 일단은 통과할 수 있어요.
입력 문자열 자체를 확인하는 것과,
쉼표로 나눈 뒤 각 이름이 제대로 들어왔는지 확인하는 것은 조금 다른 검사라서요.
현재는 그 이름이 그대로 new Player(name, ...)로 넘어가고,
Participants도 String name을 직접 들고 있어서 이름 관련 규칙이 흩어지기 쉬워 보여요.
이름은 String으로 그대로 넘기기보다 PlayerName 같은 객체로 감싸두면,
공백이나 빈 값 같은 검증 위치도 한곳에 모이고 요구사항인 문자열 포장도 더 자연스럽게 맞출 수 있을 것 같아요.
There was a problem hiding this comment.
PlayerName 클래스를 만들어 각 플레이어를 생성할 때 활용하였으며 PlayerName안에 검증코드를 넣어 수정완료했습니다!
| //승패는 플레이어 기준 | ||
| private ResultInfo calculateWinDefeatDraw(Dealer dealer, Player player) { | ||
| int dealerTotalScore = dealer.getTotalCardScore(); | ||
| int playerTotalScore = player.getTotalCardScore(); | ||
|
|
||
| if (dealerTotalScore > BLACKJACK_LIMIT_NUMBER) return ResultInfo.WIN; | ||
| if (playerTotalScore > BLACKJACK_LIMIT_NUMBER) return ResultInfo.DEFEAT; | ||
| if (dealerTotalScore < playerTotalScore) return ResultInfo.WIN; | ||
| if (dealerTotalScore > playerTotalScore) return ResultInfo.DEFEAT; | ||
| return ResultInfo.DRAW; | ||
| } |
There was a problem hiding this comment.
dealerTotalScore > 21을 먼저 검사하고 있어서, 딜러와 플레이어가 둘 다 bust인 경우에도 플레이어가 WIN이 되네요.
지금 규칙에서는 그 결과를 원하신 걸까요? (주석이 그 의미인지 확실하지 않아서 남기는 질문입니다)
이런 우선순위는 주석이 아닌 테스트로 먼저 박아두면 의도가 훨씬 선명해질 것 같아요.
저만 하더라도 저 주석의 의미가 한 번에 다 이해되지 않았으니까요!
There was a problem hiding this comment.
기존엔 플레이어와 딜러 둘 다 버스트되면 플레이어가 지도록 한 게 맞습니다! 하지만 크루들이 원래 블랙잭은 둘 다 버스트나면 플레이어의 패배라고하여 분기를 변경하였고 해당 테스트도 추가하였습니다!
| @Test | ||
| void 딜러의_총합이_플레이어의_총합보다_낮으면_플레이어가_승리한다() { | ||
| BlackJackService blackJackService = new BlackJackService(new Result()); | ||
| Result result = blackJackService.calculateResult(dealer, players); | ||
| ResultInfo info = result.getGameResult().get(winPlayer.getName()); | ||
| assertThat(info).isEqualTo(ResultInfo.WIN); | ||
| } | ||
|
|
||
| @Test | ||
| void 딜러의_총합이_플레이어의_총합보다_높으면_플레이어가_패배한다() { | ||
| BlackJackService blackJackService = new BlackJackService(new Result()); | ||
| Result result = blackJackService.calculateResult(dealer, players); | ||
| ResultInfo info = result.getGameResult().get(defeatPlayer.getName()); | ||
| assertThat(info).isEqualTo(ResultInfo.DEFEAT); | ||
| } | ||
|
|
||
| @Test | ||
| void 딜러의_총합과_플레이어의_총합이_같으면_비긴다() { | ||
| BlackJackService blackJackService = new BlackJackService(new Result()); | ||
| Result result = blackJackService.calculateResult(dealer, players); | ||
| ResultInfo info = result.getGameResult().get(drawPlayer.getName()); | ||
| assertThat(info).isEqualTo(ResultInfo.DRAW); | ||
| } |
There was a problem hiding this comment.
위 코멘트의 연장선 느낌입니다.
지금 테스트는 '점수 비교' 케이스는 잘 잡혀 있는데, dealer bust, player bust, 둘 다 bust는 아직 비어 있네요.
오히려 블랙잭에서는 이런 경계 케이스가 규칙의 우선순위를 가장 잘 드러내지 않을까요?
src/test/java/DealerTest.java
Outdated
| if (dealer.dealerRule()) { | ||
| dealer.keepCard(card); | ||
| } |
There was a problem hiding this comment.
DealerTest에서 if (dealer.dealerRule()) { ... }로 테스트를 진행하고 있네요.
테스트 안에서 다시 분기를 만들어버리면, 정작 dealerRule()이 true여야 하는 이유가 assertion으로 남지 않아요.
여기는 dealerRule() 자체를 먼저 검증해보고,
딜러 규칙의 핵심 경계인 16/17도 같이 확인해보면 더 탄탄한 테스트가 될 것 같아요.
There was a problem hiding this comment.
dealerRule() -> shouldHit() 로 변경완료하였고 if문을 제거하고 따로 shouldHit() 테스트코드 추가 완료했습니다!
| @Test | ||
| void 핸드의_총합을_에이스를_1로_간주하여_계산할_수_있다() { | ||
| Card card3 = new Card(Rank.ACE, Pattern.DIAMOND); | ||
| Card card4 = new Card(Rank.TEN, Pattern.SPADE); | ||
| dummyHand.addCard(card3); | ||
| dummyHand.addCard(card4); |
There was a problem hiding this comment.
에이스 1장 케이스까지는 잘 보셨는데, 블랙잭에서는 진짜 재밌는 함정이 그 다음에 나오더라구요 ㅎㅎ
A, A, 9 같은 손패는 얼핏 보면 31처럼 보여도 실제로는 21이 되어야 하잖아요.
이런 케이스를 하나 넣어두면,
"에이스를 11로 본다"가 아니라 "상황에 따라 가장 유리하게 다시 계산한다"는 규칙까지 테스트로 딱 드러날 것 같아요.
src/test/java/PlayerTest.java
Outdated
| @Test | ||
| @DisplayName("히트") | ||
| void 플레이어는_카드를_뽑아_핸드에_저장한다() { |
There was a problem hiding this comment.
테스트 메서드 이름을 한글로 지으신 이유가 궁금해요.
저는 보통 메서드명은 영어로, 가능하면 애플리케이션 로직이나 실제 메서드명을 활용해서 짓고,
대신 @DisplayName에 한글 설명을 적는 편인데요.
그렇게 두면 테스트 검색할 때도 조금 더 편하고,
IDE에서 메서드명으로 찾거나 실제 코드와 연결해서 읽을 때도 도움이 되더라구요.
그리고 그럴 경우 단순 '히트', '스탠드' 보다 유의미한 내용을 보여줄 것 같기도 하고요.
이름은 미래의 자신, 그리고 팀원들이 로직과 테스트를 읽을 때 나침반 기능을 해요.
잘 작동하는 나침반은 테스트를 읽기도 전에 방향을 제대로 잡아주죠.
그러니 소소해 보이더라도 꼭 잘 만들어보는 연습을 해보죠 우리 :)
이 부분은 이삭의 생각도 들어보고 싶습니다.
There was a problem hiding this comment.
영어로 테스트코드를 작성하면 영어에 익숙치않은 저로써는 가독성이 더 떨어진다고 판단하여 한글로 작성해왔습니다. 하지만 코드는 저 혼자만 읽는 것이 아닌 것을 다시 한 번 깨닫게 되어 PK께서 말씀해주신 방법대로 연습을 하되 먼 훗날에 회사의 규정에 맞게 유동적으로 작성을 할 것 같습니다!
|
아 추가로 이삭에게 질문이 있어요 :
사용해야 한다는 의도의 질문은 아니고, 이삭이 사용하지 않은 그 이유가 정말로 궁금해서 남기는 질문입니다! |
- 패키지: participants -> participant - 클래스: Participants -> participant
- 요구사항 명세서에 있는 것을 우선
- IllegalArgumentException를 NoSuchElementException로 변경
- 실제기능할 메서드네임을 앞에두고 영어로 네이밍 작성 - @DisplayName으로 의도를 명확히 전달
- @DisplayName을 이용하여 의도를 명확히 전달
기존엔 둘 다 버스트되면 무조건 플레이어가 승리하게 하였으나 원래 블랙잭 규칙에서는 둘 다 버스트되면 플레이어가 무조건 패배하게 된다하여 로직을 변경
|
안녕하세요 PK!! 꼼꼼하게 피드백 해주셔서 감사합니다!
해당 미션을 진행할 때 도메인을 설계하는 힘을 AI를 써버리면 기를 수 없다고 권장하지 않는다하여 사용하지 않았고 해당 미션을 진행하며 문법에 대해 모르는 부분만 AI를 사용하고 도메인을 구축하는 부분에는 일체 사용하지 않았습니다! |
pkeugine
left a comment
There was a problem hiding this comment.
안녕하세요 이삭, 리팩터링 잘 진행해주셨네요!
추가로 몇가지 간단한 코멘트 남겼으니 다음 미션 진행하면서 적용하거나 참고해주세요.
제가 질문 남긴건 다음 단계 요청 때 답변 꼭 부탁드립니다.
고생하셨고, 이번 사이클은 머지할게요.
조만간 다시 만나요, 해피해킹하세요! 🔥 🔥 🔥
|
|
||
| private void finalizePlayerTurn(Player player) { | ||
| OutputView.holdingCardMessage(player); | ||
| player.getTotalCardScore(); |
There was a problem hiding this comment.
여기서 player.getTotalCardScore()는 반환값을 사용하지 않고 있는데요.
혹시 bust 여부를 확인하거나 점수 계산을 "미리 수행"하려는 의도였을까요?
지금 구조에서는 이 호출이 없어도 흐름을 이해하는 데 문제가 없어 보여서,
읽는 사람 입장에서는 "무슨 부수효과를 기대한 건가?" 하고 잠깐 멈추게 될 수도 있을 것 같아요.
의도가 없다면 제거해도 괜찮아 보입니다 :)
There was a problem hiding this comment.
해당 코드는 저도 의미하는 바를 파악하지 못하여 제거 완료했습니다!
| public class PlayerName { | ||
| private static final String NAME_IS_NOT_NULL_OR_BLANK_ERROR_MESSAGE = "[ERROR] 이름은 공백이나 빈 칸 일 수 없습니다."; | ||
|
|
||
| String name; |
There was a problem hiding this comment.
PlayerName으로 한 번 감싼 방향은 좋았어요. 👍
다만 이 클래스 안의 name 필드는 private final로 두면 의도가 더 분명해질 것 같아요.
값 객체로 보이는 만큼,
"생성 이후 바뀌지 않는다"는 점까지 같이 드러나면 더 자연스러워 보이거든요 :)
| System.out.printf(DEALER_RESULT, | ||
| result.dealerResult().getFirst(), result.dealerResult().get(1), result.dealerResult().get(2)); |
There was a problem hiding this comment.
dealerResult()에서 List<Integer>로 승/무/패를 순서에 맞춰 반환하고 있는데요,
지금은 0, 1, 2 인덱스의 의미를 호출하는 쪽이 같이 알고 있어야 해서
조금만 시간이 지나도 읽는 사람이 헷갈릴 수 있을 것 같아요.
승/무/패를 표현하는 전용 객체를 두거나,
적어도 이름이 드러나는 형태로 바뀌면 결과를 다루는 코드가 더 읽기 좋아질 듯 해요 :)
There was a problem hiding this comment.
승/무/패를 관리하는 enum에 code 값을 추가하여 enum에서 getCode()로 불러오는 식으로 수정완료했었고 step2에서는 해당 메서드를 사용하지 않아 제거 완료했습니다!
| package controller; | ||
|
|
||
| import domain.card.CardDeck; | ||
| import domain.participant.*; |
There was a problem hiding this comment.
* 와일드카드 import 를 사용할 경우 어떤 장단점이 있는지 한 번 알아볼까요?
다음 단계 리뷰 요청 때 코멘트로 남겨주세요.
(기억하고 있을테니 꼭 남겨주세요.)
그리고, 단점이 더 많을 것 같다 싶으면 사용하지 않는 쪽으로 가보아요.
(힌트: 요구사항인 google java style guide 에도 사용하지 말라고 적혀있어요.)
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?
1~5점 중에서 선택해주세요.
선택한 점수의 이유를 적어주세요.
구현 완성에 집중하면서 객체 간 메시지 전달보다 상태 조회에 의존한 부분이 많았습니다.
특히 뷰/컨트롤러에서 getHand().getNowHand()처럼 내부 상태를 꺼내 사용하는 코드가 반복되어 객체가 스스로 책임을 수행하게 만들지 못하였고 이로인해 캡슐화도 충분히 지켜지지 않았다고 판단하여 2점을 선택했습니다.
어떤 부분에 집중하여 리뷰해야 할까요?
안녕하세요 PK!! 리뷰받게 된 이삭입니다.
AI를 사용하지 않고 낯선 TDD를 사용하다보니 구현이 늦어져 주말에 pr 보내는 점 양해 부탁드립니다...
페어와 함께 열심히 고민하며 코드 작성 및 피드백 원하는 부분들 작성했습니다!
잘 부탁드립니다!
1. 딜러와 플레이어를 중복 없이 어떻게 나눌지에 대한 고민입니다.
2. 카드덱에서 카드 52개 초기화를 어디서 할건지에 대한 고민입니다.
3. 테스트의 필요성에 대한 질문입니다.
상황: 셔플을 카드덱을 만들면서 하므로 pop을 해서 나온 카드가 정상적인 카드인지를 테스트할 때 값이 매번 달라져 테스트가 어려워지는 문제가 발생
페어와의 고민: enum(Rank, Pattern)으로 카드를 구성을 했는데 카드가 정상적으로 만들어졌다면 카드덱에서 pop하는 카드도 정상이지 않을까? 그러므로 카드덱에서 pop을 해서 나온 카드를 테스트하는게 필요없지 않을까?
페어와의 고민: 카드를 섞는 테스트를 작성해야 할지?
4. 테스트코드 추가시점에 대한 궁금증입니다.
5. 책임의 기준 판단을 어떻게 해야 할지 여쭤보고 싶습니다.
페어와의 고민: 히트와 스탠드는 어디에 넣어야 할지? 플레이어들이 들고 있을까? 아니면 따로 룰이라는 클래스를 새로만들어 룰에 넣을까?
페어와의 고민: 승패로직을 players에 넣을까? 아니면 service에 넣을까?
6. public 메서드 안에 private 메서드가 여러 개 있다면 private 메서드의 테스트는 어떻게 해야 하는지 여쭤보고싶습니다.
7. TDD를 처음 시작할 때 어떤 테스트코드부터 구현해야한다? 라는 기준이 있는지 여쭤보고싶습니다!
코드리뷰를 받는 것이 처음이라 모든 궁금증을 되게 장황하게 많이 쓴 경향이 있는 것 같습니다. 앞으로 효율적인 코드리뷰를 받기 위해 무엇을 지양하거나 어떤 것을 중점적으로 리뷰요청을 하면 좋은지에 대한 조언도 듣고싶습니다!
감사합니다!