32

Given that multiple return statements are acceptable (I sort of disagree, but let us digress), I'm looking for a more acceptable way to achieve the following behavior:

Option A: multiple returns, repeated code block

public bool myMethod() {
    /* ... code ... */

    if(thisCondition) {
        /* ... code that must run at end of method ... */
        return false;
    }

    /* ... more code ... */

    if(thatCondition) {
        /* ... the SAME code that must run at end of method ... */
        return false;
    }

    /* ... even more code ... */

    /* ... the SAME CODE AGAIN that must run at end of method ... */
    return lastCondition;
}

It makes me feel dirty to see the same (little) code block repeated three times each time the method returns. Furthermore, I would like to clarify that the two return false statements above can certainly be described as returning mid-method... they are absolutely not "guard statements."

Is Option B slightly more acceptable? I feel that I may abusing try/finally, and I'm hoping there is something completely different that I should be doing.

Option B: multiple returns, try/finally block (without catch blocks / exceptions)

public bool myMethod() {
    try {
        /* ... code ... */

        if(thisCondition) {
            return false;
        }

        /* ... more code ... */

        if(thatCondition) {
            return false;
        }

        /* ... even more code ... */

        return lastCondition;
    } finally {
        /* ... code that must run at end of method ... */
    }
}

Finally, Option C is the best solution in my book, but my team doesn't like this approach for whatever reason(s), hence I'm looking for a compromise.

Option C: single return, conditional blocks

public bool myMethod() {
    /* ... code ... */

    if(!thisCondition) {
        /* ... more code ... */
    }

    if(!thisCondition && !thatCondition) {
        /* ... even more code ... */
    }

    /* ... code that must run at end of method ... */
    return summaryCondition;
}

If you want to discuss multiple return statements, please do so in this question.

Community
  • 1
  • 1
