SwampChicken
|
 |
«
Posted
2010-11-09 05:24:30 » |
|
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
|
 |
«
Reply #1 - Posted
2010-11-09 05:59:29 » |
|
bottom one.
|
|
|
|
zammbi
|
 |
«
Reply #2 - Posted
2010-11-09 06:54:30 » |
|
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
|
 |
«
Reply #3 - Posted
2010-11-09 06:58:28 » |
|
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
|
 |
«
Reply #4 - Posted
2010-11-09 07:19:12 » |
|
Hm I always liked the top one more. I prefer the symmetry. I like symmetry. Cas 
|
|
|
|
oNyx
|
 |
«
Reply #5 - Posted
2010-11-09 07:47:54 » |
|
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
|
 |
«
Reply #6 - Posted
2010-11-09 07:55:25 » |
|
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
|
 |
«
Reply #7 - Posted
2010-11-09 09:03:25 » |
|
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
|
 |
«
Reply #8 - Posted
2010-11-09 09:19:08 » |
|
I think thinking about this is pointless. It simply doesn't matter.
|
Mathias - I Know What [you] Did Last Summer!
|
|
|
princec
|
 |
«
Reply #9 - Posted
2010-11-09 09:33:30 » |
|
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
|
 |
«
Reply #10 - Posted
2010-11-09 09:40:40 » |
|
With such a simple function I prefer the bottom one - less code.
|
|
|
|
JL235
|
 |
«
Reply #11 - Posted
2010-11-09 09:46:02 » |
|
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
|
 |
«
Reply #12 - Posted
2010-11-09 13:08:03 » |
|
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
|
 |
«
Reply #13 - Posted
2010-11-09 23:25:28 » |
|
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
|
 |
«
Reply #14 - Posted
2010-11-09 23:45:41 » |
|
for such small examples, I prefer the 2nd one.. as it's logic is easier read in my mind... if () doSomething, else doOther
|
|
|
|
princec
|
 |
«
Reply #15 - Posted
2010-11-10 10:48:45 » |
|
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
|
 |
«
Reply #16 - Posted
2010-11-14 02:25:48 » |
|
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
|
 |
«
Reply #17 - Posted
2010-11-14 04:18:04 » |
|
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
|
 |
«
Reply #18 - Posted
2010-11-14 04:50:36 » |
|
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
|
 |
«
Reply #19 - Posted
2010-11-14 17:07:05 » |
|
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 { if ( !otherCondition() ) { return 1; } else { return 2; } } } |
|
|
|
|
CyanPrime
|
 |
«
Reply #20 - Posted
2010-11-14 19:11:50 » |
|
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 { 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() ){ } return 1; }
else { return 2; } } |
|
|
|
|
Abuse
|
 |
«
Reply #21 - Posted
2010-11-14 20:17:11 » |
|
1 2 3 4 5 6 7 8 9 10 11 12 13
| public void foo() { if ( someCondition() || !otherCondition() ) { if(!otherCondition() ){ } 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;
return retVal; } |
|
|
|
|
CyanPrime
|
 |
«
Reply #22 - Posted
2010-11-14 20:42:18 » |
|
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){ } return 1; }
else { return 2; } } |
Edit: forgot to change it, lol.
|
|
|
|
Riven
|
 |
«
Reply #23 - Posted
2010-11-14 21:20:35 » |
|
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.
|
Hi, appreciate more people! Σ ♥ = ¾ Learn how to award medals... and work your way up the social rankings!
|
|
|
Eli Delventhal
|
 |
«
Reply #24 - Posted
2010-11-15 09:21:31 » |
|
What do people think of this paradigm: 1 2 3 4 5 6 7 8 9
| public int foo() { final int retVal;
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
|
 |
«
Reply #25 - Posted
2010-11-15 12:24:22 » |
|
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
|
 |
«
Reply #26 - Posted
2010-11-15 21:47:33 » |
|
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.
|
Hi, appreciate more people! Σ ♥ = ¾ Learn how to award medals... and work your way up the social rankings!
|
|
|
princec
|
 |
«
Reply #27 - Posted
2010-11-15 23:02:58 » |
|
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
Junior Devvie   Medals: 2
Hi
|
 |
«
Reply #28 - Posted
2010-11-23 19:52:00 » |
|
I like both, depending on the situation. The second one saves a few bytes though 
|
|
|
|
gouessej
|
 |
«
Reply #29 - Posted
2010-11-26 12:25:34 » |
|
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.
|
|
|
|
|