FuseMonsterAction implementation and unit tests

Question

I'd like to get a general review on the following code, and I'll highlight an extra point below:

public interface PlayerAction {
 boolean isActionAllowed(final Player player);
 void performAction(final Player player) throws PlayerActionNotAllowedException;
}

---

public class PlayerActionNotAllowedException extends RuntimeException {
 private static final long serialVersionUID = 4656594949564649L;
 public PlayerActionNotAllowedException() {
 super();
 }
 public PlayerActionNotAllowedException(final String message) {
 super(message);
 }
 public PlayerActionNotAllowedException(final String message, final Throwable cause) {
 super(message, cause);
 }
 public PlayerActionNotAllowedException(final Throwable cause) {
 super(cause);
 }
}

---

public class PlayerActionNotAllowedExceptionTest {
 static {
 assertTrue(true);
 }
 @Test
 public void testConstructor() {
 new PlayerActionNotAllowedException();
 }
 @Test
 public void testConstructorMessage() {
 PlayerActionNotAllowedException exception = new PlayerActionNotAllowedException("Test");
 assertEquals("Test", exception.getMessage());
 }
 @Test
 public void testConstructorMessageCause() {
 Throwable cause = new Throwable();
 PlayerActionNotAllowedException exception = new PlayerActionNotAllowedException("Test", cause);
 assertEquals("Test", exception.getMessage());
 assertEquals(cause, exception.getCause());
 }
 @Test
 public void testConstructorCause() {
 Throwable cause = new Throwable();
 PlayerActionNotAllowedException exception = new PlayerActionNotAllowedException(cause);
 assertEquals(cause, exception.getCause());
 }
}

---

public abstract class AbstractPlayerAction implements PlayerAction {
 @Override
 abstract public boolean isActionAllowed(final Player player);
 @Override
 public void performAction(final Player player) throws PlayerActionNotAllowedException {
 Objects.requireNonNull(player, "player");
 if (!isActionAllowed(player)) {
 throw new PlayerActionNotAllowedException();
 }
 internalPerformAction(player);
 }
 abstract protected void internalPerformAction(final Player player);
}

---

public class AbstractPlayerActionTest {
 static {
 assertTrue(true);
 }
 private AtomicInteger counter;
 @Before
 public void before() {
 counter = new AtomicInteger(0);
 }
 @Test
 public void testPerformAction() {
 PlayerAction playerAction = new AbstractPlayerActionImpl();
 Player player = new Player("Test", 100, new TurnActionImpl(), new Hand(5), new Field(5), new Deck(Arrays.asList(new MonsterCard("Test", 5, 5, MonsterModus.HEALING))), new Graveyard());
 playerAction.performAction(player);
 assertEquals(1, counter.get());
 }
 @Test(expected = NullPointerException.class)
 public void testPerformActionNullPlayer() {
 PlayerAction playerAction = new AbstractPlayerActionImpl();
 playerAction.performAction(null);
 }
 @Test(expected = PlayerActionNotAllowedException.class)
 public void testPerformActionNotAllowed() {
 PlayerAction playerAction = new AbstractPlayerActionImpl() {
 @Override
 public boolean isActionAllowed(final Player player) {
 return false;
 }
 };
 Player player = new Player("Test", 100, new TurnActionImpl(), new Hand(5), new Field(5), new Deck(Arrays.asList(new MonsterCard("Test", 5, 5, MonsterModus.HEALING))), new Graveyard());
 playerAction.performAction(player);
 }
 private class AbstractPlayerActionImpl extends AbstractPlayerAction {
 @Override
 public boolean isActionAllowed(final Player player) {
 return true;
 }
 @Override
 protected void internalPerformAction(final Player player) {
 counter.incrementAndGet();
 }
 }
 private static class TurnActionImpl implements TurnAction {
 @Override
 public void performTurn(Player player) { }
 }
}

And now the class where the question is about:

