Java Exception Handling - Style

Question

Historically I have always written my Exception handling code like this:

 Cursor cursor = null;
 try {
  cursor = db.openCursor(null, null);
  // do stuff
 } finally {
  if (cursor != null) cursor.close();
 }

But recently, for reasons of readability and laziness, I have started to do this:

     Cursor cursor = db.openCursor(null, null);
     try {   
         // do stuff
     } finally {
      cursor.close();
     }

Am I wrong to have the assignment to cursor (jdbc handle, whatever) out of the try-catch-finally block?

Barring the JVM actually blowing up on the assignment, or inbetween the assignment and the first line of whatever is in the try block I'm not sure if my old style was lending any extra value, and the second is certainly more readable and concise. The literature generally always does go with the first style though.

EDIT - assume I'm happy for any exceptions thrown by openCursor while initialising the cursor not to be caught in this block of code, my only concern for this example is closing the cursor if it is assigned & opened. Also assume I'm testing for nulls etc.. etc.. yadda... yadda... (I have changed the example to reflect this, it wasn't the focus of my question so I didn't include it in the first version)

Answer 1

If the openCursor() method doesn't throw an exception, the latter is fine. If it does, then you would definitely want to use the former. The reason being that any exception would be thrown to the caller. If the calling method isn't set up to handle that then there is an issue.

Answer 2

(Update: the question was corrected based on this answer, so the first part of this answer no longer makes sense.)

The first example is wrong. It will throw a NullPointerException if db.openCursor fails. The second is better.

By the way, I often see the first method done like this:

Cursor cursor = null;
try {
    cursor = db.openCursor(null, null);
    // do stuff
} finally {
    if (cursor != null) {
        cursor.close();
    }
}

It isn't more safe doing this that doing it your second way though, but it is often used in examples and I've seen it used a lot in real code.

One reason why the second method is better is that a bug in the code in the // do stuff section can set the Cursor to null, causing the cursor to not be closed and creating a leak. All in all I can see no good reasons to use the first method (even when it is corrected with the null check), and reasons to avoid using it. Stick to the second method.

(Thanks to PSpeed for the useful comments!)

Answer 3

The second style is fine as long as neither of the method arguments is a calculated expression that can throw its own exception and needs cleaning up after.

Answer 4

I always do mine the second way, because it allows for me to set cursor as final. There's no reason I can see to have the assignment in the try clause if you are not actually trying to catch exceptions from it.

EDIT: Just to note, from the further discussion that has gone on. This is how I would handle the openCursor call throwing an exception:

try
{
    // ALLOCATE RESOURCE
    final Cursor cursor = db.openCursor(null, null);

    try
    {
        // USE RESOURCE
    }
    finally
    {
        // DISPOSE RESOURCE
        cursor.close();
    }
}
catch(OpenCursorException e)
{
    // Handle this appropriately.
}

Note the clean separation of allocation, usage, and disposal. The only time this gets a little interesting is if the usage try block throws the same exception that you're catching for the allocation try block. (IOException would be a particularly good example of this, as opening and reading can both throw one.) In that case, everything will still clean and dispose correctly, but you might incorrectly attribute failure to an initialization exception instead of a usage exception. In this case, you will want to catch the exception(s) in the inner try block and handle them immediately in there.

Answer 5

If all you are doing in your finally is closing the cursor then the second form is correct. You will never have a cursor to close if openCursor() fails. The value of that variable won't even have been set.

As others allude to, the caveats are if you are doing additional initialization that requires its own clean up then that will logically have to go in the finally{} and change the scope accordingly. Though I'd argue for a restructuring in that case.

The bottom line: as written the first version is needlessly complicated. The second version is correct.

Edit: Incorporating my other comments for posterity...

The first example might seem harmless as all it does is add a bunch of needless code. (Completely needless, if that wasn't clear.) However, in classic "more code means more potential bugs" fashion, there is a hidden gotcha.

If for some reason the "// do something" code inadvertently clears the cursor variable then you will silently leak cursors whereas before you'd at least have gotten a NullPointerException. Since the extra code does absolutely nothing useful, the extra risk is completely unnecessary.

As such, I'm willing to call the first example "just plain wrong". I'd certainly flag it in a code review.

Answer 6

There is really nothing wrong in doing :

Cursor cursor = db.openCursor(null, null);  
  try {  
     // do stuff  
   } finally {  
      try {  
        cursor.close();  
     } catch( SomeOther so ){}  
 }

Answer 7

Seriously should be using Catch in your try statements. I almost never write a try-catch-finally statement without the catch.. Even if your doing nothing with the exception at least their caught and easy to debug