|
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.
|
|
|
|
|
he was talking about the exit model of the method, the indentation is just a incedent due to the html pasting.
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.'"
|
|
|
|
|
He didn't talk about anything. He simply presented code and asked for preferences.
|
|
|
|
|
I would go with #1. Number 2 should have a break statement and would work too.
Boolean DoSomething(string[] values)
{
bool retValue = false;
foreach (string s in values)
if (s == "ABC")
{
retValue=true;
break;
}
return retValue;
}
The signature is in building process.. Please wait...
|
|
|
|
|
Neither.
IMHO the best approach for a non-trivial function is to use a single return value declared and initialized at the start, along with a flag indicating premature abortion. Then, over the whole length of your function, no matter how long it is, you can always add that flag to every loop or (top level) if statement to prevent unnecessary execution of code.
In the example above, the return value can double as abortion flag: use it to prematurely break out of the loop to prevent unnecessary execution of code.
Personally, for any function with more than about half a dozen of control statements, I introduce a boolean variable called 'done' or similar that I use to short-cut later control statements. This way I don't normally need to increase the nesting level by more than 1.
|
|
|
|
|
The idea of adding an additional unnecessary variable for "control" is anathema to me. Think of it, you are wasting an assignation and then you are adding an extra comparison for each block that fails plus the one that's actually true. Put that into a function that repeat a few thousand times and you got yourself a waste of time well into the millisecond range if not more, and lot of extra work for the GC. I am not against flags, God knows i used them a lot, they are a valid control, but not in this case where you are using it as a sort of eufenism for a return
modified 27-Jul-13 23:11pm.
|
|
|
|
|
Renzo Ciafardone wrote: unnecessary variable
YMMV. If the alternative is goto , I choose the variable. If it is 15 layers of nested control statements, I choose the variable. If I know that anyone, including me, may be reading and trying to understand that code next month, I'm using a variable. I'm not saying to always use such a variable - only when it helps keeping the code clean and maintainable. IMO, the variable is sensible and necessary in these cases. (Also, these cases cover pretty much all the code I've worked on over the past 30 years)
Renzo Ciafardone wrote: you are wasting an assignation and then you are adding an extra comparison for each block that fails plus the one that's actually true
You are underestimating the efficiency of an optimizer: in most cases you won't even notice a difference, as the compiler will optimize away any variable that is only used sporadically.
The only exception is if there are multiple checks at the top nesting layer: then you need to add one condition to each top level check after the one that contains the 'abort condition'. The alternative would be just one check and moving the rest of the code down one nesting level. The former may have a minor impact on performance (but see below), the latter will always adversely affect code complexity, and thereby the likelyhood of bugs and the effort of maintenance. Your choice. In the past, I've tended to the latter. But now that I have to deal with that same code, I've decided - for my own benefit - to use the former.
Renzo Ciafardone wrote: you got yourself a waste of time well into the millisecond range if not more
I very much doubt that.
More importantly, even if it were true for one in a thousand or even one in ten (meaningful!) applications, don't design and write code under the assumption of the worst possible effect on performance, write under the assumption that you need to maintain and rewrite code often!
If you have an application with an expected lifetime measured in weeks rather than months. If that application is extremely performance-critical. If there is no meaningful numerical processing involved that may cause performance problems. If there is no external database, internet connection, file IO or just any IO involved. If you're using a compiler with a crappy optimizer. If you've profiled your code to show that the performance cost of adding that abort flag as an independent variable is raising your performance just over the threshold of acceptance. Then you have a good reason not to use one.
|
|
|
|
|
Wait a sec. When did i suggest to use a GOTO? I was saying that a RETURN is always a more readable and efficient alternative to a superfluous flag varible.
I too would use a flag if the alternative were a GOTO, there is no YMMV on this subject. For me GOTO is only acceptable in assembler. On any other language If an entanglement of If-elses requires a GOTO to get out, I would rewrite the damn thing because that sh*t is akin to blasphemy.
|
|
|
|
|
I didn't say you did, I just listed a number of possible scenarios, some of them beyond those you were considering. You asserted that introducing a flag is unneccessary, and I disagree: if you need to clean up resources, you can not immediately return ! In that case, what will you do: copy/paste the clean-up code at every location in your function where you need a premature return , use goto , or introduce a flag? For me, the answer is the latter.
|
|
|
|
|
Your function/procedure shouldn't be so long you need to do that ... perhaps you can pull each section into one of its own?
|
|
|
|
|
I agree, they shouldn't. But they are. Unfortunately most of these functions are way too complex to safely refactor.
Nevermind, I didn't want to hijack this thread into a discussion of maintaining old codebases - I just wanted to point out that any codebase may eventually develop into one. In that light, you have a point: if in your function you get to a point where you need multiple top level control statements (loops, branches), that may be an indication you should refactor this into multiple functions with less complexity.
|
|
|
|
|
It depends. There are times when it is necessary to exit as soon as a decision is reached, as I expect the performance of foreach to be O(n). For example, if a function is trying to find the first prime number within an array then you need to exit as soon as the condition is true, otherwise much time may be wasted. In the example given it depends how large I expect the string array to be, clearly the function itself can potentially take a string array of any size so method 1 seems preferable to me. However, if you don't like multiple return points then use 'break'. A more complex function is often clearer to read and debug if there is only one return point, and this is preferable if speed is not an issue. For example, if a function decides which way a bug is going to turn - left or right, then it may test many conditions, none of which may involve operations on large collections. In this example, it is probably preferable to have only a single return point.
|
|
|
|
|
|
If I were trying to determine if "ABC" exists in values then Code1.
If I were trying to determine if the last element in values (s) was equal to "ABC" then Code2.
I subscribe to early returns. Unless there is a reason to tenderize the value(s) before returning it.
|
|
|
|
|
Both and neither, i usually use what makes more sense in the task at hand, if the method is trivial or requires final cleanup, I prefer a single return point, if depending of a condition the code will run a lengthy or expensive task then I prefer early return.
|
|
|
|