Level1 블랙잭 미션 피드백 정리

step1 피드백

step2 피드백

함수형 인터페이스의 과용

입력과 출력을 담당하는 View 클래스들을 게임 흐름을 담당하는 컨트롤러에서 분리하고 싶어서 함수형 인터페이스를 사용했었다. 그럼 BlackJackGame을 사용하는 클라이언트인 컨트롤러 입장에서는 드가 매우 깔끔해진다. View의 메서드들을 람다로 함수형 인터페이스로 전달한 결과다.

 public static void main(String[] args) {
    BlackJackGame blackJackGame = new BlackJackGame(
        InputView.askNames(),
        InputView::askBet,
        new Deck(Card.getCards()));
    OutputView.printGamers(blackJackGame.getDealerDto(), blackJackGame.getPlayerDtos());

    blackJackGame.play(InputView::askHitOrStay, OutputView::checkHoldingCards);
    GameResultDto result = blackJackGame.createResult();

    OutputView.printAdditionalDrawDealer(result.getDealerDrawCount());
    OutputView.printFinalCards(blackJackGame.getDealerDto(), blackJackGame.getPlayerDtos());
    OutputView.printFinalResult(result);
}

하지만 이 함수형 인터페이스들을 사용하는 BlackJackGame에서 해당 함수형 인터페이스들이 가독성이 떨어진다는 피드백을 많이 받았다.

private void hitOrStay(String name, HitRequester hitOrStay, CardsChecker cardChecker) {
	while (!isBust(name) && hitOrStay.request(name)) {
		gamers.giveCardToPlayer(name, deck);
		cardChecker.check(name, findPlayerDtoByName(name).getCards());
	}
}

HitRequester는 플레이어에게 카드를 더 뽑을지 예/아니오를 입력받는 역할을 한다. CardChecker는 플레이어의 이름과 보유 카드를 출력하는 역할을 한다. 처음엔 이렇게 전용 함수형 인터페이스로 만든 게 아니라 표준 함수형 인터페이스를 사용했었는데 그래서 가독성이 떨어지나 싶어서 직접 만들었었는데 그럼에도 다음과 같은 피드백을 받았다.

함수형 인터페이스를 사용하면서 뷰와 분리하는 구조적 이점이 생겼지만 구현체를 인자로 전달해야 하는 클라이언트가 인자의 용도를 직관적으로 이해할 수 있을지, 가독성 측면에서도 한번 고민해보시면 좋겠습니다.

BettingInjector를 사용한 로직이 복잡하게 느껴지네요.

BettingIngector는 또 다른 전용 함수형 인터페이스이다. 

함수형 인터페이스는 인터페이스의 특징을 가지면서도 람다식으로도 구현이 가능하다는 장점이 있지만, 추상적인 만큼 과용하면 코드를 이해하기 어렵게 만들 수 있어요.

그래서 결국 함수형 인터페이스를 사용하지 않고 컨트롤러에서 View 클래스를 사용하는 방법으로 다시 돌렸다. 도메인과 뷰를 분리하는 것은 타당하지만 컨트롤러에서까지 뷰를 분리할 필요는 없다고 느꼈고 입력을 받고 출력을 하는 상호작용을 해야 하는 프로그램이기에 완전한 분리를 할 필요도 없다고 생각이 들었다.

 

그리고 사실 함수형 인터페이스로 분리를 했다고는 하지만 뷰에 출력해 주기 위해 플레이어의 이름(name)을 인자로 받는 등 뷰의 로직이 들어가 있었기에 완전한 분리도 아니였다고 생각한다.

cardChecker.check(name, findPlayerDtoByName(name).getCards());

Collections.unmodifiableXXX의 사용에 대해

Collections.unmodifiableXXX를 사용하면 컬렉션 내부 값을 변경할 수 없다. 정확히는 변경하려고 하면 UnsupportedOperationException이 발생한다. 때문에 객체 내부 컬렉션을 불변으로 만들기 위해 종종 사용했다.

아래와 같이 List.copyOf를 사용하면 원본 컬렉션과 참조도 끊고 unmmodifiable 속성으로 만들 수 있어 객체 내에서 불변을 유지하고 싶은 컬렉션을 반환할 때 이렇게 처리했었다.

public List<Card> getCards() {
    return List.copyOf(cards);
}

