My mother used to tell me that she wasn’t my maid. My USAF drill instructor used to tell us that she wasn’t our mother. An early mentor used to metaphorically beat me for having clumsy and cluttered code. The common impetus was that things can and sometimes should be neat and tidy. Being sloppy can lead to code that disagrees with your IDE, or worse, with your keen sense of appropriate practices.

I recently worked on a project with (I am not exaggerating) 50,000 compiler warnings in their Java source files.

I’ll let that sink in.

This was a very large project with thousands of classes, many with hundreds or thousands of lines, more than a few having dozens or hundreds of members, and some having classes that extended classes several levels deep. It was probably unnecessarily large and complex, but that wasn’t what I was hired to fix, nor was it a judgement I was prepared to make.

The key to the difficulty was that with so many warnings, it became impossible to find the ones that caused actual problems in the software. It had to be cleaned; someone just had to do it, and that was me.

A lot of warnings were due to untyped collections because of a code base started in or before Java 1.4. Similarly, a lot of Serializable classes didn’t have a SerialVersionUID value. A full third of the warnings were simply unused imports, and there were more than a fair share of unused local variables. A very large number of the warnings, though, were unused member variables. I’ll talk about the unused variables, both local and member.

Members First

Since I’m a little bit of an elitist, members first. Here’s an example class.

<pre>public class Foo {
private String string = "";
public void foo() {
// Ignore string
}
}</pre>

By this example I mean that all of the members were private, and there were no accessors, and except for perhaps setting the value, the members in question weren’t ever read. In its attempts to help avoid errors, Eclipse (and Ant and Maven if you allow them and scour the output) points these out to draw your attention to them.

At a glance, a simple scan for the variable in the class would point out its true nature of unused. In the trivial example there, there’s a value that gets assigned, but is never accessed in the class. Also, at first glance it appears that there’s no need for the value; since it’s private, it seems that nothing can get to it. Our friendly frameworks, though, foil us in our attempts to keep our code brief. Thanks to heavy use of reflection, naming our Foo class as the bean behind a form in an MVC framework can use this value. It can happen, then, that removing this value can cause a problem in an RIA application that uses a reflection-heavy server-side framework.

Three simple solutions exist. First, and in line with what most of us do for encapsulation, is to create accessors. The second, and more simple solution, is to not declare the member as private, which flops in the face of encapsulation. Really, since the framework is using reflection, encapsulation is already broken. Finally, adding a simple @SuppressWarning(“unused”) will hide the warning, but arguably not fix the problem.

Simply removing the “private” from our member declaration will make it use the default package access, which then allows any class in the same package to access the member, so then its use becomes undetermined. The IDE warning at least, goes away. Adding accessors won’t do any less, as they’d need at least the same kind of non-private accessor to avoid a warning on the function. Further, if you add an accessor method to some of the members, reflection can get wonky and complain about others that don’t have such methods.

Since the project in question had no code coverage tool used on it (much less unit tests in the first place), it is impossible to tell what members declared in this way will cause a problem without either thoroughly scouring (or intimately knowing) the JSPs for bean use to see what members might be used. Alternatively, the members could be removed and then the application could be run to see if errors occurred, but this application had hundreds of pages, a lot of which were generated by server-side actions, not always in JSPs, so there could be an unfortunately large number of permutations that could be hit before encountering the member in question.

To maintain the facade of encapsulation, I added thousands of @SuppressWarnings annotations to the complaining members. This removes the warning without otherwise affecting the member, and it doesn’t add any potential complexity of some members have getters and some do not, leaving the reflection framework happy. Further, it gives something that could be searched for later to identify and correct the problem.

A real solution to the problem is to add comprehensive unit and integration tests and validate with a code coverage tool, that will at the very least rule out some of the variables as never getting used. This is tough when using reflection-heavy frameworks, but it can be done with the right integration test library and good unit test practices.

Locals, Also

There were also, as mentioned, a large number of unused variables in methods. These came in a few flavors, as this example class shows:

<pre>public class Foo {
public void foo() {
String string = "unused";
String result = UtilityClass.someHelper();
// Ignore both strings
}
public boolean validate(String string) {
try {
long l = new Long(string).longValue();
return true;
} catch (Exception e) {
// Must not be a number...
}
return false;
}
}</pre>

In our foo() method, we can see that there’s an easy String and a hard String that aren’t used.

The first, named string, is just an assignment to a literal. It isn’t used. Reflection can’t get to it. It can be just eliminated. Even if this weren’t a literal, but an assignment to another variable, or even the result of a simple call it could be eliminated without concern.

The second, named result, is a little tougher. It requires a little knowledge of the called method to know whether the entire line can be eliminated or just the assignment. It may be the case that this just accessing a simple “return member value,” or even a more complex “return value from a member collection” or some similar activity. As I started cleaning these up I found that sometimes these were the results of very complex actions, such as EJB methods that created or updated database values, or made other web service calls. The easy fix for these is to comment the name and assignment and leave the call, such as /* String result = */ UtilityClass.someHelper(). Sure, this might make a useless spin as a member value of the caller is accessed and discarded, but without investigating every call, it was necessary to make sure that functionality didn’t get accidentally removed.

The latter, our validate() method, uselessly assigns an unused variable in an attempt to ensure that it conforms (in this case catching a simple NumberFormatException. These are solved by again commenting the variable and assignment.

Some of these were made more complex because the variable would be reused, assigning different values as the method progressed, but never reading from the value. This took a little more time to remove all of the assignments, but in the end, the methods were clean of unused cruft.

Resulting Value

To be fair, since the project was very large, it is undoubtedly the case that some of the unused variables (and even imports) are the result of refactoring over the years (it’s been a while since 1.4 was the JDK to use…let’s leave it at that). There’s little excuse, though, for not cleaning up after yourself when making these changes, so that later maintainers will not have such mind-boggling numbers of unnecessary warnings to go through. A simple rule to follow is that there should be no compiler warnings in your code! Java gives us the handy @SuppressWarnings annotation that can be used for those cases when the clean-up is not so easy. That and just watching the IDE or build script output for warnings will help everyone in their development efforts.

There was certainly value in removing these warnings. Cleaning the warnings for unused imports and variables, and adding SerialVersionUIDs (but sadly suppressing the collection type warnings for the moment) revealed a few dozen logic errors, a bunch of dead code, some errors in exception handling, and other warnings that could have helped identify problems if they weren’t lost in the wave of warnings. I’ll leave a sample class for you to play with that contains examples of these potential bug-causing bits.

<pre>public class Foo {
public boolean badException(){
try {
UtilityClass.mayCauseException();
return true;
} catch (Exception e) {
// Whew...program continues
} finally {
// Since this always executes, it always returns false...
return false;
}
}

final static Long expected = 1;
final static Long actual = 2;
public void deadCode() {
if(expected ! = actual) {
//Will always get here
} else {
// Can never get here!
}
}
}</pre>