[220323] 블랙잭 미션 피드백을 통해 배운 점

이번 블랙잭 미션이 끝났다!

 

확실히 자동차 경주 미션, 로또 미션에 비해선 난이도가 올라간데다가,

리뷰어님께서 굉장히 많이 핀초리 피드백을 해주셔서 포스팅할 거리도 정말 많았다.

(1단계 conversation 115개, 2단계 conversation 116개로 총 200개가 넘는 피드백 ㅎㅎ)

 

블랙잭 미션 주 내용

- Dealer, Player의 중복 로직을 줄이기 위해 interface 또는 abstract class를 익히고 활용해보는 시간을 가졌다.
- 또한, 블랙잭 결과(Blackjack, Bust, Hit 등)에 따른 다양한 상태를 객체로 만들어 사용하는 상태패턴 또한 맛볼 수 있었다.

 

많은 피드백 중에서 내가 인상깊게 배운 점들을 뽑아서 정리해보려 한다.

 

전체 피드백은 여기서 볼 수 있다.

1단계: https://github.com/woowacourse/java-blackjack/pull/253/

2단계: https://github.com/woowacourse/java-blackjack/pull/308

---

캐싱을 통해 성능을 높이자

로또 미션에서도 1 ~ 45의 번호를 캐싱하여 성능을 높였었는데,

이번 미션에서 캐싱을 적용하는 과정에서 몇 번 실수를 한 덕분에 캐싱 방법을 정리하고 포스팅할 수 있었다.

https://kth990303.tistory.com/287

[JAVA] Cache를 이용한 재사용으로 성능을 높이자

자세한 내용은 포스팅으로 따로 작성했기 때문에, 이번 포스팅에선 간단하게 코드 요약만!

private static final List<Card> CARDS;

static {
    CARDS = Arrays.stream(Suit.values())
            .flatMap(suit -> Arrays.stream(Denomination.values())
                    .map(denomination -> new Card(suit, denomination)))
            .collect(Collectors.toUnmodifiableList());
}

위와 같이 static 블록에서 카드들을 캐싱해주고,

public static Card from(Suit suit, Denomination denomination) {
    return CARDS.stream()
            .filter(card -> card.suit == suit && card.denomination == denomination)
            .findFirst()
            .orElseThrow(() -> new IllegalArgumentException(ERROR_MESSAGE_INVALID_CARD));
}

정적 팩토리 메서드를 이용해주어 캐싱된 카드를 반환(재사용)해주었다!

여기까지가 1단계에서 구현한 내용이다.

2단계에서 추가로 들어온 핀초리!

그러나 위 정적 팩토리 메서드에서 filter로 카드들을 확인하는 과정에서 O(N)의 시간복잡도가 소모되는데,

Map 자료구조를 이용해주면 O(1)로 더 성능을 높일 수 있었다.

따라서 아래와 같이 변경해주었다.

private static final Map<String, Card> CARDS = new HashMap<>();

static {
    for (Suit suit : Suit.values()) {
        addCard(suit);
    }
}

private static void addCard(Suit suit) {
    for (Denomination denomination : Denomination.values()) {
        CARDS.put(denomination.getDenomination().concat(suit.getName()), new Card(suit, denomination));
    }
}

public static Card from(Suit suit, Denomination denomination) {
    return CARDS.get(denomination.getDenomination().concat(suit.getName()));
}

정적 팩토리 메서드 from을 보면 이제 Map에서 key에 해당되는 value를 빼주는 작업으로 O(1)에 카드를 리턴할 수 있게 됐다. 추가로, 문자열을 붙이는 부분도 성능개선을 위해 +가 아닌 concat을 사용해주었다.

---

VO를 이용해 원시값을 포장하자

VO에 대한 개념을 이번 피드백을 통해 제대로 알게 됐다.

그 전엔 단순히 VO는 가장 작은 단위의 기본 클래스 정도로만 인지하고 있었는데, 피드백을 통해 고치면서 VO에 대한 개념을 잡을 수 있었다.

https://kth990303.tistory.com/288

