Which is better?

[SwampChicken]

This?

1   2   3   4   5   6   7

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

or this?

1   2   3   4   5   6

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

[CyanPrime]

bottom one.

[zammbi]

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

Games published by our own members! Check 'em out!

[ewjordan]

Some people prefer brevity:

1   2   3

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:

1   2   3   4   5   6   7

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

[princec]

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

Cas

[oNyx]

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:

1   2   3   4   5   6   7

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.

[Matzon]

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.

[JL235]

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.

[cylab]

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

[princec]

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

Cas

Games published by our own members! Check 'em out!

[fruitmaze]

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

[JL235]

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

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.

[Jono]

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."

[oNyx]

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

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

[Kova]

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

[princec]

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

[JL235]

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

1   2   3   4   5   6   7   8   9   10   11   12

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.

[CyanPrime]

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

1   2   3   4   5   6   7   8   9   10   11   12

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:

1   2   3   4   5   6   7   8   9   10

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

[Eli Delventhal]

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...

[JL235]

why not just say:

1   2   3   4   5   6   7   8   9   10

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

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...

1   2   3   4   5   6   7   8   9   10   11   12   13

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

[CyanPrime]

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...

1   2   3   4   5   6   7   8   9   10   11   12   13

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

1   2   3   4   5   6   7   8   9   10   11   12   13

public void foo() {     if ( someCondition()  || !otherCondition() ) {         if(!otherCondition() ){             //do other stuff         }         return 1;     }     else {         return 2;     } }

[Abuse]

1   2   3   4   5   6   7   8   9   10   11   12   13

public void foo() {     if ( someCondition()  || !otherCondition() ) {         if(!otherCondition() ){             //do other stuff         }         return 1;     }     else {         return 2;     } }

Ew, checking the same condition twice? I'd never do that.
Especially as you've no idea how expensive evaluating 'otherCondition()' is. (It's returned value might even change, or have side-effects!)

(p.s. has anyone noticed the examples have been returning an int for a void method?

What do people think of this paradigm:

1   2   3   4   5   6   7   8   9

public int foo() {    final int retVal;    // all the shiz that has been outlined in previous examples....    // ....but assigning to the final int instead of immediately returning.    return retVal; }

[CyanPrime]

1   2   3   4   5   6   7   8   9   10   11   12   13   14   15   16

public void foo() {     boolean b = otherCondition()     if ( someCondition()  || !b) {         if(!b){             //do other stuff         }         return 1;     }     else {         return 2;     } }

Edit: forgot to change it, lol.

[Riven]

This thread is so funny...

People are trying to 'optimize' a code sample that was created for discussion about indentation, because two branches return the same value, probably by accident.

Nothing to see here, move along people.

[Eli Delventhal]

What do people think of this paradigm:

1   2   3   4   5   6   7   8   9

public int foo() {    final int retVal;    // all the shiz that has been outlined in previous examples....    // ....but assigning to the final int instead of immediately returning.    return retVal; }

Yeah someone posted that above too.

I worked with someone who I really respected who did that so I started as well, but in the end I decided it wasn't worth it as sometimes it reduces readability (which is more important in my opinion).

The whole idea of kicking out of a method if you have something "invalid" is much clearer than a giant if statement around the actual body of the code; "if my object isn't null and I am allowed to be here right now then" is much more confusing than "if my object is nill, return, if I can't be here now, return" in my opinion.

The idea of having only one return statement is to reduce potential code paths you can do down and therefore reduce bug count, but I've honestly never found a bug related to return values if you keep things readable and small. A random return inside the middle of a massive method, on the other hand, is a bad idea because it's not immediately obvious how one gets there.

[Abuse]

I was in particular pointing out the use of a final; forcing the code to assign the return value only once, which IMO has a positive knock-on effect for code structure & complexity.

I don't think there is necessarily a 'right', or even a 'better' answer to this question; it very much depends on the circumstances of the method. (size, complexity, control structures).
I even sometimes find labeled breaks to be more readable than the equivalent for/while loop! (as they are self-documenting, often reduce the size of the loop condition, and keep the logic & it's effect together.

However there are definitely lots of wrong answers =)

[Riven]

I was in particular pointing out the use of a final;

Just in case you were wondering: I wasn't addressing you with my 'rant'.

Anyway, I feel that much of this is part of your own coding style and your own preferences. I too prefer to keep the indentation to a minimum. Early return is comparable to throwing an exception: it should be done to abort the expected flow as soon as possible. Throwing an exception in the middle of nowhere is just as bad as returning at some 'random' location.

It's all about readability, because if it's readable it means it 'maps to your mind', which means it's easier to comprehend, analyze and debug. I think it shows your mind also aborts incorrect thought patterns as soon as possible, as opposed to building a stack of thoughts. Obviously I base this on absolutely nothing.

[princec]

Whoever wrote it is right.

And if you want it done the right way you do it yourself.

This solves so many dilemmas far beyond computer programming!

Cas

[Captain Awesome]

I like both, depending on the situation. The second one saves a few bytes though

[gouessej]

None of them, I prefer this:

1   2   3   4   5   6   7   8   9

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

because there is only one return statement and I'm forced to set the variable.