[1단계 - 블랙잭 게임 실행] 제이미(임정수) 미션 제출합니다. #420

JJ503 · 2023-03-03T08:01:38Z

안녕하세요 카프카!
블랙잭 게임 1단계 PR 리뷰 요청 드립니다.
개인적으로 이번 미션이 어려워 구현에만 급급했다는 아쉬움이 남는 미션이었습니다.
부족한 부분이 많지만 리뷰 잘 부탁드립니다. 감사합니다! ☺️

특히 의문이 든 사항에 대해선 코드에 코멘트를 작성해 두었습니다.
천천히 확인해 주시면 감사하겠습니다!

include42 · 2023-03-03T09:09:12Z

반갑습니다 제이미, 리뷰어 카프카입니다.
요청 확인하였습니다. 리뷰는 내일 오전 10시 이전에 가능할 것으로 생각됩니다.
작업하시느라 고생 많으셨습니다. 😄

JJ503 · 2023-03-03T13:07:08Z

src/main/java/blackjack/domain/CardDeck.java

+ CardDeck(List<Card> cards) {
+ this.cards = cards;
+ }


리스트 셔플 API로 인해 테스트가 어려운 상태입니다.
하지만, 해당 클래스의 메서드들을 테스트하고자 테스트 코드를 위한 생성자를 선언해 사용했습니다.
그런데 이렇게 테스트를 위한 생성자를 만들어 사용하는 것이 괜찮은지 의문이 듭니다.

꼭 이 예시와 같이 생성자가 아니더라도 비즈니스 로직 등에 테스트를 위한 코드가 있어도 괜찮을까요?

비즈니스 로직에 "테스트만을 위한" 코드가 있는 것은 좋지 않습니다.
해당 코드는 비즈니스 로직에서 사용되지 말아야 하는데, 내용을 전달받지 못한 다른 개발자가 해당 코드를 사용하면 어떻게 될까요?
이러한 경우에는 관점을 바꿔서, 좀 더 테스트하기 좋은 방향으로 코드를 고치는데에 집중하는 것이 좋습니다.

다만 셔플을 하면 테스트에 왜 어려움이 있을까요? 어떠한 방식으로 테스트하고자 하는지 궁금합니다.

BlackjackGame의 테스트 중 getGameResultTest() 메서드에서는 플레이어에게 카드를 나눠주어 승패에 대한 결과를 확인합니다.
이때, 셔플된 카드 리스트를 바로 가져오게 된다면 결과를 예측해 테스트하기 어려워집니다.
이러한 문제를 해결하고자 두 가지 방법을 생각했습니다.

CardDeck에서 테스트를 위한 생성자를 생성해 원하는 카드 리스트를 입력할 시 해당 카드를 플레이어에게 주는 방법

셔플하지 않은 카드 중 랜덤 인덱스를 통해 플레이어에게 카드를 주는 방법
해당 방법에서는 NumberGenerator 인터페이스를 생성해 테스트를 위한 메서드를 만들 생각이었습니다.

그런데 카드 중 랜덤 번호의 카드를 전달하는 방법은 적절하지 않다고 판단해 1번 방법을 사용해 진행했습니다.
두 가지 방법 외에 테스트를 용이하게 하되 테스트를 위한 코드를 추가하지 않는 방법이 있을까요?

어떤 어려움이 있는지 이해했습니다. 이 부분은 고민이 될 만 하네요.
현재 상황에서는 해당 방법으로 진행한 것이 적절하다고 생각됩니다.

추후 테스트 도구에 대해 학습하다 보면 이를 해결할 수 있는 방법을 배우게 될 것이라,
일단 지금 단계에서는 이러한 방식을 해결하고 싶다는 고민을 가지는 것으로 충분하다고 생각합니다.

JJ503 · 2023-03-03T13:14:18Z

src/main/java/blackjack/domain/CardDeck.java

+ public List<Card> pickTwice() {
+ List<Card> pick = new ArrayList<>();
+ pick.add(pick());
+ pick.add(pick());
+ return pick;
+ }


자바 문법에 대해 다시 공부해봐야 할 문제긴 하지만, 질문 겸 제가 잊지 않기 위해 작성해 둡니다.

원래 해당 코드는 pick 함수를 두 번 사용하는 것이 아니라 아래와 같이 subList()를 사용하려 했습니다.

List<Card> pick = cards.subList(0, 2); cards.subList(0, 2).clear();

하지만 subList()를 통해 반환한 카드 리스트를 PlayerCards 클래스에서 해당 리스트를 더할 때 java.util.ConcurrentModificationException이 발생하는 것을 확인할 수 있었습니다.
remove()와 subList()를 통한 카드 리스트 생성의 무엇이 다른 것인지 궁금합니다.

java에서 subList로 얻은 리스트는 원래 리스트의 주소값을 참조하여, 상호간에 영향을 주는 것으로 알고 있습니다.
clear를 실행해 줬기 때문에 pick이 지정하고 있는 메모리 인덱스를 참조할 수 없어 해당 예외가 발생한 것으로 보입니다.

다만 subList로 주는게 과연 올바른 구현일까요? 현재 코드보다 좋다고 생각한 이유가 있을까요?

역시 메서드에 대한 이해가 부족해 발생한 문제였군요…
큰 이유는 없고 subList() 사용하는 것이 pick()을 두 번 호출하는 것보다 더 깔끔하다고 생각해 사용하려 했습니다.

