views:

103

answers:

7

I have a function with logic that looks like this:

doStuff1()
try:
    doStuff2()
except type1:
    error1()
    return endstuff()
except type2:
    error2()
    return endstuff()
except:
    error3()
    return endstuff()

if doStuff3():
    error4()
    return endstuff()

doStuff4()
return endstuff()

As you can see, endstuff() is done on every possible exit to the function. As it is now, endstuff() is actually 2 lines of code, and I recently had to add a third one to all possible exits. Is there any more elegant way to organize this code? I can't just use a finally, as it's not always the case that an exception is thrown.

+9  A: 

You can use a finally even if no exception is thrown and AFAIK this is the most elegant way to do what you want.

Otávio Décio
I know that, but this will still have 3 repetitions - the one in the finally, the one in the if, and the one at the end of the function. Can I turn those 3 into just one instance? If endstuff() is 5-6 lines of code that is still a ton of repetition. Are you saying I wrap all the code I have there in a separate try...finally block?
Claudiu
+2  A: 

An exception doesn't need to be thrown for a finally block to be called. The whole point of the block is that it is always reliably called. You can use this.

David M
+1  A: 

A finally clause will execute regardless of whether an exception is thrown or not, so you could just wrap your code in multiple levels of try/catch/finally... (this is C#, since you didn't specify a language)

try
{
    try
    {
         something();
    }
    catch 
    {
         // Err handler for something
    }
}
finally
{
      endstuff();   // this code always runs
}
Michael Bray
I think it's worth pointing out that there are even `try...finally` constructs that don't *require* the presence of any `catch` block at all. Such a language construct only makes sense where `finally` blocks also execute when no exception is caught. Your code example demonstrates this nicely.
stakx
A: 

Since endstuff() is returned every single time, you do not have to short circuit the return call. The function is going to return endstuff() no matter what happens, why place that code in every check? Unless you don't want to do doStuff4() if there are errors.

The more elegant way would probably be to perform endstuff() in the finally block and assign that value to a variable, and then return that variable at the end.

amischiefr
+1  A: 

Depending on the language, this is a perfect candidate for RAII. Eg, allocate an object on the stack and do "endstuff()" in its destructor; that way it gets called at the end of the scope.

class something
    def ~something()
        darkside()
        endstuff()


// in the function
def somefunc()
    something s

    doStuff1()
    try:
        doStuff2()
    except type1:
        error1()
        return
    except type2:
        error2()
        return
    except:
        error3()
        return

    if doStuff3():
        error4()
        return

    doStuff4()
    return

Jasper Bekkers
oo interesting solution
Claudiu
@Claudiu This doesn't work in GC languages though...
Jasper Bekkers
+1  A: 

I am in favor of finally block but just as an alternative you can use a while block such as:

while(true)
    doStuff1()
    try:
        doStuff2()
    except type1:
        error1()
        break
    except type2:
        error2()
        break
    except:
        error3()
        break

    if doStuff3():
        error4()
        break

    doStuff4()
end while

return endstuff()

This is useful if you are using a language that does not support exceptions. In that case your except type's would be just checking an error value on last return result.

Szere Dyeri
A: 

The other comments about the 'finally' clause are correct, but when I don't have a 'finally' clause available (not all languages do), I break the multi-exit code into a subfunction. This is useful not just with try/catch, but any code where you have to check many conditions and want to avoid deeply nested 'if' clauses. This example shows both:

ThingOne() {
    var foo;
    foo.open();

    if (ThingTwo(foo))
        ThingThree(foo);

    foo.close();
}

ThingTwo(var x) { 
    try
    {
        ...normal case...
        return true
    catch x1
        handlex1();
    catch x2
        handlex2();
    }
    return false;
}

ThingThree(var x) {
    if (x.conditionOne == false) return;
    if (x.conditionTwo == true) return;
    ...etc...
    x.GoForIt();
}
egrunin