|
Just because you can doesn't mean you should.
|
|
|
|
|
OriginalGriff wrote: I think that's an unnecessary restriction - I prefer to do all my validation code / user notification en mass at the top of a method, and exit immediately.
Exactly. I share the same view - do validation and exit as soon as possible, to avoid crippling the logic down the road.
|
|
|
|
|
I totally agree with you guys. But its also important what your boss thins. For example when i used the same technique validations with fast returns i got scold how this shouldn't be done this way because when someone reads the code he wont understand a thing ?! And also this isn't a good practice ?!
Microsoft ... the only place where VARIANT_TRUE != true
|
|
|
|
|
It seems a lot of people follow to this "rule" without ever questioning why. My manager was also an advocate of the "single return improves readability" fallacy until I explained to him why this wouldn't be the case with examples. Plus when I completely rewrote a ugly mass of haired code in our system into actually maintainable code, he was convinced.
I think it needs some luck, and having open-minded management helps too.
|
|
|
|
|
I do not see the value in always only having one return from a function. When reading the code, you still need to look through the function for every place retValue is set, so it is not easier to follow or understand. If there is some clean-up to be done, then it does make sense. But if not, why keep to this rule if it provides no benefit? In the validation example someone else posted in this thread, the early returns provide easier to read code, rather than a long block of if/then/else.
|
|
|
|
|
Quote: the early returns provide easier to read code Easier to read is a matter of opinion. Some people think C# is easier to read than VB.
There are only 10 types of people in the world, those who understand binary and those who don't.
|
|
|
|
|
ryanb31 wrote: I prefer the second method. You can't be serious!
/ravi
|
|
|
|
|
Quote: You can't be serious! I do like to joke around a lot, but yes, I can be serious at times.
There are only 10 types of people in the world, those who understand binary and those who don't.
|
|
|
|
|
So if the array contained a million strings, the first of which was "ABC", you'd check every single string even though you already know you have a match?
/ravi
|
|
|
|
|
No. I would exit the loop.
There are only 10 types of people in the world, those who understand binary and those who don't.
|
|
|
|
|
And that's what example #1 does, not #2.
/ravi
|
|
|
|
|
Yes, but it returns out of the function, which is the actual debate. So, example 2, even though it has several issues, it does not exit the function, which is what I support.
However, what I would do is set the variable as in example #2 and then exit the loop.
There are only 10 types of people in the world, those who understand binary and those who don't.
|
|
|
|
|
ryanb31 wrote: However, what I would do is set the variable as in example #2 and then exit the loop. Ah, OK then.
/ravi
|
|
|
|
|
On top of everything else, you're post isn't even appropriate for this forum!
|
|
|
|
|
I'm a fan of early exit, so I'd go with the first one (assuming that's what this post is about).
It doesn't really matter much in smaller routines like the one above, but when you have longer ones it can be difficult to follow retValue around the function to find out where you are really setting the return value. The first method is shorter because you can run a case through in your head without having to write down variables.
I think the question boils down to Early Exit versus Single Exit. There's a lot of debate on the merits of both so I think your answers are going to be somewhat distributed between them. I've yet to hear a debate for single exit that I agree with over the merits of early exit...
|
|
|
|
|
Quote: it can be difficult to follow retValue around the function to find out where you are really setting the return value. That's funny. That's the same reason people usually argue for single exit, in that it is hard to figure out why some code isn't running because it turns out there was a return statement earlier that you hadn't noticed.
To each his own.
There are only 10 types of people in the world, those who understand binary and those who don't.
|
|
|
|
|
I also go mostly for early exit. This helps to keep the code to the left as much as possible.
Besides. if a method gets too long... you're doing it wrong
|
|
|
|
|
as per nasa specs, 60 lines on the most extreme atomic task, 15 for anything else.
I'm brazilian and english (well, human languages in general) aren't my best skill, so, sorry by my english. (if you want we can speak in C# or VB.Net =p)
"Given the chance I'd rather work smart than work hard." - PHS241
"'Sophisticated platform' typically means 'I have no idea how it works.'"
|
|
|
|
|
Quote: 15 for anything else. Serious? I have lots of stored procedures that take 20 or more parameters so just adding the parameters is more code than 15 lines.
There are only 10 types of people in the world, those who understand binary and those who don't.
|
|
|
|
|
i don't think they count the parameter list, i've read that on a doc that came on a CP newsletter long ago, but i personally don't like that much parameters
I'm brazilian and english (well, human languages in general) aren't my best skill, so, sorry by my english. (if you want we can speak in C# or VB.Net =p)
"Given the chance I'd rather work smart than work hard." - PHS241
"'Sophisticated platform' typically means 'I have no idea how it works.'"
|
|
|
|
|
I think an early exit is always best. If you don't have an early exit this can often lead to more nesting later on in the method to allow you to skip the rest of it and unessassary nesting leads to more complicated code. (You could argue that you should splt the method up in this case but I say keep logical units together.)
|
|
|
|
|
First of all, the second code should have a 'break' as follows. Otherwise, it just continues to loop pointlessly, even if it find "ABC" in the first item.
Boolean DoSomething(string[] values)
{
bool retValue = false;
foreach (string s in values)
{
if (s == "ABC")
{
retValue=true;
break;
}
}
return retValue;
}
Personally, I choose the this method over your first method, since it has a single point of return to the method. About the performance, both versions should be identical as this method would only execute 2 steps extra.
|
|
|
|
|
yours is the most sensible way to handle a single exit.
I'm brazilian and english (well, human languages in general) aren't my best skill, so, sorry by my english. (if you want we can speak in C# or VB.Net =p)
"Given the chance I'd rather work smart than work hard." - PHS241
"'Sophisticated platform' typically means 'I have no idea how it works.'"
|
|
|
|
|
So, this is certainly choosing between the lesser of 2 evils
If I HAD to choose one of both, the first example would be it, even if only for performance reasons - since the loop breaks when ABC was found.
But having said that... No. Just... No.
|
|
|
|
|
Code 1 would be my preference as it is the default indentation model for Visual Studio.
I don't see the point of re-setting the default model or turning off the auto-complete for the sake of a preference; I find that the usual combination of indents provides a readable and fast coding format which is pretty good at revealing errors and incomplete blocks due to items not aligning conventionally.
In other words, you can pick up errors in peripheral vision, which is quick.
|
|
|
|