Java Worst Practices

We've all read those "best practices" articles which are great and useful. But after the good reaction to my "Java Exception Handling Anti-Patterns" article I thought I'd continue on in the same vein - things NOT to do.

So here's a list of "worst" practices - the things I see many-a-day that are particular pet peeves of mine. Frankly I've made these mistakes myself quite often when I was less experienced so they've become a kind of personal checklist for my own code as well as for that of others. I've touched on some of these issues before but have added some more and drilled-in a little more.

Of course these days I use my code checking tool trifecta (Checkstyle/PMD/FindBugs) to do it faster and more reliably but still it's good to stay sharp and do it manually every so often.

1) Exception Handling I: poor catch blocks
See the blog entry "Java Exception Handling Anti-Patterns" for a number of such anti-patterns. How to handle errors correctly is critical and not altogether that easy in Java (thus the whole checked vs. unchecked exceptions debate IMHO)

2) Exception Handling II: missing or bad finally blocks

• If a finally block is absent in a try{}catch{} combo, is one needed to release resources?
• Is there an exception thrown in the finally block that bubbles out? Talk about cause for confusion when reading the code!
• Is there a return inside a finally block? Talk about even more cause for confusion!
• Do you have returns *AND* uncaught exceptions in your finally block? Check this link out. Exceptions that just disappear into the aether? Yikes! I'd guess 90% of developers (including myself) got that wrong.
• If there are multiple methods called in a finally block, and any of them can throw an exception, will the other methods get called e.g.
  .
  .
  }
  finally{
  try{
  if(obj1!=null) obj1.close();
  if(obj2!=null) obj2.close();
  } catch(SomeException ignored) {
  }
  }
  
  In this case if an exception was thrown when closing obj1, then obj2 would not be closed creating some problem such as a memory leak.
  

3) Logging

• Too little? App crashes and there's nothing in the log file.
• Too much? App crashes and you need to plow through a 5 GB log file (just for that day!)
• Wrong level - debug vs. info vs. warn vs. fatal?
• Same log entry repeated many times
• Using Exception printStackTrace() - Do not use printStackTrace except for the simplest programs - use you logger and log.error("Some Text Here",e); to log that important and all-too-helpful stack trace information information to your log file for posterity.
  

4) Classes / Methods too big
Big classes and big methods become inevitable complexity sinks - the code becomes very very hard to maintain. Create utility classes, break-up the functionality - do some refactoring to make it easier to use, extend, adapt and especially debug!

5) Comments in code

• Too little?
• Too much?
• Comments which are no longer relevant or are out-of-date with respect to the code
• Comments which are useless/redundant e.g.

//set number of accounts to zero
numAccounts=0;

FYI some best practices in this area are listed here on Wikipedia. Most notably:

• Comment why not what - make your intent clear.
• Make your comments the code - if your comments are clearer than your code make your code look like the comments.

For example here's a pattern typical of code I often find in a review (even of my own code)

//check system Ok for startup
if(switch1 && switch2 && (switch3 || switch4))

now replace that but as follows and drop the comment

if(isSystemOkForStartup(switch1,switch2,switch3,switch4))

now you've got a nice utility method that you can re-use plus one less comment which means one less place where you have to keep the comments in synch with the code

6) Switch Statements

• Missing default: - just put something in there even if it's a log.warn("This should never happen");
• Missing break; and the resulting "fall through". God those bugs can be nasty to track down.

I usually write the "default" first to make sure it gets in there and try to give visual cues to highlight missing breaks like trying to line them all up vertically.

7) catching java.lang.Throwable
There's a few cases where I think this is OK . . . . okay just one . . . wrapping a complete application with this so you can log.error() / System.out.println() if something bad happened that for some reason wasn't handled.

But apart from that, exactly what do you plan to do with VirtualMachineError or OutOfMemoryError? Just carry on?!?!?!

8) Using exceptions as flow control
Sigh! It still happens every so often.

9) Not removing commented out code or unused classes
I've done it myself but usually I open a bug report to myself to take it out (eventually). Sometimes it's hard - you've written some good code that you might use some day just not now and you want it in there - but hey that's what Source Code Control is for.

10) Complex code with no unit tests
Don't even get me started! Yeah yeah I know the refrain - we don't have time but come on - a few unit tests? Please!?!?!? Just some happy-path cases?

11) Duplicate Code
We've all done it - we just need to make sure we undo it - refactor! If you've got production code and 1 or 2 dupes I can make an exception. 44 not so much (trust me I've seen it!). Oh and the excuse here - "Well we've got 44 separate JSP files" - turns out it was really one JSP file with some parameterized pieces of code. About 5000 lines suddenly became 250.

12) In multi-threaded code, using Object.wait()
That is the call to wait with no timeout - so the user or calling application can just wait forever eh? Oh and the bugs it causes - your code just sits there "dead", there's nothing whatsoever in the log file and then you've got to find the one thread in your 500 with the endless wait.

Every time I get one of these I cringe, cancel family vacations, hunker down and get stuck in.

By using a timeout you force yourself to think about what is a reasonable thing in most cases - what if that shared resource is simply not available? If you don't do this, effectively you just expect "the other guy" to worry about giving it back and abdicate your own responsibility.

13) Coding Standards that are directly in contradiction to accepted and known Java standards.
For example - not only allowing class names to start with lower case letters but ENFORCING it (yup - been there)

14) Enforcement of Coding Standards that is overly picky
I could pick an example (e.g. placement of braces, tabs vs. spaces) but I'd probably evoke a bazillion flames! Oh and best of all - often these rules are enforced before important things like unit tests, code coverage - those things related to code that works!

15) Overuse of XML
For example - storing XML in a database and then wondering why indexing on the contents is slow?

16) Wrapping Java platform APIs
For example - wrapping JDBC with your own API. Now I'm not talking a DAO or some abstraction layer - I'm talking about a class-for-class, method-for-method wrapping!

Oh I'm sure there are others but these are the ones I look for during a human-based code scan. I can't scan my own code because I'm terrible at it (and lazy whenever I spot anything - just too easy to look at the other 10 things pressing on my To-do list). The best thing to do here is use Checkstyle/PMD/FindBugs and have someone else prod me.