Code review by peers is known to significantly enhance the quality of
software, and is therefore an integral part of the development process
adopted by many software organizations all over the world. Some of the
problems that a code review can reveal are

missing or incorrect functionality

use of incorrect or inferior alogorithms

code documentation errors

coding guideline violations

coding errors

code design deficiencies, and

efficiency concerns

In this article, my intention is to document certain code design deficiencies
that I have come across in the process of studying several class libraries.
A word of caution is appropriate here. When I talk about "deficiencies",
I do not necessarily mean that they stop the code from working, but just
that a "good" programmer might not be accused of these! I do agree that
these guidelines are sometimes debatable, but I will present them anyway.
The examples I present here are contrived, but let me assure you that they
are inspired by real, professional (possibly commercial) code. For various
reasons, I cannot name the actual sources, but I am sure you will stumble
upon such code real soon.

Inner Class Relationships

There are two aspects to this:

Making a class an inner class of another class

Making a nonstatic class static.

Case 1: Consider the following two Java classes (in the same
package).

A look at the two classes reveals that whenever a B object is created,
it requires an A object, suggesting a tight coupling of B to A. In such
a case, it may be good idea to define B as an inner class of A as shown:

In this case, since Node is a logical component of a Tree, and other
classes may not need it, it makes sense to define Node inside of Tree.
However, it must be more appropriately a static inner class (sometimes
called "nested" class).

Confusion Between Local Variable and Instance Variable

I have come across a surprising number of examples where the programmer
uses instance variables of a class like local variables. Look at the class
given here.

As the structure itself reveals, the instance variable "i" is always
assigned to in a method before it is read. This implies that previous value
of the variable is never used. Instance variables are intended, in general,
to remember state, not as simple value holders. I would prefer to rewrite
the above example as follows:

The instance variable "MAXSIZE" is final, and initialized directly.
Can we judiciously make this a "static" member? I belive we should. Something
like this does not break the code, but tends to differentiate a good Java
programmer from the rest of the breed.

Public Constructor in a Nonpublic Class

If a class is not public, does it make sense to have a public constructor
in the class? Look at the following class structure:

class A {
public A() { /* ... */ }
// ...
}

A member is declared public to be made available to classes in other
packages. In particular, a constructor is made public to allow instantiation
of that class objects from other packages. But if the class itself is not
public, how can such an instantiation occur (the name of the class itself
is not available to other packages)? So, I believe that the constructor
of a nonpublic class need not be public. What about other instance methods
of the class? This suggestion does not cover other methods. The following
is a perfectly valid thing to do:

public class A {
public A createObject() {
return new B();
}
public void ff() {
//...
}
}

In this case, the overriding method B.ff() must be public, though the
constructors need not be!

Having A Return Expression in Try as well as Finally Block

The return statement in a finally block nullifies the
effect of return found within the corresponding try block.
The runtime system always executes the statements within the finally block
regardless of what happens within the try block. So a return statement
in a try block will have no effect if there is one in the finally block.

There does not appear to be any advantage in being able to instantiate
objects of this class since there is no "instance-specific" behaviour.
In this case, my preference is to define a private constructor that disallows
instantiation (however, if derivation is to be enabled, I might consider
making the constructor protected). The modified class is

One interesting issue is whether the code review process can be automated
by a tool, and if so, what kinds of problems can the tool identify. I believe
it is safe to say (with current technology) that no tool, however good
it may be, can replace an intelligent programmer. The benefit of automated
review is that it can bring reasonably good quality to everyone’s desktop,
by removing individuality from the picture. I know of three tools in this
category as of today, and we are the developers of one of them. These tools
are JStyleTM (www.mmsindia.com/jstyle.html),
Metamata Code AuditTM (www.metamata.com)
and Code Wizard for JavaTM (www.parasoft.com).
Two of these compute useful metrics from the Java code, in addition to
code critiquing.