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.
01 02 03 04 05 06 07 08 09 10 11 | if (foo) { return bar;} else { return baz;}// should be replaced byif (foo) { return bar;}return baz; |
Array -> List -> Stream
1 2 3 4 5 6 | List< ... > list = Arrays.asList(someArray);list.stream(...)// should be replaced by Arrays.stream(someArray) |
Test Code
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:
01 02 03 04 05 06 07 08 09 10 11 12 13 14 15 16 17 18 | // this is part 1 of twoprivate MyService myService;@Beforepublic 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 itprivate MyService myService = new MyService() .init(123); |
Test Throws
01 02 03 04 05 06 07 08 09 10 11 12 | @Testpublic 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!@Testpublic void someTest() throws Exception {} |
AssertJ for Size
1 2 3 4 5 | // long-windedassertThat(list.size()).isEqualTo(2);// should beassertThat(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.
1 2 3 4 5 | // wrong:assertEquals(something.getFoo(), 123);// it's expected IS actualassertEquals(123, something.getFoo()); |
Mockito Static Imports
1 2 3 4 5 | // this is not normalMockito.verify(mock).called();// static import all mockito methodsverify(mock).called(); |
Mockito Times(1)
1 2 3 4 5 6 7 | // this is a tautologyverify(mock, times(1)).called();// look at what verify(mock) does internally// replace withverify(mock).called(); |
Published on Java Code Geeks with permission by Ashley Frieze, partner at our JCG program. See the original article here: Ineffective Java Opinions expressed by Java Code Geeks contributors are their own. |
The first example is even more succinct as
return foo ? bar : baz;
In this trivial case, you’re right. Ternary expressions are a good solution to the most trivial cases, though extracting an explanatory function to name the calculation is often a good addition to it.
The point of the above patterns, which will grow on the source site (cited in the article) is about the structure of if statements in general. I’ve used one-liner examples to illustrate this.
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.
The first case I’ve seen mentioned before and is one I fervently disagree with. The else indicates it’s mutually exclusive. I shouldn’t have to look up further in the code for a return to figure that out. Further, it prevents errors. I’ve seen this bug many times in my career: if (something) { …a few setup lines… doSpecialThing(); // oops, should be return doSpecialThing() } return doOtherThing(); This will always doOtherThing() because of the bug in the “if” where the person forgot the return. doOtherThing() is a mutually exclusive path and if you use the else to indicate it, then… Read more »
Interesting insight there, and I’ll probably incorporate some of what you’ve said into a future article, along with a footnote on the if statement. This will appear at the original site; I don’t know if they’ll replicate the updates here. I’d like to make three points in response to yours, though. 1. As you get to more mission-critical applications, you may need to add otherwise redundant programming styles to your code to add assurance. 2. If your methods are so long that you can’t see the structure at a glance (I’m taking 5 lines of code as a “large method”),… Read more »
100% Agree.
Article is crap! Please replace author by robot