Skip to content
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

Open
wants to merge 59 commits into
base: main
Choose a base branch
from

Conversation

HaiSeong
Copy link

@HaiSeong HaiSeong commented Mar 8, 2024

No description provided.

hw0603 and others added 30 commits March 5, 2024 16:30
hw0603 and others added 27 commits March 7, 2024 20:07
- 딜러의 첫 번째 카드를 출력할 때, 첫 카드를 가져오는 전용 메서드를 삭제하고 컨트롤러에서 뷰로 전달 시 하나만 전달하도록 변경

Co-authored-by: haiseong <[email protected]>
Copy link
Member

@Libienz Libienz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 로키 🙌
6기 크루 리비입니다 👍

제 개인적인 생각으로는 블랙잭 미션이 많이 어려웠던 것 같아요 🫠
로키 코드 살펴보면서 제가 헤매고 있던 부분에 답을 찾기도 하는 등 많은 도움 얻을 수 있었던 것 같습니다 😀

이어지는 블랙잭 미션도 화이팅입니다 💪🏻

Comment on lines +6 to +10
public class Dealer {
private static final String DEALER_NAME = "딜러";

private final Player player;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오호 상속보다 컴포지션 이용하신건가요? 👍
관련해서 로키의 소감 궁금합니다

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 컴포지션 이용한 이유가 궁금합니다~~

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("중복되는 카드가 있습니다.");
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 유니크한 개수를 센다
  • 개수를 비교 검증한다

두가지 일을 하고 있는 부분인 것 같습니다 🤔

Comment on lines +3 to +5
public enum GameResult {
WIN, LOSE, DRAW;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

비기는 경우를 생각하셨군요 👍

Comment on lines +14 to +18

public static Player fromName(String name) {
return new Player(new PlayerName(name));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from이라는 이름 만으로 충분하지 않을까요 🤔
저는 정적 팩토리 메서드 네이밍 컨벤션을 참고하는 편입니다

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}
Copy link
Member

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는 카드들 관련 로직과 게임의 룰까지 알고 있는 책임이 많은 클래스 같습니다.

이를 전략으로 추상화해볼 수 있을 것 같은데 로키의 의견도 궁금합니다 🙌

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

뷰로직을 잘 분리해주신 것 같습니다 👍

Comment on lines +41 to +42
return shapeDescription + valueDescription;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringBuilder 혹은 join이 좋아보입니다 😀

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만 선택할 수 있습니다.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 .. 이렇게 처리할 수도 있군요 저는 괜히 enum으로 뺏는데 뷰 관련 로직이 한눈에 보이니 좋은 설계처럼 보이는 것 같아요 👍
배워갑니다

Comment on lines +15 to +16
public class BlackJackController {
public static final int INITIAL_CARDS_COUNT = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

초기 카드는 개수가 2개이다라는 게임 룰을 컨트롤러가 알고 있는 부분이라고 생각해요 🧐
컨트롤러가 요청을 받고 응답에 집중하는 레이어인 만큼 추상화가 필요하다는 생각입니다 😀

Copy link
Member

@leegwichan leegwichan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi 로키! 내가 최대한 보이는 부분 위주로 작성해봤어~
내 의견과 다른 부분이나 이해되지 않는 부분 있다면 코멘트 달아줘!

Comment on lines +9 to +12
InputView inputView = new InputView();
OutputView outputView = new OutputView();
BlackJackController blackJackController = new BlackJackController(inputView, outputView);
blackJackController.start();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InputViewOutputView를 Controller에서 만들지 않고, Application에서 만드는 이유가 따로 있을까요?

Comment on lines +33 to +35
public Player getPlayer() {
return player;
}
Copy link
Member

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를 넘겨주는 것 보다는 외부에게 정보를 줄 수 있는 메서드를 만드는 것이 좋지 않을까요?

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;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 뭔가... 더 줄일 수 있을 것 같은데... 좋은 방법 없을까? (나도 딱 떠오른게 없네...)
함수형 인터페이스를 적당히 도입하면 될 수 있지 않을까?

Comment on lines +14 to +18

public static Player fromName(String name) {
return new Player(new PlayerName(name));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +3 to +9
public record PlayerName(String name) {
public PlayerName {
if (name.isBlank()) {
throw new IllegalArgumentException("이름이 비어있습니다.");
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

나는 도메인 객체에 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();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate 메서드가 굳이 static 이유가 없을 것 같은데, static으로 사용한 이유가 있을까?

Comment on lines +9 to +11

final int score;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 필드의 접근 제어자가 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

공백 지워주세요~~

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@hyxrxn hyxrxn left a 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'의 문자를 가진다.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

표현이 엄밀해서 좋네요👍

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;
Copy link
Member

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위의 딜러결과 변수명엔 Dto가 안붙어있고 아래의 플레이어결과 변수명엔 Dto가 붙어있네요.
일관성 있는 네이밍이면 더 좋을 듯 합니다!

Comment on lines +6 to +10
public class Dealer {
private static final String DEALER_NAME = "딜러";

private final Player player;

Copy link
Member

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
)
);
}
Copy link
Member

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"
Copy link
Member

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("현재 점수가 다른 점수보다 더 높은지 확인할 수 있다.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이름이 세 개가 다 같은데 의도하신 건가요?
아니면 복붙의 실수...?

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();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분... 저와 아주 비슷한데
제 리뷰어가 테스트가 되지 않고 있다고 하더라고요.
로키도 마찬가지네요.
로키는 이 부분의 테스트에 대해 어떻게 생각하나요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants