Trading Card Game's Hand class and tests

Question

As part of a Trading Card Game, I have created a Hand that will need to hold all cards that a player currently has in his hand. The code is built using Java 8.

The structure is the following:

• Class for exception throwing.
• The Card interface.
• The Hand class.
• The HandTest test class.

---

public final class ExceptionUtils {
    private ExceptionUtils() {
        throw new UnsupportedOperationException();
    }

    public static <E extends RuntimeException> void throwOnFail(final BooleanSupplier resultSupplier, final Supplier<E> exceptionSupplier) throws E {
        Objects.requireNonNull(resultSupplier);
        throwOnFail(resultSupplier.getAsBoolean(), exceptionSupplier);
    }

    public static <E extends RuntimeException> void throwOnFail(final boolean result, final Supplier<E> exceptionSupplier) throws E {
        Objects.requireNonNull(exceptionSupplier);
        if (!result) {
            throw exceptionSupplier.get();
        }
    }

    public static <E extends RuntimeException> void throwOnFail(final BooleanSupplier resultSupplier, final Function<String, E> exceptionFunction, final String message) throws E {
        Objects.requireNonNull(resultSupplier);
        throwOnFail(resultSupplier.getAsBoolean(), exceptionFunction, message);
    }

    public static <E extends RuntimeException> void throwOnFail(final boolean result, final Function<String, E> exceptionFunction, final String message) throws E {
        Objects.requireNonNull(exceptionFunction);
        Objects.requireNonNull(message);
        if (!result) {
            throw exceptionFunction.apply(message);
        }
    }

    public static <E extends RuntimeException> void throwOnSuccess(final BooleanSupplier resultSupplier, final Supplier<E> exceptionSupplier) throws E {
        Objects.requireNonNull(resultSupplier);
        throwOnSuccess(resultSupplier.getAsBoolean(), exceptionSupplier);
    }

    public static <E extends RuntimeException> void throwOnSuccess(final boolean result, final Supplier<E> exceptionSupplier) throws E {
        Objects.requireNonNull(exceptionSupplier);
        if (result) {
            throw exceptionSupplier.get();
        }
    }

    public static <E extends RuntimeException> void throwOnSuccess(final BooleanSupplier resultSupplier, final Function<String, E> exceptionFunction, final String message) throws E {
        Objects.requireNonNull(resultSupplier);
        throwOnSuccess(resultSupplier.getAsBoolean(), exceptionFunction, message);
    }

    public static <E extends RuntimeException> void throwOnSuccess(final boolean result, final Function<String, E> exceptionFunction, final String message) throws E {
        Objects.requireNonNull(exceptionFunction);
        Objects.requireNonNull(message);
        if (result) {
            throw exceptionFunction.apply(message);
        }
    }
}

---

public interface Card {
    public String getName();
}

---

public class Hand {
    private final List<Card> list = new ArrayList<>();
    private final int capacity;

    public Hand(final int capacity) {
        ExceptionUtils.throwOnFail(capacity > 0, IllegalArgumentException::new, "capacity should be strictly positive");
        this.capacity = capacity;
    }

    public boolean isFull() {
        return (list.size() == capacity);
    }

    public void add(final Card card) {
        Objects.requireNonNull(card);
        ExceptionUtils.throwOnSuccess(this::isFull, IllegalStateException::new, "hand is full");
        list.add(card);
    }

    public Card play(final int index) {
        assertIndex(index);
        return list.remove(index);
    }

    public void swap(final int indexOne, final int indexTwo) {
        assertIndex(indexOne);
        assertIndex(indexTwo);
        Collections.swap(list, indexOne, indexTwo);
    }

    private void assertIndex(final int index) {
        ExceptionUtils.throwOnFail(index >= 0 && index < list.size(), IndexOutOfBoundsException::new);
    }

    @Override
    public String toString() {
        return "Hand(" + capacity + ", " + list + ")";
    }
}

---

public class HandTest {
    {
        assertEquals(true, true);
    }