그랬더니 아래와 같은 패드백을 받았다.

문제는 이 예외가 던져진다는 것은 코드가 실제로 실행돼야 알 수 있는데요. 테스트에서 이 점을 인지하지 못하면 사용자가 애플리케이션을 실행할 때 예기치 못하게 이 예외가 던져질 수 있습니다.

하지만 정확한 정보를 클라이언트에게 보여주어야 할 것 같았고 반환한 컬렉션을 누가 수정해서 View 등에 전달하면 정확하지 않은 정보가 전달될 수도 있다고 생각하여 그럼에도 unmmodifiable을 써야 하지 않을까 생각했다. 

컬렉션의 항목이 불변인 한, 새 컬렉션으로 반환된 리스트를 조작하는 것은 DTO 자체에 어떤 영향도 주지 않고, 클라이언트의 의사에 따른 동작이기 때문에 문제 없지 않을까요?

아무래도 리뷰어는 예상치 못한 예외가 터지는 것을 더 막고 싶은 것 같았다. 실제로 런타임이 되어서야 캐치할 수 있는 예외는 없앨 수 있으면 없애는 것이 좋은 것 같기도 하다. 다음부터는 unmmodifiable의 사용에 더 주의를 기울일 필요가 있을 것 같다.

Stack 대신에 Deque

52장의 카드를 한 장씩 뽑아서 반환하는 Deck 클래스에서 Stack을 사용했었다.

private final Stack<Card> deck = new Stack<>();

Stack을 사용하지 말고 Deque를 사용하라는 피드백을 받았다. 이유는 링크에 설명되어 있다.

순환 참조를 없애라

클래스A가 클래스B를 의존하는데 클래스B 또한 A를 의존하면 이를 순환 참조라고 한다. 미션을 진행하다가 이런 순환 참조가 몇 군데 발견되었고 리뷰어는 이를 제거하라고 하셨다.

 

A가 B를 의존한다는 것은 A가 B를 사용한다는 것이고 이는 또 다르게 말해서 B의 변경이 A에 전파된다는 것이다. 단방향 의존이라면 B의 변경이 A에게 영향을 미칠 뿐이지만 순환 참조를 하고 있다면 A가 변경되어도 B가 영향을 받고, 그 영향에 B가 바뀌면 또 A에게 영향이 갈 수 있다. 이런 이유 때문에 순환 참조를 없애도록 리뷰어가 여러 번 지적해 주신 듯하다.

getXXX()는 정말 단순히 값을 가져올 때만 사용하자.

get으로 명명한 메서드들을 전부 점검해보시기 바랍니다.
특정 처리를 하고 있다면 get을 붙이지 마시고 어떤 작업인지 의미 있는 이름을 부여하세요.

아래 메서드는 딜러와 첫 번째 카드를 가져오는 메서드이다. 그래서 getDealerFirstCard()라고 이름 붙였는데 이처럼 특정한 처리를 하고 있다면 getXXX()로 이름을 붙이지 않는다고 한다.

private List<Card> getDealerFirstCard(GamerDto dealerDto) {
	return dealerDto.getCards()
		.subList(0, DEALER_OPEN_COUNT_FIRST);
}

DTO에서 값을 꺼내서 어떤 작업을 한다면 DTO를 잘못 쓰고 있는 것

DTO는 계층 간 데이터 전달을 목적으로 쓰이며 어떤 로직을 가지지 않고 getter/setter 정도만 가지고 있는 객체를 말한다. 내부에선 로직이 없었기에 괜찮은 줄 알았는데 값을 전달하는 용도가 아닌 다른 작업을 외부에서 처리해도 안 된다고 한다.

 

아래에서도 DTO를 가져와 각 내부의 값을 꺼내서 어떠한 처리를 하고 있다. DTO는 DTO 답게 계층 간 데이터 전달을 위해서만 사용하도록 하자.

createPlayerDtos().forEach(p -> cardChecker.check(p.getName(), p.getCards()));

public List<GamerDto> createPlayerDtos() {
	return gamers.getPlayers().stream()
		.map(GamerDto::new)
		.collect(Collectors.toList());
}

테스트 코드는 이류 시민이 아니다.

