레디 step2 #13

reddevilmidzy · 2024-03-15T10:17:19Z

No description provided.

* docs: 기능 요구 사항 작성 * test: 카드 합 구하기 * feat: 카드 합 구하기 * chore: 패키지 변경 * feat: 참여자 생성 기능 구현 * feat: null 이나 공백인 경우 예외가 발생 기능 구현 * feat: 참여자 이름이 중복시 예외 발생 기능 구현 * feat: 총 참여자의 수는 2이상 8이하가 아닐시 예외 발생 기능 구현 * feat: 카드를 추가하는 기능 구현 * feat: Player가 Packet을 필드로 갖도록 구현 * feat: CardDeck 생성 기능 구현 * test: 플레이어에게 카드 2장을 나눠주는 기능 구현 * refactor: 리스트에서 스택으로 변경 * feat: 참여자에게 카드를 2장씩 분배하는 기능 구현 * feat: 딜러가 카드 2장을 분해하다. * refactor: 스택에서 덱으로 변경 * fix: CardNumber ONE 제거, TEN으로 추가 * feat: 카드 내의 ACE 가 포함된 경우 합이 21이하이면 에이스는 11로 계산 기능 구현 * feat: 카드 합 계산시 ACE 고려하는 로직 추가 * refactor: Packet 클래스 이름을 Hands로 변경 * feat: 참여자의 대답이 y, n 가 아닐시 예외가 발생한다. * feat: 참여자 이름 입력받는 기능 구현 * feat: 플레이어의 시작 카드 출력 기능 구현 * feat: 딜러 카드 출력하는 기능 구현 * docs: 완료한 기능 체크 * feat: answer에 따라 카드를 추가 기능 구현 * refactor: Dealer 필드에서 Players 제거 * feat: 추가된 카드 출력하는 기능 구현 * style: 사용하지 않는 메서드 삭제 * feat: 참여자의 대답이 y일시 한번 더 deal 기능 구현 * feat: 변경된 카드가 있을 경우 출력하는 기능 구현 * feat: 딜러의 카드가 17 이상이 될때까지 카드 받는 기능 구현 * feat: 딜러가 카드를 받을 때 메시지 출력기능 구현 * feat: 모든 참여자의 카드의 합을 출력하는 기능 구현 * feat: hands의 승패 판단 기능 구현 * feat: 참여자의 승패 확인 기능 구현 * style: '플레이어'를 '참여자'로 변경 * fix: 컴파일 에러 수정 * feat: 딜러의 승패를 확인 기능 구현 * docs: 구현해야 할 기능 목록 추가 * feat: 최종 결과 출력하는 기능 구현 * feat: 참가자의 카드의 합이 21을 초과하면 BUST 메시지 출력 기능 구현 * feat: 모든 참여자가 버스트라면 딜러는 딜하지 않는 기능 구현 * feat: 무승부 기능 구현 * fix: 딜러가 버스트일때 참여자가 버스트가 아니면 WIN * feat: 블랙잭 시 메시지 출력후 카드 더 받지 않는 기능 구현 * docs: 블랙잭 규칙 추가 * refactor: 'CardNumber'를 'Rank'로 변경, 'CardShape'를 'Shape'로 변경 및 card 패키지 생성 * refactor: players 관련 메서드의 파라미터 및 변수명 변경 * refactor: 메서드 삭제 및 이름 변경 * refactor: Player 클래스 메서드명 및 순서 변경 * refactor: CardDeck의 generate메서드 스트림 활용 * refactor: Dealer 초기 카드 넘버 상수명 변경 * refactor: Dealer가 Participant 상속받도록 변경 * docs: 기능 요구 사항 수정 * fix: 카드의 사이즈가 같을 때 21에 가까운 카드 합이 이기는 기능 수정 * refactor: Participant 추상클래스 생성 * refactor: Dto의 이름 변경 및 정적 팩터리 메서드 명 변경 * refactor: getCardNames 메서드 동작 로직을 hands 안으로 이동 * refactor: Participant 생성자 접근제한자 변경 및 메서드 정렬 * refactor: Dealer 의 deal을 Dealer 클래스로 이동 * refactor: controller 인덴트 줄이기 및 메서드 분리 * refactor: cardDeckTest 클래스 접근제한자 변경 * refactor: null과 blank 테스트 코드 메서드 분리 * refactor: 중복되는 테스트코드 변수 분리 * style: static 변수 import * refactor: 테스트 코드내에 중복되는 변수를 필드로 선언 * refactor: outputView FORM 상수static 으로 변경 * refactor: 사용하지 않는 필드 및 메서드 삭제 * style: 코드 정렬 * refactor: HandsTest 중복되는 테스트코드 분리 * refactor: ParticipantDto 레코드로 변경 * refactor: Hands 메서드 분리 * refactor: calculateResult 메서드 인덴트 줄이기 * docs: 리팩터링 목록 추가 * refactor: createEmptyPacket 메서드명 createEmptyHands로 수정 * feat: 카드덱이 비었을때 꺼낸 경우 예외 발생 * refactor: Name 객체 포장 * refactor: 승패무 판단을 Result의 책임으로 이동 (재미있게 BiPredicate 활용) * refactor: Game 클래스 삭제 및 책임 분리 * refactor: TestFixture 생성 * style: import 문 순서 변경 * refactor: 테스트코드 변수명 변경 및 테스트 추가 * fix: LinkedHashMap으로 변경하여 참여자 순서 유지 * refactor: System.out.println 대신 %n과 System.lineSeparator 사용 * docs: 리팩터링 목록 수정 * feat: 참가자의 이름에 딜러가 사용될 시 예외 발생 기능 구현 * refactor: isHit 메서드 생성 * docs: 리팩터링 목록 수정 * style: 메서드 위치 변경 * refactor: controller 메서드 추출 * chore: 패키지 정리 * docs: 리팩터링 목록 추가 * feat: 예외 시 재입력 기능 구현 * feat: 커스텀 예외 구현 * refactor: Participant 추상 메서드 사용 * refactor: InvalidPlayerName 예외 클래스 이름 변경 * refactor: 카드 생성 책임을 Card로 변경 및 카드를 캐시하여 가져오는 기능 구현 * docs: 리팩터링 목록 수정 * refactor: cardDeck 생성 책임 위치 변경 --------- Co-authored-by: jinwoo22 <[email protected]>

