Which is better?

This?


private Object checkSomething() {
  if(getSomeObject() == null) {
    return something;
  } else {
    return somethingElse;
  }
}

or this?


private Object checkSomething() {
  if(getSomeObject() == null) {
    return something;
  }
  return somethingElse;
}

bottom one.

I personally use the bottom one as its cleaner. I believe FindBugs also warns you if your not doing the bottom iirc.

Some people prefer brevity:


private Object checkSomething() {
  return (getSomeObject() == null) ? something : somethingElse;
}

And others get very upset with more than one return statement in a function; they also tend to hate the ternary operator:


private Object checkSomething() {
  Object obj = something;
  if (getSomeObject() != null) {
    obj = somethingElse;
  }
  return obj;
}

Hm I always liked the top one more. I prefer the symmetry. I like symmetry.

Cas :slight_smile:

2nd one. I too prefer symmetry, however, in this case it’s more like always doing one thing with some quick exit at the beginning if you get some fishy parameter(s).

Consider this:

private void bar(Baz a) {
    if (a == null) {
        catchFire();
        return;
    }
    [... 1000 lines of code here w/o return ...]
}

I’d rather jump out right away and not have yet another indention level.

+1

I usually have a list of conditions for jumping out of a method and then the “normal” flow following.

I prefer the top one too because due to the braces and indentation it’s clear that the second return has nothing to do with the first. The structure of the code in the bottom one shows that the second return statement will be executed after the first one (because it follows the if statement). It’s laid out like this even though clearly this is untrue.

With the second you also require the knowledge that the statements there specifically are return statements in order to work out that only one of them will ever be reached. You don’t need this knowledge with the first example because the control flow points this out already.

I think thinking about this is pointless. It simply doesn’t matter.

er yeah but that’s a completely different bit of code. This is 5 lines. See what I mean?

Cas :slight_smile:

With such a simple function I prefer the bottom one - less code.

For something so small, I agree. But I’ve seen the same style of code stretched across pages. Then this starts to make a difference.

Plus keeping to a common set of coding standards ensures that all sections of code which are structured similarly to the above will all be laid out in the same way. This makes the code patterns more recognisable on an instinctive level.

I normally work in the same way as Matzon, and here the two bits of code have two different meanings to me.

The first one says, “there are two equally valid cases to this function, and I will return different values depending on which it is”.

The second one says “there is an exceptional case when the object is null, and the function behaves differently in this case.”

Well, yes. But for symmetry reasons (heh) one should handle both things the same way. :wink:

for such small examples, I prefer the 2nd one… as it’s logic is easier read in my mind… if () doSomething, else doOther

I find that if-else lends itself to describing two equally reasonable outcomes to a function; the latter way I use when the earlier returns are “early exit” conditions.

Cas :slight_smile:

I think what gets more complex tho is when you have code like:


public void foo()
{
    if ( someCondition() ) {
        return 1;
    } else {
        if ( !otherCondition() ) {
            return 1;
        } else {
            return 2;
        }
    }
}

Where you have one return happen multiple times and the other return only happen once. Moving the first return out will make the code smaller (and so potentially easier to understand). But that goes against my preference to show the code flow.

why not just say:


public void foo()
{
    if ( someCondition() ||  !otherCondition()) {
        return 1;
    } 

    else {
         return 2;
    }
}

My first computer science professor in college marked off points because I did the second one. Not because of style, but because he said it was incorrect - i.e. it would return TWO values. My jaw dropped and I was close to leaving my university. In the end I had to show the professor through output that there was no way for it to be wrong…

Yeah… Fortunately he got sacked and my other professors were much better.

I have since tried to imagine a programming language where it would split itself into another thread for each return value. It would be the worst possible idea ever, but might be amusing. But yeah, implicitly creating threads is probably one of the dumbest things a language could ever do. It’d be even worse than the nil pointer in ObjC silently ignoring all calls to it…

The example I gave was based on some PHP code I was writing at the time, and between the two if statements was lots of work (like database access) preventing the conditions from being concatenated together. I skipped that to simplify my example, but I’ve obviously simplified too far. So how about…


public void foo()
{
    if ( someCondition() ) {
        return 1;
    } else {
        // do lots of work
        if ( !otherCondition() ) {
            return 1;
        } else {
            return 2;
        }
    }
}