Field class used by my trading card game

Question

As many may know, I am developing a Trading Card Game for the Coding Challenge.

I have just completed the Field class and have not used it in practice yet, so all theoretical and looking for a review.

Using Java 8 and all custom classes will be included in this post as well, if you wish you can see all source on GitHub.

The field currently only consists of monsters that can be on the field, though in the future other types of cards may be added.

The Field class:

public class Field {
    private final MonsterCard[] monsters;

    public Field(final int monsterCapacity) {
        Arguments.requirePositive(monsterCapacity, "monster capacity");
        this.monsters = new MonsterCard[monsterCapacity];
    }

    public void setMonster(final int index, final MonsterCard monsterCard) {
        Arguments.requireIndexInRange(index, 0, monsters.length);
        Objects.requireNonNull(monsterCard);
        monsters[index] = monsterCard;
    }

    public MonsterCard getMonster(final int index) {
        Arguments.requireIndexInRange(index, 0, monsters.length);
        MonsterCard monsterCard = monsters[index];
        States.requireNonNull(monsterCard, NoSuchElementException::new, "no monster exists on index: " + index);
        return monsterCard;
    }

    public boolean hasMonster(final int index) {
        Arguments.requireIndexInRange(index, 0, monsters.length);
        return (monsters[index] != null);
    }

    public MonsterCard destroyMonster(final int index) {
        Arguments.requireIndexInRange(index, 0, monsters.length);
        MonsterCard monsterCard = monsters[index];
        States.requireNonNull(monsterCard, NoSuchElementException::new, "no monster exists on index: " + index);
        monsters[index] = null;
        return monsterCard;
    }

    public Stream<MonsterCard> getMonsters() {
        return Arrays.stream(monsters).filter(obj -> obj != null);
    }

    public int getMonsterCapacity() {
        return monsters.length;
    }

    @Override
    public String toString() {
        return "Field(" + Arrays.toString(monsters) + ")";
    }
}

The unit tests:

