7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Did you really test that Exception gets correctly implemented? Talk about paranoid... \$\endgroup\$ – Vogel612 May 30 '14 at 16:06
3
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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