[JAVA] VO(Value Object)로 원시값을 포장해보자

따로 포스팅을 했으니 읽어보면 좋을 듯하다.

요약하자면, VO를 정의한 것 자체가 원시값을 포장하기 위함이므로 equals 재정의 필수, VO를 만들었을 경우 그에 대한 검증 및 책임을 위임해줄 수 있다.

카드 점수 VO, 베팅 금액 VO 또한 만들어주었다!

---

View, Controller에서 도메인 로직을 최소화하자

view에서 도메인 로직을 의존할 경우, 도메인 로직이 수정될 때 view의 수정도 불가피하다.

변경에 유연한 코드가 되기 위해선 의존성을 낮춰야 한다는 소리는 이제 귀에 딱지가 앉을 정도로 많이 들었을 것이다.

view에서 participant의 점수 계산 로직을 의존한다.

view에서 도메인 로직에 의존하지 말고, 도메인에서 처리된 반환값을 view에 넘겨주는 방법으로 수정하자!

// AS-IS
System.out.printf(OUTPUT_MESSAGE_PARTICIPANT_GAME_RESULT, participant.getName(), playerCardsInfo,
                participant.calculateFinalScore());
                
// TO-DO
System.out.printf(OUTPUT_MESSAGE_PARTICIPANT_GAME_RESULT, participant.getName(), playerCardsInfo,
                participant.getScore());

위와 같이 값을 넘겨주거나 아예 이름, 점수값 자체를 넘겨주는 것이 좋다.

또는 DTO를 넘겨주도록 하자.

 

컨트롤러에서도 위와 같은 이유로 domain 로직을 최소화하는 것이 좋다.

컨트롤러는 domain 로직의 결과값을 view에 넘겨주고, view에서 받은 입력을 domain에게 넘겨주어 비즈니스 로직을 실행하는 데에 충실해야 한다.

---

부생성자 또는 정적 팩토리 메서드를 고려하자

위와 같이 각자 다른 타입의 파라미터를 받는 생성자 (부생성자 또는 정적 팩토리 메서드)가 존재할 경우,

테스트가 쉬워지고, 클래스의 사용성이 높아진다.

 

특히 아래와 같은 경우에서 장점을 뚜렷하게 볼 수 있다.

카드들의 합계를 구하는 테스트 코드

카드들의 합계를 구하는 테스트 코드인데, 

합계를 구하기 위해 receiveCard를 해준 후에 테스트를 하고 있는 상황이다.

 

List<Card> cards 또는 Card... cards와 같은 타입의 파라미터를 받아 생성할 수 있다면, receiveCard 메서드 없이 바로 calculateScore() 메서드를 사용할 수 있어 테스트 의도가 명확해진다.

public Cards(Set<Card> cards) {
    this.cards = cards;
}

public Cards(List<Card> cards) {
    this(new HashSet<>(cards));
}

public Cards(Card... cards) {
    this(List.of(cards));
}

현재 코드는 이렇게 바뀌었으며,

덕분에 Cards를 상태로 가지고 있는 다른 객체에서 타입 변환없이 사용할 수 있게 되었다. (클래스 사용성이 높아졌다.)

---

값을 꺼내지 말고 메시지를 던지자

이 원칙을 모르는 사람은 없을 것이다.

그렇지만 몇번을 강조해도 부족하지 않다고 생각한다.

위 코드에서 어디가 잘못됐는지 보이는가?

각 카드의 denomination 값을 꺼내고, Denomination.ACE 와 같은지 확인해주고 있다.

값을 꺼내지 말고 Card에게 아래와 같이 메시지를 보내주자.

 

"너 ACE니?"

 

따라서 아래와 같이 Card 클래스에 메서드를 추가하여 카드에게 메시지를 던져주는 방식으로 바꿔줄 수 있다.

public boolean isAce() {
    return denomination == Denomination.ACE;
}

 

메시지를 던지지 않고 값을 꺼내는 행위는 객체의 책임 할당을 적절하지 못하게 처리할 위험 가능성을 높인다.

또 다른 예시를 보자.

