Avoid common mistakes by conducting regular code inspections
It is often easier to find fault with other people’s work than your own. The advantage of a second opinion explains why writers have editors and why athletes have coaches. Instead of resisting criticism, we should benefit from another’s ability to find shortcomings in our programming efforts.
Formal code inspections are one of the most powerful techniques available for improving code quality. Code inspections — peers reviewing code for bugs — complement testing because they tend to find different mistakes than testing does.
Code inspections are even more useful when inspectors hunt for specific errors rather than casually browse for bugs. In this article, I present 11 common mistakes that have been made at one time or another in the Java code base on which I am currently working. You can make these items into a checklist for your own code inspections. Then you can be confident that your code doesn’t repeat these typical mistakes.
Common mistake #1: Copying Strings excessively
One mistake that you won’t detect by testing is making unnecessary copies of immutable objects. Immutable objects cannot change, so there is no need to copy them. The most commonly used class of immutable objects is String. If you must change the contents of a String, you must instead use a StringBuffer.
So, copying String in this way would work:
String s = new String ("Text here");
The code, however, is unnecessarily slow and complicated. You could also write the above code as:
String temp = "Text here";
String s = new String (temp);
But this rewrite also contains an extra, unnecessary String. A better way to write this is by simply writing:
String s = "Text here";
One warning: this may not be true if you run a pre- or post-processor on your code, as, for example, many ODMG-93 databases do. The pre- or post-processor can modify your code so that what appear to be String objects really aren’t.
Common mistake #2: Failing to clone returned objects
Encapsulation is one of the key tenets of object-oriented programming. Unfortunately, Java makes it easy to accidentally break encapsulation by returning references to private data.
The following class illustrates this:
import java.awt.Dimension;
/**
* Example class. The x and y values should never
* be negative.
*/
public class Example
{
private Dimension d = new Dimension (0, 0);
public Example ()
{
}
/**
* Set height and width. Both height and width must be nonnegative
* or an exception is thrown.
*/
public synchronized void setValues (int height, int width)
throws IllegalArgumentException
{
if (height < 0 || width < 0)
throw new IllegalArgumentException();
d.height = height;
d.width = width;
}
public synchronized Dimension getValues()
{
// Ooops! Breaks encapsulation
return d;
}
}
The Example class promises that the height and width values stored inside will never be negative. Attempting to set negative values with the setValues() method fails with an exception. Unfortunately, because getValues() returns a reference to d instead of a copy of d, you can write the following nasty code:
Example ex = new Example();
Dimension d = ex.getValues();
d.height = -5;
d.width = -10;
Now the Example object contains negative numbers. Testing may not detect this sort of bug if the initial callers of the getValues() method never set the height and width values of the returned Dimension object. Unfortunately, over time you can expect some client code to change the values in the returned Dimension object. Hunting down the resulting bug is tedious and time-consuming, especially in a multithreaded environment.
A much better approach would be having the getValues() method return a copy:
public synchronized Dimension getValues()
{
return new Dimension (d.x, d.y);
}
Now the internal values of the Example object are safe. Callers can get copies and change them as often as needed, but callers cannot change the internal state of Example objects without using the setValues() method.
Common mistake #3: Cloning when it isn’t necessary
Having learned that get methods should return copies of internal data objects instead of references to the internal data, developers began writing code like this:
/**
* Example class. The value should never
* be negative.
*/
public class Example
{
private Integer i = new Integer (0);
public Example ()
{
}
/**
* Set x. x must be nonnegative
* or an exception will be thrown.
*/
public synchronized void setValues (int x)
throws IllegalArgumentException
{
if (x < 0)
throw new IllegalArgumentException();
i = new Integer (x);
}
public synchronized Integer getValue()
{
// We can't clone Integers so we make
// a copy this way.
return new Integer (i.intValue());
}
}
The code above is safe, just as creating the extra String in the first mistake is safe, but in both cases you’ve done more work than is necessary. The Integer object, like the String object, is immutable once constructed. It is, therefore, safe to return the internal Integer object instead of a copy of it. You should write getValue() like this:
public synchronized Integer getValue()
{
// 'i' is immutable, so it is safe to
// return it instead of a copy.
return i;
}
Java programs contain more immutable objects than typical C++ programs. An incomplete list of immutable classes provided by the JDK includes:
BooleanByteCharacterClassDoubleFloatIntegerLongShortString- Most
Exceptionsubclasses
Common mistake #4: Copying arrays by hand
Java lets you clone arrays, but developers commonly make the mistake of writing code such as the example below. The problem is that the loop below does in three lines what the clone method provided by Object does in one.
public class Example
{
private int[] copy;
/**
* Save a copy of 'data'. 'data' cannot be null.
*/
public void saveCopy (int[] data)
{
copy = new int[data.length];
for (int i = 0; i < copy.length; ++i)
copy[i] = data[i];
}
}
This code is correct, but much more complicated than it needs to be. A better implementation of saveCopy is:
void saveCopy (int[] data)
{
try
{
copy = (int[])data.clone();
}
catch (CloneNotSupportedException e)
{
// Can't get here.
}
}
If you clone arrays often, a utility method like the following is often a good idea:
static int[] cloneArray (int[] data)
{
try
{
return (int[])data.clone();
}
catch (CloneNotSupportedException e)
{
// Can't get here.
}
}
Then our saveCopy looks even clearer:
void saveCopy (int[] data)
{
copy = cloneArray ( data);
}
Common mistake #5: Copying the wrong thing
Sometimes a programmer knows that a copy must be returned, but inadvertently copies the wrong thing. The following code doesn’t do what the developer wants because only a partial copy is made:
import java.awt.Dimension;
/**
* Example class. The height and width values should never
* be negative.
*/
public class Example
{
static final public int TOTAL_VALUES = 10;
private Dimension[] d = new Dimension[TOTAL_VALUES];
public Example ()
{
}
/**
* Set height and width. Both height and width must be nonnegative
* or an exception will be thrown.
*/
public synchronized void setValues (int index, int height, int
width)
throws IllegalArgumentException
{
if (height < 0 || width < 0)
throw new IllegalArgumentException();
if (d[index] == null)
d[index] = new Dimension();
d[index].height = height;
d[index].width = width;
}
public synchronized Dimension[] getValues() throws
CloneNotSupportedException
{
return (Dimension[])d.clone();
}
}
The problem here is that the getValues() method clones the array, but not the Dimension objects contained in the array. The clone() prevents callers from changing the internal array to point to different Dimension objects, but fails to prevent callers from changing the contents of the Dimension objects. A better version of getValues() is:
public synchronized Dimension[] getValues() throws
CloneNotSupportedException
{
Dimension[] copy = (Dimension[])d.clone();
for (int i = 0; i < copy.length; ++i)
{
// NOTE: Dimension isn't cloneable.
if (d[i] != null)
copy[i] = new Dimension (d[i].height, d[i].width);
}
return copy;
}
This mistake is also made with multidimensional arrays of atomic types, like int and float. A simple clone of a one-dimensional array of ints is correct, as shown below:
public void store (int[] data) throws CloneNotSupportedException
{
this.data = (int[])data.clone(); // OK
}
Copying a two-dimensional array of ints is trickier. Java doesn’t have two-dimensional arrays, so a two-dimensional array of ints is really a one-dimensional array of objects of type int[]. A simple clone of an int[][] makes the same mistake that the first version of getValues did in the example above, so you want to avoid doing this. The following code demonstrates the right and wrong way to clone two-dimensional int arrays:
public void wrongStore (int[][] data) throws
CloneNotSupportedException
{
this.data = (int[][])data.clone(); // Not OK!
}
public void rightStore (int[][] data)
{
// OK!
this.data = (int[][])data.clone();
for (int i = 0; i < data.length; ++i)
{
if (data[i] != null)
this.data[i] = (int[])data[i].clone();
}
}
Common mistake #6: Testing new for null
Beginning Java programmers sometimes test the results of a new operation for a null. The code for this test looks like this:
Integer i = new Integer (400);
if (i == null)
throw new NullPointerException();
This test is not wrong, but it is unnecessary. The two lines making up the if and the throw are wasted. They serve only to make the program fatter and slower.
C/C++ programmers often do this initially because testing the results of malloc() in C is necessary, and failing to do so creates a bug. Testing the results of new in C++ can be good practice, depending on whether or not exceptions are enabled (many C++ compilers allow exceptions to be disabled, in which case a failed new returns null). In Java, new is not permitted to return null. If it does, the JVM is most likely crashing and the test isn’t going to help.
Common mistake #7: Using == instead of .equals
There are two ways to test equality in Java: the == operator and the .equals method implemented by all objects. Atomic types (int, float, char, and so forth) are not objects, so they can use only the == operator, as shown here:
int x = 4;
int y = 5;
if (x == y)
System.out.println ("Hi");
// This 'if' test won't compile.
if (x.equals (y))
System.out.println ("Hi");
Objects are trickier. The == operator tests whether two references are pointing to the same object, while the equals() method implements a less specific equality test. Even more confusingly, the default equals method, supplied by java.lang.Object, uses == to simply test whether the two objects being compared are the same.
Many classes override the default equals method to do something more useful. The String class, for example, tests whether the two Strings contain the same characters in the same order. The Integer class overrides equals to test whether the contained int values are the same.
Most of the time, when testing objects for equality you should use the equals method, while atomic types must use the == operator.
Common mistake #8: Confusing nonatomic and atomic operations
Java guarantees that reading or writing to 32-bit or smaller quantities is atomic, meaning each action takes place in one step that cannot be interrupted. Thus, these reads and writes need not be synchronized. The following code is thread safe:
public class Example
{
private int value;
// More code here...
public void set (int x) // NOTE: No synchronized keyword
{
this.value = x;
}
}
The guarantee applies only to reading and writing, however. This method is not thread-safe:
public void increment ()
{
// This is effectively two or three instructions:
// 1) Read current setting of 'value'.
// 2) Increment that setting.
// 3) Write the new setting back.
++this.value;
}
You might not catch this mistake during testing. First, testing for threading bugs is very difficult and time consuming. Second, on some CPUs this code might translate into a single instruction, and thus work correctly. The bug would appear only when tested on other JVMs. It is better to simply synchronize the code correctly from the start:
public synchronized void increment ()
{
++this.value;
}
Common mistake #9: Putting cleanup code in a catch block
Putting cleanup code in a catch block often results in code that looks like this:
OutputStream os = null;
try
{
os = new OutputStream ();
// Do something with os here.
os.close();
}
catch (Exception e)
{
if (os != null)
os.close();
}
Although this code is wrong for several reasons, the mistake(s) could easily be overlooked during testing. Here are three problems with this code:
- The
os.close()statement appears in two places. This is unnecessary as well as a potential maintenance problem. - The code above handles only
Exceptions, notErrors. Even if the try block fails with anError, the stream should be closed. close()can throw an IOException.
A much-improved version is:
OutputStream os = null;
try
{
os = new OutputStream ();
// Do something with os here.
}
finally
{
if (os != null)
os.close();
}
This repairs the first two problems: code is no longer duplicated and Errors are handled properly. There are no good solutions to the last problem. Your best solution is probably to put the close() inside a try/catch block itself.
Common mistake #10: Adding unnecessary catch blocks
Some developer hear the term try/catch block and assume that all try blocks must have an associated catch block. This is reinforced for C++ programmers because C++ has no concept of a finally block. In C++, the only reason to have a try block is to pair it with an associated catch block.
The result of adding unnecessary catch blocks is code like this, where exceptions are caught and then immediately rethrown:
try
{
// Nifty code here
}
catch (Exception e)
{
throw e;
}
finally
{
// Cleanup code here
}
You can shorten the above code by eliminating the unnecessary catch block:
try
{
// Nifty code here
}
finally
{
// Cleanup code here
}
Common mistake #11: Failing to implement equals, hashCode, or clone
The default implementations of equals, hashCode, and clone, provided by java.lang.Object, are correct. Unfortunately, these implementations are not always useful. To correct this, many classes override one or more of these methods to provide more useful functionality.
Unfortunately, when extending a class that overrides one or more of these methods, the child class usually needs to override the methods, too. Code reviews should check to ensure that if parent classes implement equals, hashCode, or clone, the child classes are still correct. Writing correct implementations of equals, hashCode, and clone is tricky. The Resources section provides a link to an article explaining how you can do this.
Conclusion
I have seen each of these mistakes in at least one code inspection. In fact, I’ve made more than one of them myself. The good news is that code inspections are easy to administer, and the mistakes are easy to find and fix, as long as you know what to look for. Even if you can’t find the time for formal code inspections, rooting out these mistakes from your own code will save you debugging time. The time spent reviewing will be time well spent.
