views:

455

answers:

10

Consider the following C# code structure (S0-S3 are placeholders for arbitrary code blocks):

try
{
    S0;
}
catch (Exception ex)
{
    S1;
}
finally
{
    S2;
}

S3;

In the case that S1 throws an exception inside the catch handler, S2 inside the finally will still execute (but S3 will not).

Question

Assuming that S1 cannot throw, is there any point in having S2 inside a finally block, rather than having it outside the try/catch/finally, just before S3?

Example

try
{
    // Do something that might throw
}
catch (Exception ex)
{
    // Save the exception to re-throw later
    // NB: This statement cannot throw an exception!
    this.cachedException = ex;
}
finally
{
    S2;
}

S3;

Is there any point in having the finally block? Would the following code not be equivalent (under the strict assumption that what's inside the catch block cannot throw):

try
{
    // Do something that might throw
}
catch (Exception ex)
{
    // Save the exception to re-throw later
    // NB: This statement cannot throw an exception!
    this.cachedException = ex;
}

// No finally block needed (?)
S2;
S3;

Secondary Question

Update: If it is accepted that the two code blocks above are equivalent (under the assumptions stated) then, taking into account the feedback on code clarity in the answers, would it be preferrable (and equivalent) to combine S2 and S3 inside the finally block?

try
{
    // Do something that might throw
}
catch (Exception ex)
{
    // Save the exception to re-throw later
    // NB: This statement cannot throw an exception!
    this.cachedException = ex;
}
finally
{
    S2; // Put S2 and S3 together inside the `finally` block to guard against
    S3; // future changes in the `catch` filter, or handling code.
}
+1  A: 

Yes you don't need finally block in your current scenario.

However its a best practice and should be followed to make your code consistent. If you accidentally write a return statement in catch block your code in finally block will still execute.

Hasan Khan
+3  A: 

finally block is used to do resource clean up either exception happens or not, finally block will both be called, so it's ideal place to do cleanup work.

Benny
+2  A: 

In your particular case there is no difference..

But if do you did not catch Exception, but rather some specific subclass of Exception then there would be a difference. And that is probably the normal pattern when looking at a try-catch-finally block.

Arve
+11  A: 

The assumption that S1 cannot throw is a fragile one, considering resource depletion scenarios (i.e., you run out of memory). Even if warranted (a big if), minor changes to the code can introduce an exception.

Since S2 is usually concerned with cleaning up and releasing valuable resources, putting it in a finally block communicates that intention clearly. Putting such code, where possible, in a Dispose() method of a resource owning object and replacing the try/finally clause with a using clause can communicate the intention even better (and more idiomatically for C#).

Whenever you can write something in two or more different ways, use the one that is clearest and most stable against changes.

Re the secondary question: S3 should be placed inside the finally if it's concerned with cleanup. If it presupposes the success of the try block, it should be placed after the finally block. If your catch statement doesn't rethrow, I personally would interpret it to mean that you have succeeded and can proceed with normal operations. However, the whole 'Save the exception to re-throw later' thing confuses me. Generally, I'd advise against storing an exception for rethrowing outside the method. It's unusual and seems confusing to me. The fewer surprises your code contains, the easier it is to maintain (including yourself, three months later).

Pontus Gagge
Pontus: Agree with your sentiments about saving the exception. It is unusual. This arises from code that is implementing the Asynchronous Programming Model and is executing code on a worker thread — in needs to save any exception for later and re-throw it when EndXxx is called.
Daniel Fortunov
+1  A: 

You might be able to ensure your code doesn't throw an Exception today, but what about when you come back to it in 6 months time? Or someone else has to make changes?

Using a finally block is a recognised pattern that will make sense to any programmer, if your code does something special and different, it will trip you or someone else up.

roryf
+1  A: 

In second case, control will only reach S2 surely if you swallow any exception ever caught in catch block! Try-Catch should not be use here-and-there but carefully.

Ex:

try
{
    // Do something that might throw
}
catch (Exception ex)
{
    // Save the exception to re-throw later
    // NB: This statement cannot throw an exception!
    // this.cachedException = ex;

    // Must swallow any exception here to let control go further!

    // If you're not sure enough [which you and me both are not likely to be] - use finally to execute S2 in any condition
}

// No finally block needed (?)
S2;
S3;
this. __curious_geek
or if there is no exception thrown.
Hogan
+11  A: 

One case where you might not anticipate S1 throwing: if the thread is interrupted, the exception will automatically be rethrown at the end of the catch block anyway.

As Pontus says, a finally block indicates the intention that this code should always be run whatever happens. That's clearer than catching everything and then continuing.

Jon Skeet
What exactly does it mean that a thread is interrupted? When will that happen?
Arve
@Arve: If anything calls Thread.Interrupt for that thread... see the docs for details.
Jon Skeet
Oh yes, that old chestnut. I forgot about asynchronous exceptions for a second there!
Daniel Fortunov
A: 

No you dont have to use it. Its not required in that case. Its a choice for you and depends on the resources you may/may not be using. If you need to cleanup any resources you might have then the finally block is the best option.

Matt Joslin
+1  A: 

Note that the finally clause is executed even if the try or catch block contains a return statement.

Regards,

Sebastiaan

Sebastiaan Megens
+3  A: 

I think this is similar to putting brackets around if statement contents.

While you could argue that

if (x) y;

will not fail, whats to say that some less experienced programmer will not come along later and edit it to

if (x) y; z;

It may be the case that the finally block is not necessary for your code as it stands, but it is always a good idea to leave it in in case the code in the catch block ever changes.

if (x) 
{
    y;
}

always wins for me.

Fiona Holder