Dolph
  • 49,714
  • 13
  • 63
  • 88
  • 1
    I would have answered with option C if you had not already provided it! What are your teammate's objections to option C? What is their answer to What happens if the "code that must run at end of method" needs to change? – Dónal Boyle Feb 09 '10 at 16:52
  • I agree with Donal. I like Option C. What are their specific objections? – Geo Ego Feb 09 '10 at 16:57
  • I don't really want to speak for them, but as I understand it, they don't like Option C simply because it doesn't have multiple return statements. I'm not sure that's a defensible position, but that's another topic. – Dolph Feb 09 '10 at 17:07
  • if it's something like `if(!save(foo)) return false;` I'd suggest to throw an exception from `save(..)` to force people to handle this case instead of ignoring it (or forgetting about it!). If it's closer to `if (count() == 0) return false;` an exception obviously wouldn't make sense and C would be my choice (with @Loadmaster's simplifications). – sfussenegger Feb 09 '10 at 18:22
  • 1
    It is worth pointing out that try-finally as used in option B is equivalent to scope-bound resource management (SBRM, sometimes called RAII) which is so often praised in other languages. –  Feb 11 '10 at 17:09
  • If the code in question should be executed if an exception is thrown then hands down b. If i cant reorder code i'd use c. option A is not acceptable to me. –  Jan 28 '11 at 07:33

11 Answers11

30

If the code needs to run even when any other code throws an exception, then the finally block is the correct solution.

If it need not run in the case of an exception (i.e. it's only necessary for "normal" returns), then using finally would be abusing the feature.

Personally I'd rewrite that method in the single-return-point style. Not because I religiously subscribe to that idea (I don't), but because it is best suited for these kind of end-of-method codes.

When that code turns out to be overly complicated (and that's a very real possibility), then it's time to refactor the method by extracting one or more methods.

The simplest refactoring would be something like this:

public boolean  myMethod() {
    boolean result = myExtractedMethod();
    /* ... code that must run at end of method ... */
    return result;
}

protected boolean myExtractedMethod() {
    /* ... code ... */

    if(thisCondition) {
        return false;
    }

    /* ... more code ... */

    if(thatCondition) {
        return false;
    }

    /* ... even more code ... */
    return lastCondition;
}
Joachim Sauer
  • 302,674
  • 57
  • 556
  • 614
28

Exceptions should be exceptional so I don't like option B if there are no other exceptions around(Note for downvoters - I don't say that having finally is incorrect just that I prefer not to have it if there are no exceptions - if you have reasons please comment)

If code is always needed how about refactoring into 2 functions

public bool myMethod() {
    bool summaryCondition = myMethodWork();
    // do common code
    return summaryCondition;
}

private bool myMethodWork() {
   /* ... code ... */

    if(thisCondition) {
        return false;
    }

    /* ... more code ... */

    if(thatCondition) {
        return false;
    }

    /* ... even more code ... */

    return lastCondition;
}
mmmmmm
  • 32,227
  • 27
  • 88
  • 117
  • 19
    +1 for correctly answering the second half of the question. But the comment that finally is incorrect in the absence of exceptions is just plain wrong. try/finally exists in order to execute some block of code and then always do something else regardless of how that block exits - finally is often tied to exception handling but not limited to that; it is perfectly legitimate to use finally for exit handling too. – Lawrence Dol Feb 09 '10 at 16:57
  • Accepting this answer since this code was posted five minutes before Joachim Sauer's. This is the *duh* solution I was hoping for. – Dolph Feb 09 '10 at 17:33
  • But I did not say it is incorrect. I said I did not like it - it is like the original question a matter of style – mmmmmm Feb 09 '10 at 17:33
  • 2
    I agree with @SoftwareMonkey; a `finally` block is independent of any `catch` block. In fact, there are way too many instances of `try-catch` in Java code that really should be `try-finally`. In other words, exceptions are often caught and mis-handled, when what is needed is to clean up a resource and let the exception be handled by a caller with more context. – erickson Feb 09 '10 at 17:38
  • 1
    @Mark: Sorry read in an inference that you may not have intended (but hey, I still +1'd you!) – Lawrence Dol Feb 09 '10 at 19:07
  • I keep hearing the catchy phrase "Exceptions should be exceptional" but never an explanation of what this *means*. Rare in code? This is probably the only place he does this. Rare in execution? I get a lot of IOExceptions, so would you say I should make IO conditions not use Exceptions? Or do you mean "exceptional" in the more general "unusual and impressive", in which case this use looks fine? In general, what do you mean by "exceptional"? – Ken Feb 11 '10 at 18:50
  • I suppose nearest is "unusual and impressive". Iy can also depend on the language, e.g. Python will use exceptions more than C++ or Java. If the bad condition is normal in the execution path than don't use an exception – mmmmmm Feb 12 '10 at 11:58
  • myMethodWork() is too long. It should be refactored to extract smaller methods like in this answer http://stackoverflow.com/a/2230932/2020801 – Erik Madsen Jun 25 '13 at 14:37
15

This is a perfect place for a GOTO

*ducks*

David Oneill
  • 12,502
  • 16
  • 58
  • 70
3

If the code needs to run even when there is an Exception, then finally is not just a good choice, it is a must. If that is not the case, finally is not necessary. Looks like you want to find format that "looks" best. But there is little more at stake here.

Lawrence Dol
  • 63,018
  • 25
  • 139
  • 189
fastcodejava
  • 39,895
  • 28
  • 133
  • 186
3

Your option C solution is not far from optimal, since it adequately codes the proper execution sequence you're trying to accomplish.

Similarly, using nested if-statements do the same thing. It may be visually less appealing, but it's simpler to understand, and makes the execution flow pretty obvious:

public bool myMethod() { 
    boolean  rc = lastCondition; 

    /* ... code-1 ... */ 

    if (thisCondition) { 
        rc = false;
    } 
    else {  
        /* ... code-2 ... */ 

        if (thatCondition) { 
            rc = false;
        } 
        else {  
            /* ... code-3 ... */ 
            rc = ???;
        }  
    }

    /* ... the code that must run at end of method ... */ 
    return rc;  
}

Simplifying the code yields:

public bool myMethod() { 
    boolean  rc = false; 

    /* ... code-1 ... */ 

    if (!thisCondition) { 
        /* ... code-2 ... */ 

        if (!thatCondition) { 
            /* ... code-3 ... */ 
            rc = lastCondition;
        }  
    }

    /* ... the code that must run at end of method ... */ 
    return rc;  
}

The simplified code also reveals what you're actually trying to achieve: you are using the test conditions to avoid executing code, therefore you should probably be executing that code when the conditions are false instead of doing something when they are true.

To answer your question about try-finally blocks: Yes, you can abuse them. Your example is not sufficiently complex enough to warrant the use of try-finally. If it were more complex, though, it might.

See my take on it at: Go To Statement Considered Harmful: A Retrospective, "Exception Handling".

David R Tribble
  • 11,918
  • 5
  • 42
  • 52
2

How about breaking it up a little more to give something more ( excusing my not having used Java's logical operators in quite some time ) like this:

public bool findFirstCondition()
{
   // do some stuff giving the return value of the original "thisCondition".
}

public bool findSecondCondition()
{
   // do some stuff giving the return value of the original "thatCondition".
}

public bool findLastCondition()
{
   // do some stuff giving the return value of the original "lastCondition".
}

private void cleanUp() 
{
   // perform common cleanup tasks.
}


public bool myMethod() 
{ 


   bool returnval = true;
   returnval = returnval && findFirstCondition();
   returnval = returnval && findSecondCondition();

   returnval = returnval && findLastCondition();
   cleanUp();
   return returnval; 
}
glenatron
  • 11,018
  • 13
  • 64
  • 112
  • @Dolph Mathews it would look cleaner, but each method will be executed, regardless of returnval! – sfussenegger Feb 09 '10 at 18:12
  • 1
    @Dolph Mathews - That's how I thought to write it first but then I couldn't recall if Java actually has an &= operator and as sfussenegger mentions it would lose the benefit of short-circuit evaluation... – glenatron Feb 09 '10 at 19:52
  • thanks for helping with the technical term ;) I still have the felling it's a little Perl'ish tough: `$val = foo() unless !$val;` - at least as far as I remember - thank god I've forgotten most of it :) – sfussenegger Feb 09 '10 at 23:36
  • There is no such thing as perlish. Perl encompasses pretty much every possible way of doing something. That is why it is so irritatingly difficult to read Perl written by anyone else :) – glenatron Feb 10 '10 at 14:20
  • it's a tiny thing (in the context of this question) but I probably wouldn't expose the extracted methods as public. They're still implementation detail rather than part of the contract. But definitely +1 for the cleanest solution so far. – Erik Madsen Jun 25 '13 at 14:32
2

Don't abuse try/finally unless you need to break out of inner loops. Abuse do/while.

bool result = false;
do {
  // Code
  if (condition1) break;
  // Code
  if (condition2) break;
  // . . .
  result = lastCondition
} while (false);
Rex Kerr
  • 166,841
  • 26
  • 322
  • 407
  • 5
    you cannot be serious! Anybody will instantly know what `finally` is used for. The intention of `do { ... } while (false);` is by far less obvious. – sfussenegger Feb 09 '10 at 18:05
  • But anybody will not know instantly what the `try` is for--you think "dangerous exception-throwing code ahead", not "some regular control-flow construct", and you might want real exceptions to fall through without executing finally (i.e. they're real uncaught exceptions). Since both constructs are weird, my real suggestion would be to refactor into two methods, but sometimes there's a lot of internal state to maintain and if your coding team refuses if-chains, having options like this can be helpful. – Rex Kerr Feb 09 '10 at 18:22
  • If you can't instantly understand what the finally block is doing, the code is flawed anyway: Either a) the try block is too long (having to scroll from the beginning of a try block to its end is a no-no - extract some methods!) or b) the catch block is too long (it shouldn't be doing more than some basic cleanup that should be quick to grasp too). As I haven't ever seen this abuse of do/while anywhere before, it took me two seconds to get it's intention - a good indicator that it isn't a very good choice. – sfussenegger Feb 09 '10 at 18:33
  • It's a bad choice unless used widely throughout that codebase; then at least those programmers would know what's going on. Given all the blocks of code in the example, it looked very likely to me that the try block *was* too long. – Rex Kerr Feb 09 '10 at 21:32
0

Using try/finally for controlling flow feels to me like using GOTO.

cherouvim
  • 31,725
  • 15
  • 104
  • 153
  • And what is wrong with judicious use of goto when it makes the program flow clearer and less error-prone? After all, that is what break, continue and return are, esp labelled break and continue. – Lawrence Dol Feb 09 '10 at 16:58
  • I think breaks are okay, continues are okay, and returns are okay, but if you use them all in the same method... YIKES! – Dolph Feb 09 '10 at 17:05
  • 1
    @Dolph: If you haven't used all of them in the same method, you probably haven't been writing code for very long. (smile). Get back to me in 10 or 20 years. – Lawrence Dol Feb 09 '10 at 17:13
  • I think GOTO can be very useful at times, but like most things in life they are only good in moderation. Use the tool at the right time and you shall be rewarded, but if you use it all the time you might find yourself in a bad place. – Brian T Hannan Feb 09 '10 at 17:25
  • 1
    @Software Monkey: If I ever see all three in one method *in Java*, heads will roll. – Dolph Feb 09 '10 at 17:38
  • @Dolph: It's very good to have principles and to follow them... just remember that programming is best approached as an art, not a religion (and despite us calling ourselves engineers, it's not strictly an engineering discipline). – Lawrence Dol Feb 09 '10 at 19:04
  • using return, break and continue in one method is blasphemy nevertheless ;) – sfussenegger Feb 09 '10 at 23:40
