[🚀 사이클1 - 미션 (블랙잭 게임 실행)] 이안 미션 제출합니다#1014
[🚀 사이클1 - 미션 (블랙잭 게임 실행)] 이안 미션 제출합니다#1014Chocochip101 merged 48 commits intowoowacourse:eian1106from
Conversation
이름 리스트 공백 검증은 이름을 담을 vo에 존재해야하는 로직이라고 판단하여 InputParserTest에서 해당 테스트를 삭제함
플레이어 히트 로직 구현 버스트 판별 구현 카드 점수 계산 로직 구현 에이스 점수 결정 구현
플레이어의 승패 계산 구현 딜러의 결과 계산 구현 최종 결과 출력 구현
Chocochip101
left a comment
Chocochip101
left a comment
There was a problem hiding this comment.
안녕하세요 이안:) 만나서 반갑습니다, 리뷰어 초코칩입니다 🍪 :)
블랙잭 미션을 잘 구현해주셨네요!👍
피드백이 많아져서 나눠서 리뷰해드리도록 하겠습니다.
코멘트로 남겼으니 확인해보시고 재요청주세요!
|
|
||
| private void validatePlayerNum(List<Player> players){ | ||
| if (players.size() > 5) { | ||
| throw new IllegalArgumentException(); |
There was a problem hiding this comment.
현재 에러 메시지에서는 참여자 수가 어떤 범위를 벗어났는지 알기 어려운 것 같아요 😔
사용자가 입력한 참여자 수나 허용 범위를 함께 보여주면 사용자가 상황을 파악하기 더 쉽지 않을까요?
- 읽어보면 좋을 글: 좋은 에러 메시지를 만드는 6가지 원칙
| static Scanner sc = new Scanner(System.in); // TODO: close 어떻게...? | ||
|
|
||
| // 플레이어 이름 입력받기 | ||
| public static List<String> askName() { |
There was a problem hiding this comment.
static으로 선언한 이유가 무엇인가요? 🧐 우테코에서는 '3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다'라는 제약을 두었는데, 그 이유가 무엇일까요?
There was a problem hiding this comment.
static으로 선언한 이유가 무엇인가요?
-
객체마다 서로 다른 상태(데이터)를 가질 필요가 없는 유틸리티성 클래스라고 판단했습니다.
따라서 인스턴스 생성 비용을 줄이고 어디서든 편리하게 접근할 수 있도록 static 메서드와 필드로 구성했습니다. -
그런데 어디서든 편리하게 접근할 수 있다면, Controller 외에서도 사용할 수 있기에 controller에서 view를 생성하여 사용하거나, Application에서 주입해주는 방식으로 수정해야 할 것 같습니다.
우테코에서는 '3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다'라는 제약을 두었는데, 그 이유가 무엇일까요?
- 인스턴스 변수가 3개 이상이 되면 하나의 클래스가 많은 책임을 지닐 수 있게 될 것 같습니다.
따라서 해당 제약을 통해 클래스를 분리하고, 책임을 분산할 수 있도록 유도하는 것이 아닐까 싶습니다. 🤔
| this.gameService = gameService; | ||
| } | ||
|
|
||
| public void run() { |
|
|
||
| import java.util.List; | ||
|
|
||
| public class Player { |
|
안녕하세요 초코칩 자세한 리뷰 정말 감사합니다!😄 PlayerGroups 내부에서 index를 사용함으로써 발생되는 여러 문제들이 있었습니다. Domain과 Service의 차이와 기준 리팩토링
Java 기본기에 대해서도 아직 많이 부족함을 느낍니다. 휘몰아치는 새로운 내용들에 체력적으로 정신적으로 힘이 들기도 하지만 열심히 나아가겠습니다!
|
Chocochip101
left a comment
Chocochip101
left a comment
There was a problem hiding this comment.
빠르게 수정해주셨네요!
리팩터링 덕분에 코드의 가독성이 좋아졌습니다 👍🏻
조금 더 개선해보면 좋을것 같아 리뷰를 커맨트로 남겨두었으니
확인 후 다시 요청해주세요~
| cards.add(new Card(suit, number)); | ||
| } | ||
| } | ||
| Collections.shuffle(cards); |
There was a problem hiding this comment.
Collection.shuffle이 로직에 존재해서 테스트하기가 어렵군요 😢 어떻게 구조를 바꿔볼 수 있을까요?
| List<PlayerResult> playersEndStatus = gameService.getPlayersStatus(); | ||
| Map<String, Integer> playersTotalScore = gameService.getPlayersTotalScore(); |
There was a problem hiding this comment.
DealerResult, PlayerResult는 중복 같아 보입니다. 이처럼 객체를 활용하고 싶은데, 무언가 감이 잡히질 않습니다.
PlayerResult와 DealerResult의 차이는 점수 필드 차이인데, 여기서는 totalScore 따로 주고 있네요. PlayerResult를 삭제하고 DealerResult를 GameResult(?)로 바꿔서 중복을 줄일 수 있지 않을까요?
private void printFinalStatus() {
GameResult dealerResult = gameService.getDealerResult();
ResultView.printCardSumResult(dealerResult);
List<GameResult> playersResults = gameService.getPlayersStatus();
ResultView.printCardSumResult(playersResults);
}
Chocochip101
left a comment
Chocochip101
left a comment
There was a problem hiding this comment.
안녕하세요, 이안 😄
이번 사이클에서 구현해야 하는 것은 충족한 듯하여 다음 사이클을 위해 Approve & Merge합니다 👍🏻
고민해보면 좋을 코멘트 남겨두었으니, 확인해주세요.
수고하셨습니다~
| } | ||
| } | ||
|
|
||
| Collections.shuffle(cards); |
There was a problem hiding this comment.
이렇게 되면 Deck의 응집도가 낮아질 것 같아요.(덱이라는 것은 생성되는 순간 셔플된 카드의 상태를 뜻하는 것일테니까요). 또한 결국에 서비스에서 initCardDeck의 테스트가 제어 불가능한 Collections.shuffle(cards); 테스트가 불가능해보여요.
그럼 요구사항이 'Deck에서 셔플하되, Deck은 테스트 가능해야한다.' 가 되겠네요. 이때 활용할 수 있는게 전략 패턴이에요.
셔플 전략을 외부로 분리해서 테스트 가능하게 하는거죠.
public interface ShuffleStrategy {
void shuffle(List<Card> cards);
}
public class RandomShuffleStrategy implements ShuffleStrategy {
@Override
public void shuffle(List<Card> cards) {
Collections.shuffle(cards);
}
}
public class CardDeck {
private final Deque<Card> cardDeck = new ArrayDeque<>();
public CardDeck(List<Card> cards, ShuffleStrategy shuffleStrategy) {
shuffleStrategy.shuffle(cards);
addAllCards(cards);
}
....
}
// 테스트에서는 테스트 케이스를 통제 가능하기 위해 섞지 않는다.
class NoShuffleStrategy implements ShuffleStrategy {
@Override
public void shuffle(List<Card> cards) {
// do nothing
}
}- 참고: 디자인 패턴
| playerGroups = new PlayerGroups(playerNames); | ||
| } | ||
|
|
||
| public void initAllPlayerCard() { |
| return playerGroups.getDealerResult(); | ||
| } | ||
|
|
||
| private CardDeck initCardDeck() { |
|
|
||
| public class HandCards { | ||
| private static final int MAX_SCORE = 21; | ||
| private static final int ACE_BONUS_SCORE = 10; |
There was a problem hiding this comment.
현재 에이스를 1 또는 11로직이 드러나있지 않아요.(TrumpNumber를 보면 에이스에는 1만 존재하고 11이 없죠.)
만약 게임 로직이 에이스 점수를 1 또는 13으로 바꾼다고 가정해볼게요. 개발자가 이를 변경하려면 Hand의 로직을 변경해야하고 계산 로직에 익숙하지 않다면, 시간을 소모하게 되어요.
1 또는 11의 의미가 그 자체로 드러날 수 있도록, TrumpNumber의 score를 int value;가 아닌 List<Integer> 고려해봐도 좋을 것 같아요.
ACE("A", listOf(1, 11)),
TWO("2", listOf(2)),
3 participants
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?어떤 부분에 집중하여 리뷰해야 할까요?
1. 데이터 단순 조회의 getter와 잘못된 설계에 사용된 getter의 차이
그래서 도메인 데이터 처리 관련 로직은 최대한 가장 원초적인 도메인 안으로 밀어 넣었습니다. 이렇게 하니 호출하는 상위 객체에는 관련 로직이 없긴 하지만, 해당 메소드를 호출하기 위해 상위객체-서비스-컨트롤러단까지 동명(혹은 유사한 이름의) 메소드로 해당 메소드를 체인처럼 호출해야했는데(처리 로직은 도메인에만 존재), 이 방식이 맞는 걸까요?
2. 일급컬렉션의 사용
3. DTO
현재는 돌아가는 구현에 집중하느라 DTO를 작성하지 못했습니다. 지금 코드에서도 ResultView에 대해서는 전반적으로 DTO를 사용해 리팩토링해야한다고 생각합니다. 그런데 name(String) 처럼 정말 단순한 데이터도 DTO로 감싸야 하는지, 과한 것은 아닌지, 그 선이 어디인지가 고민됩니다.
처음에는 DTO 사용에 대해 무지했습니다. 이번 미션을 진행하면서 DTO에 대해서 배웠고 필요성을 직접 체화 했던 것 같습니다. 직접 깨달은 것은 아래와 같은데, 제대로 이해한 것이 맞는지 궁금합니다
4. Player와 Dealer
5. TDD
초반 이후에는 구현에 쫓겨 거의 TDD를 활용하지 못했습니다. 저는 원래 TDD가 그래도 기본 도메인 구조와 메소드 이름, 메소드 인자 및 리턴값까지는 생각을 해서 스켈레톤 코드로 만들어 놓고 테스트 RED를 만든 다음 이후 로직을 짜 GREEN으로 만들고 리팩토링을 하는 거라고 생각했습니다. 그런데 다른 크루들과 이야기를 나누다보니 컴파일 에러도 RED의 일종이며, 실제로 엄격한 TDD에서는 정말 테스트 코드 작성 전에는 한 줄의 기능 코드도 안 쓰는 경우가 있다는 걸 알게 되었습니다. 그러나 저는 미리 생각해서 커다란 기능(요구사항에서 입출력값이 명확히 존재하는 가장 윗 단계의 기능)정도는 스켈레톤 코드로 작성해놓는 걸 선호하는데, 이러면 TDD에 본질에 어긋나는 건지도 궁금합니다.
테스트 코드를 작성 → 기능 구현 → 통과 확인을 과정을 반복하는데, 책임을 다른 객체에게 주어야 한다는 것을 알았을 땐 테스트 코드의 변경이 필요했습니다.
6. git commit의 역할
7. 리팩토링