Checking return values in Java

The other day I made a stupid coding error. It took me the better part of an hour to track it down. We all have those moments. Blame it on a bad night’s sleep, a brain fart, or perhaps the fact that your colleague at the next desk has been whistling that tune from Gotye’s “Somebody I used to know” (you know the one) all day, completely driving you crazy and taking your focus off your code.

Anyway. Here’s my coding error:

<br>
// If the finishedTest is an exam or a<br>
// practiceTest, go to the next chapter or paragraph<br>
getNextChapterParagraph(chapters, taskContentIdentifier);<br>
if (taskContentIdentifier == null) return null;<br>
return new Task(taskContentIdentifier, Type.ASSESSMENT);<br>

And here’s the getNextChapterParagraph() method:

<br>
/**<br>
 * Returns the next paragraph or chapter given a list<br>
 * of chapters and the current chapter/paragraph.<br>
 */<br>
private ContentIdentifier getNextChapterParagraph(<br>
    List&lt;Chapter&gt; chapters,<br>
    ContentIdentifier contentIdentifier) {</p>
<p>  // Do some stuff.....</p>
<p>  return new ContentIdentifier(<br>
    chapter.getStream().getId(),<br>
    chapter.getContentId(),<br>
    paragraph.getContentId());<br>
}<br>

I’ve omitted and simplified some code to stick to the point, but I’m sure some of you have figured out what I did wrong by now. The getNextChapterParagraph() method doesn’t modify the ContentIdentifier instance passed as its second parameter; it returns a new one. Yet I wrote the calling code as if it did change the state of my taskContentIdentifier. D’oh!

A more common case of this is calling methods on immutable objects, like String. For example:

<br>
String string = "this is a test";<br>
string.substring(10);	// Bad!<br>

Now, of course, you can catch errors like this in your unit tests, which is how I found the error in the first place. But wouldn’t it be nice if there was some way to find out right when you make this mistake?

What’s wrong?

So what exactly is wrong with the code above? Simply put: the getNextChapterParagraph (and String.substring) method’s sole purpose is to return a value; it doesn’t change the objects whose references were passed to it as parameters, and it doesn’t change the state of the object it’s a method of. In other words, it has no side effects. Because of this, if we call the method, we should always do something with the return value. If not, why did we call it in the first place? (There is actually one reason you might, but I’ll discuss this later on.)

So we’d like to have the compiler or some tool warn us when we call a method that has no side effects, yet discard its return value. Any solution would have two parts:

1. Mark a method as having no side effects (perhaps it would be possible to determine by static analysis if a method has no side effects, but that’s a different blog post)
2. Warn when a method with no side effects is called and the return value is discarded (that is, not assigned to a variable, not used in an expression, and not passed to another method).

Enter: FindBugs and @CheckReturnValue

As it turns out, FindBugs, the code analysis tool most of you are probably familiar with, has exactly the tools for this. I’ll describe how to set-up FindBugs in Eclipse further down. First, the solution to our problem: the @CheckReturnValue annotation. Simply put, @CheckReturnValue marks a method as having no side effects. If FindBugs then detects a method call to such a method where the return value is discarded, it emits a warning.

To use @CheckReturnValue you’ll need the findbugs-annotations.jar on your classpath. Maven users can add the following dependency to their POM:

<br>
&lt;dependency&gt;<br>
  &lt;groupId&gt;findbugs&lt;/groupId&gt;<br>
  &lt;artifactId&gt;annotations&lt;/artifactId&gt;<br>
  &lt;version&gt;1.0.0&lt;/version&gt;<br>
&lt;/dependency&gt;<br>

Example of using @CheckReturnValue:

<br>
package nl.trifork.examples.checkreturnvalue;</p>
<p>import edu.umd.cs.findbugs.annotations.CheckReturnValue;</p>
<p>public class StringHelper {</p>
<p>	@CheckReturnValue<br>
	public String shift(String str) {<br>
		if (str.length() &lt; 2) return str;<br>
		return str.substring(1) + str.charAt(0);<br>
	}<br>
}<br>

Now, let’s try to call this method without using the return value:

<br>
public class CheckReturnValueExample {</p>
<p>	public void testStringHelper() {<br>
		StringHelper stringHelper = new StringHelper();<br>
		String str = "string";<br>
		stringHelper.shift(str);<br>
		//str = stringHelper.shift(str);<br>
		System.out.println(str);<br>
	}<br>
}<br>

Running FindBugs yields the following warning:

Bug: return value of StringHelper.shift(String) ignored in nl.trifork.examples.checkreturnvalue.CheckReturnValueExample.testStringHelper()

Now, we can’t annotate methods in JDK classes with @CheckReturnValue, but luckily FindBugs already seems to have checks for most common cases built-in:

<br>
public class CheckReturnValueExample {</p>
<p>	public void jdkStringExample() {<br>
		String str = "this is a test";<br>
		str.substring(10);<br>
		System.out.println(str);<br>
	}<br>
}<br>

FindBugs:

Bug: return value of String.substring(int) ignored in nl.trifork.examples.checkreturnvalue.CheckReturnValueExample.jdkStringExample()

Exceptions

So are there any cases in which it is valid to discard the return value of a call to a side effect-less method? Actually, there is, but it stems from the fact that none except the most trivial of methods are truly without side effects. Consider String.substring():

substring

public String substring(int beginIndex)

Returns a new string that is a substring of this string. The substring begins with the character at the specified index and extends to the end of this string.

Examples:

 "unhappy".substring(2) returns "happy"
 "Harbison".substring(3) returns "bison"
 "emptiness".substring(9) returns "" (an empty string)

Parameters:

beginIndex – the beginning index, inclusive.

Returns:

the specified substring.

Throws:

IndexOutOfBoundsException – if beginIndex is negative or larger than the length of this String object.

Let’s say we wanted to write a unit test for substring testing that indeed passing a negative beginIndex causes an IndexOutOfBoundsException to be thrown:

<br>
	@Test(expected=IndexOutOfBoundsException.class)<br>
	public void testSubstring() {<br>
		String str = "this is a test";<br>
		str.substring(-1);<br>
	}<br>

If we run this through FindBugs, we get:

Bug: return value of String.substring(int) ignored in nl.trifork.examples.checkreturnvalue.CheckReturnValueExample.testSubstring()

The long and short of this case is that, since we’re calling the method in a way it should never be in “normal” (non-testing) code (they’re called “exceptions” for a reason), this test class should not be run through FindBugs, or with a limited set of detectors enabled.

IDE integration

Now it’s all good and well to run FindBugs every once in a while to validate your code’s quality, but to catch these “check return value”-errors, you would need to be warned immediately, since by the time you would otherwise run FindBugs, you’d have already spent hours tracking down the bug. What we want is a FindBugs check everytime the incremental compiler runs. The FindBugs plugin for Eclipse can do exactly that.

The easiest way to get the FindBugs plugin in Eclipse is to download the SpringSource Tool Suite, SpringSource’s version of the Eclipse IDE. On the Extensions page of the Dashboard view, you can select FindBugs, press Install, agree to the licence, press Next a few times and the Finish, and behold, after an Eclipse restart FindBugs is installed and ready to use.

The “plain vanilla” Eclipse users can add download site http://findbugs.cs.umd.edu/eclipse and install the FindBugs feature. IntelliJ users: I’m sure you’ve already got FindBugs installed 🙂
By default FindBugs only runs when it’s launched from the Find Bugs context menu on a project or class. However, with a quick change you can get automatic FindBugs analysis every time your code is built automatically (usually when saving). Select your project, open up the project properties (Alt-Enter on *nix and Windows or ⌘I on Mac). Go to FindBugs in the left-hand panel. Then, check the “Run automatically” check box.

Now, when you save your class file, FindBugs will run and you will see FindBugs warnings immediately in the gray “gutter” to the left of your source code:

Clicking the bug marker will open of the Bug Info panel with a description of the bug.

Conclusion

Using the @CheckReturnValue annotation with FindBugs can warn you about forgetting to use a return value before you realize you forgot. So go, annotate all your side effect-less methods with @CheckReturnValue (that includes all those getter methods!) and make FindBugs a part of your regular build cycle. I know I will.

On to better code!