public class FuseMonsterAction extends AbstractPlayerAction implements PlayerAction {
 private final int fusionCardIndex;
 private final int baseMonsterCardIndex;
 private final int fuserMonsterCardIndex;
 public FuseMonsterAction(final int fusionCardIndex, final int baseMonsterCardIndex, final int fuserMonsterCardIndex) {
 Arguments.requirePositiveOrZero(baseMonsterCardIndex, "baseMonsterCardIndex");
 Arguments.requirePositiveOrZero(fuserMonsterCardIndex, "fuserMonsterCardIndex");
 if (baseMonsterCardIndex == fuserMonsterCardIndex) {
 throw new IllegalArgumentException("baseMonsterCardIndex == fuserMonsterCardIndex");
 }
 this.fusionCardIndex = Arguments.requirePositiveOrZero(fusionCardIndex, "fusionCardIndex");
 this.baseMonsterCardIndex = baseMonsterCardIndex;
 this.fuserMonsterCardIndex = fuserMonsterCardIndex;
 }
 @Override
 public boolean isActionAllowed(final Player player) {
 Objects.requireNonNull(player, "player");
 Hand hand = player.getHand();
 Field field = player.getField();
 if (fusionCardIndex >= hand.getCapacity()) {
 return false;
 }
 if (baseMonsterCardIndex >= field.getMonsterCapacity()) {
 return false;
 }
 if (fuserMonsterCardIndex >= field.getMonsterCapacity()) {
 return false;
 }
 if (!(hand.get(fusionCardIndex) instanceof FusionCard)) {
 return false;
 }
 if (!field.hasMonster(baseMonsterCardIndex)) {
 return false;
 }
 if (!field.hasMonster(fuserMonsterCardIndex)) {
 return false;
 }
 return true;
 }
 @Override
 protected void internalPerformAction(final Player player) {
 Objects.requireNonNull(player);
 Hand hand = player.getHand();
 Field field = player.getField();
 Graveyard graveyard = player.getGraveyard();
 FusionCard fusionCard = (FusionCard)hand.play(fusionCardIndex);
 MonsterCard baseMonsterCard = field.destroyMonster(baseMonsterCardIndex);
 MonsterCard fuserMonsterCard = field.destroyMonster(fuserMonsterCardIndex);
 MonsterCard fusedCard = baseMonsterCard.fuseWith(fuserMonsterCard, fusionCard);
 graveyard.add(baseMonsterCard);
 graveyard.add(fuserMonsterCard);
 graveyard.add(fusionCard);
 field.setMonster(baseMonsterCardIndex, fusedCard);
 }
}

---

public class FuseMonsterActionTest {
 static {
 assertTrue(true);
 }
 private Player player;
 private final PlayerConfiguration playerConfiguration = new PlayerConfigurationBuilder()
 .hitpoints(100)
 .turnAction(p -> { })
 .handCapacity(5)
 .fieldMonsterCapacity(5)
 .deckCards(Arrays.asList(new MonsterCard("Random", 5, 5, MonsterModus.HEALING)))
 .build();
 @Before
 public void before() {
 player = Player.createFromConfiguration(playerConfiguration, "Test");
 }
 @Test
 public void testConstructor() {
 new FuseMonsterAction(0, 0, 1);
 }
 @Test(expected = IllegalArgumentException.class)
 public void testConstructorNegativeFusionCardIndex() {
 new FuseMonsterAction(-1, 0, 0);
 }
 @Test(expected = IllegalArgumentException.class)
 public void testConstructorNegativeBaseMonsterCardIndex() {
 new FuseMonsterAction(0, -1, 0);
 }
 @Test(expected = IllegalArgumentException.class)
 public void testConstructorNegativeFuserMonsterCardIndex() {
 new FuseMonsterAction(0, 0, -1);
 }
 @Test(expected = IllegalArgumentException.class)
 public void testConstructorEqualBaseMonsterCardIndexAndFuserMonsterCardIndex() {
 new FuseMonsterAction(0, 0, 0);
 }
 @Test
 public void testIsActionAllowed() {
 Hand hand = player.getHand();
 Field field = player.getField();
 hand.add(new FusionCard("Fusion Card", 50, FusionStat.ATTACK));
 field.setMonster(0, new MonsterCard("Base Monster", 10, 50, MonsterModus.HEALING));
 field.setMonster(1, new MonsterCard("Fuser Monster", 40, 50, MonsterModus.HEALING));
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(0, 0, 1);
 assertTrue(fuseMonsterAction.isActionAllowed(player));
 }
 @Test
 public void testPerformAction() {
 Hand hand = player.getHand();
 Field field = player.getField();
 Graveyard graveyard = player.getGraveyard();
 FusionCard fusionCard = new FusionCard("Fusion Card", 50, FusionStat.ATTACK);
 MonsterCard baseMonster = new MonsterCard("Base Monster", 10, 50, MonsterModus.HEALING);
 MonsterCard fuserMonster = new MonsterCard("Fuser Monster", 40, 50, MonsterModus.HEALING);
 hand.add(fusionCard);
 field.setMonster(0, baseMonster);
 field.setMonster(1, fuserMonster);
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(0, 0, 1);
 fuseMonsterAction.performAction(player);
 assertFalse(hand.contains(fusionCard));
 assertFalse(field.hasMonster(1));
 assertTrue(field.hasMonster(0));
 assertNotSame(baseMonster, field.getMonster(0));
 assertNotSame(fuserMonster, field.getMonster(0));
 assertTrue(graveyard.contains(fusionCard));
 assertTrue(graveyard.contains(baseMonster));
 assertTrue(graveyard.contains(fuserMonster));
 }
 @Test(expected = NullPointerException.class)
 public void testIsActionAllowedNullPlayer() {
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(0, 0, 1);
 fuseMonsterAction.isActionAllowed(null);
 }
 @Test(expected = NullPointerException.class)
 public void testPerformActionNullPlayer() {
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(0, 0, 1);
 fuseMonsterAction.performAction(null);
 }
 @Test
 public void testIsActionAllowedFusionCardIndexOverHandCapacity() {
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(5, 0, 1);
 assertFalse(fuseMonsterAction.isActionAllowed(player));
 }
 @Test(expected = PlayerActionNotAllowedException.class)
 public void testPerformActionFusionCardIndexOverHandCapacity() {
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(5, 0, 1);
 fuseMonsterAction.performAction(player);
 }
 @Test
 public void testIsActionAllowedBaseMonsterCardIndexOverFieldMonsterCapacity() {
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(0, 5, 1);
 assertFalse(fuseMonsterAction.isActionAllowed(player));
 }
 @Test(expected = PlayerActionNotAllowedException.class)
 public void testPerformActionBaseMonsterCardIndexOverFieldMonsterCapacity() {
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(0, 5, 1);
 fuseMonsterAction.performAction(player);
 }
 @Test
 public void testIsActionAllowedBaseFuserCardIndexOverFieldMonsterCapacity() {
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(0, 0, 5);
 assertFalse(fuseMonsterAction.isActionAllowed(player));
 }
 @Test(expected = PlayerActionNotAllowedException.class)
 public void testPerformActionBaseFuserCardIndexOverFieldMonsterCapacity() {
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(0, 0, 5);
 fuseMonsterAction.performAction(player);
 }
 @Test
 public void testIsActionAllowedNonFusionCardInHand() {
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(0, 0, 1);
 player.getHand().add(new MonsterCard("Random", 5, 5, MonsterModus.HEALING));
 assertFalse(fuseMonsterAction.isActionAllowed(player));
 }
 @Test(expected = PlayerActionNotAllowedException.class)
 public void testPerformActionNonFusionCardInHand() {
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(0, 0, 1);
 player.getHand().add(new MonsterCard("Random", 5, 5, MonsterModus.HEALING));
 fuseMonsterAction.performAction(player);
 }
 @Test
 public void testIsActionAllowedBaseMonsterCardNotOnField() {
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(0, 0, 1);
 player.getHand().add(new FusionCard("Fusion Card", 50, FusionStat.ATTACK));
 assertFalse(fuseMonsterAction.isActionAllowed(player));
 }
 @Test(expected = PlayerActionNotAllowedException.class)
 public void testPerformActionBaseMonsterCardNotOnField() {
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(0, 0, 1);
 player.getHand().add(new FusionCard("Fusion Card", 50, FusionStat.ATTACK));
 fuseMonsterAction.performAction(player);
 }
 @Test
 public void testIsActionAllowedFuserMonsterCardNotOnField() {
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(0, 0, 1);
 player.getHand().add(new FusionCard("Fusion Card", 50, FusionStat.ATTACK));
 player.getField().setMonster(0, new MonsterCard("Base Monster", 5, 5, MonsterModus.HEALING));
 assertFalse(fuseMonsterAction.isActionAllowed(player));
 }
 @Test(expected = PlayerActionNotAllowedException.class)
 public void testPerformActionFuserMonsterCardNotOnField() {
 FuseMonsterAction fuseMonsterAction = new FuseMonsterAction(0, 0, 1);
 player.getHand().add(new FusionCard("Fusion Card", 50, FusionStat.ATTACK));
 player.getField().setMonster(0, new MonsterCard("Base Monster", 5, 5, MonsterModus.HEALING));
 fuseMonsterAction.performAction(player);
 }
}

