Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

This is a related Q: Is use of finally clause for doing work after return bad style/dangerous?

In the referenced Q, the finally code is related to the structure used and the necessity of pre-fetching. My question is a little different, and I believe it's germane to the broader audience. My particular example is a C# winform app, but this would apply to C++ / Java finally usage as well.

I'm noticing quite a few try-catch-finally blocks where there is a lot of code unrelated to exceptions and exception handling / cleanup buried within the block. And I will admit my bias towards having very tight try-catch-finally blocks with the code closely related to the exception and handling. Here are some examples of what I'm seeing.

Try blocks will have lots of preliminary calls and variables being set leading up to the code that could throw. Logging information will get setup and run in the try block as well.

Finally blocks will have form / module / control formatting calls (despite the app being about to terminate, as expressed in the catch block), as well as creating new objects such as panels.

Roughly:

    methodName( ... )
    {
        try
        {
            // Lots of code for the method ...
            // code that could throw ...
            // Lots more code for the method and a return ...
        }
        catch( something )
        { // handle exception }
        finally
        {
            // some cleanup due to exception, closing things
            // more code for the stuff that was created (ignoring that any exceptions could have thrown) ...
            // maybe create some more objects
        }
    }

The code works, so there is some value to it. It is not well encapsulated and the logic is a bit convoluted. I'm (painfully) familiar with the risks in shifting code around as well as refactoring, so my question boils down to wanting to know others' experience with similarly structured code.

Does the bad style justify making the changes? Has anyone been badly burned from a similar situation? Would you care to share the details of that bad experience? Leave it be because I'm over-reacting and it's not that bad of style? Gain the maintenance benefits of tidying things up?

share|improve this question
    
The "C++" tag doesn't belong here, as C++ doesn't have (and doesn't need) finally. All good uses are covered under RAII/RRID/SBRM (whichever acronym you like). –  David Thornley May 4 '12 at 21:24
2  
@DavidThornley: Aside from example where 'finally' keyword is used, the rest of the question applies perfectly fine to C++. I'm a C++ developer and that keyword didn't really confuse me. And considering I have similar conversations with my own team on semi-regular basis, this question is very much relevant to what we do with C++ –  DXM May 4 '12 at 21:32
    
@DXM: I'm having trouble visualizing this (and I do know what finally does in C#). What is the C++ equivalent? What I'm thinking of is code after the catch, and that applies the same for code after the C# finally. –  David Thornley May 4 '12 at 21:37
    
@DavidThornley: code in finally is code that executed no matter what. –  nightcracker May 4 '12 at 22:16
1  
@nightcracker, that's not really correct. It will not be executed if you do Environment.FailFast(); it may not be executed if you have an uncaught exception. And it gets even more complicated if you have an iterator block with finally that you iterate manually. –  svick May 4 '12 at 22:26
show 5 more comments

3 Answers 3

up vote 5 down vote accepted

I've been through a very similar situation when I had to deal with a terrible legacy Windows Forms code written by developers that clearly didn't know what they were doing.

First of all, you're not overacting. This is bad code. Like you said, the catch block should be about aborting and preparing to stop. It's not time to create objects (specially Panels). I can't even start explaining why this is bad.

That being said...

My first advice is: if it's not broken, don't touch it!

If your job is to maintain the code you have to do your best not to break it. I know it's painful (I've been there) but you have to do your best not to break what is already working.

My second advice is: if you have to add more features, try keeping the existing code structure as much as possible so you don't break the code.

Example: if there's a hideous switch-case statement that you feel could be replaced by proper inheritance, you must be careful and think twice before you decide to start moving things around.

You will definitely find situations where a refactoring is the right approach but beware: refactoring code is more likely to introduce bugs. You have to make that decision from the application owners perspective, not from the developer perspective. So you have to think if the effort (money) necessary to fix the problem is worth a refactoring or not. I've seen many times a developer spending several days fixing something that is not really broken just because he thinks "the code is ugly".

My third advice is: you will get burned if you break the code, it doesn't matter if it's your fault or not.

If you've been hired to give maintenance it doesn't really matter if the application is falling apart because somebody else made bad decisions. From the user perspective it was working before and now you broke it. You broke it!

Joel puts very well in his article explaining several reasons why you should not rewrite legacy code.

http://www.joelonsoftware.com/articles/fog0000000069.html

So you should feel really bad about that kind of code (and you should never write anything like that) but maintaining it is a whole different monster.

About my experience: I had to maintain the code for about 1 year and eventually I was able to rewrite it from scratch but not all at once.

What happened is that the code was so bad that new features were impossible to implement. The existing application had serious performance and usability issues. Eventually I was asked to make a change that would take me 3-4 months (mostly because working with that code took me way more time than usual). I thought I could rewrite that whole piece (including implementing the desired new feature) in about 5-6 months. I brought this proposition to the stakeholders and they agree to rewrite it (luckily for me).

After I rewrote this piece they understood I could deliver much better stuff than what they already had. So I was able to rewrite the entire application.

My approach was to rewrite it piece by piece. First I replaced the entire UI (Windows Forms), then I started to replace the communication layer (Web Service calls) and last I replaced the entire Server implementation (it was a thick client / server kind of application).

A couple years later and this application has turned into a beautiful key tool used by the entire company. I'm 100% sure that would've never been possible had I not rewritten the whole thing.

Even though I was able to do it the important part is that the stakeholders approved it and I was able to convince them it was worth the money. So while you have to maintain the existing application just do your best not to break anything and if you're able to convince the owners about the benefit of rewriting it then try to do it like Jack the Ripper: by pieces.

share|improve this answer
1  
+1. But the last sentence refers to a serial killer. Is that really necessary? –  MarkJ May 5 '12 at 6:19
    
I like part of this but at the same time leaving broken designs in place and adding stuff often leads to a worse design. It's better to work out how to fix it and fix it, although in a measured approach of course. –  Ricky Clarkson May 7 '12 at 14:19
    
@RickyClarkson I agree. It's really tough to know when you're overdoing it (a refactoring is not really necessary) and when you're really hurting the application by adding changes. –  Alex May 7 '12 at 15:17
    
@RickyClarkson I agree with that so much that I was even able to rewrite that project I was involved. But for a long time I had to carefully add stuff trying to cause the minimum damage possible. Unless the request is to fix something that the major cause is a bad design decision (which usually is not the case), I'd say the design of the application shouldn't be touched. –  Alex May 7 '12 at 15:26
    
@RickyClarkson That was part of the community's experience I wanted to tap into. Declaring all legacy code as bad is an equally bad anti-pattern. However, there are things in this code I'm working on that really shouldn't be done as part of a finally block. Alex's feedback and response are what I was looking to read about. –  GlenH7 May 7 '12 at 17:09
show 2 more comments

Suppose you have six methods to call, four of which should only be called if the first completes with no error. Consider two alternatives:

try {
     method1();  // throws
     method2();
     method3();
     method4();
     method5();
 } catch(e) {
     // handle error
 }
 method6();

vs

 try {
      method1();  // throws
 }
 catch(e) {
      // handle error
 }
 if(! error ) {
     method2();
     method3();
     method4();
     method5();
 }
 method6();

In the second code, you are treating exceptions like return codes. Conceptually, this is identical to:

rc = method1();
if( rc != error ) {
     method2();
     method3();
     method4();
     method5();
 }
 method6();

If we are going to wrap every method that can throw an exception in a try/catch block, what is the point of having exceptions in a language feature at all? There's no benefit and there's more typing.

The reason that we have exceptions in languages in the first place is that in the bad old days of return codes, we often had situations above where we had multiple methods that could return errors, and each error meant aborting all methods entirely. In the beginning of my career, in the old C days, I saw code like this all over the place:

rc = method1();
if( rc != error ) {
     rc = method2();
     if( rc != error ) {
         rc = method3();
         if( rc != error ) {
             rc = method4();
             if(rc != error ) {
                 method5();
             }
         }
     }
 }
 method6();

This was an incredibly common pattern, and one that exceptions very neatly solved by allowing you to go back to the first example above. A style rule that says each method has its own exception block completely throws this out the window. Why even bother, then?

You should always endeavor to make the try block surrond the set of code that is conceptually a unit. It should surround code for which if there is any error in the code unit, then there's no point in running any other code in the code unit.

Going back to the original purpose of exceptions: remember that they were created because often code can't easily deal with errors right when they actually occur. The point of exceptions is to allow you to deal with errors significantly later in the code, or further up the stack. People forget that and in many cases catch them locally when they'd be better off simply documenting that the method in question can throw this exception. There's too much code in the world that looks like this:

void method0() : throws MyNewException 
{
    try {
        method1();  // throws MyOtherException
    }
    catch(e) {
        if(e == MyOtherException)
            throw MyNewException();
    }
    method2();
}

Here, you're just adding layers, and layers add confusion. Just do this:

void method0() : throws MyOtherException
{
    method1();
    method2();
}

In addition to that, my feeling is that if you follow that rule, and find yourself with more than a couple try/catch blocks in the method, you should question whether the method itself is too complex and needs to be broken down into multiple methods.

The takeaway: a try block should contain the set of code which should be aborted if an error occurs anywhere in the block. Whether this set of code is one line or one thousand lines doesn't matter. (Though if you've got a thousand lines in a method, you've got other problems.)

share|improve this answer
    
Steven - thank you for well laid out response, and I fully agree with your thoughts. I don't have a problem with reasonably related or sequential type code living in a single try block. Had I been able to post an actual code snippet, it would have shown 5 - 10 completely unrelated calls prior to the first throwable call. One particular finally block was much worse - several new threads were being spun off and additional, non-cleanup routines were called. And for that matter, all of the calls in that finally block were unrelated to anything that could have thrown. –  GlenH7 May 7 '12 at 17:15
    
One major problem with this approach is that it makes no distinction between cases where method0 might quasi-expect that an exception might be thrown, and those where it doesn't. For example, suppose method1 might sometimes throw an InvalidArgumentException, but if it doesn't, it will put an object into a temporarily-invalid state which will get corrected by method2, which is expected to always put the object back into a valid state. If method2 throws an InvalidArgumentException, code which expects to catch such an exception from method1 will catch it, even though... –  supercat Mar 27 at 15:26
    
...the system state will match the code's expectations. If an exception thrown from method1 should be handled differently from one thrown by method2, how can that sensibly be accomplished without wrapping them? –  supercat Mar 27 at 15:27
    
Ideally your exceptions are granular enough that this is a rare situation. –  Steven Burnap Mar 27 at 19:53
add comment

If there is no need to change the code, leave it as it is. But if you have to change the code because you have to fix some bug or change some functionality, you will have to change it, if you like it or not. IMHO it is often more safe and less error-prone, if you first refactor it into smaller, better understandable pieces and then add the functionality. When you have automatic refactoring tools at hand, this can be done relatively safe, with only a very small risk of introducing new bugs. And I recommend that you should get a copy of Michael Feathers book

http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052

which will give you valuable hints how to make the code more testable, add unit tests and avoid breaking the code when adding new features.

If you do it right, your method will hopefully become simple enough the the try-catch-finally will not contain any unrelated code at the wrong places any more.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.