robinjoon

궁금한 점 몇개 남겼습니다. 같이 미션 화이팅 합시다!!

robinjoon · 2024-03-16T09:17:34Z

src/main/java/controller/BlackJackController.java

+    public void run() {
+        final Names names = inputController.getNames();
+        final Dealer dealer = new Dealer();
+        final Players players = registerPlayers(names);
+
+        final Game game = new Game(dealer, players);
+
+        initHands(game);
+        dealWithPlayers(game);
+        dealerTurn(game);
+
+        printFinalResult(players, dealer);
+    }


Controller란, 어떤 요청이 들어왔을 때 그 요청을 처리할 수 있는 model 에게 그 요청의 처리를 위임하고, 처리 결과를 View에 전달하는 것을 말합니다. 하지만, 우리 미션에서는 요청을 표현하는 것이 없습니다. 그저 프로그램을 실행하면 알아서 동작을 하기 때문에 Controller라는 이름은 부적절하다고 생각합니다!

robinjoon · 2024-03-16T09:21:38Z

src/main/java/domain/Game.java

+    public void initHands(BiConsumer<DealerHandsDto, ParticipantsDto> handsPrinter) {
+        dealer.initHands(players);
+        handsPrinter.accept(DealerHandsDto.from(dealer), ParticipantsDto.of(players));
+    }


도메인 내부에서 간접적으로 뷰의 로직을 주입받아 실행하고 있는데, 코드에 명시적인 의존성은 없지만, 실질적인 의존성은 여전히 남아있습니다. 뷰의 메서드 시그니처가 변경된다면 도메인이 변경되어야 하기 때문입니다. 저도 비슷한 코드를 작성했는데, 구구와 리뷰어 모두에게 이런 취지의 피드백을 받았습니다!