The specific question is about the testIsActionAllowedNonFusionCardInHand for example, should I there enforce all other conditions which could make a test fail? In this case it would be setting the other monsters on the field, as that might cause the test to fail.

Few extra notes:

• I prefer to explicitely list all interfaces a class implements, especially when using abstract superclasses.
• static { assertTrue(true); } is needed to stop Netbeans from complaining that it wants to remove the static import if I didn't use an assert yet...

If you would like to take a look at the other classes, then feel free to take a look at this repository.

• Vogel612: Did you really test that Exception gets correctly implemented? Talk about paranoid...

Answer 1

I just want to make a few minor remarks, IMO these are more "preference" than "convention", so take 'em with a grain of salt

Test "sectioning"

@Test
public void testPerformAction() {
 PlayerAction playerAction = new AbstractPlayerActionImpl();
 Player player = new Player("Test", 100, new TurnActionImpl(), new Hand(5), new Field(5), new Deck(Arrays.asList(new MonsterCard("Test", 5, 5, MonsterModus.HEALING))), new Graveyard());
 playerAction.performAction(player);
 assertEquals(1, counter.get());
}

taking this here as example:
I personally prefer really placing an empty line between the test sections: given, when, then. reformatting the example would look like that:

@Test
public void testPerformAction() {
 PlayerAction playerAction = new AbstractPlayerAction();
 Player player = new Player(/* [...] */);
 playerAction.performAction(player);
 assertEquals(1, counter.get());
}

Positive or Zero and similar:

Arguments.requirePositiveOrZero(baseMonsterCardIndex, "baseMonsterCardIndex");
Arguments.requirePositiveOrZero(fuserMonsterCardIndex, "fuserMonsterCardIndex");

let's make a definitely non-working example...

new FuseMonsterAction(1000, 1001, 1002);

Well that went somewhat wrong :(

Objects.requireNonNull(player, "player");
Hand hand = Player.getHand(); //NPE inbound!

this one looks extremely NullPointerException prone to me. IMO you trust the Player Object too much that it will not have a null hand.