괜찮습니다. 이런 실수는 충분히 할 수 있고, 테스트를 잘 짜두었으면 발견했을테니 문제가 되지 않습니다. 😄

JJ503 · 2023-03-03T13:16:47Z

src/main/java/blackjack/controller/GameCommand.java

+public enum GameCommand {
+ PLAY("y"),
+ STOP("n");
+
+ private final String command;


플레이어의 답변에 대한 예외 처리 및 y/n의 상수보다는 PLAY/STOP을 통한 변수가 더 가독성이 좋다고 생각해 enum 클래스를 사용했습니다.

위와 같은 이유에 대해서도 enum 클래스를 통해 객체로 처리해 주는 것이 좋다고 생각하시나요?
혹은 이런 굳이 enum 클래스를 만들지 않고 컨트롤러에서 바로 사용하는 것이 좋다고 생각하시나요?

저는 이렇게 구현해 주는 것이 좋다고 생각합니다.
입력되는 값에 대해 타입별로 관리해줄 수 있고, 해당 값을 전달할 때에도 더 명확해진다고 생각합니다.
다만 이러한 내용에는 당연히 정답이 없으므로, 적절히 몇 개의 타입으로 나눠 관리할 수 있는 경우에 사용해주시면 된다고 생각되네요.

JJ503 · 2023-03-03T13:20:30Z

src/main/java/blackjack/controller/BlackjackController.java

+ private void giveCard(Player player, BlackjackGame blackjackGame, GameCommand command) {
+ if (command.isPlay()) {
+ blackjackGame.giveCard(player);
+ }
+ }


player, blackjackGame 파라미터는 오로지 giveCard()를 위해서 상위 메서드에서부터 타고 타고 전달된 파라미터입니다.

특히 이번 미션에서는 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.라는 조건으로 인해 해당 객체를 인스턴스 변수로 선언할 수 없기에 더더욱 상위 메서드에서부터 사용되는 메서드까지 파라미터를 통해 전달되는 형태로밖에 작성할 수 없었습니다.

그런데 이러한 형태가 괜찮은지 궁금합니다.

이부분은 확실히 찜찜하실 수 있겠다 생각이 드네요 😓
다만 제가 controller 소스를 보니, view에서 온 값을 바탕으로 수행되는 만큼 어쩔 수 없는 측면이 있다고 생각됩니다.
이 부분은 내일 리뷰 진행하면서 다시 한 번 보고, 개선 가능한 방향이 있다면 코멘트 드릴게요. 😄

JJ503 · 2023-03-03T13:28:14Z

src/main/java/blackjack/view/InputView.java

+ public List<String> readNames() {
+ System.out.println(INPUT_NAME_MESSAGE);
+ return Arrays.stream(scanner.nextLine()
+ .split(","))
+ .collect(Collectors.toList());
+ }


InputView에서는 입력 값을 어느 정도까지 원하는 형태로 변형해 controller 및 domain에 전달해 주는 것이 가장 이상적일까요?

기존에 저는 InputView에서는 어느 처리도 하지 않길 바랐기에 split과 같은 로직은 모두 Participants와 같은 도메인에서 작업해 Player를 생성하는 형식으로 진행해 왔습니다.
그런데 이번에는 페어와 실제 프론트가 존재했다면, form과 같은 포맷을 통해 뒷단에서 원하는 형태로 전달되지 않을까에 대해 이야기해 보았습니다.

아직 웹에서의 프론트를 작업하기 이전이기에 둘 중 어느 방법이 이상적일지 아직 판단이 서지 않는 상태입니다.

말씀해주신대로, 프론트엔드에서 값을 전달해준다면 적절히 split된 형태의 이름이 전달될 것입니다.
다만 프론트엔드에 대해 고민해보기보다, 현재의 제한된 스펙 내에서 고민할 필요가 있어 보입니다.

말씀해주신대로 InputView가 어디까지 책임을 져야 하는지는 고민이 되는 부분이고, 정답이 없습니다.

저도 리뷰 진행하면서 해당 부분에 대해서는, 최대한 크루의 의견을 존중해서 진행하고 있습니다.

다만 위처럼 split하는 로직이 있다면 이에 대한 검증이 필요해질텐데, 해당 검증 책임을 어디까지 InputView에 줄 것이냐가 문제가 되겠네요.

예를 들어 전체 입력값이 null/empty("")이거나 특정 이름이 null/empty/blank(" ") 인 경우, 어디에서 이것을 검증해주는 것이 좋을까요? 이 부분을 InputView에서 해주는 것이 옳다고 생각되시면 현재 구성으로 문제가 없을 것이고, 그것도 controller에서 검증해주고 싶다면 입력받은 String 을 split 없이 전달해주면 될 것으로 생각되네요.

이 부분은 페어와 함께 논의해보면서 더 좋은 답을 찾아보기를 바랍니다. 😄

의견 감사합니다!

그런데 split(”,”)를 통해 문자열 리스트를 만들었을 때 해당 값이 빈 값이어도 오류가 발생하지 않고 빈 배열이 생성되는 것으로 알고 있습니다.
그렇다면 Name 객체를 생성 시 null과 isBlank()를 통해 빈 값을 검증해 예외를 발생하는데 이러한 예외처리로는 부족할까요?

물론 현재 스펙에서는 split시 빈 배열이 생성되므로, Name에서의 검증을 통해 예외 케이스를 잡아낼 수 있으므로, 위의 검증사항을 모두 구현할 필요는 없습니다.
다만 이러한 경우에 대해 어디에 책임이 있을지, 고민해본다면 좋겠다고 생각해서 코멘트를 드렸습니다. 😄

현재의 구조 상 뷰의 역할이 복잡해지는 면이 있는데, 리뷰를 하면서 저도 함께 고민하게 되네요.
이부분은 앞으로 리뷰 진행하면서도 의견 나눠볼 수 있다면 좋겠습니다. 👍

의견 감사합니다!
저도 항상 입력값 검증에 대해 어디에 역할을 부여해야 하는지에 대해 많은 고민을 해보았습니다.
해당 부분에 대해 2단계 미션을 진행해 보며 좀 더 고민해 본 후 의견 나눌 수 있도록 하겠습니다!

include42

안녕하세요 제이미, 리뷰어 카프카입니다.

코드 작업을 깔끔하게 잘 해주셨네요.
클래스가 잘 분리되어 있어, 금방 구조를 이해할 수 있었습니다. 💯

일부 개선 가능한 부분에 코멘트 남겨두었습니다.
제이미가 남겨준 코멘트에도 답글을 달았는데, 함께 고민해보면 좋겠습니다.
진행하시면서 어려우신 점 있으시면 편하게 dm이나 코멘트 부탁드립니다.

고생 많으셨습니다. 😄

include42 · 2023-03-04T01:25:58Z

src/main/java/blackjack/controller/BlackjackController.java

+ public void run() {
+ Participants participants = getParticipants();
+ BlackjackGame blackjackGame = new BlackjackGame(participants);
+ startGame(participants, blackjackGame);
+ printResult(blackjackGame);
+ }


run의 구성이 아주 깔끔하네요! 😄

include42 · 2023-03-04T01:37:02Z

src/main/java/blackjack/domain/Card.java

+ private static final List<Card> CACHE = new ArrayList<>();
+
+ static {
+ for (CardSuit suit : CardSuit.values()) {
+ generateCard(suit);
+ }
+ }


카드 한 벌을 미리 만들어두자는 아이디어 좋습니다. 💯
다만 캐시라는 용어가 이 역할에 정확히 맞지는 않고, 카드 한벌을 만드는 역할이 Card 클래스에 맡겨져 있어 코드가 복잡해졌다고 생각합니다.
CardFactory라는 클래스를 만들어서 한 벌을 미리 만들어두고 전달해주는 책임을 부여하면 어떨까요? 팩토리 패턴에서 이름을 따온 것인데, 캐시보다 조금 더 명확한 의미로 전달할수 있다고 생각했네요.

카드 캐시를 만드는 기능을 CardFactory 클래스로 분리해보았습니다.
제가 의미를 잘 이해해서 구현했는지 확인해주시면 감사하겠습니다!

그리고 CACHE라는 상수 명은 CARDS로 수정했습니다.
그런데 해당 상수 명이 CACHE라는 용어가 왜 맞지 않다고 생각하셨는지 궁금합니다.
static이었기에 CardFactory와 이전 클래스였던 Card에 있다면 CACHE가 카드에 대한 캐시 데이터라는 유추가 어려워 보일까요?
혹은 아예 해당 부분은 캐시의 역할이 아닌 걸까요?

해당 부분은 제가 조언드린 방향대로 잘 구현해 주셨습니다.
아무래도 별도의 Factory 클래스를 통해서 생성하도록 하면, 코드를 사용하는 입장에서 구현 방향이 명확해질 것이라고 생각했습니다.

그리고 CACHE라는 용어가 해당 개념에 어느정도 부합하기는 합니다.
다만 저는 서버 개발자의 입장에서, 캐시라는 단어를 볼 때 서버가 외부에 전달하는 데이터에 대해 임시 저장하여 전달하는 것을 연상하게 되어서요.(이후 단계에서 배우게 될 수도 있겠네요 😄 ) 그리고 정적인 데이터보다는, 일시적으로 저장되는 데이터라는 개념으로 이해하여 이름을 이상하게 느꼈습니다.
현재는 Factory 내에 들어가서 CACHE라는 이름을 쓰더라도 문제가 안되지만, CardDeck 클래스 내에 있을때에는 조금 더 이해에 시간이 걸린 점도 있었습니다.

이러한 부분은 개인차가 있는 부분이니까요. 이야기해보니 재미있네요 👍

include42 · 2023-03-04T01:40:00Z

src/main/java/blackjack/domain/Name.java

+ private void validateBlank(String name) {
+ if (name == null || name.isBlank()) {
+ throw new IllegalArgumentException("이름은 공백일 수 없습니다.");
+ }
+ }


검증 아주 잘해주셨습니다! 💯
다만 확인해보니, 이름 앞뒤로 공백을 넣고 입력시 잘 작동되더군요. 혹시 명세에 이 부분에 대한 검증을 추가하실 생각이 있으시다면, trim을 통해 해결해보는 것도 좋다고 생각합니다. (필수는 아닙니다 )

검증 명세 추천 감사합니다!
사실 이 부분을 하려고 했었는데 시간상의 이유로 하지 못했던 부분이었습니다.
해당 기능 처리 후 플레이어 이름 중복 처리에 대한 로직으로 수정해보았습니다!
이상적인 중복 검증인지 확인해주시면 감사하겠습니다!
로직 바로가기

해당 로직 확인해 보았을때 잘 구현된 것으로 보았습니다. 바로 구현해주시고 고생하셨습니다 😄

include42 · 2023-03-04T01:45:04Z

src/main/java/blackjack/domain/Participants.java

+ this.participants = participants;
+ }
+
+ public static Participants from(List<String> names) {


생성자를 닫아두고 특정 동작에 의해서만 생성되도록 잘 구현해 주셨네요 💯

include42 · 2023-03-04T01:48:30Z

src/main/java/blackjack/domain/Participants.java

+ public List<Player> getPlayers() {
+ return participants.stream()
+ .filter(participant -> participant instanceof Player)
+ .map(participant -> (Player) participant)
+ .collect(Collectors.toList());
+ }
+
+ public Dealer getDealer() {
+ return (Dealer) participants.stream()
+ .filter(participant -> participant instanceof Dealer)
+ .findAny()
+ .orElseThrow(() -> new IllegalArgumentException("딜러를 " +
+ "찾지 못했습니다."));
+ }


이부분을 보면 instanceof를 통해 섞여있는 클래스들을 구분해주고 있고, 게다 딜러는 예외적 상황에 대한 코드도 추가되어 복잡해진 면이 있네요.
그런데 궁금한 것이, Participants가 List와 Dealer를 따로 필드로 가지면 어떻게 될까요?
이렇게 구현하지 않고 하나의 리스트로 관리하도록 구현한 이유가 궁금합니다.

일급 컬렉션을 위해서라고 생각했는데 생각해보니 Participant 자체가 래퍼 클래스가 아니었네요!
Player와 Dealer가 원래 이름만 갖고 있던 클래스여서 헷갈렸던 것 같습니다.
Players에 대한 리스트와 Dealer로 분리해 저장하도록 수정했습니다!

include42 · 2023-03-04T01:57:21Z

src/main/java/blackjack/view/InputView.java

+ }
+
+ public String readIsContinue(String name) {
+ System.out.printf(INPUT_CONTINUE_MESSAGE, name);


위에서 INPUT_CONTINUE_MESSAGE 마지막에 System.lineSeparator() 를 사용하고 있더군요.
여기서 printf로 name을 인자에 넣어주려고 그렇게 구현한 것일까요? (그냥 궁금해서 질문입니다 😄 )

네 맞습니다.
해당 출력문은 이름과 함께 출력해주어야 하기에 printf를 사용하는 것이 더 좋을 것이라 판단했습니다.
혹시 더 좋은 방법 혹은 추천하는 방법이 있으실까요?

아니요, 좋은 판단이라고 생각되어서 코멘트 남긴 것도 있습니다.
포맷을 통해 인자를 넣어주게 되니, 메시지 상수의 의미가 조금 더 명확해지네요.
리뷰하면서 더 좋은 방법이 떠오르면 코멘트 남기겠습니다.

include42 · 2023-03-04T02:04:28Z

src/main/java/blackjack/view/OutputView.java

+ public void printInitCards(Participants participants) {
+ System.out.printf(INIT_FORMAT, String.join(", ", participants.getPlayerNames()));
+ printDealerInitCard(participants.getDealer());
+ for (Player player : participants.getPlayers()) {
+ printPlayerCards(player);
+ }
+ System.out.println();
+ }


OutputView가 전체적으로 도메인 의존도가 강하고, 단순 출력 이상의 동작까지 수행하고 있다는 생각이 드네요.
이 부분도 이전에 코멘트 주신것처럼 프론트엔드라고 생각해서 책임을 부여하도록 진행하신 걸까요?

로직을 가지는 것이 문제라기보다는, Controller와 서로 다른 layer인데 도메인에 대해 의존하는 부분이 추후 코드를 리팩토링하거나 명세를 변경하게 되었을 때 문제가 될 수 있다고 생각합니다.
이 부분은 페어와도 함께 논의해보면 좋을 것으로 생각됩니다. 그리고 혹시 여유가 난다면(아직 필수는 아닙니다) DTO에 대해 학습해보는 것이 도움이 될 것으로 생각되네요.

https://tecoble.techcourse.co.kr/tags/dto/

DTO 참고 자료 감사합니다!

프론트엔드에 책임을 부여했다기보다는 당시 시간이 없어 뷰의 도메인 의존성 제거를 진행하지 못했습니다.
도메인 의존성 제거를 수행 완료했으며 해당 부분에 대해 확인해주시면 감사하겠습니다!
그런데 아직 DTO를 어느 상황에서부터 사용해야 할지 감이 오지 않는 상태입니다.

이번 미션에서 DTO를 사용하기 적절할까요?
또한, 현재 뷰에서 원하는 출력 상태를 만들기 위해 컨트롤러 및 도메인에서 여러 작업을 수행하는 상태입니다.
이때, 컨트롤러와 도메인에서 뷰를 위한 값을 만드는 로직이 있는 것이 잘못된 것인지 궁금합니다.

작업하시느라 고생하셨습니다.
DTO의 개념을 간단히 이해하셨다면, 우선 controller -> OutputView 로 값을 보내는 부분에 적용해보면 좋을 것 같네요.
OutputView에서 필요한 값들을 정제한 DTO 클래스를 구현하신 뒤, controller에서 해당 값을 전달하도록 로직 수정해주시면 됩니다.

그리고 뷰를 위한 값을 만드는 로직이 있는 것은 당연하다고 생각합니다.
적절하게 값을 가공해서 DTO에 담아 전달하는 것이고, 그렇기 때문에 DTO도 조금 더 뷰에 친화적인 구조가 될 수 있습니다.

이부분은 다음 단계를 진행하면서 해보시면 좋을 것으로 생각되는데,
혹시 진행하면서 어려운 부분이 있다면 말씀해주세요. 👍

DTO에 대한 개념을 확인했지만 아직 정확히 어떻게 사용해야 할지 감이 잡히지 않아 이번 PR 요청 때 적용하지 못했습니다.
좀 더 공부한 본 후 2단계 미션 수행 때 적용해 요청드릴 수 있도록 하겠습니다!

include42 · 2023-03-04T02:05:36Z

src/test/java/blackjack/controller/GameCommandTest.java

+
+ @Test
+ @DisplayName("올바른 입력값이 아니면 예외가 발생한다")
+ void wrongInputTest() {


입력값에 대한 검증인 만큼 NullAndEmptySource를 활용한 메소드를 하나 더 추가해보는 것도 좋겠네요.

실제로 Null에 대한 예외 처리는 이루어지지 않았었네요..!
입력값 검증에 대한 테스트에서 NullAndEmptySource에 대한 테스트 진행을 습관화해야겠습니다 😓

그런데 궁금한 점이 있습니다.
입력값의 경우 반값은 null이 아닌 “”로 입력되는 것으로 알고 있습니다.
그렇기에 null이 아닌 반값은 stream 필터 로직을 통해 예외 반환이 되는 상태입니다.
그래도 null에 대한 예외 처리를 해주는 것이 좋을까요?

해당 테스트 및 로직을 수정하며 고민해본 결과 제가 지금껏 입력값에 대해 Null을 체크한 이유에 대해 고민해보았습니다.
저는 다른 개발자에 의해 해당 객체가 생성되는 경우가 입력을 통한 방법 외의 방법으로 객체를 생성할 수도 있기 때문이라고 생각해왔습니다.
이러한 생각이 맞는지 혹은 다른 이유가 존재하는 것인지 궁금합니다.

위에서 검증 코드가 필요한지 여쭤보셨을 때에는 저도 고민을 하게 되었습니다.
다만 테스트에서는, null에 대한 검증도 필요하다고 생각합니다.
추후 InputView의 구조가 변경되거나, GameCommand의 구조가 변경되면 해당 테스트가 실패할 수도 있습니다.
이를 통해, 구조의 개선이 필요한 것을 미리 알 수 있지요.

물론 말씀해 주신 것처럼 예상 외의 동작을 방어하는 의미도 있겠지만,
기본적으로는 가능한 여러 입력 케이스(성공 케이스, 실패 케이스, 극단적인 케이스)에 대해 테스트를 촘촘히 짜 두는것이 이후의 리팩토링에도 유리하다고 저는 생각합니다.

의견 감사합니다!
확실히 해당 객체에서 일어날 수 있는 예외 사항에 대해 최대한 처리해 두는 것이 추후 리팩터링 및 코드 수정에 유리하겠네요!
어제 책을 읽어보며 "잘못된 일을 하는 것을 처음부터 불가능하도록 발생 자체가 불가능하게 만드는 것이 좋다"라는 이야기가 있었는데 이에 해당되는 이야기 같군요..!
아직 현재 코드 작성 외에는 잘 생각되지 않는데 이에 대해 좀 더 연습해 보도록 하겠습니다!

입력 케이스에 대한 테스트 이야기가 나와 추가적으로 궁금한 점 여쭤봅니다!
테스트를 진행할 때 동일한 동작을 수행하는 테스트의 경우 ParameterizedTest를 통해 여러 번 수행하는 것은 크게 의미가 없을까요?
예를 들어 Name에 검증 및 객체 생성을 성공한다라는 테스트를 위해 "jamie", "include42"와 같이 여러 입력값을 주는 상황을 이야기하고 싶습니다.
들어오는 값 자체는 다르지만 결국 동일한 동작을 하기에 굳이스러울까요?
각 케이스 및 경계 값 등에 대해 한 가지 테스트만 진행하는 것이 더 좋을지 궁금합니다.

정확하게 이해해 주셨네요! 예문까지 적절히 달아주시고, 아주 좋습니다 💯 💯

그리고 말씀해주신 부분에 동의합니다. 다만 그것이 유사한 입력값의 모음이면 필요가 없을 것 같고, 보다 경계값에 가까운 경우라면 필요하다고 생각합니다.
예를 들어, 1자 ~ 10자의 이름을 입력할 수 있다면 아래와 같이 테스트를 구성해보면 좋습니다.

parameterizeTest로 1자, 5자(선택), 10자에 대한 테스트를 구현해서 길이 조건이 바뀌는 경우에 대응

당연히 예외 검증 케이스에서는 0자(null/blank), 11자 에 대한 검증이 필요하겠죠?

만약 정규식 검증에서 문제가 될 부분이 있다면(예: 특정한 특수문자는 허용, 한글 허용 등) 그에 맞는 이름도 추가

저의 경우 이런 케이스가 실제로 있어서, 번체자/간체자/영어가 허용되도록 테스트를 구성해본 적이 있네요 😓

include42 · 2023-03-04T02:08:03Z

src/test/java/blackjack/domain/CardDeckTest.java

+
+ Card card = cardDeck.pick();
+
+ assertThat(CardSuit.values()).contains(card.getSuit());


뽑은 카드의 suit가 CareSuit의 values에 있는 건 조금 당연해보이는 부분이네요.
그보다는 CardDeck이 해당 카드를 포함하지 않는지 (드로우하면 remove해주는 것으로 기억나는데 맞지요?) 검증한다던가, CardDeck의 남은 장수를 검증한다던가 하는게 좋을 것으로 생각됩니다.

그리고 CardDeck에서 카드를 다 뽑으면 어떻게 되나요?
이 부분에 대한 방어로직이 없어보이는데, 당장은 플레이어 수를 생각하면 문제가 안될거같긴 합니다.
만약 이 부분에 대한 명세를 추가하고 싶다면, 테스트부터 만들어보는 식으로 진행하면 좋겠네요.

뽑은 카드의 suit가 CareSuit의 values에 있는 건 조금 당연해보이는 부분이네요. 그보다는 CardDeck이 해당 카드를 포함하지 않는지 (드로우하면 remove해주는 것으로 기억나는데 맞지요?) 검증한다던가, CardDeck의 남은 장수를 검증한다던가 하는게 좋을 것으로 생각됩니다.

해당 방법으로 테스트해볼 생각은 하지 못했네요!
테스트를 생성하면서도 너무 당연하게 아닌가 싶었는데 좋은 테스트 방법이 생각나지 않아 수정하지 못했었습니다.
좋은 테스트 방향에 대해 알려주셔서 감사합니다.
해당 방법으로 수정해두었습니다!

그리고 CardDeck에서 카드를 다 뽑으면 어떻게 되나요? 이 부분에 대한 방어로직이 없어보이는데, 당장은 플레이어 수를 생각하면 문제가 안될거같긴 합니다. 만약 이 부분에 대한 명세를 추가하고 싶다면, 테스트부터 만들어보는 식으로 진행하면 좋겠네요.

21을 넘으면 다음 순서로 넘어가기에 해당 부분까지 미처 생각지 못했네요..!
추가한 카드 소모에 대한 방어 로직은 다음과 같습니다.

참여자 입력은 10명까지 가능하다

카드를 모두 소모한 경우 예외를 반환해 게임을 강제 종료한다

해당 부분 피드백에 대해 적절하게 잘 구현해 주셨네요! 💯
앞으로 테스트를 많이 작성해 보시면서 더 좋은 방향을 배워나가실 것이라 생각합니다.

include42 · 2023-03-04T02:11:43Z

src/test/java/blackjack/domain/DealerTest.java

+ @Test
+ @DisplayName("딜러는 처음에 카드를 한 장만 보여준다")
+ void showFirstCardTest() {
+ Dealer dealer = new Dealer();
+ Card card1 = new Card(CardSuit.HEART, CardNumber.ACE);
+ Card card2 = new Card(CardSuit.HEART, CardNumber.ACE);
+
+ dealer.addCard(card1);
+ dealer.addCard(card2);
+
+ assertThat(dealer.getCards()).contains(card1, card2);
+ }


제가 이 테스트의 목적을 정확하게 파악을 못했습니다. 혹시 어떤 목적으로 만든 테스트일까요?

해당 테스트는 원래 딜러에서 첫 카드의 경우 모두 반환하는 것이 아니라 첫 번째 카드만 반환하려고 했기에 작성된 테스트였습니다.
그런데 코드 수정 과정에서 바뀌게 되어 의미가 없어졌네요..!
해당 메서드는 삭제했습니다.

그에 따른 중복 검증 방법 수정

에이스에 따른 점수 계산 로직 수정

GameResult를 ResultState로 수정

JJ503 · 2023-03-07T22:52:17Z

src/main/java/blackjack/domain/Participants.java

+ List<Player> players = getParticipantsByNames(stripNames(names));
+ validateDuplicate(players);


리뷰 답변 참고
이름 앞뒤 공백 및 예외 처리 로직입니다.
이름 앞뒤 공백에 대한 처리 후 중복된 이름에 대해 처리했습니다.

JJ503 · 2023-03-07T22:58:07Z

src/main/java/blackjack/domain/PlayerCards.java

+ private int getFinalScore(int score) {
+ if (ScoreState.of(score + ACE_HANDLE_SCORE)
+ .isBust()) {
+ return score;
+ }


리뷰 답변 참고
조건문의 부정문의 피하기 위한 메서드 분리 로직입니다.

이부분은 재미있게 잘 구현해 주셨네요! 이렇게 되면 bust의 조건이 바뀌거나 상수값이 바뀌어도 적절히 대응할 수 있고요. (물론 블랙잭 룰이 바뀌지는 않겠지만요!)
잘 구현해 주셨습니다 👍

JJ503 · 2023-03-07T23:10:58Z

src/main/java/blackjack/domain/PlayerCards.java

+ private int getFinalScore(int score) {
+ if (ScoreState.of(score + ACE_HANDLE_SCORE)
+ .isBust()) {
+ return score;
+ }


조건문을 확인해보면 체인닝 메서드에 대해 모두 개행을 해주고 있습니다.
이는 가독성 및 디버깅의 편의를 위해 자동으로 포맷팅 되도록 설정해두었습니다.
그런데 위와 같은 경우도 모두 개행을 해주는 것이 더 좋을까요?

객체지향 생활체조 원칙에서는 한 줄에 점을 하나만 찍으라고 하는데, 그 목적은 각 라인이 하나의 동작만을 수행하도록 하여 가독성을 높이기 위함이라고 저는 생각하고 있습니다.
해당 부분은 애매해보일 수는 있으나, 첫 라인에서 값의 정의를 하고 둘째 라인에서 bust 여부를 검사하므로 개생하는 것이 나쁘지 않다고 생각합니다.

물론 더 좋은 방법은, ScoreState.of(score + ACE_HANDLE_SCORE)를 적절한 이름의 변수로 빼는 것이겠지요.

답변 감사합니다!
변수로 뺀다는 부분을 미처 생각하지 못했네요..!
이 경우는 카프카 말대로 변수를 빼는 것이 좋은 방법이겠네요.
해당 방법으로 수정을 진행하도록 하겠습니다!

JJ503 · 2023-03-07T23:22:28Z

안녕하세요 카프카!
꼼꼼한 1단계 리뷰 감사합니다 😊
피드백에 대한 수정 후 리뷰 재요청 드립니다.

해결 및 추가 궁금한 사항에 대해 코멘트 남겨두었습니다.
이번 리뷰도 잘 부탁드립니다!

include42 · 2023-03-08T00:44:22Z

안녕하세요 제이미, 리뷰어 카프카입니다.
리뷰 요청 확인하였습니다. 코멘트도 정리를 잘 해서 달아주셔서, 리뷰하면서 참고하겠습니다 😄
리뷰는 오늘 저녁 9시 ~ 12시 사이에 진행 예정입니다.

작업하시느라 고생 많으셨습니다 👍

include42

안녕하세요 제이미! 리뷰어 카프카입니다.

코드 잘 정리해 주셨네요. 전반적으로 퀄리티가 더 좋아져서, 크게 고칠 부분이 없었습니다. 💯
일부 개선하면 좋을 부분과, 이전에 논의한 부분에 대해 코멘트 남겨두었으니 확인해보시길 바랍니다.
코멘트로 정리를 잘 해주셔서, 답변을 드리기에도 편했네요. 이 부분은 감사합니다 😄

이번 단계는 여기서 approve 하겠습니다.
고생 많으셨습니다 👍

include42 · 2023-03-08T10:43:53Z

src/main/java/blackjack/domain/Participant.java

+ playerCards.addAll(cards);
+ }
+
+ ScoreState getState() {


전반적으로 점수 계산 로직이 잘 짜여져 있어서, 구조가 많이 깔끔해졌네요 💯

include42 · 2023-03-08T10:46:54Z

src/main/java/blackjack/domain/Player.java

+ public static Player from(String name) {
+ validateName(name);
+ return new Player(new Name(name));
+ }
+
+ private static void validateName(String name) {
+ if (INVALID_NAME.equals(name)) {
+ throw new IllegalArgumentException("딜러라는 이름은 사용할 수 없습니다.");
+ }
+ }


잘못된 값이 들어간 채로 생성되지 않도록 구조를 잘 짜주셨네요. 좋습니다 💯
그리고 검증 로직이 재미있네요 😄 이런 세부적인 명세를 찾아내서 추가해주는 것도 개발자의 중요한 역량이라고 생각합니다 👍

다만 해당 검증 로직에 대한 테스트가 PlayerTest에 없는 것 같던데, 테스트도 추가해 주면 좋겠습니다.

include42 · 2023-03-08T10:49:47Z

src/test/java/blackjack/domain/CardDeckTest.java

+ @DisplayName("52장의 카드를 모두 소진한 경우 오류를 반환한다.")
+ void pickCardMoreThan52Test() {
+ CardDeck cardDeck = new CardDeck();
+
+ assertThatThrownBy(() -> {
+ for (int count = 0; count < 53; count++) {
+ cardDeck.pick();
+ }
+ }).isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("모든 카드가 소진되어 게임을 종료합니다.");
+ }


카드 소진에 대한 테스트를 잘 작성해 주셨네요. 👍

include42 · 2023-03-08T10:50:24Z

src/test/java/blackjack/domain/CardTest.java

+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+class CardTest {


CardTest에 isAce 메소드에 대한 테스트도 추가되면 좋겠네요 👍

include42 · 2023-03-08T10:52:08Z

src/test/java/blackjack/domain/DealerTest.java

+
+ dealer.addCard(card);
+
+ assertThat(dealer.getCards()).contains(card);


containsExactly 나 containsOnly 메소드에 대해 학습해보시고, 적절히 적용해보면 좋을 것으로 생각됩니다.

include42 · 2023-03-08T10:53:22Z

src/test/java/blackjack/domain/PlayerCardsTest.java

+ @DisplayName("에이스가 있을 때의 점수를 계산한다")
+ void calculateAceScoreTest() {
+ PlayerCards playerCards = new PlayerCards();
+ Card one = new Card(CardSuit.SPADE, CardNumber.ACE);
+ Card two = new Card(CardSuit.HEART, CardNumber.ACE);
+ Card three = new Card(CardSuit.HEART, CardNumber.TEN);
+
+ playerCards.add(one);
+ playerCards.add(two);
+ playerCards.add(three);
+
+ assertThat(playerCards.getScore()).isEqualTo(12);
+ }


이부분의 테스트를 두 케이스로 나눠서 작성해보면 좋을 것으로 생각됩니다.

ACE + TEN 처럼, +10이 되어야 하는 경우

현재 테스트처럼, +10이 되지 말아야 하는 경우

테스트는 잘 구현해 주셨습니다 💯

JJ503 added 30 commits February 28, 2023 14:49

docs: 기능 목록 추가

9b6a8a1

docs: 기능 목록 추가

90a412e

feat: 플레이어 생성

7fcafdd

feat: 카드의 구성을 생성

c613c63

refactor: 패키지 이동

e7528c9

feat: 카드 생성

945d2bc

docs: 기능 목록 추가

46b36fa

feat: 카드 덱 생성

547f9f6

docs: 기능 목록 수정

1fafb7e

refactor: 카드에 대한 캐시 적용

a4288fc

feat: 카드를 한 장 나눠주는 기능 추가

a67d041

feat: 플레이어 카드들 생성

b638d38

refactor: 이름 원시값 포장

a76bae1

feat: 플레이어 카드 추가 기능 추가

9d23e34

feat: 플레이어 카드 점수 계산 기능 추가

cd52015

feat: 딜러 생성

fa3cc6d

feat: 처음에 딜러는 카드를 한 장만 공개 기능 추가

0beee9e

feat: 중복된 이름이 있으면 예외 발생 기능 추가

4c4ba95

feat: 카드 섞기 기능 추가

110b31a

feat: 점수 상태 추가

3569235

refactor: 추상 클래스 추가

71ba45d

feat: 카드 두 장 뽑기 기능 추가

736b4d1

refactor: 클래스명 수정

9085ca7

feat: 게임 결과 반환 기능 추가

0400bc2

refactor: 필요 없는 메서드 제거

26d72bf

refactor: 카드 전달 책임 이동

40340a9

refactor: 게임 결과 책임 이동

9265862

feat: 플레이어 이름을 입력받는 기능 추가

760cc54

feat: 딜러와 플레이어에게 나눠준 카드를 출력하는 기능 추가

e7a11df

feat: 플레이어가 카드를 계속 받는 기능 추가

ddb334a

JJ503 added 3 commits March 3, 2023 16:24

feat: 예외 메시지 출력 기능 추가

ed0f3ea

refactor: 상위 패키지 생성

b1fdf0c

Merge remote-tracking branch 'origin/step1' into step1

74c58f4

JJ503 commented Mar 3, 2023

View reviewed changes

include42 requested changes Mar 4, 2023

View reviewed changes

JJ503 added 12 commits March 6, 2023 14:25

refactor: 코드 개행 수정

f1de4da

feat: 이름 앞뒤 공백 제거 기능 추가

41d750d

그에 따른 중복 검증 방법 수정

refactor: participant 리스트를 dealer와 players로 분리

0fe55c1

refactor: 필요 없는 static 키워드 제거

7d73d32

refactor: 카드 캐시 생성 클래스 분리

2248a24

refactor: 점수 계산 로직 수정

cb76a1d

에이스에 따른 점수 계산 로직 수정

rename: 클래스 명 수정

abb720e

GameResult를 ResultState로 수정

refactor: view의 도메인 의존성 제거

79ac183

refactor: 개행 수정

f38488b

fix: command null 입력 오류 해결

d7a6fe3

refactor: 필요없는 테스트 제거

e44e82d

feat: 카드를 모두 소모했을 때의 예외처리 추가

ab01b5b

JJ503 commented Mar 7, 2023

View reviewed changes

refactor: 메서드 순서 변경

1a05194

JJ503 commented Mar 7, 2023

View reviewed changes

include42 approved these changes Mar 8, 2023

View reviewed changes

include42 merged commit d76ec68 into woowacourse:jj503 Mar 8, 2023


		Card card = cardDeck.pick();

		assertThat(CardSuit.values()).contains(card.getSuit());

		List<Player> players = getParticipantsByNames(stripNames(names));
		validateDuplicate(players);


		dealer.addCard(card);

		assertThat(dealer.getCards()).contains(card);

[1단계 - 블랙잭 게임 실행] 제이미(임정수) 미션 제출합니다. #420

[1단계 - 블랙잭 게임 실행] 제이미(임정수) 미션 제출합니다. #420

Conversation

JJ503 commented Mar 3, 2023 • edited Loading

include42 commented Mar 3, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JJ503 Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

include42 Mar 3, 2023 • 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

include42 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

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

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

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

JJ503 Mar 7, 2023 • 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

JJ503 commented Mar 7, 2023

include42 commented Mar 8, 2023

include42 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

Choose a reason for hiding this comment

JJ503 commented Mar 3, 2023 •

edited

Loading

JJ503 Mar 3, 2023 •

edited

Loading

include42 Mar 3, 2023 •

edited

Loading

JJ503 Mar 7, 2023 •

edited

Loading