Java Bugs with Static Analysis Tool findbugs Help Improve Software Code Quality
This post is regarding the static analysis tool that finds real bugs in Java
programs that help to improve software code quality. Static analysis tools can find real defects and issues in the code. We can effectively incorporate static analysis into our software development process.
FindBugs - Static analysis Tool
FindBugs is an open source
static analysis tool that analyzes Java
class
files, looking for programming defects. The analysis engine reports nearly 300 different bug patterns. Each bug pattern is grouped into a category example,
- correctness
- bad practice
- performance
- internationalization
- priority
- high
- medium
- low
Let’s start with some of the selected bug categories with some examples.
Correctness
Comparing incompatable types for equality
Consider the following code,
if ((!value.equals(null)) && (!value.equals(""))) { Map spaces = (Map) vm.get(SpaceConstants.AVAILABLESPACEMAP); }
One would expect that the condition would true
, when value is not null
and is not empty. However, value.equals(null)
according to the contract of the equals() method, would always return false
.
Consider the another similar example,
if ((bean.getNoteRate() != null) && !bean.getNoteRate().equals("") && (bean.getNoteRate() > 0)) { item.setNoteRate(bean.getNoteRate()); }
We might expect that the condition would true
, when noteRate is not null
, not empty and is greater than 0. However, the condition would never be true
.
The reason is that bean.getNoteRate().equals("")
would always return false
regardless of being equal
value.
According to the contract of equals(), objects
of different classes
should always compare as unequal; therefore, according to the contract defined by java.lang.Object.equals(Object), the result of this comparison will always be false
at runtime.
Null pointer dereference
Consider the following code,
if ((list == null) && (list.size() == 0)) { return null; }
This will lead to a Null Pointer Exception when the code is executed while list
is null
.
Suspicious reference comparison
Consider the following code,
if (bean.getPaymentAmount() != null && bean.getPaymentAmount() != currBean.getPrincipalPaid()) { // code to execute }
This code compares two reference values (Double
paymentAmount) using the != operator
, where the correct way to compare instances
of this type is generally with the
equals() method. It is possible to create distinct instances
that are equal
but do not compare as ==
since they are different objects
.
Doomed test for equality to NaN
Consider the following code,
if ((newValue == Double.NaN) || (newValue < 0d)) { // the code to execute }
This code checks to see if a floating point
value is equal to the special Not A Number value. However, because of the special semantics of NaN, no value is equal to Nan
, including NaN. Thus, x == Double.NaN
always evaluates to false
. To check if a value contained in 'x' is the special Not A Number value, use Double.isNaN(x) (or Float.isNaN(x) if x is floating point precision).
Also see How can you compare NaN values? .
Method whose return value should not ignore
string is immutable object
. So ignoring the return value of the method would consider as bug.
String name = "Muhammad"; name.toUpper(); if (name.equals("MUHAMMAD"))
Performance
Method invokes inefficient Boolean constructor;use Boolean.valueOf(...) instead
Consider the following code,
if ((record.getAmount() != null) && !record.getAmount().equals(new Boolean(bean.isCapitalizing()))) { // code to execute }
Creating new instances
of java.lang.Boolean
wastes memory, since Boolean
objects are immutable
and there are only two useful values of this type. Use the Boolean.valueOf()
method (or Java 1.5 autoboxing) to create Boolean objects
instead.
Inefficient use of keySet iterator instead of entrySet iterator
Consider the following code,
Iterator iter = balances.keySet().iterator(); while (iter.hasNext()) { // code to execute }
This method accesses the value of a Map
entry, using a key
that was retrieved from a
keySet
iterator. It is more efficient to use an iterator
on the entrySet
of the map
, to avoid the Map.get(key)
lookup.
Method invokes inefficient Number constructor; use static valueOf instead
Consider the following code,
Integer number1 = new Integer(123); Integer number2 = Integer.valueOf(123); System.out.println("number1 = " + number1); System.out.println("number2 = " + number2);
Using new Integer(int)
is guaranteed to always result in a new object whereas Integer.valueOf(int)
allows caching of values to be done by the class library
, or JVM. Using of cached values avoids object allocation and the code will be faster.
Also see: Integer Auto Boxing
Method concatenates strings using + in a loop
for (int x = 0; x < exceptions.size(); x++) { errorMessage += getStackTrace( exceptions.get(x) + "\n"); }
In each iteration
, the String
is converted to a StringBuffer/StringBuilder
, appended to, and converted back to a String
. This can lead to a cost quadratic in the number of iterations
, as the growing string
is recopied in each iteration
.
Better performance can be obtained by using a StringBuilderexplicitly.
Dodgy
Code that is confusing, anomalous, or written in a way that leads itself to errors. Examples include dead local stores, switch fall through, unconfirmed casts, and redundant null
check of value known to be null
.
instanceof will always return true
The instanceof test will always return true (unless the value being tested is null)
NodeList nodeList = root.getElementsByTagName("node"); int nodeListLength = nodeList.getLength(); for (int i = 0; i < nodeListLength; i++) { Node node = nodeList.item(i); if (node instanceof Node && node.getParentNode() == root) { //do code } }
Test for floating point equality
Consider the following code,
private double value = 0d; if (value > diff) { // code to excute } else if (value == diff) { // code to excute }
The above code compares two floating point
values for equality. Because floating point calculations may involve rounding, calculated float
and double
values may not be accurate. For values that must be precise, such as monetary values,
BigDecimal would be more appropriate.
Also see Effective Java 2nd Ed, Item 48:Avoid float and double if exact answers are required
Integral division result cast to double or float
Consider the code,
int x = 2; int y = 5; // Wrong: yields result 0.0 double value1 = x / y;
This code casts the result of an integral division operation to double
or float
. Doing division on integers
truncates the result to the integer
value closest to zero. The fact that the result was cast to double
suggests that this precision should have been retained. We should cast one or both of the operands to double before performing the division like
// Right: yields result 0.4 double value2 = x / (double) y;
References:
- Download the latest version of FindBugs
- Visit the blog
- Using the FindBugs Ant task
- Using the FindBugs Eclipse plugin
- Using the PMD
- Visit the blog