public class FieldTest {
    static {
        assertTrue(true);
    }

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

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorZeroMonsterCapacity() {
        new Field(0);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorNegativeMonsterCapacity() {
        new Field(-1);
    }

    @Test
    public void testSetMonster() {
        Field field = new Field(6);
        MonsterCard monsterCard = createMonsterCard();
        field.setMonster(0, monsterCard);
        assertEquals(monsterCard, field.getMonster(0));

        MonsterCard monsterCard2 = createMonsterCard2();
        field.setMonster(5, monsterCard2);
        assertEquals(monsterCard2, field.getMonster(5));
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testSetMonsterUnderRange() {
        Field field = new Field(6);
        field.setMonster(-1, createMonsterCard());
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testSetMonsterAboveRange() {
        Field field = new Field(6);
        field.setMonster(6, createMonsterCard());
    }

    @Test(expected = NullPointerException.class)
    public void testSetMonsterMonsterCardNull() {
        Field field = new Field(6);
        field.setMonster(0, null);
    }

    @Test
    public void testGetMonster() {
        Field field = new Field(6);
        MonsterCard monsterCard = createMonsterCard();
        field.setMonster(0, monsterCard);
        assertEquals(monsterCard, field.getMonster(0));
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testGetMonsterUnderRange() {
        Field field = new Field(6);
        field.getMonster(-1);
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testGetMonsterAboveRange() {
        Field field = new Field(6);
        field.getMonster(6);
    }

    @Test(expected = NoSuchElementException.class)
    public void testGetMonsterNoMonster() {
        Field field = new Field(6);
        field.getMonster(0);
    }

    @Test
    public void testHasMonster() {
        Field field = new Field(6);
        field.setMonster(0, createMonsterCard());
        assertTrue(field.hasMonster(0));
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testHasMonsterUnderRange() {
        Field field = new Field(6);
        field.hasMonster(-1);
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testHasMonsterAboveRange() {
        Field field = new Field(6);
        field.hasMonster(6);
    }

    @Test
    public void testDestroyMonster() {
        Field field = new Field(6);
        MonsterCard monsterCard = createMonsterCard();
        field.setMonster(0, monsterCard);
        assertEquals(monsterCard, field.destroyMonster(0));
        assertFalse(field.hasMonster(0));
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testDestroyMonsterUnderRange() {
        Field field = new Field(6);
        field.destroyMonster(-1);
    }

    @Test(expected = IndexOutOfBoundsException.class)
    public void testDestroyMonsterAboveRange() {
        Field field = new Field(6);
        field.destroyMonster(6);
    }

    @Test(expected = NoSuchElementException.class)
    public void testDestroyMonsterNoMonster() {
        Field field = new Field(6);
        field.destroyMonster(0);
    }

    @Test
    public void testGetMonsters() {
        Field field = new Field(6);
        assertEquals(0, field.getMonsters().count());
        assertFalse(field.getMonsters().anyMatch(m -> m == null));

        MonsterCard monsterCard = createMonsterCard();
        MonsterCard monsterCard2 = createMonsterCard2();
        field.setMonster(0, monsterCard);
        field.setMonster(1, monsterCard2);
        List<MonsterCard> list = field.getMonsters().collect(Collectors.toList());
        assertTrue(list.contains(monsterCard));
        assertTrue(list.contains(monsterCard2));
    }

    @Test
    public void testGetMonsterCapacity() {
        Field field = new Field(6);
        assertEquals(6, field.getMonsterCapacity());
    }

    @Test
    public void testToString() {
        Field field = new Field(5);
        MonsterCard monsterCard = createMonsterCard();
        MonsterCard monsterCard2 = createMonsterCard2();
        field.setMonster(1, monsterCard);
        field.setMonster(3, monsterCard2);
        assertEquals("Field([null, " + monsterCard.toString() + ", null, " + monsterCard2.toString() + ", null])", field.toString());
    }

    private MonsterCard createMonsterCard() {
        return new MonsterCard("Test", 5, 5, MonsterModus.HEALING);
    }

    private MonsterCard createMonsterCard2() {
        return new MonsterCard("zzz", 7, 7, MonsterModus.OFFENSIVE);
    }
}

The relevant Arguments and States snippets:

public static int requirePositive(final int value, final String name) throws IllegalArgumentException {
    Objects.requireNonNull(name);
    if (value <= 0) {
        throw new IllegalArgumentException("the " + name + " must be positive: " + value);
    }
    return value;
}

---

public static int requireIndexInRange(final int index, final int lowInclusive, final int highExclusive) throws IndexOutOfBoundsException {
    if (index < lowInclusive || index >= highExclusive) {
        throw new IndexOutOfBoundsException("the index must be in range: " + index + ", expected: [" + lowInclusive + ", " + highExclusive + ")");
    }
    return index;
}

---

public static <T, E extends RuntimeException> T requireNonNull(final T object, final Function<String, E> exceptionFunction, final String message) throws E {
    Objects.requireNonNull(exceptionFunction);
    Objects.requireNonNull(message);
    if (object == null) {
        throw exceptionFunction.apply(message);
    }
    return object;
}

A few notes:

• In the unit tests I explicitely create a new Field(6) every time, as the arguments are important to know.
• The static { assertTrue(true); } is there such that Netbeans stop annoying me, it is standard in all my unit tests.
• I use an experimental approach of providing a Stream<E> as view of a class as opposed to the Java 7-and-below standard of Collection<E>. The Java 8 developers encourage working with streams over the old collections API for writing API's.

Answer 1

First, I don't think Field is a good name – it is really generic and doesn't reveal any information.

Now a few notes on your unit tests:

Instead of duplicating the setup Field field = new Field(6) in every test, you could use a @Before annotated setup method:

public class FieldTest {
    private Field field;

    @Before
    public void before() {
        field = new Field( 6 );
    }

    // …
}

I would also recommend getting rid of @Test(expected = XyzException.class) and instead using the ExpectedException rule. This way, you can ensure that the exception is thrown exactly in the place you expect it to be thrown – and not, for example, in the test setup. This especially holds for standard exceptions such as IllegalArgumentException and NullPointerException. You should also assert for the message if you really want to make sure you are asserting the correct exception:

public class FieldTest {
    @Rule
    public ExpectedException thrown = ExpectedException.none();

    @Test
    public void someTest() {
        // some setup

        thrown.expect( IllegalArgumentException.class );
        thrown.expectMessage( "Your custom message" );

        // execute the behavior that is to be tested
    }
}

For application specific exceptions you should also use custom exceptions such as MonsterNotFoundException or similar (that you can derive from the built-in classes if you like). It makes stack-traces easier to read and allows error handling that is both more understandable and more flexible:

try {
    // a few lines of code
} catch( NoSuchElementException e ) {
    // reader: no such element of what? and where?
}

try {
    // a few lines of code
} catch( MonsterNotFoundException e ) {
    // reader: oh, no monster found. I see what happened here!
}

This is more of a personal thing, but I would try to avoid allowing any kind of NullPointerException for expected behavior.

Something I like to do to counter this is using Optionals which before Java 8 would be using Google's Guava, but as of Java 8 it is a built-in functionality (though I still like Guava's implementation better).

One major upside to this is that it forces the user of the class to think about handling these cases, while null values are easily forgotten about and then cause bugs.

This also applies to your getMonster – if you just return Optional<MonsterCard>, the caller doesn't need to do either one of these

• call hasMonster and if it returns true, call getMonster
• call getMonster and wrap it in try .. catch.

Instead, it is up to them and they can handle it in a more flexible way:

MonsterCard monster = field.getMonster( 1 ).orNull();
// or:
MonsterCard monster = field.getMonster( 2 ).or( defaultMonster );
// or:
Optional<MonsterCard> monster = field.getMonster( 3 );
if( monster.isPresent() ) {
    // …
}

For example, getMonster and hasMonster would look like this:

public Optional<MonsterCard> getMonster( final int index ) {
    Arguments.requireIndexInRange( index, 0, monsters.length );
    return Optional.fromNullable( monsters[index] );
}

public boolean hasMonster( final int index ) {
    return getMonster( index ).isPresent();
}

Two more things:

• In your toString method, instead of hard-coding "Field", you could use Field.class.getSimpleName(). This has the benefit of automatically adapting to any kind of refactoring. Boilerplate methods like toString are very prone to being forgotten.
• I wouldn't obsess over making all arguments final. There is little to no benefit in it. I like making fields final whenever possible because looking at the class it becomes clear that this object will be assigned no more than once. Making classes final is fine if you are providing a library, otherwise it's somewhat pointless. Making arguments final really only makes sense to me if you are forced to do so (because you want to use it in a callback etc.). But this really is just a personal opinion and you should feel free to do so if you judge it differently.