추가적으로, 자바에 기본으로 정의되어있는 함수형 인터페이스는 그 의도를 명확히 전달할 수 없습니다. 꼭 이렇게 하고 싶다면, 별도의 인터페이스를 정의하여 타입으로부터 의도를 설명하는 것이 좋을 것 같습니다.

robinjoon · 2024-03-16T09:43:26Z

src/main/java/domain/amount/Amount.java

+
+import java.util.Objects;
+
+public class Amount {


대단히 모호한 클래스 이름인 것 같습니다.

robinjoon · 2024-03-16T09:45:42Z

src/main/java/domain/card/Card.java

+    public static List<Card> values() {
+        return List.copyOf(CACHE);
+    }


코드를 분석하면, 모든 가능한 카드가 반환된다는 것을 알 수 있지만, 메서드 이름만으로 알 수 있었으면 좋을 것 같습니다.

robinjoon · 2024-03-16T09:49:49Z

src/main/java/domain/participant/Players.java

+    public void forEach(Consumer<? super Player> action) {
+        names.forEach(action);
+    }
+
+    public Stream<Player> filter(Predicate<? super Player> predicate) {
+        return names.stream().filter(predicate);
+    }


매우 활용성이 높은 메서드가 추가되어있네요. 의도대로 사용한다면 유용하겠지만, 이 메서드만 봤을 때, 그 의도를 파악하기 어렵고 오용의 여지가 있어보입니다.

저도 로빈 의견에 동의해요
일단 메서드 시그니처만 봤을 때 의도를 한 눈에 파악하기 힘듭니다.

그리고 범용적이고, 활용성이 높다는건 이 메서드를 매우 많은 곳에서 의존할 수 있다는 것을 의미합니다.
지금 당장은 편하지만 이 메서드 의존도가 점점 높아지면 유지보수 하기 정말 힘들거라 생각해요.

robinjoon · 2024-03-16T09:51:34Z

src/main/java/exception/CustomException.java

+public class CustomException extends RuntimeException {
+
+    private final ErrorCode errorCode;
+
+    public CustomException(final ErrorCode errorCode) {
+        this.errorCode = errorCode;
+    }
+
+    public ErrorCode getErrorCode() {
+        return errorCode;
+    }
+}


커스텀 예외를 사용하셨는데, 커스텀 예외를 사용하신 이유가 궁금합니다!

저는 개인적으로 메시지만으로도 의도를 충분히 드러낼 수 있다면 자바 기본 예외를 사용하는 편인데
특별히 커스텀 예외에서 별도의 동작을 해주는게 아니라면 굳이 커스텀 예외를 사용한 이유가 궁금하네요

오 요 이야기 할이야기가 많은데 글로 적긴 귀찮으니 다음 스터디때 말해드리죠 ㅎ.ㅎ

robinjoon · 2024-03-16T09:57:29Z

src/test/java/domain/participant/PlayerTest.java

+        Assertions.assertThatThrownBy(() -> new Player(name, hands))
+                .isInstanceOf(ReservedPlayerNameException.class);


모든 예외 검증 코드에서, 예외 타입만으로 구분하시는 것 같은데, 정의하신 커스텀 예외의 ErrorCode 는 왜 필요한건가요?

에러 코드마다 매핑된 메시지가 있고 그 메시지들을 view 에서 출력할 때 사용합니다!

seokmyungham

레디~ 코드 잘 읽어봤습니다!
사실 step2에서 크게 구현할 건 따로 없었죠 😎
몇가지 궁금한 사항만 리뷰 남겨놨습니다

step1보다 메서드 네이밍이 점점 나아지고 있는게 보이네요 👍
다음 미션도 화이팅 하시길!!

seokmyungham · 2024-03-16T13:19:10Z

src/main/java/domain/GameResult.java

+    private static boolean winningCondition(final Hands hands, final Hands target) {
+        return (!hands.isBust() && target.isBust()) || (hands.sum() > target.sum() && !hands.isBust());
+    }
+
+    private static boolean tieCondition(final Hands hands, final Hands target) {
+        return hands.sum() == target.sum() && !hands.isBust();
+    }
+
+    private static boolean loseCondition(final Hands hands, final Hands target) {
+        return hands.isBust() || hands.sum() < target.sum() || !target.isBust();


if 안의 조건들을 캡슐화하고 부정 연산자를 사용하지 않도록 하는건 어떨까요?
Robert Martin의 저서 clean code 내용 일부를 같이 남겨드립니다.

G28: 조건을 캡슐화하라

부울 논리는 (if나 while 문에다 넣어 생각하지 않아도) 이해하기 어렵다.
조건의 의도를 분명히 밝히는 함수로 표현하라.

G29: 부정 조건은 피하라

부정 조건은 긍정 조건보다 이해하기 어렵다.
가능하면 긍정 조건으로 표현한다.

seokmyungham · 2024-03-16T14:14:47Z

src/main/java/domain/participant/Players.java

+    public void forEach(Consumer<? super Player> action) {
+        names.forEach(action);
+    }
+
+    public Stream<Player> filter(Predicate<? super Player> predicate) {
+        return names.stream().filter(predicate);
+    }


저도 로빈 의견에 동의해요
일단 메서드 시그니처만 봤을 때 의도를 한 눈에 파악하기 힘듭니다.

그리고 범용적이고, 활용성이 높다는건 이 메서드를 매우 많은 곳에서 의존할 수 있다는 것을 의미합니다.
지금 당장은 편하지만 이 메서드 의존도가 점점 높아지면 유지보수 하기 정말 힘들거라 생각해요.

seokmyungham · 2024-03-16T14:39:08Z

src/main/java/exception/CustomException.java

+public class CustomException extends RuntimeException {
+
+    private final ErrorCode errorCode;
+
+    public CustomException(final ErrorCode errorCode) {
+        this.errorCode = errorCode;
+    }
+
+    public ErrorCode getErrorCode() {
+        return errorCode;
+    }
+}


저는 개인적으로 메시지만으로도 의도를 충분히 드러낼 수 있다면 자바 기본 예외를 사용하는 편인데
특별히 커스텀 예외에서 별도의 동작을 해주는게 아니라면 굳이 커스텀 예외를 사용한 이유가 궁금하네요

seokmyungham · 2024-03-16T14:44:13Z

src/main/java/controller/InputController.java

+public class InputController {
+
+    private final InputView inputView;
+    private final OutputView outputView;


사실 MVC의 Controller의 역할이 무엇인지 생각해보면
해당 컨트롤러는 어떤 Model과 view 사이의 연결다리 역할을 해주는 것인지 의문이 드는데
클래스를 따로 두신 의도가 궁금합니다.

hyxrxn

체스도 화이팅!

hyxrxn · 2024-03-17T05:31:23Z

src/main/java/controller/BlackJackController.java

+        game.dealWithPlayers(
+                inputController::getAnswer,
+                outputView::printHands,
+                outputView::printPlayerEndMessage);


game에 메서드를 넘겨주는군요
궁금한게 이렇게 해도 모델과 뷰가 분리되었다고 할 수 있는건가요??
mvc에 대해 잘 몰라서 물어봅니다!

hyxrxn · 2024-03-17T05:37:50Z

src/main/java/domain/amount/Amount.java

+
+import java.util.Objects;
+
+public class Amount {


이 클래스를 따로 두신 이유가 뭔가요?

요 녀석은 최종 상금을 가지는 클래스인데(이름이 모호하긴 하네요 ㅎ)
배팅금액과 최종상금은 서로 요구사항이 다르다고 판단해 분리하였습니다

배팅금액은 0이하의 값을 가질 수 없지만 최종 상금은 0 이하의 값도 가질 수 있기에 서로를 분리하였습니다.

hyxrxn · 2024-03-17T05:39:40Z

src/main/java/domain/card/Card.java

+    @Override
+    public String toString() {
+        return rank.getName() + shape.getName();
+    }


요것이 도메인의 역할일지요....?!
저는 약간 의문이 드네요...!!
애는 출력과 관련된 형식이라는 생각입니다

hyxrxn · 2024-03-17T05:41:30Z

src/main/java/domain/card/CardDeck.java

+        this.cards = cards;
+    }
+
+    static CardDeck generate(int size) {


사용할 덱의 개수를 설정할 수 있군요

hyxrxn · 2024-03-17T05:43:48Z

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

+    private static ArrayDeque<Card> generate() {
+        final List<Card> deck = new ArrayList<>();
+        for (int i = 0; i < DECK_SIZE; i++) {
+            deck.addAll(Card.values());
+        }
+        Collections.shuffle(deck);
+        return new ArrayDeque<>(deck);
+    }


CardDeck의 generate(6)과 같은 상황 아닌가요....??

hyxrxn · 2024-03-17T05:53:00Z

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

+    public void initHands(final Players players) {
+        for (int i = 0; i < INIT_HANDS_SIZE; i++) {
+            players.forEach(player -> player.add(cardDeck.pop()));
+            super.add(cardDeck.pop());


제가 리뷰어에게 받은 피드백인데
super 키워드를 언제 사용하는지 알아보면 좋을 것 같아요!

hyxrxn · 2024-03-17T05:54:58Z

src/main/java/domain/participant/Hands.java

+    public static Hands createEmptyHands() {
+        return new Hands(new ArrayList<>());
+    }


Hand() {
this(new ArrayList<>());
}

랑 다른 점이 있나요???
이것도 몰라서 물어봅니다 ㅋㅋㅋ
이름에 의미를 담고 싶었던 건가요?

hyxrxn · 2024-03-17T05:56:43Z

src/main/java/domain/participant/Names.java

+    private static void validateSize(final List<Name> names) {
+        if (names.size() < MIN_SIZE || MAX_SIZE < names.size()) {
+            throw new InvalidPlayersSizeException(ErrorCode.INVALID_SIZE);
+        }
+    }
+
+    private static void validateDuplicate(final List<Name> names) {
+        if (names.size() != Set.copyOf(names).size()) {
+            throw new DuplicatePlayerNameException(ErrorCode.DUPLICATE_NAME);
+        }
+    }


저는 Players의 역할이라고 생각했는데
클래스를 따로 두셨군요

hyxrxn · 2024-03-17T05:58:06Z

src/main/java/domain/participant/Player.java

+
+public class Player extends Participant {
+
+    private final BetAmount betAmount;


이렇게 하면 필드가 몇 개일까요

hyxrxn · 2024-03-17T07:46:53Z

src/test/java/domain/CardTest.java

+        final List<Card> values = Card.values();
+
+        Assertions.assertThat(values).hasSize(52);
+        Assertions.assertThat(Set.copyOf(values)).hasSize(52);


제 리뷰어가 굳이 copyOf 쓰지 말고 new HashSet<>(values) 쓰는 것을 추천해주었답니다!

오 이유가 궁금해요!

reddevilmidzy and others added 30 commits March 12, 2024 22:58

docs: 구현 목록 작성

c7a83f1

feat: 배팅 금액이 양의 정수가 아닐 시 예외 발생 기능 구현

2206919

test: player의 canDeal 테스트 추가

dec4970

refactor: testFixture 상수 변경

ffbc5e3

feat: 카드의 갯수가 2장이고 카드의 합이 21인 경우 블랙잭이다.

ee8b66e

fix: 참여자의 이름의 공백이 포함된 경우 제거하여 중복 판단 기능

f37f288

feat: 참가자가 승패에 따라 배팅한 금액 잃거나 받는 기능 구현

285f322

feat: 각 참여자의 배팅 금액을 입력 받는 기능 구현

197f942

feat: 도메인 toString 재정의

93e5b86

feat: 참가자만 블랙잭인 경우 배팅 금액의 1.5배를 받는 기능 구현

8aba953

feat: 참가자와 딜러 모두 블랙잭인 경우 배팅한 금액 돌려 받는 기능 구현

b8b1ef9

feat: 딜러의 최종 수익 계산 기능 구현

d51ecdf

feat: 딜러와 참가자의 최종 수익 출력 기능 구현

4a03db2

fix: test 코드 오류 수정

874cd41

refactor: 딜러가 블랙잭인 경우 출력 메서드 view 로 이동

52a9b0f

fix: 무승부 조건 변경

f5c0b4d

refactor: 배팅 금액을 최종 결과 금액을 변경하는 로직 함수형 프로그래밍 이용

87d8f09

feat: 10으로 나누어 떨어지지 않으면 예외 발생 기능 구현

2988947

refactor: Participant equals & hashCode 에서 Hands 제거

24e020d

style: 코드 리포매팅

2fb529f

refactor: Player의 필드에 BetAmount 포함하는 형태로 구현

09ea27a

refactor: repository 삭제 및 테스트 코드 위치 변경

02f44e0

refactor: Player equals && hashCode 재정의

6b5c8e6

style: 테스트 코드 디스플레이네임 수정

e166122

feat: 배팅 금액이 1억을 초과할 시 예외 발생 기능 구현

e580c16

refactor: controller 변수 명 수정 및 메서드명 수정

930c385

refactor: Result 이름 GameResult로 변경

c401e21

refactor: Answer, Amount, BetAmount package 변경

e5cc892

refactor: 테스트 변수에 final 추가

f22ec42

reddevilmidzy added 10 commits March 15, 2024 13:30

refactor: Player 의 테스트에서 사용하는 생성자 접근제한자 디폴트로 변경

353e923

refactor: betAmount 필드 이름 변경

2fb5001

refactor: 카드의 합이 21이 된 경우 출력 메시지 수정

28f5b8d

refactor: 정적 팩터리 메서드 접근제한자 protected 에서 디폴트로 변경

b9f19af

feat: 수익 출력시 세자리 마다 쉼표를 붙여 출력 기능 구현

c48d744

refactor: players 등록 메서드로 분리

8b9c0ca

refactor: 컨트롤러의 책임을 Game 객체를 만들어서 분리

6ca77d6

style: 주석 제거

4721bfa

refactor: 불필요한 테스트 삭제

7f672e4

docs: 구현 목록 수정

5aa83c3

robinjoon reviewed Mar 16, 2024

View reviewed changes

seokmyungham reviewed Mar 16, 2024

View reviewed changes

hyxrxn reviewed Mar 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

레디 step2 #13

레디 step2 #13

reddevilmidzy commented Mar 15, 2024

robinjoon left a comment

robinjoon Mar 16, 2024

robinjoon Mar 16, 2024

robinjoon Mar 16, 2024

robinjoon Mar 16, 2024

robinjoon Mar 16, 2024

robinjoon Mar 16, 2024

seokmyungham Mar 16, 2024

robinjoon Mar 16, 2024

seokmyungham Mar 16, 2024

reddevilmidzy Mar 17, 2024

robinjoon Mar 16, 2024

reddevilmidzy Mar 17, 2024

seokmyungham left a comment

seokmyungham Mar 16, 2024

seokmyungham Mar 16, 2024

seokmyungham Mar 16, 2024

seokmyungham Mar 16, 2024

hyxrxn left a comment

hyxrxn Mar 17, 2024

hyxrxn Mar 17, 2024

reddevilmidzy Mar 17, 2024

hyxrxn Mar 17, 2024

hyxrxn Mar 17, 2024

hyxrxn Mar 17, 2024

hyxrxn Mar 17, 2024

hyxrxn Mar 17, 2024

hyxrxn Mar 17, 2024

hyxrxn Mar 17, 2024

hyxrxn Mar 17, 2024

reddevilmidzy Mar 17, 2024

		Assertions.assertThatThrownBy(() -> new Player(name, hands))
		.isInstanceOf(ReservedPlayerNameException.class);


		public class Player extends Participant {

		private final BetAmount betAmount;

레디 step2 #13

Are you sure you want to change the base?

레디 step2 #13

Conversation

reddevilmidzy commented Mar 15, 2024

robinjoon 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

seokmyungham 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

hyxrxn 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