로키 1차 구현입니다. #5
Open
로키 1차 구현입니다. #5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
- 딜러의 첫 번째 카드를 출력할 때, 첫 카드를 가져오는 전용 메서드를 삭제하고 컨트롤러에서 뷰로 전달 시 하나만 전달하도록 변경 Co-authored-by: haiseong <wjdgotjd9908@gmail.com>
Libienz
reviewed
Mar 9, 2024
Comment on lines
+6
to
+10
| public class Dealer { | ||
| private static final String DEALER_NAME = "딜러"; | ||
|
|
||
| private final Player player; | ||
|
|
There was a problem hiding this comment.
오호 상속보다 컴포지션 이용하신건가요? 👍
관련해서 로키의 소감 궁금합니다
Comment on lines
+22
to
+32
|
|
||
| private void validateUniqueCard(List<Card> cards) { | ||
| int distinctCount = (int) cards.stream() | ||
| .distinct() | ||
| .count(); | ||
|
|
||
| if (distinctCount != cards.size()) { | ||
| throw new IllegalArgumentException("중복되는 카드가 있습니다."); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
- 유니크한 개수를 센다
- 개수를 비교 검증한다
두가지 일을 하고 있는 부분인 것 같습니다 🤔
Comment on lines
+3
to
+5
| public enum GameResult { | ||
| WIN, LOSE, DRAW; | ||
|
|
Comment on lines
+14
to
+18
|
|
||
| public static Player fromName(String name) { | ||
| return new Player(new PlayerName(name)); | ||
| } | ||
|
|
There was a problem hiding this comment.
from이라는 이름 만으로 충분하지 않을까요 🤔
저는 정적 팩토리 메서드 네이밍 컨벤션을 참고하는 편입니다
There was a problem hiding this comment.
Comment on lines
+22
to
+34
| public Score calculateScore() { | ||
| int scoreValue = cards.stream() | ||
| .mapToInt(Card::getScore) | ||
| .sum(); | ||
|
|
||
| Score score = new Score(scoreValue); | ||
| int currentAceAmount = getAceCount(); | ||
|
|
||
| if (currentAceAmount > 0 && score.isBusted()) { | ||
| return score.convertToSmallAce(currentAceAmount); | ||
| } | ||
| return score; | ||
| } |
There was a problem hiding this comment.
다른 크루분들한테도 한번씩 물어봤던 것인데 로키한테도 물어보고 싶어요
카드에서 점수를 계산하는 로직이 PlayersCards에 있습니다 🤔
만약 Ace는 채점시에 15점으로 계산할 수 있도록 변경해주세요라는 요구사항이 들어오면 현재 로키의 코드에서는 PlayersCards를 찾아야 합니다.
이에 위화감이 있다는 것이 제 개인적인 생각이에요.
즉 PlayersCards는 카드들 관련 로직과 게임의 룰까지 알고 있는 책임이 많은 클래스 같습니다.
이를 전략으로 추상화해볼 수 있을 것 같은데 로키의 의견도 궁금합니다 🙌
Comment on lines
+6
to
+12
| public enum ShapeDescription { | ||
| SPADE(Shape.SPADE, "스페이드"), | ||
| HEART(Shape.HEART, "하트"), | ||
| DIAMOND(Shape.DIAMOND, "다이아몬드"), | ||
| CLOVER(Shape.CLOVER, "클로버"); | ||
|
|
||
| private final Shape shape; |
Comment on lines
+41
to
+42
| return shapeDescription + valueDescription; | ||
| } |
Comment on lines
+17
to
+25
| System.out.println(playerName + "는 한장의 카드를 더 받겠습니까? (y/n)"); | ||
| String choice = scanner.nextLine(); | ||
| if ("y".equals(choice)) { | ||
| return true; | ||
| } | ||
| if ("n".equals(choice)) { | ||
| return false; | ||
| } | ||
| throw new IllegalArgumentException("y 또는 n만 선택할 수 있습니다."); |
There was a problem hiding this comment.
오 .. 이렇게 처리할 수도 있군요 저는 괜히 enum으로 뺏는데 뷰 관련 로직이 한눈에 보이니 좋은 설계처럼 보이는 것 같아요 👍
배워갑니다
Comment on lines
+15
to
+16
| public class BlackJackController { | ||
| public static final int INITIAL_CARDS_COUNT = 2; |
There was a problem hiding this comment.
초기 카드는 개수가 2개이다라는 게임 룰을 컨트롤러가 알고 있는 부분이라고 생각해요 🧐
컨트롤러가 요청을 받고 응답에 집중하는 레이어인 만큼 추상화가 필요하다는 생각입니다 😀
leegwichan
reviewed
Mar 10, 2024
Comment on lines
+9
to
+12
| InputView inputView = new InputView(); | ||
| OutputView outputView = new OutputView(); | ||
| BlackJackController blackJackController = new BlackJackController(inputView, outputView); | ||
| blackJackController.start(); |
There was a problem hiding this comment.
InputView와 OutputView를 Controller에서 만들지 않고, Application에서 만드는 이유가 따로 있을까요?
Comment on lines
+33
to
+35
| public Player getPlayer() { | ||
| return player; | ||
| } |
There was a problem hiding this comment.
상속보다 컴포지션을 이용하기 위해, Dealer에서 Player를 가지고 있는 것은 좋다고 생각해요. 하지만 dealer.getPlayer()라는 메서드가 어색한 것 같아요. 외부에게 필요한 정보가 있다면, player를 넘겨주는 것 보다는 외부에게 정보를 줄 수 있는 메서드를 만드는 것이 좋지 않을까요?
Comment on lines
+6
to
+21
| public static GameResult calculatePlayerResult(Score playerScore, Score dealerScore) { | ||
| if (playerScore.isBusted()) { | ||
| return LOSE; | ||
| } | ||
| if (dealerScore.isBusted()) { | ||
| return WIN; | ||
| } | ||
|
|
||
| if (playerScore.isGreaterThan(dealerScore)) { | ||
| return WIN; | ||
| } | ||
| if (playerScore.isLessThan(dealerScore)) { | ||
| return LOSE; | ||
| } | ||
| return DRAW; | ||
| } |
There was a problem hiding this comment.
아 뭔가... 더 줄일 수 있을 것 같은데... 좋은 방법 없을까? (나도 딱 떠오른게 없네...)
함수형 인터페이스를 적당히 도입하면 될 수 있지 않을까?
Comment on lines
+14
to
+18
|
|
||
| public static Player fromName(String name) { | ||
| return new Player(new PlayerName(name)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Comment on lines
+3
to
+9
| public record PlayerName(String name) { | ||
| public PlayerName { | ||
| if (name.isBlank()) { | ||
| throw new IllegalArgumentException("이름이 비어있습니다."); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
나는 도메인 객체에 Record를 잘 안쓰는 편인데, record를 사용한 이유가 있을까?
Comment on lines
+8
to
+26
| public Players(List<Player> playerGroup) { | ||
| validate(playerGroup); | ||
| this.playerGroup = playerGroup; | ||
| } | ||
|
|
||
| private static void validate(List<Player> players) { | ||
| if (duplicatedNameExist(players)) { | ||
| throw new IllegalArgumentException("중복된 이름이 존재합니다."); | ||
| } | ||
| } | ||
|
|
||
| private static boolean duplicatedNameExist(List<Player> players) { | ||
| int distinctCount = (int) players.stream() | ||
| .map(Player::getName) | ||
| .distinct() | ||
| .count(); | ||
|
|
||
| return distinctCount != players.size(); | ||
| } |
There was a problem hiding this comment.
validate 메서드가 굳이 static 이유가 없을 것 같은데, static으로 사용한 이유가 있을까?
Comment on lines
+9
to
+11
|
|
||
| final int score; | ||
|
|
There was a problem hiding this comment.
해당 필드의 접근 제어자가 private로 되어야 하지 않을까요?
Comment on lines
+1
to
+5
| package blackjack.domain; | ||
|
|
||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; |
Comment on lines
+3
to
+10
| import static blackjack.domain.card.Shape.DIAMOND; | ||
| import static blackjack.domain.card.Value.ACE; | ||
| import static blackjack.domain.card.Value.FOUR; | ||
| import static blackjack.domain.card.Value.KING; | ||
| import static blackjack.domain.card.Value.QUEEN; | ||
| import static blackjack.domain.card.Value.THREE; | ||
| import static blackjack.domain.card.Value.TWO; | ||
| import static org.assertj.core.api.Assertions.assertThat; |
hyxrxn
reviewed
Mar 10, 2024
|
|
||
| ### 카드 | ||
| - [x] 카드는 4가지(스페이드, 클로버, 하트, 다이아몬드) 문양중 하나를 가진다. | ||
| - [x] 카드는 2~9의 숫자 또는 'A', 'J', 'Q', 'K'의 문자를 가진다. |
Comment on lines
+7
to
+20
| if (playerScore.isBusted()) { | ||
| return LOSE; | ||
| } | ||
| if (dealerScore.isBusted()) { | ||
| return WIN; | ||
| } | ||
|
|
||
| if (playerScore.isGreaterThan(dealerScore)) { | ||
| return WIN; | ||
| } | ||
| if (playerScore.isLessThan(dealerScore)) { | ||
| return LOSE; | ||
| } | ||
| return DRAW; |
There was a problem hiding this comment.
메서드가 10줄을 넘었네요!
플레이어가 이기는 조건, 지는 조건을 정리해 메서드로 빼보면 어떨까요?
| public record Score(int value) { | ||
| private static final int MINIMUM_VALUE = 0; | ||
| private static final int BUST_THRESHOLD = 21; | ||
| private static final int DEALER_MINIMUM_SCORE = 17; |
There was a problem hiding this comment.
DEALER_MINIMUM_SCORE는 딜러가 가지고 있어야 할 정보같은데, 어떻게 생각하세요?
Comment on lines
+104
to
+109
| PlayerResultDto dealerResult = PlayerResultDto.from(dealer.getPlayer()); | ||
|
|
||
| List<PlayerResultDto> playerResultDtos = players.getPlayers().stream() | ||
| .map(PlayerResultDto::from) | ||
| .toList(); | ||
| outputView.printCardStatus(dealerResult, playerResultDtos); |
There was a problem hiding this comment.
위의 딜러결과 변수명엔 Dto가 안붙어있고 아래의 플레이어결과 변수명엔 Dto가 붙어있네요.
일관성 있는 네이밍이면 더 좋을 듯 합니다!
Comment on lines
+6
to
+10
| public class Dealer { | ||
| private static final String DEALER_NAME = "딜러"; | ||
|
|
||
| private final Player player; | ||
|
|
| import java.util.Map; | ||
|
|
||
| public class OutputView { | ||
| public void printPlayerInitialCards(List<PlayerDto> playerDtos) { |
There was a problem hiding this comment.
printPlayerCard 와 비슷한 역할을 하는 듯 해 중복 메서드로 보여요~
따로 관리하는 이유가 있을까요?
Comment on lines
+44
to
+59
| static Stream<Arguments> playerAndGameResult() { | ||
| return Stream.of( | ||
| Arguments.arguments( | ||
| List.of(new Card(DIAMOND, ACE), new Card(DIAMOND, QUEEN)), GameResult.WIN | ||
| ), | ||
| Arguments.arguments( | ||
| List.of(new Card(SPADE, ACE), new Card(SPADE, QUEEN)), GameResult.WIN | ||
| ), | ||
| Arguments.arguments( | ||
| List.of(new Card(SPADE, KING), new Card(SPADE, NINE)), GameResult.DRAW | ||
| ), | ||
| Arguments.arguments( | ||
| List.of(new Card(SPADE, TEN), new Card(SPADE, EIGHT)), GameResult.LOSE | ||
| ) | ||
| ); | ||
| } |
| @DisplayName("플레이어가 버스트 되지 않고 점수가 딜러보다 높으면 플레이어가 승리한다.") | ||
| @CsvSource({ | ||
| "20, 19", | ||
| "15, 22" |
| "20, 20, false", | ||
| "19, 20, false", | ||
| }) | ||
| @DisplayName("현재 점수가 다른 점수보다 더 높은지 확인할 수 있다.") |
There was a problem hiding this comment.
이름이 세 개가 다 같은데 의도하신 건가요?
아니면 복붙의 실수...?
Comment on lines
+33
to
+48
| public static Deck createShuffledDeck() { | ||
| List<Card> cards = new ArrayList<>(SHUFFLED_DECK_SIZE); | ||
|
|
||
| for (Shape shape : Shape.values()) { | ||
| cards.addAll(createAllCardsOf(shape)); | ||
| } | ||
| Collections.shuffle(cards); | ||
|
|
||
| return new Deck(cards); | ||
| } | ||
|
|
||
| private static List<Card> createAllCardsOf(Shape shape) { | ||
| return Arrays.stream(Value.values()) | ||
| .map(value -> new Card(shape, value)) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
이 부분... 저와 아주 비슷한데
제 리뷰어가 테스트가 되지 않고 있다고 하더라고요.
로키도 마찬가지네요.
로키는 이 부분의 테스트에 대해 어떻게 생각하나요?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.