Newsletter sign-up
View all newsletters

Enterprise Java Newsletter
Stay up to date on the latest tutorials and Java community news posted on JavaWorld

Sponsored Links

Optimize with a SATA RAID Storage Solution
Range of capacities as low as $1250 per TB. Ideal if you currently rely on servers/disks/JBODs

JavaWorld.com > Java Development Tools >

Make bad code good

Refactor broken Java code for fun and profit

By Dr. John Farrell, JavaWorld.com, 03/23/01

  • Print
  • Feedback

Page 7 of 7

With exceptions, it's best to realize that you don't have to handle every one. If you don't know what to do with a ClassNotFoundException, or an InterruptedException, don't do anything. You're allowed to pass them back to your caller. Of course, that changes the method's signature, which is not always possible, so it's best to declare your methods as throwing a nested exception (an exception that has another exception as its cause). That way, your caller can discover that the FooException you threw was caused by a ClassNotFoundException, and deal with it appropriately (or rethrow it).

It's advisable for your foo package to have a generic FooException to throw from any method, just in case something goes wrong. Then your caller has to catch only FooException. For particular problems, you can implement FooNotFoundException and IllegalFooException. As long as these subclass FooException, you won't have to change your method signature, and the caller can include enough detail to make sensible responses to the problem.

Now consider the following code, which uses the Reflection API in a fairly simple way:

public Object getInstance(String s) throws FooException {
   try {
      Class c = Class.forName(s);
      Class[] argTypes = { Integer.TYPE };
      Constructor constr = c.getConstructor(argTypes);
      Object[] args = { new Integer(0) };
      return constr.newInstance(args);
   } catch (ClassNotFoundException ex) {
      throw new FooException("class not found", ex);
   } catch (NoSuchMethodException ex) {
      throw new FooException("no such constructor", ex);
   } catch (InvocationTargetException ex) {
      throw new FooException("exception in constructor", ex);
   } catch (IllegalAccessException ex) {
      throw new FooException("can't access constructor", ex);
   } catch (InstantiationException ex) {
      throw new FooException("something bad", ex);
   }
}


In the example above, I consider the five catch statements in a row to be bad code, as they're essentially useless, they take up a lot of space, and if I ever change FooException I might have to modify them. Instead, I recommend putting them in a method by themselves, like this: 

public Object newGetInstance(String s) throws FooException {
   try {
      Class c = Class.forName(s);
      Class[] argTypes = { Integer.TYPE };
      Constructor constr = c.getConstructor(argTypes);
      Object[] args = { new Integer(0) };
      return constr.newInstance(args);
   } catch (Exception ex) {
      throwFooException(ex, "constructing new " + s);
      return null;
   }
}
private void throwFooException(Throwable ex1, String explain)
      throws FooException {
   explain = toString() + ": " + explain;
   try {
      throw ex1;
   } catch (ClassNotFoundException ex) {
      throw new FooException(explain, ex);
   } catch (NoSuchMethodException ex) {
      throw new FooException(explain, ex);
   } catch (InvocationTargetException ex) {
      ex = ex1.getTargetException();
      throw new FooException(explain, ex);
   } catch (IllegalAccessException ex) {
      throw new FooException(explain, ex);
   } catch (InstantiationException ex) {
      throw new FooException(explain, ex);
   } catch (Exception ex) {
      throw new FooException(explain, ex);
   }
}


