|
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.
|
|
|
|
|
One way in, one way out and we stop when we find the value we want.
Boolean DoSomething(string[] values)
{
bool bResult = false;
int i = 0;
while ( (!bResult) && (i < values.GetLength()) )
{
bResult = (values[i++] == "ABC");
}
return bResult;
}
OR
Boolean DoSomething(string[] values)
{
bool bResult = false;
int i = values.GetLength();
while ( !bResult && i )
{
bResult = (values[--i] == "ABC");
}
return bResult;
}
Just realized with the my second one, if the array is empty it still works. The first one does too but I like the second better and will try using that logic from now on. Thanks for making me think.
M__k T J.hnnnnn
What my signature looks like in courier.
modified 26-Jul-13 11:11am.
|
|
|
|
|
I refuse to hire anyone who subscribes to that inane single exit nonsense.
|
|
|
|
|
Oh, you must be lots of fun to work with.
The "correct code"? The "correct code" is something that is easy to maintain and produces the desired result.
In addition your comment does not address the original question. Which is better, multiple returns or a single return? As a maintenance programmer, I fully support a single return statement. Multiple returns are lazy and prone to creating code that doesn't get tested before getting sent to QA.
Granted the example was trivial but the underlying question was not.
|
|
|
|
|
The code you posted with a complex condition and the loop variable incremented inside the assignment is much less readable than returning from inside the loop to me and, I suspect, most people.
|
|
|
|
|
I'm a pleasure to work with because I know my craft and I write readable, maintainable code, not the kind of garbage that was posted here ... a result of a) harebrained adherence to a pointless single-return dogma and b) not being familiar the available tools, such as LINQ, and not being familiar with the state of the art, which now includes the sort of functional programming that LINQ represents. Functional programming is the ultimate in "single return", because there are no return statements, there are just functions that yield expressions. But if you're writing crufty imperative code, single return is bad because a) several empirical studies show that it has high cognitive load, making it difficult to interpret and subsequently more likely have bugs in the original and more likely to be broken when changed, and b) it results in the Arrow Anti-Pattern (http://www.codinghorror.com/blog/2006/01/flattening-arrow-code.html).
Multiple returns are lazy and prone to creating code that doesn't get tested before getting sent to QA.
Oh, you must be a joy to work with as you dishonestly defend positions by making up BS arguments. Laziness and failing to unit test has nothing whatsoever to do with single vs. multiple returns (but it does have a lot to do with people who don't read the literature and so know nothing about the technology of software construction). Multiple returns in guard clauses as mentioned at codinghorror are not "lazy", they are orderly and logical. It's not laziness that leads good programmers to avoid unnecessary structure and condition variables, it's competence.
|
|
|
|
|
At the risk of further baiting the troll, man you take this stuff WAY to seriously. "Garbage"? "Harebrained adherence"? "Not being familiar with the state of the art"? "But if you're writing crufty imperative code"? Dude, you need to get out more, seek some kind of relaxation therapy because you are headed for a coronary if you don't find an outlet for your anger.
Did I insult you? Maybe with the "joy to work with" comment, but really, your first statement was "I would never hire anyone . . ." Someone asked a question that related to opinion. I responded with my opinion, which is just as valid as every other opinion that has been posted in response to the original question.
Heaven forbid I assume anything about you but, how much maintenance programming have you done in your career. Real maintenance programming of a system you didn't start but was hired on well after version 1 or even 2 had shipped? In that world, you play with the hand you are dealt, and on more occasions than I care to remember I have had to deal with code that had NOT been tested before being sent to QA because the programmer just threw in multiple return statements because they ASSUMED they knew how the user would interact with the system. Code goes to the field and the user does something different than what the programmer thought the user would do and BOOM goes the code. The else condition was never tested because the programmer only tested with "good" entries.
It is a mistake we ALL make. We make this mistake because we write the code and know what it SHOULD do and get locked into the mindset that "the user will only try to enter the correct information like I have been testing because that what makes the code work and produce the result the users want." Negative testing and single exits can simplify the maintenance of the code long after the original developer has moved on.
You don't have the luxury of rewriting every module you come across to meet your coding standard, you have time to fix the bug and move on to the next one, after of course you thoroughly test your changes and make sure the changes you made didn't break something else. That something else might not even be in the file you are modifying.
So in conclusion, jibalt, you will find over time that life is too short to worry about this stuff and code is just code. Write it so others can see what is going on because in 6 months from now you will have a hard time remembering why you did what you did when you look back at something you thought was so elegant. That is competence AND experience talking.
Mark T Johnson
Old School Programming
K.I.S.S. because you will probably be the S. who has to go back into that file and fix something.
|
|
|
|
|
|
I debated last night when I saw your comment. Should I respond to this person who waited 5 months to flame me over a lengthy post about life being too short?
Then my curiosity got the better of me and I decided I had to find out why.
So, why, jibalt, why did it take you 5 months to respond to my post? Why after 5 months were you compelled to once again call me a pathetic hypocrite? Every post you have made towards me in this thread has been filled with venom and vitriol. What is it about me that brings out that much anger from you?
5 months. Sounds about right for a jail sentence. Is that where you've been? I can see you having road rage or being in a bar fight or beating up your significant other.
Oh, my blood pressure was quite normal when I wrote that last post. Because you see, I enjoy poking the troll.
|
|
|
|
|
I refuse to hire anyone who subscribes to that inane single exit nonsense. And that code is awful for other reasons too. The correct code is
return values.Any(s => s == "ABC") unless it can be proved to be a performance bottleneck.
|
|
|
|
|
That may be OK for C#; but how does it translate to other languages?
|
|
|
|
|
Modern languages these days have similar functional facilities ... this includes C++11, and even Java finally in Java 8. Others are Ruby, Python (although limited to one line), Scala, Haskell, Clojure of course ...
|
|
|
|
|
I prefer #3 offered by "NeverJustHere":
return values.Contains("ABC");
Each will get the same result, except #2 wastes unnecessary cycles. #3 is best because it is the most efficient, uses the least about of your code, and is already debugged (hopefully MS did their job).
The example in #2 can be made more efficient if you change it to:
Boolean DoSomething(string[] values)
{
bool retValue = false;
foreach (string s in values)
if (s == "ABC")
{
retValue=true;
break;
}
return retValue;
}
-- modified 27-Jul-13 0:03am.
|
|
|
|
|
It's tragic that anyone would ask this, since the second chunk of code is so clearly inferior.
But I would of course write
return values.Any(s => s == "ABC")
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
