patternjavaMinor
FuseMonsterAction implementation and unit tests
Viewed 0 times
testsandimplementationfusemonsteractionunit
Problem
I'd like to get a general review on the following code, and I'll highlight an extra point below:
```
public abstract class AbstractPlayerAction implements PlayerAction {
@Override
abstract public boolean isActionAllowed(final Player player);
@Override
public void performActio
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 performActio
Solution
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"
taking this here as example:
I personally prefer really placing an empty line between the test sections:
Positive or Zero and similar:
let's make a definitely non-working example...
Well that went somewhat wrong :(
this one looks extremely NullPointerException prone to me. IMO you trust the Player Object too much that it will not have a null hand.
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.
Code Snippets
@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
public void testPerformAction() {
PlayerAction playerAction = new AbstractPlayerAction();
Player player = new Player(/* [...] */);
playerAction.performAction(player);
assertEquals(1, counter.get());
}Arguments.requirePositiveOrZero(baseMonsterCardIndex, "baseMonsterCardIndex");
Arguments.requirePositiveOrZero(fuserMonsterCardIndex, "fuserMonsterCardIndex");new FuseMonsterAction(1000, 1001, 1002);Objects.requireNonNull(player, "player");
Hand hand = Player.getHand(); //NPE inbound!Context
StackExchange Code Review Q#52099, answer score: 3
Revisions (0)
No revisions yet.