I claim that almost all of the class's exceptions will be converted into FooException, and almost always in the same mindless way, so you might as well do it explicitly and efficiently. The throwFooException() method can be reused many times. For each invocation, you can change 10 lines of exception-handling code into 3. In addition, you automatically add contextual information (toString()), providing useful debugging information in all FooExceptions. If you're ever required to improve that debugging information, you only have to do it in one place. Remember the lesson from Animal Farm: "Less code good, more code bad." (That's not exactly what George Orwell wrote ...)

Rearchitecting

As you increase your knowledge of the code, making it more comprehensible and better factored, you'll inevitably find some bits you don't like. When you started this project, you weren't really capable of changing much, but now you've played with the code, and it's not as damaged as it once was. Along the way, you have seen some bad things and had some ideas on how to fix them. It's time to make some serious changes.

Rewrite code you don't understand

If you know what the code should do, and it seems to do it, but you can't figure out quite how, it's bad code. If the original author did something tricky, some comments should have been provided. More likely though, the author was clueless and the code really is a disaster. Rewrite it.

By rewriting the code, you have the opportunity to do it the easy way, and to include useful comments. Start by writing down in Javadoc comments what the new code does (just to make sure that you know). Then you can take advantage of the byproducts of your refactoring: call this new method that does that, use that new data structure to store this. The new code will make more sense to everybody and will probably run faster as well. Your unit tests will tell you whether your new code works. Go into your bug tracking system and close all the bugs you have fixed.

Move to a layered architecture

Let's hope that your inherited, formerly bad code now looks better. It's time to look at the bigger picture: how the packages relate to each other. Using some sort of tool that tells you what classes invoke what methods (that information can be extracted from the class files), find out the interclass dependencies. From those dependencies, infer the interpackage dependencies. If you can't draw a hierarchy (without any loops!) of what packages depend on what other packages, you have some architectural work to do.

Using your instincts, a code browser, and a great deal of guesswork, figure out what such an architecture might look like one fine day. Then identify the particular problem spots. A particular class that frequently refers to other packages or a particular constant referred to from other packages hints that classes or parts of classes reside in the wrong package. Ask yourself whether it would make sense to move that code to another package.

After a lot of consideration, you can derive a package hierarchy that looks sensible. Although it may require a lot of work to achieve the clean architecture, it gives you a goal to work toward. Your unit tests have clarified what the code should do, and now the architecture allocates the responsibility to do it to packages and classes. All you have to do is fill in the code.

Tools

All this cutting and pasting, extracting, and creating does not come without a cost. You will find yourself mindlessly changing parameter lists more frequently than you would like. You will have more compilation errors than you ever dreamed possible. You will (sometimes) insert more bugs than you take out. To help you through the boring bits, you need good tool support.

Assuming you have a good editor with search and replace functionality (preferably on multiple files at a time), you need a fast compiler to quickly detect what you've broken. Multiple invocations of javac probably won't run fast enough, instead, to avoid the startup cost, you may turn to a tool like Jikes or something that keeps javac running in the background.

A good IDE, one that compiles fast and saves you from referring to the Javadoc all the time, will stand you in good stead. I was never a fan of Java IDEs, but they seem to have evolved into useful tools. Being able to compile without changing windows and losing concentration proves vital.

Moreover, a reverse-engineering tool would be useful for determining the architecture of the system. As implied above, I don't think the architecture of the system is anything to be concerned about when you start. After all, there's not much you can do about it. However, when you have the small details under control, you'll almost certainly need automated support to figure out the existing architecture. Look for the buzzword "round-trip engineering" on the Web.

Note: I won't make any specific recommendations for tools, as I have not used most of the sophisticated ones. I'm usually happy with find, grep, and a Python interpreter. I've cobbled together utilities to do most of the things I need.

Conclusion

If you're just starting out with bad code, you've got a long way to go. But don't fret, when the going gets rough, the tough get their chance to shine. You can write good code with a smug sense of superiority and a more than average amount of job security. Moreover, you can learn from the mistakes of others (but blame them when you mess up). You can run your unit tests and say, "It works now." And you can write on your resumé, "I can make bad code good!"

That's got to be worth paying for.

Acknowledgements

I would like to thank all the bad programmers in the world, without whom life would present no challenges. I would also like to thank all the good programmers in the world, without whom sympathetic shoulders to cry upon would not be found. Working on bad code can be frustrating, and kindred spirits will keep you sane.

Further, I would like to acknowledge the contribution of the Extreme Programming inventors, and all contributors on the WikiWikiWeb, without whom the evils of Big Design Up Front and the value of refactoring and unit tests would not be duly recognized.

I would like to thank Ian Clatworthy, Darryl Collins, Christian Larney, Simon McMullen, and Thomas Maslen for their comments on the first draft; they have made this a better article. I would also like to thank David O'Brien for his comments on my English -- it's amazing what you can learn from a professional.

About the author

Dr. John Farrell has been a Java programmer since the days when even programmers thought it would be mostly used for writing fancy Webpages. He currently maintains a distributed object server infrastructure that stretches from JDBC through CORBA to just below the GUI layer. It was formerly bad code, but he made it good. Before being a Java programmer, John completed a PhD in functional programming, which gave him an appreciation of elegant languages that run slowly; and worked as a C programmer, during which time he gained an appreciation of automatic garbage collection. He currently spreads the word that Java runs fast enough for real applications, if done right; and that good object-oriented design almost always represents the right way. On the second Monday of every month, he is an active heckler at the Australian Java User Group meetings in Brisbane, Australia.

Read more about Tools & Methods in JavaWorld's Tools & Methods section.

  • Print
  • Feedback

Resources