Trading Card Game's Hand class and tests

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 void throwOnFail(final BooleanSupplier resultSupplier, final Supplier exceptionSupplier) throws E {
Objects.requireNonNull(resultSupplier);
throwOnFail(resultSupplier.getAsBoolean(), exceptionSupplier);
}

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

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

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

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

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

public stat

-
If NetBeans requires this:

{
    assertEquals(true, true);
}

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

-
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.)

-
I'd rename

• testConstructorIAE() to testConstructorWithInvalidCapacity()

• testAddISE() to testAddCouldNotExceedHandLimit()

• testPlayIOOB1() to testPlayWithInvalidCardIndex()

• testPlayIOOB2() to testPlayWithoutAnyCard()

• testPlayIOOB3() to testPlayWithOneCardButInvalidCardIndex()

• ...

which are more descriptive.

-
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());

-
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.

-
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);
    ...
}

-
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.

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

-
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);
}

-
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);
}

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

-
I like your finals, they help reading.

-
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.)

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

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

StackExchange Code Review Q#47273, answer score: 10