Archivo mensual: marzo 2010

FindBugs: better default values for Nullability Annotations

One of the nicest features in FindBugs is its support for annotations indicating whether a certain value is nullable or not, and the way it uses that information to perform additional checks.

There are three annotations: @NonNull, @CheckForNull and @UnknownNullnes.
By default, FindBugs assumes that method parameters, method return values and fields are annotated implicitly with @UnknownNullness.

But, to me, the best policy is:

  • For method parameters: use @CheckForNull.
  • For method return values: use @NonNull.
  • For object fields: use @NonNull.

Why?

Easy. We are trying to follow this principle: give as much as as you can, demanding as little as possible -in a safe way.

For method parameters, the worst case is receiving a null value, because your code will need to be careful and check for null if it uses the value in any meaningful way. Therefore, we should use @CheckForNull by default because this demands less from our code users.

For method return values, life will be easier for our code users if they need not care about a returned value being null. Therefore, use @NonNull to give as much as you can.

For object fields, things are not that clear cut, because they do not affect the user (directly), as they should be private. I just chose to make them @NonNull because I feel this is the safest policy.

Of course, these are the “ideal” settings: you will have to provide an override from time to time, annotating an element explicitly with a different annotation.

How to do this?

You can provide the desired default annotations for a package by adding the following annotations to package-info.java:


@DefaultAnnotationForParameters( value=CheckForNull.class)
@DefaultAnnotationForFields( value=CheckForNull.class)
@DefaultAnnotationForMethods( NonNull.class)

Really neat!

If you start using these annotations with FindBugs, you will be surprised by the clarity they add and the way they can help diagnose null values misuse.

Anuncios

Taking a look at FindBugs in a real project: checking DirectJNgine

I have been testing FindBugs effectiveness and ease of use in an existing project, DirectJNgine. A small project, 5.600 Java lines, plus 2.000 Javascript lines.

First conclusions

With all settings set to the maximum strictness, FindBugs reported 70 issues. Do not get alarmed, though! Issues are things FindBugs thinks you should check, not necessarily bugs.

In fact, only one of these issues ended up being a bug, one that would happen once in a lifetime -but it was a real bug.

The experience was very positive: in fact, I never found a diagnostic I disagreed with. And, in a few cases, FindBugs pinpointed code that I just did not know could be suspicious.

In many cases, FindBugs reported performance issues that, I knew beforehand were absolutely irrelevant, but would have been a serious problem in a different context.

That said I converted all code to the better performing version, if only to make FindBugs happy.

Now, this is very important: I strongly believe you should never commit code with outstanding warnings or issues to your version control system. This destroys credibility and trust.

Another way to say this is do not live with broken windows… Yes, sometimes you will have to do some extra work just to please FindBugs, but I think this will pay off later.

Back to FindBugs, in 80% of the cases the diagnostic explanation allowed me to “fix” the code directly.

When that was not the case,  it provided me sufficient information to let me find the way to fix the problem with ease.

To be fair, the only “poor diagnostic” case I found was for circular class dependencies!

Of all checks performed by FindBugs, I disabled only the checks for circular class dependencies (two cases), and only because FindBugs is a stable and mature project with a published API I am committed to support. In an ongoing project I would probably have fixed that.

For issues I did not want to “fix” (because I considered that the issues were not real problems, and the “fix” was too convoluted or impossible), FindBugs provided me with the @SuppressWarnings annotation -which you should use scarcely!

All in all, making FindBugs happy was not difficult, enhanced my code in some cases, helped me uncover and fix one bug, and only in a few cases got in the way. And all of this setting it to the maximum strictness. A really good balance sheet!

Feasibility of a “cleanup campaign” for existing projects

Of course, you will need to make especial provisions if you want to introduce FindBugs in an ongoing project, unless you are prepared to launch a cleanup campaign that ensures your application has zero issues and uses FindBugs annotations (more on that later!).

Now, I have to say that the full cleanup campaign for DirectJNgine, with all settings set to the maximum strictness (!), took arount 10 to 12 hours, which I think is very little time.

If you take into account that I had to learn FindBugs way too, I’m quite sure that would have taken 6/8 hours if I had to do it now in a similar project.