Bust 판단여부는 Cards에서 충분히 처리할 수 있는 로직인데 Participant에서 처리하고 있다.

또한, calculateFinalScore() 부분도 Cards에서 충분히 처리할 수 있는 로직인데 Participant에 존재한다.

이 때 당시 calculateFinalScore 메서드에선 카드 점수를 getter 메서드로 값을 꺼내어 구하고 있었다!

 

위 문제는 아래 사진에서 명확하게 드러난다.

Cards에서 충분히 구할 수 있는 카드 점수 로직을,

각 카드 점수를 getter로 꺼내어 Cards가 아닌 Player에서 카드 점수를 구하는 로직이 존재하게 하였고,

그 결과 객체 책임 할당이 잘못되는 결과를 초래하게 만들었다.

 

getter 메서드는 책임 로직을 한 단계 상위 클래스에 존재시킬 가능성을 유발하고, 이는 객체 책임 할당을 잘못할 가능성을 높여주기 때문에, 반드시 필요한 상황이 아니라면 최대한 지양하는 것이 좋다.

값 뿐만 아니라 상수가 잘 쓰이고 있는지도 중요하다.

객체 책임 할당이 잘못됐다면, 상수도 적절하지 못한 위치에 존재할 확률이 높다.

 

Score VO 객체가 존재하는데, Cards에서 점수 관련 상수가 사용된다면, 무언가 책임 할당이 잘못됐을 확률이 높다.

메시지를 던져서 해결할 수 있지 않을까 고민해보면 좋을 듯하다 :)

---

네이밍은 중요하다

네이밍 착각으로 인해 잘못된 api 사용을 원치 않는다면, 그리고 낮은 가독성으로 인한 생산성 증가를 원치 않는다면 네이밍을 신중하게 고려하도록 하자.

 

물론 이 얘기는 질리도록 많이 들어봤을 것이다.

그럼에도 불구하고 자신의 코드에 심취하여 개발하다보면 어느새 이상한 네이밍을 사용하게 될 수도 있다.

점수의 합계에 ACE 보너스 점수를 더해주어 최종 점수를 계산하는 로직이라 생각해서 calculateFinalScore로 메서드명을 지어주었다.

그렇지만, api 사용자 입장에선 이게 최종 점수인지, 그냥 점수인지 알 필요가 없다.

애초에 최종 점수가 아니라면 api 사용자 입장에선 사용할 필요가 없다. (그렇지 않다면 캡슐화가 무너진것이겠다...)

 

그러므로 calculateScore 로 고치는 것이 좋다.

위 내용도 마찬가지이다.

어차피 정적 메서드이기 때문에 Deck.create라고 적어주기만 해도 Deck을 생성한다는 내용이라는 점이 명확하다.

 

중복된 의미가 존재한다면 오히려 사용자 입장에서 헷갈릴 수 있으며, 메서드명이 길면 그만큼 이해하기 어렵다는 단점이 존재하기 때문에 메서드명은 명확하게, 그리고 가능하다면 짧게 작성해주자.

(근데 너무 짧게 작성하다가 오히려 불명확해질 수 있으니, 명확한 범위 내에서 짧게 해주자.)

개발자가 영어 실력이 중요한 이유 중 하나.

이 부분은 긴 말이 필요 없을 것 같다.

 

네이밍을 명확하게 작성해주도록 하자.

---

View 검증 VS 도메인 검증?

아래와 같은 조건이 존재한다면 여러분들은 어디에서 검증을 처리할 것인가?

배팅 금액은 0원 초과로 입력해야 한다.

위 조건을 확안하기 위해 해야하는 작업은 두 가지.

1. 배팅 금액이 문자열("aaa", "-b")로 들어왔는지 숫자로 들어왔는지 확인한다.
2. 배팅 금액이 양의 정수인지 확인한다.

위 두 가지 조건을 검증하는 것은 도메인 로직일까? View 로직일까?

 

정답이 없는 문제라 생각한다.

리뷰어님마다도 약간의 의견차이가 존재하는 것으로 알고 있다.

 

핀은 아래와 같이 피드백해주었다.

