Java Tip 88: Complement testing with code inspections

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:

  • Boolean
  • Byte
  • Character
  • Class
  • Double
  • Float
  • Integer
  • Long
  • Short
  • String
  • Most Exception subclasses

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;
    }
1 2 Page 1