코드리뷰
이 프로젝트를 OKKY에 올리고 나서 OKKY의 연예인이신 fender님의 리뷰을 받을 수 있었습니다.
개인적으로는 블로그에 글을 올리고나서 OKKY에 항상 바로 올리는 이유가 이런 선배님들의 피드백때문입니다.
바로 리뷰에 대한 수정을 진행하려고 했으나, 이전부터 작업하던 Spring AOP 정리를 마무리하는게 먼저라 시작이 늦었습니다.
다행히 이번주 평일전체를 연차로 쉴수 있게되어 AOP 내용도 정리하고, 이제라도 리뷰 수정을 진행할 수 있게 되었습니다.
fender님의 리뷰에서 나온 수정사항들은 아래와 같습니다.
- Card의 패턴과 끗수를 enum으로
- 자연스러운 네이밍
- isReceiveCard, receiveCardAllPlayers 등은 정상적인 문법은 아님
- list 대신 stack을 사용
- API와 View영역을 분리
- Player.showCards()가 과연 player의 역할인지
- receiveCard는 호출자가 그 결과를 알 수 있는지
- 테스트 코드 작성
너무 많죠? ㅠㅠ...
(으헝헝헝)
제가.. 아직 많이 부족하구나 라고 느끼게 되었습니다.
자! 그래서 지금부터 이 부분을 하나하나 고쳐나가보겠습니다.
코드리뷰 - 반영 (1)
제일 먼저 Card의 패턴과 끗수를 enum으로 변경해보겠습니다.
이게 왜 문제가 되냐하면 enum이 아닌 문자열 배열 혹은 단순 숫자로 입력을 받도록 하게 되어도 체크가 안되기 때문입니다.
enum을 사용하게 되면 제가 제시한 것외에는 허용되지 않기에 아예 Exception이 발생하지만, 단순 문자열과 숫자로 표현하게 되면 컴파일러에서는 잘못된 값이 들어온지 알수가 없습니다.
enum의 사용법은 넥스트리 블로그 를 참고하시면 좋습니다.
패턴과 끗수 중 패턴을 먼저 진행하겠습니다.
단! 여기서부터는 테스트코드를 작성하면서 수정을 진행해나가겠습니다.
테스트 프레임워크는 가장 무난한 Junit을 사용할 예정입니다. 그래서 build.gradle에 아래와 같이 Junit 의존성을 추가하겠습니다.
build.gradle
group 'java'
version '1.0-SNAPSHOT'
apply plugin: 'java'
sourceCompatibility = 1.8
repositories {
mavenCentral()
}
dependencies {
compile group: 'org.apache.commons', name: 'commons-lang3', version: '3.5'
// Junit 의존성 추가
compile group: 'junit', name: 'junit', version: '4.11'
}
의존성이 추가되었다면 본격적으로 리팩토링을 진행해보겠습니다.
Card의 기존 코드를 다음과 같이 수정하겠습니다.
Card.java
package domain;
/**
* Created by jojoldu@gmail.com on 2016-11-17.
* Blog : http://jojoldu.tistory.com
* Github : http://github.com/jojoldu
*/
public class Card {
private Pattern pattern;
private String denomination;
private int point;
public Card(Pattern pattern, int index) {
...
}
public Pattern getPattern() {
return pattern;
}
public void setPattern(Pattern pattern) {
this.pattern = pattern;
}
....
public enum Pattern {
SPADE("spade"),
HEART("heart"),
DIAMOND("diamond"),
CLUB("club");
private String value;
Pattern() {}
Pattern(String value) {
this.value = value;
}
}
}
pattern의 타입을 문자열이 아닌 Pattern이란 enum으로 대체하였습니다.
Pattern enum의 경우 Card외에는 사용되는 곳이 없기에 inner type으로 (즉, Card 내부에 선언) 하였습니다.
Card의 생성자 조건이 변경되었기에, CardDeck 역시 코드를 수정하겠습니다.
CardDeck.java
public class CardDeck{
....
private List<Card> generateCards() {
List<Card> cards = new LinkedList<>();
for(Card.Pattern pattern : Card.Pattern.values()){
for(int i=1; i<=CARD_COUNT; i++) {
Card card = new Card(pattern, i);
cards.add(card);
}
}
return cards;
}
....
}
그전에 있던 문자열 배열인 patterns를 제거하고, Card내부의 Pattern enum으로 Card 인스턴스 생성 코드를 수정하였습니다.
enum.values() 를 사용할 경우 그 결과는 해당 enum이 갖고 있는 모든 타입의
Java에서 enum의 타입들을 전부 순회하기 위해서는 해당 타입.values() 메소드를 사용하면 됩니다.
이 경우 values 메소드의 결과는 해당 enum이 갖고 있는 모든 타입들이기 때문에 변경에 따라 코드 수정이 필요없게 됩니다.
Card와 CardDeck의 수정이 완료되었기에 이를 테스트코드로 검증해보겠습니다.
테스트 구조는 아래와 같이 src/test/java 아래에 ApplicationTest.java 입니다.
테스트 코드는 아래와 같습니다.
import org.junit.Test;
import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertThat;
public class ApplicationTest {
@Test
public void test_카드패턴비교() {
CardDeck cardDeck = new CardDeck();
List<Card> cards = cardDeck.getCards();
assertThat(cards.get(0).getPattern(), is(Card.Pattern.SPADE));
assertThat(cards.get(13).getPattern(), is(Card.Pattern.HEART));
}
}
테스트 코드 작성이 어색하신 분들은 Junit이 Spring 환경에서만 구동되는 것으로 오해를 많이 하십니다.
하지만 Junit은 Java 테스트 프레임워크입니다.
즉, 웹 프레임워크와 무관하게 Java 환경이면 구동이 가능합니다.
테스트 코드는 단순합니다.
CardDeck을 생성하고, CardDeck의 0번째와 13번째 카드를 뽑아서 그게 SPADE와 HEART 패턴인지 검증하는 것입니다.
왜냐하면 CardDeck의 카드들은 Pattern의 타입이 순차적으로 등록되어 0~12까지는 SPADE가 13~25까지는 HEART로 되어있기 때문입니다.
테스트를 실행해보면!!
성공적으로 테스트가 통과되었음을 확인할 수 있습니다.
이 기세를 몰아 끗수(denomination)도 enum으로 변경해보겠습니다.
Card.java
public class Card {
private Pattern pattern;
private Denomination denomination;
public Card(Pattern pattern, Denomination denomination) {
this.pattern = pattern;
this.denomination = denomination;
}
public Denomination getDenomination() {
return denomination;
}
public void setDenomination(Denomination denomination) {
this.denomination = denomination;
}
.....
public enum Denomination {
ACE("A", 1),
TWO("2", 2),
THREE("3", 3),
FOUR("4", 4),
FIVE("5", 5),
SIX("6", 6),
SEVEN("7", 7),
EIGHT("8", 8),
NINE("9", 9),
TEN("10", 10),
JACK("J", 10),
QUEEN("Q", 10),
KING("K", 10);
private String mark;
private int point;
Denomination() {
}
Denomination(String mark, int point) {
this.mark = mark;
this.point = point;
}
public int getPoint() {
return this.point;
}
}
}
Pattern과 마찬가지로 inner enum으로 Denomination을 선언하였습니다.
이전에 String denomination과 int point로 선언된 부분을 전부 enum에 담았습니다.
생각해보면 point도 Card의 역할보다는 끗수의 역할에 더 가깝다는 생각에 Denomination으로 옮겼습니다.
이로 인해 오히려 생성자 코드가 훨씬 간결해졌습니다.
다음으로 CardDeck을 수정하겠습니다.
CardDeck.java
public class CardDeck{
private List<Card> cards;
public CardDeck() {
cards = this.generateCards();
}
private List<Card> generateCards() {
List<Card> cards = new LinkedList<>();
for(Card.Pattern pattern : Card.Pattern.values()){
for(Card.Denomination denomination : Card.Denomination.values()){
Card card = new Card(pattern, denomination);
cards.add(card);
}
}
return cards;
}
....
}
int와 String으로 분리되었던 끗수를 enum에 담으니 CardDeck역시 코드가 훨씬 간결해졌습니다.
더불어 기존의 큰 문제였던, 값의 제한이 없다는 문제를 enum으로 완전히 그 범위를 제한할 수 있게 되었습니다.
자 그럼 정상적으로 기능이 작동되는지 확인하기 위해 테스트 코드를 작성해보겠습니다.
ApplicationTest.java
public class ApplicationTest {
private CardDeck cardDeck;
private List<Card> cards;
@Before
public void setup() {
cardDeck = new CardDeck();
cards = cardDeck.getCards();
}
@Test
public void test_카드패턴비교 () {
assertThat(cards.get(0).getPattern(), is(Card.Pattern.SPADE));
assertThat(cards.get(13).getPattern(), is(Card.Pattern.HEART));
}
@Test
public void test_카드끗수비교 () {
assertThat(cards.get(0).getDenomination(), is(Card.Denomination.ACE));
assertThat(cards.get(0).getDenomination().getPoint(), is(1));
}
}
이번 테스트 코드는 이전 코드를 조금 더 수정하였습니다.
CardDeck인스턴스를 생성하고, cards를 받는 것은 이전의 패턴비교 테스트와 현재 끗수테스트 모두에서 사용하고, 앞으로도 사용할 확률이 높기에 모든 테스트에서 적용되도록 @Before 어노테이션을 사용하였습니다.
@Before 어노테이션은 각 @Test별로 테스트 시작전에 수행되도록 하는 어노테이션입니다.
자 그럼 첫번째 리뷰대상이였던 enum과 관련된 문제가 해결되었습니다!!
아울러 테스트코드 작성도 같이 진행될 수 있게 되었습니다!
(1타 2피!)
출발이 순조로운것 같습니다!
그럼 다음 리팩토링 대상으로 가보겠습니다.
2번째 대상은 List<Card> card
입니다.
이 부분은 get+remove를 사용하는것보다는 pop을 사용하는 것이 의미상 더 적절한 뜻이 되기도하고 코드가 더 깔끔해지기 때문에 List를 Stack으로 변경하겠습니다.
하는 김에 기존에 갖고 있던 성능상 이슈도 같이 해결하겠습니다.
기존의 성능상 이슈는 무엇이냐하면, draw 할때마다 매번 랜덤하게 카드를 뽑는것입니다.
draw 할 때마다 랜덤으로 뽑기 위해 매번 랜덤함수를 실행시키는 것은 그만큼 draw 할 때마다 성능상 이슈를 발생시킵니다.
그래서 이를 해결하기 위해 처음에 카드덱을 생성하는 시점에서 카드들을 랜덤으로 정렬 하겠습니다.
이렇게 하면 이미 랜덤정렬이 된 카드들 사이에서 맨위부터 차례로 뽑기만 하면 성능상의 이슈가 함께 해결이 되는 것입니다.
CardDeck.java
public class CardDeck{
private Stack<Card> cards;
public CardDeck() {
this.cards = this.generateCards();
Collections.shuffle(this.cards);
}
private Stack<Card> generateCards() {
Stack<Card> cards = new Stack<>();
for(Card.Pattern pattern : Card.Pattern.values()){
for(Card.Denomination denomination : Card.Denomination.values()){
Card card = new Card(pattern, denomination);
cards.push(card);
}
}
return cards;
}
public Stack<Card> getCards() {
return cards;
}
public Card draw(){
return this.cards.pop();
}
....
}
CardDeck은 List를 Stack으로 변경한 것외에 큰 변화는 2가지 입니다.
- Collections.shuffle() 추가
- draw 메소드가 pop()으로만 처리
Stack의 pop메소드는 추출이란 의미로 데이터를 뽑아내고 제거를 같이 하는 메소드 입니다.
(제거하지 않고 값만 받으려면 peek 메소드를 사용하면 됩니다.)
이렇게 코드가 수정이 되면, 의도했던 대로 CardDeck 생성 시점에서 52개의 카드들이 생성되고 랜덤정렬이 됩니다.
그리고 draw할때도 특별히 다른 행위 없이 pop으로만 처리가 되니 성능상 이슈도 함께 해결이 되었습니다.
그럼 해당 기능이 잘 작성되었는지 확인하기 위해 테스트코드를 다시 작성해보겠습니다.
@Test
public void test_List를Stack으로변환() {
assertThat(cardDeck.getCards().size(), is(52));
cardDeck.draw();
assertThat(cardDeck.getCards().size(), is(51));
cardDeck.draw();
assertThat(cardDeck.getCards().size(), is(50));
cardDeck.draw();
assertThat(cardDeck.getCards().size(), is(49));
}
카드가 랜덤으로 정렬이 되기 때문에 특별한 Card 값을 가지고 테스트를 할수는 없고, draw 할 때마다 cards.size가 감소하는지로 검증을 하겠습니다.
디버깅 모드로 현재 테스트를 실행해보면
(카드들의 랜덤정렬을 확인)
우선 랜덤정렬된 것을 확인후!
(테스트 결과!)
정상적으로 pop이 되는것을 확인할 수 있습니다!
짧지만 코드리뷰 내용 중 2가지 문제를 해결하였습니다.
당시에는 몰랐지만 다시보니 부족함이 많이 느껴지는것 같습니다!
남은 리뷰 내용을 반영하면서 점점 더 좋은 코드로 발전시켜보겠습니다.
okky란 사이트를 통해 들어왔습니다.
열심히 하시는게 보기 좋네요. 블랙잭에 대한 의견 남겨봅니다.
블랙잭은 게임, 덱, 사용자, 딜러로 구성되어지죠. 이를 들여다 보면..
1. 덱 - 게임 객체의 상태.
54장의 카드생성, 셔플, 한 장꺼내는 기능을 가져야 하니
list comprehension 4-5줄로 게임 클래스 내에 배열로 구현하면 됩니다.
2. 게이머 - 사용자와 딜러
게이머는 카드와 상태(nil, burst, stay)를 두 가지를 갖습니다.
카드획득, 초이스(딜러여부에 따라 자동hit, 수동hit) 두 메소드를 구현하면 됩니다.
3. 게임 - gamers, deck을 갖는 클래스
초기화(덱 생성, 게이머 생성, 카드 2장 할당)
플레이(gamers.each { gamer.choice until 게이머.상태가 burst 또는 stay
저지(사용자의 상태와 카드합으로 승자 출력)
Gamer클래스를 상속하는 User와 Dealer를 분리 구현해도 되는데
둘을 구분하는 초이스 내용이 크지 않으니 하나로 합해서 구현해도 괜찮겠죠.
결과적으로 Game(덱) -> Gamer 또는 Game -> Gamer <|- User, Dealer 가 됩니다.
물리적으로는 2개 파일 소스는 언어에 따라 다르지만 대략 60~80라인 정도로 구현되겠네요.
답글
펜더라는 분의 스칼라 소스를 잠시 봤습니다.
블랙잭이 물리적으로 29개 소스파일로 구성될 만큼 복잡한지를 생각해 봐야합니다.
개념과 로직의 과도한 분리는 지식의 파편화로 이어지기에 적당히 분리하는게 좋습니다.
패키지 제품 설계는 대체로 분리에 집중되는 경향이 있고
복잡한 도메인의 설계는 책임의 통합과 통합된 롤사이의 협력을 정의하는 것에 집중합니다.
문제가 고립되는지 협업사이즈가 얼마나 되는지에 따라 양상이 달라지는건데요.
둘 모두를 경험하며 균형감을 갖는게 중요하다고 생각됩니다.
정답이 아닌 지나가는 과객의 경험과 생각이니 가볍게 받아들여주시면 좋겠습니다. 건승하시길~!
답글
과객// 우선 의견 주셔서 감사드립니다.
설계에 있어서 절대적인 정답은 없다는 전제로 이야기하자면, 개인적으로 코드의 복잡성은 유형의 숫자나 라인수에 비례하지 않는다고 생각합니다.
물론 블랙잭 콘솔 게임 정도라면야 마음만 먹으면 메서드 몇 개에 다 구현해도 무방하겠습니다만, 애초에 블랙잭이라는 도메인을 고른 건 모델링 연습을 위한 것이니 만큼 최대한 개념의 역할이 겹치지 않고 확장할 수 있는 형태로 만드는데 집중했습니다.
게이머와 딜러를 구분하는 것을 조건문 플래그로 할지, 아니면 상속 관계의 유형으로 표현할지, 또 카드 덱을 그냥 문자와 숫자의 배열로 표현할지, 혹은 각각의 특화된 자료구조로 표현할 지는 역시 각각의 장단점이 존재하는 접근 방법일 것입니다.
하지만 당초 프로젝트의 의의가 콘솔 기반 블랙잭 게임을 가장 간결한 코드로 구현한다 같은 것이 아닌, 최대한 객체지향적인 모델링 방법을 적용해서 풀어내는 것임을 감안하면 기능에 비해 클래스 갯수가 많아지는 건 어느 정도 감안해야 하지 않나 생각합니다.
답글
과객, fender
앗 ㅠㅠ 두분 고수님들의 좋은말씀 감사드립니다!
과객님이 말씀하신 지식의 파편화에 대한 주의에 대한 이야기 정말 감사드립니다.
fender님이 얘기해주신 내용은 제가 처음 의도했던 목적을 다시 상시시키게 된것 같습니다. 감사드립니다!
이 다음의 내용은 좀더 고민을 해서 코드를 작성하겠습니다!
답글