Use FindBugs annotations -really!

Most people seem to be using FindBugs to check their code, but they seem not to be using FindBugs Annotations to help it diagnose even more problems.

Too bad, for these annotations are hidden gems. Use them! They will help FindBugs to check for adequate resource cleanup, correct null handling, etc. I have discussed them in other articles and posts about FindBugs.

My experience “porting” DirectJNgine to use these annoations was straightforward: 70 minutes only!

To be fair, I’m sure that was partly due to the fact that I have near 500 assertions in DJN. Many of these are checks for nulls,and that helped a lot when trying to make sure whether values were nullable or not.

While I do not expect most programs to be “ported” so easily, I don’t think it will take too much time.

That said, and even though I’m almost paranoid with checks for null values, FindBugs found two places where I had forgotten to assert that a value could not be null. So, again, a good result.

The Eclipse plugin

The Eclipse plugin for FindBugs works quite well. However, you need to know that some kinds of checks are not performed automatically when you save the source code.

Therefore, for some kinds of issues, you will need to manually run the FindBugs checker or rebuild the whole app. This happens only for some kinds of issues, and in most cases having real-time checking works and is useful, so not much of a problem if you are aware of this.

One caveat, though: DirectJNgine is a small project, so maybe FindBugs will be more taxing for medium to big projects. I’ll report on this when I introduce FindBugs in bigger projects.

On closing

FindBugs is an excellent and unobtrusive tool, worth using. In fact, I just hope the compiler could provide many of its checks as warnings.

FindBugs: make your code better with Nullability Annotations

Handling null values is not that easy. For example, there are scenarios in which null values will never happen: this makes code checking whether a value is null not only unnecessary, but misleading -because it creates the wrong expectations.

However, in other cases, a null value might happen, and we should test for this before using it -unless we are happy when with null related exceptions are raised.

Sadly, the fact is the Java compiler will not provide help with this. But all is not lost: FindBugs provides several annotations that might help, especially because it can perform some code analysis that will detect potentially dangerous or unnecesary code.

Null checks

There are three scenarios when it comes to non-primitive values: you assume that values can’t be null, you assume that they can…or you assume nothing. FindBug provides us with three corresponding annotations: @NonNull, @CheckForNull and @UnknownNullness.

A quick example

Let’s take an in-depth look at this class:

public static class NullHandling {
  public String value;
  public int getValueLength() {
    return this.value.length();
  } 
  public String myToUppercase( String parameter )
  {
    if( parameter == null )
      return "";
    return parameter.toUpperCase(Locale.getDefault());
  }

  public String getValue() {
    return this.value;
  } 
  public void justUseGetValue() {
    int length;
    if( getValue() == null )
      length = 0;
    else
      length = getValue().length();
    System.out.println( "Value length: " + length);
  } 
  public String justUseMyToUppercase() {
    return myToUppercase(null);
  }
}

Let me pinpoint some potential problems in this code:

  • If the value field can be null, then this.value.length() in getValueLength will bomb.
  • If getValue can’t return a null value, then code using it (as justUseGetValue does) need not check for null values and can be simpler.
  • If parameter can’t be null in myToUppercase, checking for it is unnecesary -and misleading.
  • If myToUppercase can’t receive a null parameter, then calling it the way justUseMyToUppercase does is a bug.

Well, we are not short of potential problems or unnecesary code, but the compiler remains completely mute. And this is only logical, because there is no way to tell the compiler about the nullability restrictions for fields, parameters or method return values.

Now, let’s use FindBugs annotations to provide clarity and helpful diagnostics, if only for illustative purposes.

As a first change, let’s pretend that the value field might be null. To that end, we need to annotate it with @CheckForNull as follows:

@CheckForNull
public String value;

This causes a warning to be issued by FindBugs for the following code:

public int getValueLength() {
  return this.value.length();
}

Effectively, this.value migth be null, and therefore calling this.value.length() in getValueLength without checking whether it is null is not something I would recommend.

Here is a better implementation of getValueLength, which makes FindBugs happier:

public int getValueLength() {
  if( this.value == null )
    return 0;
  return this.value.length();
}