    @Test
    public void testConstructor() {
        Hand hand = new Hand(1);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorIAE() {
        Hand hand = new Hand(0);
    }

    @Test
    public void testIsFull() {
        Hand hand = new Hand(2);
        hand.add(createCard());
        assertEquals("hand should not be full", false, hand.isFull());
        hand.add(createCard());
        assertEquals("hand should be full", true, hand.isFull());
        hand.play(1);
        assertEquals("hand should not be full anymore", false, hand.isFull());
    }

    @Test
    public void testAdd() {
        Hand hand = new Hand(1);
        hand.add(createCard());
    }

    @Test(expected = NullPointerException.class)
    public void testAddNPE() {
        Hand hand = new Hand(1);
        hand.add(null);
    }

    @Test(expected = IllegalStateException.class)
    public void testAddISE() {
        Hand hand = new Hand(1);
        hand.add(createCard());
        hand.add(createCard());
    }

    @Test
    public void testPlay() {
        Hand hand = new Hand(1);
        Card card = createCard();
        hand.add(card);
        assertEquals("card should be equal", card, hand.play(0));
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testPlayIOOB1() {
        Hand hand = new Hand(1);
        hand.play(-1);
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testPlayIOOB2() {
        Hand hand = new Hand(1);
        hand.play(0);
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testPlayIOOB3() {
        Hand hand = new Hand(1);
        hand.add(createCard());
        hand.play(1);
    }

    @Test
    public void testSwap() {
        Hand hand = new Hand(2);
        Card card = createCard();
        Card card2 = createCard2();
        hand.add(card);
        hand.add(card2);
        hand.swap(0, 1);
        assertEquals("card should be equal", card, hand.play(1));
        assertEquals("card2 should be equal", card2, hand.play(0));
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testSwapIOOB1() {
        Hand hand = new Hand(1);
        hand.add(createCard());
        hand.swap(-1, 0);
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testSwapIOOB2() {
        Hand hand = new Hand(1);
        hand.add(createCard());
        hand.swap(1, 0);
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testSwapIOOB3() {
        Hand hand = new Hand(1);
        hand.add(createCard());
        hand.swap(0, -1);
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testSwapIOOB4() {
        Hand hand = new Hand(1);
        hand.add(createCard());
        hand.swap(0, 1);
    }

    @Test
    public void testToString1() {
        Hand hand = new Hand(1);
        assertEquals("Hand(1, [])", hand.toString());
    }

    @Test
    public void testToString2() {
        Hand hand = new Hand(2);
        Card card = createCard();
        Card card2 = createCard2();
        hand.add(card);
        hand.add(card2);
        assertEquals("Hand(2, [" + card + ", " + card2 + "])", hand.toString());
    }

    private Card createCard() {
        return new MonsterCard("Test", 10, 100, MonsterModus.OFFENSIVE);
    }

    private Card createCard2() {
        return new MonsterCard("Test2", 15, 150, MonsterModus.HEALING);
    }
}

WIth the given tests I score a 100% instruction and branche coverage percentage on Hand testing:

The assertEquals(true, true) is there to ensure that Netbeans does not get rid of my static import there.

Special focus may be given on the ability to understand the code without javadoc.
I am aware that the ExceptionUtils class does not have unit tests yet.
Github repository for reference: https://github.com/skiwi2/TCG/

Answer 1

1. If NetBeans requires this:

   {
       assertEquals(true, true);
   }
   

   I would put a comment about that or move it into a method like workaroundForNetBeansNotToRemoveStaticImports.

2. Instead of this:

   @Test
   public void testConstructor() {
       Hand hand = new Hand(1);
   }
   

   I'd use the following, it's the same:

   @Test
   public void testConstructor() {
       new Hand(1);
   }
   

   (Eclipse shows a warning about the unused variable.)

3. I'd rename

   • testConstructorIAE() to testConstructorWithInvalidCapacity()
   • testAddISE() to testAddCouldNotExceedHandLimit()
   • testPlayIOOB1() to testPlayWithInvalidCardIndex()
   • testPlayIOOB2() to testPlayWithoutAnyCard()
   • testPlayIOOB3() to testPlayWithOneCardButInvalidCardIndex()
   • ...

   which are more descriptive.

4. You could use assertTrue and assertFalse here instead of assertEquals:

   assertEquals("hand should not be full", false, hand.isFull());
   hand.add(createCard());
   assertEquals("hand should be full", true, hand.isFull());
   

5. If I have two objects with similar names in the same test I usually postfix them like cardOne and cardTwo or prefix them as firstCard as secondCard.

   Card card = createCard();
   Card card2 = createCard2();
   

   I've found that easier to read/separate from each other than numbers.

6. I'd put an assertNotEquals(card, card2) into the testSwap method just to make sure that test data is correct:

   @Test
   public void testSwap() {
       Hand hand = new Hand(2);
       Card card = createCard();
       Card card2 = createCard2();
       assertNotEquals(card, card2);
       ...
   }
   

7. For this:

   public Hand(final int capacity) {
       ExceptionUtils.throwOnFail(capacity > 0, IllegalArgumentException::new, "capacity should be strictly positive");
       this.capacity = capacity;
   }
   

   Google Guava has a great checkState() method for that with more compact form. Consider using it. Here is the constructor and the add method with Guava's Preconditions:

   public Hand(final int capacity) {
       checkArgument(capacity > 0, "capacity should be strictly positive");
       this.capacity = capacity;
   }
   
   public void add(final Card card) {
       checkNotNull(card, "card cannot be null");
       checkState(!isFull(), "hand is full");
       list.add(card);
   }
   

   Having unit tests is great (keep it up!), I could change these methods and the tests checked that they're still doing the same thing as before. It's very handy.

8. For me assertIndex means that it can be disabled at runtime like assertions. I'd consider renaming that to checkValidIndex.

9. It would be useful for debugging to have the invalid index in the exception message here:

   private void assertIndex(final int index) {
       ExceptionUtils.throwOnFail(index >= 0 && index < list.size(), IndexOutOfBoundsException::new);
   }
   

10. Objects.requireNonNull has an overloaded version with a second, message parameter. Using that here would help debugging:

    public void add(final Card card) {
        Objects.requireNonNull(card);
        ExceptionUtils.throwOnSuccess(this::isFull, IllegalStateException::new, "hand is full");
        list.add(card);
    }
    

11. I would wrap the longer lines. 180 character is could be much.

12. I like your finals, they help reading.

13. I might not be so strict here:

    @Test
    public void testToString2() {
        Hand hand = new Hand(2);
        Card card = createCard();
        Card card2 = createCard2();
        hand.add(card);
        hand.add(card2);
        assertEquals("Hand(2, [" + card + ", " + card2 + "])", hand.toString());
    }
    

    It might overspecify the format without any reason. Consider the following:

    import static org.fest.assertions.api.Assertions.assertThat; 
    
    @Test
    public void testToString2() {
        Hand hand = new Hand(2);
        Card firstCard = createCard();
        Card secondCard = createCard2();
        hand.add(firstCard);
        hand.add(secondCard);
    
        assertThat(hand.toString()).contains("2");
        assertThat(hand.toString()).contains(firstCard.toString());
        assertThat(hand.toString()).contains(secondCard.toString());
    }
    

    (The import is from fest.)

14. I think cards would be a better name here, it describes the purpose of field better:

    private final List<Card> list = new ArrayList<>();
    

Answer 2

There are two sharp code smells that suggest you are attacking this project the wrong way around.

One is that your HandTest is primarily concerned with verifying that the Hand class produces the correct behavior when various pre-conditions are invalid. Unless you are expecting to write a lot of broken code, these checks are not providing a lot of value.

The second is that Hand doesn't insulate the caller from the fact that it is really "just a List". The public interface wraps standard List calls, with precondition enforcement. Where's the business value?

Some class, somewhere, is going to need to know where in the list to get the cards it needs, and that class doesn't gain any benefit from using a Hand instead of a List.

Instead of writing a container class, test and write a piece of your game, so that you can demonstrate how Hands behave - how the abstractions of the trading game map to standard container abstractions.

Answer 3

I think Hand does the information hiding well, it checks the invariants and does not allow invalid state. It fails early, throws proper exceptions when the clients tries something invalid and does not postpone or ignore errors. Bugs happen, so it's safer to be defensive. (The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)

Furthermore, it hides the method of the underlying list which are not required for a card game client (like clear(), indexOf(), remove()) and it's easier and safer to use and than a plain java.util.List. (It also makes possible to change the underlying implementation without changing clients.)

One thing which smells a little bit that client classes need indexes for calling play and swap and this knowledge is implicit in the model. Clients have to know that the index of the first added card is zero, the index of the second card is one and if they play a card it decreases the indexes of the subsequent cards in the hand. It might be better if add returns the index of a card (but it still not solve the issue of play). (I haven't checked the other classes in the GitHub repository so this might be completely fine.)