code style: multiple returns

Question

I'm writing a method along these lines:

if (hasFoo()) {
   return calculateFoo();
} else if (hasBar()) {
   return calculateBar();
} else {
   return calculateBaz();
}

The getters are rather expensive and the has...() checks would either duplicate a lot of the logic or just have to reuse the getters. I could have the has...() methods store the result of the get...() in a field and make the getter lazy, but it would be nice for has...() not to have any side effects. I could write this with nested try{} catch{} blocks, but that doesn't look elegant. seems like there should be a better solution to this...

EDIT: changed get...() to calculate...() to make it clear that they're expensive.

Answer 1

int result = 0;

if (hasFoo()) {
   result = getFoo();
} else if (hasBar()) {
   result = getBar();
} else {
   result = getBaz();
}

return result;

is the idiom I prefer to use - makes it far easier to inspect variable values while debugging.

Answer 2

I see nothing wrong in doing

Object fooBarBaz = null;

if (hasFoo()) {
   foo = getFoo();
} else if (hasBar()) {
   fooBarBaz = getBar();
} else {
   fooBarBaz = getBaz();
}

return fooBarBaz;

Answer 3

EDIT: If I'm interpreting your comments correctly, it sounds like you actually want something like:

Result result = calculateFoo();
if (result != null) {
    return result;
}
result = calculateBar();
if (result != null) {
    return result;
}
return calculateBaz();

... where each of the calculate methods returns null if the corresponding has method returns false. Now if null is a valid "real" return value, you could always wrap the result so that calculateFoo returns a value which can basically say, "Yes, I've got a valid value and it's X" or "no, I haven't got a valid value" (a "maybe" type).

---

Original answer

I would keep your code exactly as it is. I see no problems with having multiple return statements when that's the clearest approach - and in this case I believe it is.

You're making it clear that once you've reached each of the "leaf" parts, you know exactly what the return value is, and the only other code which should be executed before leaving the method is any clean-up code in finally blocks.

Having a single exit point makes sense in languages which don't have try/finally or GC (where you really want to make sure you do all the cleanup in a single place) but in Java, I think returning when you know the result states your intention more clearly than using a separate local variable.

Having said that, another alternative to consider is using the conditional operator, laying out your code so it's obviously going through a series of tests and returning as soon as it finds the first "match":

return hasFoo() ? getFoo()
     : hasBar() ? getBar()
     : getBaz();

The disadvantage is that this pattern looks a little odd the first time you see it - but once you get used to it, I find it a really neat way of encoding this sort of logic.

Answer 4

I prefer it this way:

if (hasFoo()) {
    return calculateFoo();
}

if (hasBar()) {
    return calculateBar();
}

return calculateBaz();

All a matter of taste and convention.

Answer 5

I am not sure if this is your case, but I would try to fully refactor the code. Currently, as far as I understand, your code looks something like this (example):

boolean hasFoo() {
    DataObject do = getSomeDataSource().getSomeDataObject();
    if (do.getF() != null && do.getO() != null) {
        return true;
    } else {
        return false;
    }
}

Foo getFoo() {
    DataObject do = getSomeDataSource().getSomeDataObject();
    Foo result = new Foo(do.getF(), do.getO());
    return result;
}

Basically what happens here is that the same code is used to check if Foo can be returned and to construct the Foo itself too. And I would refactor it to this:

/**
* @returns instance of Foo or null if Foo is not found
*/
Foo getFoo() {
    DataObject do = getSomeDataSource().getSomeDataObject();
    F f = do.getF();
    if (f == null) {
        return null; //Foo can not be created
    }
    O o = do.getO();
    if (o == null) {
        return null; //Foo can not be created
    }

    return new Foo(f,o);
}

Now your original code would become similar to this:

Result r;

r = getFoo();
if (r == null) {
    r = getBoo();
}
if (r == null) {
    r = getDoo();
}
return r;

Answer 6

This is not an "is it OK to do multiple returns" problem - your multiple returns are fine.

This is a refactoring and/or state storage problem.

If you have:

bool hasXXX() {
    // do lots of stuff
    ...

    return has_xxx;
}

and

double calculateXXX() {
    // do the same lots of stuff
    ...

    // do some more stuff
    ...

    return xxx;
}

then the complexity of the problem depends on whether the hasXXX() calculation produces lots of intermediate values that are necessary for calculateXXX().

You likely need something like:

bool checked_xxx = false;
double xxx_state;

bool hasXXX() {
   // do expensive stuff
   ...

   // save temporary state variables
   xxx_state = ... 

   // remember that we've been here
   checked_xxx = true;

   // send back the required value
   return has_xxx;
}

double calculateXXX() {
    // make sure that hasXXX was called, and is valid
    if (!checked_xxx && !hasXXX()) {
        // should never happen - you called calculateXXX when hasXXX() == false
        throw new RuntimeException("precondition failed");
    }

    // use the previously calculated temporary state variables
    ...

    // send back the final result
    return xxx;
}

Answer 7

Instead of doing hasXXX() and calculateXXX() you could factor those calculations out to separate objects eg

public interface CalculationModel {
    Object calculate();
}

public class FooCalculationModel implements CalculationModel {
    @Override
    public Object calculate() {
        // Perform Foo calculations
        return result;
    }
}

and your if-statement can then be replaced with:

return getCalculationModel().calculate();

You will need some way of deciding the CalculationModel of course, but this would then replace the hasFoo(), hasBar() etc methods.

Answer 8

you could do something like this :

Object bar;
if ((bar = getFoo()) != null) {
  return bar;
} else if ((bar = getBoo()) != null) {
  return bar;
} else {
  return getBaz()
}

this way you only need to call the get methods, but not the has ones

EDIT

this is the same in a more readable format that also elminates the need to call the has methods

Object bar = getFoo()

if (bar == null) {
  bar = getBoo()
}

if (bar == null) {
  bar = getBaz()
}

return bar;