Blog Stats
  • Posts - 24
  • Articles - 0
  • Comments - 61
  • Trackbacks - 92

 

Anti-pattern: incorrect try..finally usage

I am so sick of seeing this incorrect usage of try..finally:

Stream s = null;
try { 
    s = new FileStream(...);
    ...
}
finally {
    if(s!=null) s.Close();
}

This is WRONG. The work done in the finally block need only be done if the stream is opened.  Until the assignment of 's' completes, the finally block is not to be executed.  The 'if' statement is basically checking that fact.  The correct usage is:

Stream s = new FileStream(...);
try {     
    ...
}
finally {
    s.Close();
}

No excuses!  Programmers are lucky that the 'if' check can even be done - the anti-pattern doesn't work in general.  Consider this scenario involving thread synchronization:

Monitor.Enter();
try {     
    ...
}
finally {
    Monitor.Exit();
}

To write the above using the anti-pattern, one would need to be able to check that the monitor was entered, which may or may not be possible. Without a check, the anti-pattern might exit a monitor that was not entered.

With respect to exception handling, to catch an exception for Enter through to Exit the correct pattern is:

try {
    Monitor.Enter();
    try {     
        ...
    }
    finally {
        Monitor.Exit();
    }
} catch(Exception ex) { ... }

To handle an exception on Enter separately from the rest, use:

try {
    Monitor.Enter();
}
catch(Exception ex) { ... }
try {     
    ...
}
catch(Exception ex) { ... }
finally {
    Monitor.Exit();
}

Microsoft programmers use this anti-pattern all the time.  Stop it!


Feedback

# re: Anti-pattern: incorrect try..finally usage

Gravatar Your corrected version doesn't allow for exceptions being thrown by the new FileStream(...) statement. 1/3/2004 4:50 PM | Iain

# re: Anti-pattern: incorrect try..finally usage

Gravatar Iain, I have updated the post in light of your concern. Obviously other variations are possible. 1/5/2004 1:07 PM | Eron Wright

# re: Anti-pattern: incorrect try..finally usage

Gravatar You could just simply avoid the entire first issue and use the 'using' keyword: It automatically disposes of the created object if an exception is thrown. 5/6/2004 11:41 AM | Omer van Kloeten

# re: Anti-pattern: incorrect try..finally usage

Gravatar Your pattern 'to catch an exception for Enter through to Exit' serves to move the exception handling code even further from the source of the exception. Code present in the outer catch which serves to handle exceptions from the Monitor.Enter() (or even new FileStream(...)) would not intuitively be linked to the code that caused the exception, since it's so far away.

In my view, the original so-called anti-pattern is easier to read! 12/13/2005 8:31 AM | Joe

# re: Anti-pattern: incorrect try..finally usage

Gravatar he link seems to be dead on GotDotNet.

Any chance of posting your tool again?
10/19/2009 10:15 PM | tiffany key ring

# re: Anti-pattern: incorrect try..finally usage

Gravatar Bullshit.
First code is completely different than the second. In first You catch exceptions from FileStream constructor. In second You don't. 10/28/2009 11:29 AM | Jiggaboo

Post a comment





 

 

 

Copyright © Eron Wright