0

Is there a reason that you can't simply store the return value, and fall out of the if?

   bool retVal = true;
   if (retVal && thisCondition) {
   }

   /* more code */

   if ( retVal ) {
     /* ... code that must run at end of method, maybe inside an if or maybe not... */

   }
   return retVal;
chris
  • 36,094
  • 53
  • 157
  • 237
  • 2
    Because this is horrible code that keeps getting more horrible the more conditions that exist. – Lawrence Dol Feb 09 '10 at 17:00
  • Some times you have to break the rules. If it solves your problem, then do it. Architectural purity and abstract concepts sometimes gets in the way of reality. – chris Feb 09 '10 at 19:32
  • Absolutely. But using try/finally is far better code than this. – Lawrence Dol Feb 10 '10 at 04:09
  • That's what I meant: it doesn't matter if it's an "abuse" of try/finally, if it works and solves your problems, do it. – chris Feb 10 '10 at 12:42
0

IMO the idea is to put a try block over a small part of code (e.g. a method call) that can throw a known exception (e.g. reading from a file, reading an int to a String). So putting a try block over the whole code of a method is really not the way to go, unless you are actually expecting each and every if condition code to potentially throw the same set of exceptions. I don't see a point in using a try block just for the sake of using a finally.

If I am not mistaken, putting large chunks of code inside a try also makes it a lot slower, but not sure if this is true in the latest versions of Java.

