Trading Card Game's Hand and HandView implementation and unit tests

This question continues on my previous implementations of the Hand class of a Trading Card Game, earlier questions can be found here:

• Earlier model: Trading Card Game's Hand class and tests

• Older model having similar functionality as this one, likely deprecated for current one: Trading Card Game's Hand and HandView implementation using composition and decoration

I have tried to implement as much of the old reviews as I thought was neccessary, except the point about the unit test method names, I haven't had time to do that yet.

The goal of this implementation is to allow concrete instances of a HandView to listen for add, play and swap events on the Hand class. I expect concrete implementations to work together with for example the console or a GUI to relay the information back to the user.

I will first list some dependency classes without unit tests, and then my implementations with unit tests.

Dependencies:

```
public final class Arguments {
private Arguments() {
throw new UnsupportedOperationException();
}

public static int requirePositive(final int value) throws IllegalArgumentException {
return requirePositive(value, "value");
}

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 negative: " + value);
}
return value;
}

public static int requirePositiveOrZero(final int value) throws IllegalArgumentException {
return requirePositiveOrZero(value, "value");
}

public static int requirePositiveOrZero(final int value, final String name) throws IllegalArgumentException {
Objects.requireNonNull(name);
if (value 0) {
throw new IllegalArgumentException("the " + name + " must be negative or zero: " + value);
}
return value;
}

public static i

public interface Viewable, V extends View> {
    public void addViewCallback(final V view);

    public void removeViewCallback(final V view);
}

Drop the public keywords from the methods. Methods in interfaces are automatically public. See this SO thread for reference.

@Test(expected = IllegalArgumentException.class)

As mentioned in another review, there are good reasons to choose ExpectedException rules over @Test(expected = …).

public void testConstructorIAE() {

Maybe a matter of taste, but I don't like these acronyms for exceptions. If you read it for the first time, it's definitely something you have to think about for half a second.

@Test(expected = NullPointerException.class)
public void testAddViewCallbackViewNull() {
    Hand hand = new Hand(5);
    hand.addViewCallback(null);
}

I still think your job should be preventing internal NPEs to shoot back to the caller. NullPointerException is uselessly generic. Your method knows what went wrong, so give the caller better feedback.

I also agree with what was said in the comments: A test that doesn't assert is useless. I know it seems like "but it's okay here" – I used to think like that and wrote tests like that. Granted, I work with a larger code base, but I ended up finding my own tests that had slightly changed over time to be useless because of this.

I also agree with the other thing from the comments – testXyz is a good name for simple unit tests. Integration tests should have clear, descriptive names. In our project, we use a givenXyzWhenXyzThenXyz pattern. Of course you don't have to stick to this, it's just one of many possibilites.

The nice thing about this pattern is that it forces you to state the abstraction of what is to be asserted in the test name. And really, that is what integration tests do: they don't test the tiny little innards of the system – they test the big picture. On top of that, such proper test names – again, speaking heavily from experience here – make it drastically easier to quickly understand what a test does.

Really, the problem with integration tests is that they are more complex than unit tests and therefore need time to be understood. And just like you want clear method names in the production code to abstract the implementation details away, you should go for clear names in tests that do the same.

Just FYI, our test names can be really long. Really long. Something along the lines of

@Test
public void givenTwoBusinessRequestsViaSoapWithSameDataWhenExecuteRequestProcessingThenSecondRequestIsignored() {
    // …
}

is common and by far not the longest. So don't feel bad giving your tests long names – descriptive is more important.

The last thing in the comment mentions that you should write tests first, make sure they fail and then work on making them pass. While I agree that this is an effective approach, this so-called Test-Driven Development (TDD) is not "mandatory". But I definitely do recommend it!

Other than these things, it looks solid to me. I didn't look at the dependency/helper classes since I assume they didn't change.

StackExchange Code Review Q#48509, answer score: 3