My thoughts as an enterprise Java developer.

Wednesday, March 30, 2005

How not to use JDBC

This bad example came from production code. :-) (DatabaseManager is a wrapper for JDBC and behaves similarly.)

String select = "select ...";
Result r = null;
List numberList = new ArrayList();
try {
r = DatabaseManager.ExecuteQuery(select);
while (r != null) {
r.next();
numberList.add(r.getString("col"));
}
}
catch (Exception e) {
Result.Close(r);//Closes the result while handling null and any IOException.
}

A few of the problems with this code:
  1. DatabaseManager.ExecuteQuery can never return null (but I can let that slide some since a programmer would have to look at the code to know that -- or look at the other 64 examples in code to see that no other code checks for a null return value).
  2. while(r != null): Knowing nothing about JDBC it should be obvious that r could never change from not null to null.
  3. while(r.next()) is the standard way to use JDBC.
  4. This code uses an Exception to get out of the while loop! (Note that I have done something like that once but there was at least a somewhat good reason and it was documented.)
  5. The result should always be closed in a finally block.
So here is the improved code:

String select = "select ...";
Result r = null;
List trackingNumberList = new ArrayList();
try {
r = DatabaseManager.ExecuteQuery(select);
while (r.next()) {
trackingNumberList.add(r.getString("col"));
}
} catch (Exception e) {
e.printStackTrace();
//Logging would be nice but a stack trace to standard error is better than nothing.
} finally {
Result.Close(r);
}

How may classes does it take to screw in a light bulb?

Is this factory overkill? First the factory class just caches an instance of the finder so the finder could just as easily have just static methods (there are no member variables in DocumentFinder). Second there seems to be no reason to declare finder as an IFinder and check instanceof since it must always be a DocumentFinder and even if it wasn't it would throw a NullPointerException. This looks like another case of mis-applied patterns. (BTW I think patterns are good when correctly applied.)

//FinderFactory just instantiates the class with Class.forName and stores
// the instance in a Map (it will return that instance on subsequent calls).

IFinder finder = FinderFactory.getFinder("com.app.finders.DocumentFinder");

DocumentFinder documentFinder = null;
if (finder instanceof DocumentFinder) {
documentFinder = (DocumentFinder)finder;
}
return documentFinder.findDocuments(documentSourceId);