Personally, I would go with Option C. But I don't have anything against Option A either.

MAK
  • 26,140
  • 11
  • 55
  • 86
  • I didn't consider a performance impact, I would like to know if there is one. – Dolph Feb 09 '10 at 17:03
  • 1
    Exception *generation* is *relatively* expensive, I don't think exception *handling* or try/finally itself is particularly so. – Lawrence Dol Feb 09 '10 at 17:18
  • @Dolph Matthews: I am really not sure about this, and Software Monkey is probably right. I guess the best thing to do to know for sure is to profile your code with the large try block and find out yourself. – MAK Feb 09 '10 at 17:52
0

Unless the code that must run at the end of the method uses method local variables, you could extract it into a method like:

public boolean myMethod() {
    /* ... code ... */

    if(thisCondition) {
        return myMethodCleanup(false);
    }

    /* ... more code ... */

    if(thatCondition) {
        return myMethodCleanup(false);
    }

    /* ... even more code ... */

    return myMethodCleanup(lastCondition);
}

private boolean myMethodCleanup(boolean result) {

    /* ... the CODE that must run at end of method ... */
    return result;
}

this still doesn't look very nice, but it is better than using goto-like constructs. To convince your team mates that a 1-return solution might not be that bad, you can also present a version using 2 do { ... } while (false); and break's (*evil grin*.)

rsp
  • 23,135
  • 6
  • 55
  • 69