안녕하세요 케이
사용자 입력에 대한 기본 검증과 도메인에 대한 검증은 따로 봐야하지 않을까요?
“문자열을 숫자로 파싱할 수 있어야 한다“가 도메인 규칙일까요?
“베팅 금액은 1원 이상이어야 한다“가 도메인 규칙이지 않나요?

View가 String을 반환하기 때문에 객체를 생성할 때도 String을 받아서 처리하는 건 생성자가 View에 의존적이라고 볼 수 있을 것 같아요.
View에서 가능하면 숫자 타입으로 변환하는 게 좋지 않을까요? 문자열이 아닌 숫자타입을 입력 받는 코드가 있다면 매번 불필요하게 String으로 변환하여 생성자를 호출하고 내부에서 다시 숫자 타입으로 변환하는 작업이 생기겠죠?

 

어느 규칙에 해당되는지 관점으로 접근할 경우, 핀의 피드백이 맞다고 생각하여 나도 핀의 의견을 반영했다.

그런데 다른 크루의 리뷰어님의 피드백은 내용이 약간 달랐다.

 

베팅 머니를 입력받은 값을 InputView에서 바로 int로 바꿔주는 것 같아요!! view는 단순히 사용자에 대한 입력을 받도록 하고 생성의 책임을 도메인에 넘겨주는 건 어떨까요?? BettingMoney가 문자열 값을 받아서 생성하도록 해주는 것도 좋을 것 같아요!!

LocalDateTime 클래스를 생각해보면 사용자 입장에서는 올바른 문자 타입이 들어가는지, 정수인지 문자인지 고민하지 않고 생성할 수 있어요. 만약 "ㅁ"라는 문자열을 넘겨주면 그에 맞는 적절한 예외가 발생하면서 객체가 생성되지 않습니다. 이렇게 다양한 타입의 값을 받아서 생성할 수 있도록 하면 도메인 객체가 더 풍부해지고 사용성이 좋아질 것 같아요!!

 

출처: 2단계 수달 미션의 스티치 피드백 https://github.com/woowacourse/java-blackjack/pull/304

 

클래스 사용성 면으로 접근하면 위 피드백이 일리가 있다.

 

그래서 내가 내린 결론은 아래와 같다.

도메인 규칙의 관점으로 볼 땐, 핀의 의견이 일리가 있으므로 타입 반환 부분은 view 검증에 맡긴다.

다만, 클래스 사용성을 높이고 싶다면 주생성자/부생성자를 이용한다!

---

이 외에도 피드백이 많지만, 전부 다 적기엔 양이 너무 많아질 듯하다.​

 

추가로 수업시간에 배운 상태 패턴 관련 내용도 적고 싶었지만 따로 쓰지 않았다.

상태 패턴을 적용하면서 객체를 이렇게도 사용할 수 있다는 새로운 시야를 얻었기 때문에 쓰고 싶은 마음도 충분히 있었지만, 아직은 강의를 참고하면서 맛보기한 정도와 다름없다고 생각하고, 그 정도의 추상화 실력을 키우기 위해선 많은 연습이 필요하다고 생각했기 때문이다.

하루에 3번 리뷰요청을 보내고 8번 울고 6번의 피눈물을 흘려...

리뷰어 핀한테 핀초리를 맞으면서 정말 많이 배웠다!

정말 성실하고 대단하다고 생각된다.

나도 이번엔 꽤 부지런하게 했다고 생각되는 편인데, 핀의 성실함과 속도를 따라가기엔 역부족이었다ㅋㅋㅋ

 

심지어 어느날은 새벽 2시반까지 내 질문을 DM으로 받아주고, 2시반부터 5시까지 게더타운에서 개발토론하고, 5시 이후에 다시 리뷰피드백해준 경우도 존재한다. 또 어떤 날은, 리뷰요청드리지 않고 push만 했는데도 피드백이 온 적도 있다!

 

핀의 성실함을 본받아 더 많은 경험과 실력을 쌓아서 더 이쁘게 설계할 수 있도록 열심히 해야겠다 ㅎㅎ