Field class used by my trading card game

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 getMonsters() {
return Arrays.stream(monsters).filter(obj -> obj != null);
}

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

@Override
pu

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, 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 monster = field.getMonster( 3 );
if( monster.isPresent() ) {
    // …
}

For example, getMonster and hasMonster would look like this:

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

StackExchange Code Review Q#48255, answer score: 5