Now, let’s pretend that myToUppercase can’t be called with a null value, as declared in the following code:

public String myToUppercase(
@NonNull tString parameter )

This causes FindBugs to report two issues. The first one in myToUppercase, because we are checking whether parameter is null, when that should never happen.

To solve this, we just remove the null check, leaving the method body as follows:

return parameter.toUpperCase(Locale.getDefault());

The second issue is reported in justUseMyToUppercase, which was calling myToUppercase passing a null value, something illegal: the fix is to pass it a valid value.

We have seen examples of nullability annotations for fields and method parameters: let’s see such an annotation for a method, so that we have an example of each possible usage of these annotations. To that end, we will pretend that getValue just can’t return a null value, as follows:

@NonNull
public String getValue() {
  return this.value;
}

That makes FindBugs complain, because this.value might be null, and in that scenario getValue will be breaking the guarantee. We can fix getValue by checking for this.value being null as follows:

@NonNull
public String getValue() {
  if( this.value == null )
    return "";
  return this.value;
}

FindBugs will complain about justUseGetValue (which I made rather convoluted on purpose!), because it is checking for getValue returning null when it just can’t do that. The fix is as easy as rewriting justUseGetValue as follows:

public void justUseGetValue() {
  int length = getValue().length();
  System.out.println( "Value length: " + length);
}

I think that these examples illustrate quite well the usefulness of explicitly declaring whether a field, method parameter or returned value can be nullable or not, and how things can go wrong in subtle ways when you choose to ignore the issue (the default case for Java code).

By the way, I want to mention the fact that I have made all code self contained in a class to make code shorter and easier to understand: however, FindBugs will make checks when external code uses fields, parameters or methods with nullability annotations. This is very interesting, because users consuming our code will probably know very little about our assumptions: annotations and the ability to check them will be even more valuable in that scenario.

What to annotate: method parameters, fields and method results

Our examples have shown all places where these annotations can be placed: in fields, method parameters and method results. The only thing worth mentioning is the fact that, for fields, @NonNull just means the fields will never be null after construction, whereas @CheckForNull means they can be null after construction.

And, of course, for methods the annotation refers to the returned value.

The @Nullable annotation

I have to make a little confession: FindBugs provides an additional @Nullable annotation too. I haven’t discussed this one because, as far as I know, this annotation has no well defined semantics: see the discussion at http://code.google.com/p/google-collections/issues/detail?id=247, or http://osdir.com/ml/java.findbugs.general/2006-09/msg00063.html, for example.

For the time being, just ignore it unless you want to make your life more difficult:@CheckForNull and @NonNul seem to be more than enough.

And, of course, if somebody out there knows better, I’ll be glad to hear from him!

Other issues

I’m a fervent fan of Java assertions, and I write assertions checking that a parameter is not null so often that it has become part of the landscape in my projects. Granted, we’ve got the FindBugs @NonNull annotation, but I still want to write an assertion that checks the parameter is not null.

Why? Because I might be writing my code in an environment where FindBugs is not used, and they might refuse my request to add the required jars for compile time only –stuff happens!

I have verified that FindBugs will ignore the attempt to check for a value being null in an assertion, making it possible for me to have both things: the annotation plus FindBugs checks, and the assertion. Therefore, this code will compile and pass FindBugs checks.

public String myToUppercase(
@NonNull String parameter )
{
assert parameter != null;
return parameter.toUpperCase(Locale.getDefault());
}

And, just for completeness: there is a fourth annotation, @PossiblyNull, which is deprecated –and equivalent to @CheckForNull.

Following a strict policy

I hope the prior examples have shown that there are many subtle issues with regard to null values handling and that FindBugs makes handling them a breeze.

Therefore, I would advise a very strict policy: add @CheckForNull and @NonNull annotations to all fields, parameters and methods handling non primitive types. To be fair, I find this so helpful that I feel you should never have a I can’t assume anything about nullability policy in our own code, unless there is some really good reason.

Besides, it might be very interesting to set the default policy to @CheckNotNull or @NonNull, something you can do at the package or class level using one or more of the following annotations: @DefaultAnnotation, @DefaultAnnotationForFields, @DefaultAnnotationForMethods, @DefaultAnnotationForParameters or @ReturnValuesAreNonnullByDefault.

