-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
로키 1차 구현입니다. #5
base: main
Are you sure you want to change the base?
로키 1차 구현입니다. #5
Conversation
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
Co-authored-by: haiseong <[email protected]>
- 딜러의 첫 번째 카드를 출력할 때, 첫 카드를 가져오는 전용 메서드를 삭제하고 컨트롤러에서 뷰로 전달 시 하나만 전달하도록 변경 Co-authored-by: haiseong <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 로키 🙌
6기 크루 리비입니다 👍
제 개인적인 생각으로는 블랙잭 미션이 많이 어려웠던 것 같아요 🫠
로키 코드 살펴보면서 제가 헤매고 있던 부분에 답을 찾기도 하는 등 많은 도움 얻을 수 있었던 것 같습니다 😀
이어지는 블랙잭 미션도 화이팅입니다 💪🏻
public class Dealer { | ||
private static final String DEALER_NAME = "딜러"; | ||
|
||
private final Player player; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호 상속보다 컴포지션 이용하신건가요? 👍
관련해서 로키의 소감 궁금합니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 컴포지션 이용한 이유가 궁금합니다~~
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 유니크한 개수를 센다
- 개수를 비교 검증한다
두가지 일을 하고 있는 부분인 것 같습니다 🤔
public enum GameResult { | ||
WIN, LOSE, DRAW; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
비기는 경우를 생각하셨군요 👍
|
||
public static Player fromName(String name) { | ||
return new Player(new PlayerName(name)); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from
이라는 이름 만으로 충분하지 않을까요 🤔
저는 정적 팩토리 메서드 네이밍 컨벤션을 참고하는 편입니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다른 크루분들한테도 한번씩 물어봤던 것인데 로키한테도 물어보고 싶어요
카드에서 점수를 계산하는 로직이 PlayersCards
에 있습니다 🤔
만약 Ace는 채점시에 15점으로 계산할 수 있도록 변경해주세요
라는 요구사항이 들어오면 현재 로키의 코드에서는 PlayersCards를 찾아야 합니다.
이에 위화감이 있다는 것이 제 개인적인 생각이에요.
즉 PlayersCards
는 카드들 관련 로직과 게임의 룰까지 알고 있는 책임이 많은 클래스 같습니다.
이를 전략으로 추상화해볼 수 있을 것 같은데 로키의 의견도 궁금합니다 🙌
public enum ShapeDescription { | ||
SPADE(Shape.SPADE, "스페이드"), | ||
HEART(Shape.HEART, "하트"), | ||
DIAMOND(Shape.DIAMOND, "다이아몬드"), | ||
CLOVER(Shape.CLOVER, "클로버"); | ||
|
||
private final Shape shape; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
뷰로직을 잘 분리해주신 것 같습니다 👍
return shapeDescription + valueDescription; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringBuilder 혹은 join이 좋아보입니다 😀
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 .. 이렇게 처리할 수도 있군요 저는 괜히 enum으로 뺏는데 뷰 관련 로직이 한눈에 보이니 좋은 설계처럼 보이는 것 같아요 👍
배워갑니다
public class BlackJackController { | ||
public static final int INITIAL_CARDS_COUNT = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
초기 카드는 개수가 2개이다
라는 게임 룰을 컨트롤러가 알고 있는 부분이라고 생각해요 🧐
컨트롤러가 요청을 받고 응답에 집중하는 레이어인 만큼 추상화가 필요하다는 생각입니다 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi 로키! 내가 최대한 보이는 부분 위주로 작성해봤어~
내 의견과 다른 부분이나 이해되지 않는 부분 있다면 코멘트 달아줘!
InputView inputView = new InputView(); | ||
OutputView outputView = new OutputView(); | ||
BlackJackController blackJackController = new BlackJackController(inputView, outputView); | ||
blackJackController.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InputView
와 OutputView
를 Controller에서 만들지 않고, Application에서 만드는 이유가 따로 있을까요?
public Player getPlayer() { | ||
return player; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상속보다 컴포지션을 이용하기 위해, Dealer
에서 Player
를 가지고 있는 것은 좋다고 생각해요. 하지만 dealer.getPlayer()
라는 메서드가 어색한 것 같아요. 외부에게 필요한 정보가 있다면, player
를 넘겨주는 것 보다는 외부에게 정보를 줄 수 있는 메서드를 만드는 것이 좋지 않을까요?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 뭔가... 더 줄일 수 있을 것 같은데... 좋은 방법 없을까? (나도 딱 떠오른게 없네...)
함수형 인터페이스를 적당히 도입하면 될 수 있지 않을까?
|
||
public static Player fromName(String name) { | ||
return new Player(new PlayerName(name)); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public record PlayerName(String name) { | ||
public PlayerName { | ||
if (name.isBlank()) { | ||
throw new IllegalArgumentException("이름이 비어있습니다."); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
나는 도메인 객체에 Record를 잘 안쓰는 편인데, record를 사용한 이유가 있을까?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate
메서드가 굳이 static 이유가 없을 것 같은데, static으로 사용한 이유가 있을까?
|
||
final int score; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 필드의 접근 제어자가 private
로 되어야 하지 않을까요?
package blackjack.domain; | ||
|
||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.api.Assertions.assertThatThrownBy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공백 지워주세요~~
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로키 안녕하세요! 아토입니다🤭
다른 분들에 비해 코드가 전반적으로 비슷해서... 아주 중요하게 할 말은 없네요.
컴포지션을 사용한 이유가 가장 궁금한데 대답 기다리겠습니다!
|
||
### 카드 | ||
- [x] 카드는 4가지(스페이드, 클로버, 하트, 다이아몬드) 문양중 하나를 가진다. | ||
- [x] 카드는 2~9의 숫자 또는 'A', 'J', 'Q', 'K'의 문자를 가진다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
표현이 엄밀해서 좋네요👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드가 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEALER_MINIMUM_SCORE
는 딜러가 가지고 있어야 할 정보같은데, 어떻게 생각하세요?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위의 딜러결과 변수명엔 Dto가 안붙어있고 아래의 플레이어결과 변수명엔 Dto가 붙어있네요.
일관성 있는 네이밍이면 더 좋을 듯 합니다!
public class Dealer { | ||
private static final String DEALER_NAME = "딜러"; | ||
|
||
private final Player player; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 컴포지션 이용한 이유가 궁금합니다~~
import java.util.Map; | ||
|
||
public class OutputView { | ||
public void printPlayerInitialCards(List<PlayerDto> playerDtos) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
printPlayerCard
와 비슷한 역할을 하는 듯 해 중복 메서드로 보여요~
따로 관리하는 이유가 있을까요?
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 | ||
) | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 방법이 있군요... 👍
@DisplayName("플레이어가 버스트 되지 않고 점수가 딜러보다 높으면 플레이어가 승리한다.") | ||
@CsvSource({ | ||
"20, 19", | ||
"15, 22" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 소스는 DisplayName과 맞지 않는 것 같아요
"20, 20, false", | ||
"19, 20, false", | ||
}) | ||
@DisplayName("현재 점수가 다른 점수보다 더 높은지 확인할 수 있다.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이름이 세 개가 다 같은데 의도하신 건가요?
아니면 복붙의 실수...?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분... 저와 아주 비슷한데
제 리뷰어가 테스트가 되지 않고 있다고 하더라고요.
로키도 마찬가지네요.
로키는 이 부분의 테스트에 대해 어떻게 생각하나요?
No description provided.