-
Notifications
You must be signed in to change notification settings - Fork 8
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단계] 오지민 제출합니다 #6
base: Ojimin
Are you sure you want to change the base?
Conversation
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.
안녕하세요 지민님!!
우선 현재까지 구현해주신 부분에 대한 리뷰를 남겼습니다 ㅎㅎ
최종 승,패 결정과 21이 넘었을 경우 카드를 더 뽑지 못한다는 요구사항을 만족시켜주시면 좋을 것 같습니다!!
controller를 언급해 주셨는데 Application 대신 controller 역할을 하는 객체를 통해 로직을 구현해주시면 더 좋을 것 같네요 ㅎㅎ
추가 리뷰는 기능 구현을 완료한 뒤에 남기겠습니다!!
@Override | ||
public String toString() { | ||
return playerList.stream() | ||
.map(Player::getName) | ||
.collect(Collectors.joining(", ")); | ||
} |
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.
toString은 입,출력 문구를 지정하기에는 적절치 못합니다!
도메인에 출력의 책임이 추가되어 SRP를 위반하기 때문이죠!! 참고 글
private List<Card> cardList; | ||
private int sum = 0; |
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.
card가 있다면 합은 언제나 결정할 수 있습니다!!
그래서 sum은 cardList로 부터 추론할 수 있는 값이기에 불필요한 필드입니다 ㅎㅎ
오히려 sum을 두는 것이 실제 card 합과 sum의 합이 다른 정합성의 문제가 발생할 수 있는 것이죠!
public int calculateSum(List<Card> cardList) { | ||
return cardList.stream() | ||
.mapToInt(card -> card.getNumber().getRank()) | ||
.sum(); | ||
} |
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.
calculateSum이 있다면 getSum은 필요 없지 않을까요??
src/main/java/Application.java
Outdated
public static void main(String[] args) { | ||
OutputView.printInputPlayerNameMessage(); | ||
String playerNames = InputView.inputPlayerName(); | ||
List<String> playerNameList = Arrays.asList(playerNames.split(",")); |
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에서 관리하는 것이 어떨까요??
Type randomType = Type.getRandomType(); | ||
Number randomNumber = Number.getRandomNumber(); | ||
//객체 간 중복체크 필요 | ||
this.type = randomType; | ||
this.number = randomNumber; |
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.
카드에 Random을 넣지 않고
잘 섞인 고유한 카드들을 가진 Deck 같은 객체를 만들어서 카드들을 반환해주는 것이 어떨까요?
public Player(String name) { | ||
this.name = name; | ||
this.cardList = new CardList(); | ||
} |
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.
Player와 Dealer가 생성과 동시에 카드 두 장을 가지게 만든 이유가 있을까요??
giveCard(List cards) 와 같은 메서드를 둘 수도 있었을 것 같아요!!
src/main/java/Application.java
Outdated
//2)카드 뽑기 - player, 딜러 순 | ||
//player가 n 을 입력하면 현재 player의 카드를 보여주고 다음턴 | ||
for (Player player : playerList.getPlayerList()) { | ||
while (true) { |
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.
만약 유저의 카드 합이 21이 넘어버린다면 더 이상 카드를 받을 수 없습니다!
return cardList; | ||
} | ||
|
||
public int calculateSum(List<Card> cardList) { |
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.
CardList가 스스로 점수를 계산하는 것 너무 좋습니다 👍👍👍
아직 최종 출력 관련 기능과 controller 코드, test 구현은 미완성되어 계속 수정 후 커밋하겠습니다!