Ineffective Java

Perhaps I can be replaced by a robot for code review. There are some bits of feedback I find myself giving over and over again. Here are some of my least favourites:

General Code Structure

Drop The Else

When if ends in return the else is superfluous and creates unnecessary indentation.

Something calc(Foo foo) {
    if (foo.isAwesome()) {
       return bar();
    } else { 
       return baz();
    }
}

// if can be simplified
if (foo.isAwesome()) {
   return bar();
}
return baz();

The above comes with a caveat. In the original form, the presence of the else, along with no final return statement, means that the if statement MUST also return. This is an example where we could use the compiler to avoid a drop-through bug.

This is a subject worthy of its own discussion at another time.

Array -> List -> Stream

List< ... > list = Arrays.asList(someArray);
list.stream(...)

// should be replaced by 

Arrays.stream(someArray)

Test Code

Test Name has Test in it!

Don’t name a test test_something any more than you name every file file. We know it’s a test. What’s the given/when/then of it?

Before is a Heavy Initializer

We use a @Before method to set up complex objects, often where we need to do processing to calculate what the class instance member needs to have in it. At the other end of the spectrum, it’s overkill:

// this is part 1 of two
private MyService myService;

@Before
public void before() {
    // now initialize
    myService = new MyService().init(123);
}

// the above code can be expressed in the initializer
// and is simple to read there...
// if it threw exceptions or needed some more complex 
// set up, it wouldn't be

// it's in one clear place where we know where to 
// find it
private MyService myService = new MyService()
    .init(123);

Test Throws

@Test
public void someTest() 
    throws IOException, JsonException {
}

// never bother with multiple or specific exception 
// throws in tests nobody cares and it's just noise
// the test runner will catch anything!

@Test
public void someTest() throws Exception {
}

AssertJ for Size

// long-winded
assertThat(list.size()).isEqualTo(2);

// should be
assertThat(list).hasSize(2);

AssertJ for Everything

The built in JUnit assertions are not as rich as those provided by AssertJ. As a minimum, I recommend using some form of assertThat, so you don’t end up using an assertion that’s a bit weak for the situation.

Your assertEquals Is The Wrong Way Around

60% of the time, when I review code with assertEquals in, the order is wrong. Hint: use AssertJ!!! JUnit is wrong on this one! We should read from left to right.

// wrong:
assertEquals(something.getFoo(), 123);

// it's expected IS actual
assertEquals(123, something.getFoo());

Mockito Static Imports

// this is not normal
Mockito.verify(mock).called();

// static import all mockito methods
verify(mock).called();

Mockito Times(1)

// this is a tautology
verify(mock, times(1)).called();

// look at what verify(mock) does internally
// replace with

verify(mock).called();

Mockito matchers

We know that once you start using matchers like any in Mockito, you also need to use eq in the same statement. But using it always?

given(mockFoo.callMe(eq("John"), eq("Smith"))
    .willReturn(mockPerson);

// replace use of matchers with literals when possible
given(mockFoo.callMe("John", "Smith")
    .willReturn(mockPerson);

As a side-note, unless it’s particular to the assertion of the test, or there’s any doubt, I would default to the use of any in a Mockito expression, rather than create fragile implementation-heavy mocking.

Don’t create Mockito Objects Yourself

Behold a lot of typing:

FooService service = mock(FooService.class);
ArgumentCaptor<Foo> fooCaptor = ArgumentCaptor.forClass(Foo.class);

This implies a couple of things:

  • You’re making mock objects for multiple tests
  • You’re doing the work of the mockito plugin for JUnit
@Mock
private FooService service;

@Captor
private ArgumentCaptor<Foo> fooCaptor;

This is a lot cleaner, less work.

And please never call MockitoAnnotations.initMocks(this). While you may find examples of how to do this online, I’ve nearly always seen this technique add confusion to tests.

The use of the annotations requires the MockitoJUnitRunner or the MockitoJUnitRule (JUnit4) or the MockitoExtension (JUnit 5). Use them properly and have less code to worry about. Simple!

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s