Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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:

enter image description here

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/

share|improve this question
4  
I agree with the response below but those are just "minor" tweaks. Overall, I have to say I love the code. Also the defensive programming, nice approach. –  Vojta Apr 15 at 20:03
    
Can two Cards have the same name? What if they are of the same type(c1.getClass()==c2.getClass())? Can a Hand contain Cards of different types? –  abuzittin gillifirca Apr 16 at 14:28
    
@abuzittingillifirca Yes and yes. It can contain all kinds of Cards. –  skiwi Apr 16 at 14:29
    
You'll eventually have to have some sort of concurrency control and indexes of cards changing will cause problem. returning an order of addition on add(), which would be immutable can be a solution, that's also my understanding of what @palacsint mentions in his second answer. –  abuzittin gillifirca Apr 16 at 15:31
    
Throw on "success" and "fail" seem like weird terminology to me. Wouldn't simply "true" and "false" be better? –  Ben Aaronson Apr 18 at 0:26

3 Answers 3

up vote 7 down vote accepted
  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<>();
    
share|improve this answer
    
Ah nice, I see you have made an edit with more points! I agree with most of them, though I might not incorporate everything. Only comment is that on your last point, the contains("2") is not specific enough as the Cards themselves might also contain 2. –  skiwi Apr 16 at 10:22
    
@skiwi: Yeah, an assertDoesNotContain(firstCard.toString(), "2") (or something similar) might be good here but it's just a minor issue. –  palacsint Apr 16 at 12:17

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.

share|improve this answer
2  
I do not think you are entirely correct here. 1) The test class is concerned with testing the pre-conditions, because the goal is to test everything, yes that also includes those pesky pre-condition errors which you never hope to make. 2) The user of Hand does not care about the actual implementation and it surely does not want to have to do anything with a generic List<Card>, it simply just wants a Hand. And suggesting to wait for a failure to happen on List<Card>.add() (exceeding capacity), is not a good practice. –  skiwi Apr 16 at 10:30
    
@skiwi On point 2, I agree with VoiceOfUnreason. How am I supposed to use this class without being able to actually see what cards are in my hand? External code should definitely NOT be burdened with tracking the cards and indexes itself. Checking preconditions is great and all but it seems like it's the entire thrust of your design here. Feels like putting the cart before the horse. –  Ben Aaronson Apr 18 at 0:35
    
@BenAaronson I disagree there, in your hand the cards are still ordered and have an index. If you come from an interface, you need to say which card you want to pick, as duplicates are actually allowed. –  skiwi Apr 18 at 9:42
    
@skiwi But I don't even know what cards I hold unless I also track them outside the Hand! –  Ben Aaronson Apr 18 at 10:29
    
@BenAaronson That is the job of an UI (or View), which still needs to be integrated. –  skiwi Apr 18 at 10:30

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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.