For example, this code sets @NonNull as the default policy for MyClass:

@edu.umd.cs.findbugs.annotations.DefaultAnnotation(value=NonNull.class)
public class MyClass {
// ...

It is possibly to set the default policy for all classes in a package too.

In fact, several papers suggest that the non-null policy should probably be the default one for Java, because “the authors conducted an empirical study of five open source projects totalling 700 K lines-of-code, which confirms that, on average, 75% of reference declarations are meant to be non-null, by design” (see Reducing the use of nullable types through non-null by default and monotonic non-null), or http://www.disi.unige.it/person/AnconaD/FTfJP06/paper03.pdf

Tool and library versions

The FindBugs version we have used for this article is 1.3.9, with the corresponding plugin running in Eclipse 3.3.0.

By the way, note that, to use these annotations, you need to add a pair of jars FindBugs provides: annotations.jar and jsr305.jar. However, these jars need not be redistributed, they are used at compile time only.

Related articles

You might be interested in taking a look at other FindBugs posts and articles:

  • This article is about using FindBugs annotations to make sure that resources (connections, streams, sockets, etc.) are released.
  • This article is about using the FindBugs @CheckReturnValue annotation to make sure programmers consuming our methods use the returned value for some of them.

FindBugs: make your code better with the CheckReturnValue Annotation

I decided to take a look at FindBugs recently, and used my DirectJNgine project as a testbed. Even though torture testing DJN with FindBugs made me make some changes to it, I just found a bug (thankfully, one that will arise so rarely I doubt any DJN user will ever come across it). Here is the code:

file.delete();
if( logger.isDebugEnabled() ) {
  logger.debug( "Api file deleted: '" + file.getAbsolutePath() + "'");
}

And here is the fix:

if( !file.delete() ) {
  throw new IOException( "Unable to delete " + fullFileName);
}
if( logger.isDebugEnabled() ) {
  logger.debug( "Api file deleted: '" + file.getAbsolutePath() + "'");
}

I just forgot to check the value returned by delete, because I assumed inadvertently that file.delete would raise an IOException. But it just returns a boolean value telling you whether it succeeded or not. Uh, oh!

Fortunately, FindBugs check for this, and helped me find the bug.

Now, wouldn’t it be nice if there were a way to tell FindBugs that users of a certain method you have implemented should never ignore the return value?

And, indeed, there is a way to do that: just annotate the method with @CheckReturnValue, as follows:

@edu.umd.cs.findbugs.annotations.CheckReturnValue( explanation="...")
public String methodWhoseReturnValueShouldNeverBeIgnored( String value ) {
  return value;
}

Now, if you call this method without using the returned value, as follows,

public void useMethodWithCheckReturnValue() {
  // FindBugs warning: you are ignoring the returned value!
  methodWhoseReturnValueShouldNeverBeIgnored ("a value");
}

then FindBugs will complain.

Now, I can think of two main scenarios where you should annotate a method with @CheckReturnValue:

  • The method performs an operation for which it is very important to check the result because it indicates success or failure, and your code will probably need to handle these scenarios differently. This is the“forgot to check the result of delete” scenario that caused the bug in DJN.
  • You applied an operation to an immutable object, and are returning a new object that is the result of applying the operation to the original unmodified object: it is clear that you must use the returned object from then on, instead of continue using the old unmodified object. For example, most operations on BigDecimal should probably have this annotation.

Using this annotation is so easy that you should probably add it to your programming arsenal.

Tool and library versions

The FindBugs version we have used for this article is 1.3.9, with the corresponding plugin running in Eclipse 3.3.0.

By the way, note that, to use these annotations, you need to add a pair of jars FindBugs provides: annotations.jar and jsr305.jar. However, these jars need not be redistributed, they are used at compile time only.

Related articles

You might be interested in taking a look at other FindBugs posts and articles:

  • This article is about using FindBugs annotations to make sure that resources (connections, streams, sockets, etc.) are released.

FindBugs: make your code better with Resource Management Annotations

FindBugs is a nice tool that helps finding problematic areas in your code. But, not only that: it provides several annotations that you can add to your own code to make it easier to use in a robust way, by helping FindBugs pinpoint potential problems when other programmers use it.

Several of these annotations are intended to help catch a very common error with resource handling: forgetting to call cleanup methods, such as the proverbial close. Too bad that these annotations are not documented in the FindBugs manual: I hope this post contributes to make using them easier.

Diagnosing missing cleanup

Sometimes you create classes that handle expensive resources. For those classes, you should probably perform cleanup as soon as you have finished using the resource, exactly the same way you need to call a cleanup method for a JDBC Connection, a Stream, sockets, etc.

There is a hidden gem in FindBugs that allows you to annotate your own resource classes so that, if you forget to call their cleanup method, it will flag the mistake.

Note that, to use these annotations, you need to add a pair of jars FindBugs provides: annotations.jar and jsr305.jar. However, these jars need not be redistributed, they are used at compile time only.

The following class, ResourceClass, uses the @CleanupObligation, @CreatesObligation and @DischargesObligation annotations in FindBugs to provide that functionality:

@edu.umd.cs.findbugs.annotations.CleanupObligation
public static class ResourceClass {
  @edu.umd.cs.findbugs.annotations.CreatesObligation()
  public ResourceClass() {
    // Do nothing: just for illustrative purposes
  }

  @edu.umd.cs.findbugs.annotations.DischargesObligation
  public void doCleanup() {
    // Do nothing: just for illustrative purposes
  }
}

First of all, you annotate the resource class with the @CleanupObligation annotation. This makes it clear that the class manages a resource that will need to be release.

Secondly, you need to specify which is/are the method(s) or constructor(s) that acquire the resource: you just annotate them with @CreatesObligation.

Thirdly, you need to specify which is/are the method(s) that perform cleanup, using the @DischargesObligation annotation. That’s it!

Let’s see what happens when FindBugs sees code that acquires the resource but forgets to perform cleanup, such as this:

public void handleResourceForgettingCleanup() {
  // FindBugs warning
  // Method ...handleResourceForgettingCleanup() may fail to clean up...
  new ResourceClass();
}

This sample code acquires a resource, as it invokes the constructor marked with @CreatesObligation, but it forgets to call the corresponding cleanup method –doCleanup, marked with @DischargesObligation. When checking this code, FindBugs pinpoints the issue as follows (in Eclipse):

FindBugs Resource Handling Annotations Error Message

Neat, huh? Once the code is corrected, as shown in the following code, FindBugs will not complain anymore:

public void handleResourceWithCleanup() {
  ResourceClass resource = new ResourceClass();
  try {
    // Just use resource in some way to please our very demanding compiler
    resource.toString();
  }
  finally {
    resource.doCleanup();
  }
}

This is a very nice thing, but apparently FindBugs does not perform inter-procedural analysis for this, so this will not be diagnosed correctly in a complex scenario.

No problem. I think that the annotations would be useful even if they were used to document only. The fact that they help FindBugs diagnose wrong code nine out of ten times just makes it better.

But I’ve got a false positive!

Well, yes, false positives can happen: FindBugs is good, but not perfect.

The simplest solution is to add a @SuppressWarnings annotation to make FindBugs ignore the issue. I will not discuss that annotation here, as it deservers more time than what I have available right now.

Be careful, for there is a FindBugs specific @SuppressWarnings annotation, which is different from @java.lang.SuppressWarnings: don’t mix them!

Tool and library versions

The FindBugs version we have used for this article is 1.3.9, with the corresponding plugin running in Eclipse 3.3.0.

‘Debugging the Development Process’ & Timeless Principles

These days I’m re-reading Steve Maguire’s Debugging the Development Process, which is one of the first “realistic development process” books I read, almost two decades ago!

By “realistic” I mean that it takes into account that it is human beings who make software, and that how we communicate and what we feel about ourselves, the team and the process itself is crucial. Processes do not create software: people do. People comes first, processes second; it follows that it is processes that must fit people, not the other way around.

But, back to the book…This book is at least 16 years old, and it shows: no agile-speak here, for example. Yet, this must be the third or fourth time I read it, and the book still feels fresh.

Why is this so? I think that Maguire just keeps pounding on some underlying principles that will always hold, timeless principles that will never change:

  • You must be focused and have a clear purpose.
  • You must be concrete and explicit.
  • You must do things, instead of letting them happen.
  • You must be courageous.
  • You must believe that good enough never is: there’s always a better way.
  • The declared problem tends not to be the real problem: difficult problems tend to be difficult because they are not what they look like, what you are seeing is not what is there.
  • You must care about people: care that is shown via actions, not PR speeches -people almost always resent just-talk-the-talk PR campaigns aimed at them.
  • You must choose the right people/company: you must know your culture and choose the people and companies that are closer to it, because bypassing what’s deeply important to us is very straining, and you just can’t be the best at doing something you don’t believe in.

If you read the book paying attention to this, you will find Maguire provides a thousand concrete policies that work in the right direction to fulfill one or more of these timeless principles. Here are some examples…

Be focused

From chapter 1, Laying the Groundwork:

“Focus on improving the product… any work that does not result in an improved product is potentially wasted or misguided effort”

“The project lead should ruthlessly eliminate any obstacles that keep the developers from the truly important work: improving the product”

“Always determine what you are trying to accomplish, and then find the most efficient and pleasurable way to have your team do it”

“…one way to keep (housekeeping work) to a minimum is to regularly ask the questions ‘what am I ultimately trying to accomplish?’ and ‘How can I keep the benefits of the task yet eliminate the drawbacks?’”

From chapter 3, Of Strategic Importance:

“Don’t waste time on questionable improvement work. Weigh the potential value returned against the time you would have to invest”

From chapter 4, Unbridled Enthusiasm:

“If you hold a meeting that doesn’t end in a decision, that meeting has probably been a waste of time…Try to eliminate unnecessary follow-up work. I think you’ll be surprised at how many follow-up tasks don’t seem nearly as important by the end of the meeting as they did earlier, in the middle of an intense discussion”

From chapter 6, Constant, Unceasing Improvement:

“People often ask for something other than what they really need. Always determine what they are trying to accomplish before dealing with any request”

Be concrete and explicit

From chapter 1:

“At the very least, you should establish a ranking order for these priorities: size, speed, robustness, safety, testability, maintainability, simplicity, reusability, portability…in addition to ranking coding priorities, you should also establish a quality bar for each priority…otherwise, the team will have to waste time rewriting misconceived or substandard code”.

From chapter 2, The Systematic Approach:

“As you put systems in place, explain the purposes behind them so that the development team can understand what aspect of the product the systems are meant to improve”

From chapter 6, Constant, Unceasing Improvement:

“If you want your team members to make great leaps as well as take incremental daily steps to improvement, you must see that they actively pursue the greater goals…let them choose the skills they want to pursue…and merely verify that each goal is worth going after [by making sure] the skill would benefit the programmer, the project, and the company…the goal is achievable within a reasonable time frame…the goal has measurable results…[ideally] the skill will have immediate usefulness to the project”

Be courageous

From chapter 3, Of Strategic Importance:

“Sometimes there are questionable requests…a large corporate client had requested the LAYOFF macro so that they could lay people off without anybody being able to claim that the selections were biased [!]. The company would be in a position to simply point to Excel to prove their innocence…The task fell to me and I refused to implement the request…for months we beat the marketing team’s persistent requests for the feature. Marketing felt they needed the macro to close the sale…”

Care about people

From chapter 6, Constant, Unceasing Improvement:

“If you constantly expose a team member to new tasks that call for new skills, he or she will eventually reach a point at which your project no longer offers room to grow…for the benefit of the company, you should kick such an expert off your team…You have a duty to the programmers and the company to find the programmers positions in which they can grow”

From chapter 8, That Sinking Feeling:

“If your project is slipping, something is wrong. Don’t ignore the causes and demand long hours of the team members. Find and fix the problems”

On and on…

Maguire keeps on adding lots of foolproof advice throughout the book; most of it solidly rooted on the belief that it is people that makes software great -or a mess. I can’t agree more with him!

Yes, Debugging the Development Process is old…so, what? Some principles are timeless, and Maguire knows how to materialize them with concrete policies.

Highly recommended if you care about preserving what’s essential!