클린코드 25p
의미 있게 구분하라
컴파일러나 인터프리터만 통과하려는 생각으로 코드를 구현하는 프로그래머는 스스로 문제를 일으킨다. 예를 들어, 동일한 범위 안에서 다른 두 개념에 같은 이름을 사용하지 못한다
....
컴파일러를 통과할지라도 연속된 숫자를 덧붙이거나 불용어(noise word)를 추가하는 방식은 적절하지 못하다. 이름이 달라야 한다면 의미도 달라져야 한다.

연속적인 숫자를 덧붙인 이름(a1, a2 ... aN)은 의도적인 이름과 정반대다. 이런 이름은 그릇된 정보를 제공하는 이름도 아니며, 아무런 정보를 제공하지 못하는 이름일 뿐이다. 저자 의도가 전혀 드러나지 않는다.

클린코드 157p
테스트 코드는 실제 코드 못지않게 중요하다. 테스트 코드는 이류 시민이 아니다.
테스트 코드는 사고와 설계와 주의가 필요하다.
실제 코드 못지 않게 깨끗하게 짜야한다.

테스트는 유연성, 유지보수성, 재사용성을 제공한다. 테스트 케이스가 있으면 변경이 쉬워지기 때문이다.
따라서 테스트 코드가 지저분하면 코드를 변경하는 능력이 떨어지며 코드 구조를 개선하는 능력도 떨어진다.
테스트 코드가 지저분할수록 실제 코드도 지저분해진다. 결국 테스트 코드를 잃어버리고 실제 코드도 망가진다.

부끄럽게도 테스트 코드를 작성할 때 통과만 되도록 불용어를 많이 사용했었다.

함수형 인터페이스를 인자로 받아야 하는 자리에 (뷰에 출력을 위한 인터페이 스였어서) 아무 처리를 하지 않는다는 람다를 넘기기도 하였다.

BlackJackGame blackJackGame = new BlackJackGame(List.of("name"), s -> 10, new Deck(Card.getCards()));
blackJackGame.play(answer -> false, (s, c) -> {});

처음 보는 사람이 위와 같은 코드를 본다면 정말 이해하기 힘들 거라는 생각이 들었다. 테스트도 유지보수 대상이라는 말이 정말 와닿았다. 테스트 코드도 깔끔하고 가독성 좋게 작성하려는 노력을 많이 해야겠다.

테스트 클래스에 테스트 대상 클래스 이외의 클래스나 대상 메서드 이외 메서드가 호출되는가?

테스트 클래스에 테스트 대상 클래스 외의 다른 클래스가 import 되거나, 다른 클래스의 메서드를 호출하고 있다면 리팩터링을 진지하게 고민해 보세요

위 피드백은 리뷰어한테 받은 피드백은 아니고 코치인 제이슨이 해준 말이다. 무슨 말인가 하면 아래의 코드르 보자.

 

Card의 일급 컬렉션 Cards에 대한 테스트이며 더 자세하게는 sum()이라는 메서드의 테스트이다. ACE와 TEN 카드를 받았을 때 합이 21이면 통과한다.

 

그런데 Cards의 add() 메서드도 함께 쓰이고 있다. 이 경우 문제가 되는 건 add() 메서드에 문제가 생기면 sum() 메서드가 정상 동작하더라도 아래 테스트가 깨질 위험이 있다.

 

sum()에 대한 테스트인데 add()에 문제가 생긴다고 깨진다면 유지 보수할 때 엄청 힘들어질 것 같은 느낌이 나지 않는가? 이런 문제 때문에 테스트 코드를 작성할 때 테스트 대상 클래스, 테스트 대상 메서드만 호출하도록 테스트 코드를 짜야한다는 것이다.

@Test
@DisplayName("Ace와 TEN을 받으면 합이 21이다.")
void sum() {
	Cards cards = new Cards();
	cards.add(CLOVER_ACE);
	cards.add(CLOVER_TEN);

	assertThat(cards.sum()).isEqualTo(21);
}

 

add() 메서드를 없애기 위해 Cards 생성자에서 가변 인자로 Card를 받아서 생성이 가능하도록 만들었다. sum() 메서드만 쓰이는 것뿐만 아니라 코드가 깔끔해지기도 했다.

@Test
@DisplayName("Ace와 TEN을 받으면 합이 21이다.")
void sum() {
	Cards cards = new Cards(CLOVER_ACE, CLOVER_TEN);
	assertThat(cards.sum()).isEqualTo(21);
}