Which code you suggest?
-
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.
-
Code1:
Boolean DoSomething(string\[\] values) { foreach (string s in values) if (s == "ABC") return true; return false; }Code2:
Boolean DoSomething(string[] values)
{
bool retValue = false;
foreach (string s in values)
if (s == "ABC")
retValue=true;
return retValue;
}in the above 2 codes which code you will suggest and why? waiting for your valuable comments. Thanks --RA
Obviously you missed a break in your 2nd method (or I hope so). If you do this, it's actually a hybrid between the 2 methods. A more "strictly" no-goto (even hidden as a break / premature return) would be as per MarkTJohnson's code samples - i.e. add a check in the loop's conditions to see if the state variable has changed. As for those advocating premature returns, but (in the same breath) dissing goto: Such a return is very related to a goto, as is a break inside a loop. All 3 result in much the same assembly code, the only difference is return/break is safer to use than goto - since goto has more freedom to screw-up. It would be similar in stating that you should always use typed pointers, very true - but it's not as if an untyped pointer is actually "that" different, just possible to do more damage. Not to say that goto's should be used, just to point out that you're walking a fine line between a good idea and contradicting yourself. Also I'll give you the benefit of the doubt and not try to encourage to use the Contains (or other similar stuff) instead of the entire function, as some have already pointed out. With the thinking that you used this as a sample to make a point, not a sample to take literally. That said, I tend to find making the 2nd method work properly and efficiently becomes more code. And with more complex functions the extra code becomes exponentially more. Although I don't have trouble reading the principle itself (either one, as long as formatted so the state-set or returns are clearly highlighted, e.g. a blank line after each), I tend to try and write less code (if possible) - so I'd probably go with method 1 if no other reasons are apparent. This particular thing (i.e. premature return vs state variable) I see as subjective in the same sense that some would like / understand recursive more than iterative loops (or visa versa). If all else is the same - i.e. no need for other stuff to happen too just before the return. Where it does become a pain, is if your function is later extended. You might later add a further portion which should run even if the value is already found. Perhaps some side effect, like counting how many times a positive was returned through the object's life (sorry - stupid example, but it happens).
-
If we ever get around to refactor this, then maybe that is the way to go. But not anytime soon. When I said 'really old', I meant it: some of that code predates exception handling by a decade. Besides, there are plenty of good reasons not to use exceptions at every possible opportunity. E. g. I suppose you wouldn't suggest the use of exceptions in the case of the OP ;) Flags (or states, if you prefer), are perfectly valid mechanisms for keeping track of the state of your processing. They're definitely not the only way to handle this, but there is no real downside to them either.
Flags and/or gotos are both reasonable approaches in languages that don't have a try...finally construct and I've written code using both approaches in many different languages. Since the OP was obviously C# I assumed that is what we were talking about, and in C# the try..finally (or the "using" construct, when applicable) is definitely the cleanest approach to making sure your resource cleanup happens, even when you don't think exceptions enter into the picture, though in my experience most cases where resource cleanup happens, exceptions at the .NET framework level are almost always a possibility. And no I wouldn't suggest try..finally for the original post because no resource cleanup is involved.
-
I prefer the second method. I try not to have multiple places where a function can return.
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
My new year resolution: 2048 x 1536 Home | Articles | My .NET bits | Freeware ravib(at)ravib(dot)com
-
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 c
:confused: 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.:confused: 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 shit is akin to blasphemy. :wtf:
-
ryanb31 wrote:
I prefer the second method.
You can't be serious! /ravi
My new year resolution: 2048 x 1536 Home | Articles | My .NET bits | Freeware ravib(at)ravib(dot)com
-
:confused: 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.:confused: 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 shit is akin to blasphemy. :wtf:
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 prematurereturn, usegoto, or introduce a flag? For me, the answer is the latter. -
Code1:
Boolean DoSomething(string\[\] values) { foreach (string s in values) if (s == "ABC") return true; return false; }Code2:
Boolean DoSomething(string[] values)
{
bool retValue = false;
foreach (string s in values)
if (s == "ABC")
retValue=true;
return retValue;
}in the above 2 codes which code you will suggest and why? waiting for your valuable comments. Thanks --RA
Second method if security is a concern (methods like that are not vulnerable to timing attacks[^]); first in all other cases (the first method will always execute in N time, the second is O(N)).
He who asks a question is a fool for five minutes. He who does not ask a question remains a fool forever. [Chinese Proverb] Jonathan C Dickinson (C# Software Engineer)
-
Flags and/or gotos are both reasonable approaches in languages that don't have a try...finally construct and I've written code using both approaches in many different languages. Since the OP was obviously C# I assumed that is what we were talking about, and in C# the try..finally (or the "using" construct, when applicable) is definitely the cleanest approach to making sure your resource cleanup happens, even when you don't think exceptions enter into the picture, though in my experience most cases where resource cleanup happens, exceptions at the .NET framework level are almost always a possibility. And no I wouldn't suggest try..finally for the original post because no resource cleanup is involved.
Ok, lets forget language (old C) specific concerns: Assuming you have exceptions, yes, I agree that you can reasonably handle many cases of premature returns that way. However, my take on exceptions is that code running as expected shouldn't throw one! Finding an element with specific properties in a container does not warrant throwing an exception, whether or not you need clean-up. Look at the following code, ignoring language specific elements:
// find first elementwithin tolerance of a given value
// return element ID
int find(Container c, double value, double tol)
{
int result = INVALID_ID; // some constant that is not a valid element ID
do_some_intialization();
for(size_t index = 0; index < c.size(); ++index)
{
if (element.distance(value) < tol)
{
result = element.ID();
}
}
do_some_clean_up();
return result;
}There are various solutions to short-cut the loop once it finds a fitting element: 1. use some command that breaks out of the innermost loop (in C/C++ you can use
break) 2. attach the check(result==INVALID_ID)to the loop header, so it quits once you assign a valid ID. (not sure how to do that withfor_eachin C#, but a standardforloop lets you add an arbitrary number of stop conditions easily) 3. Introduce a flag variable that indicates when you're done searching. As it would pretty much just store the current value of(result==INVALID_ID)in this case, you might as well go with solution 2 above 4. throwing an exception, catching it with try/finally to ensure proper cleaning up In this example, my suggestion of introducing a flag variable turns out to be unneccesary, as the result variable itself can be used to pretty much the same effect. In my experience, this is often the case, so the effort to use these kind of checks instead of prematurereturns orgotos is often rather low. Using an exception would certainly work, but it conflicts with my understanding of what an exception means. Also, if you do catch actual error cases with exceptions within that same code, you need to make sure to not catch the 'good-case-exceptions' as errors! If you have no problem with that, the more power to you. But I prefer not to go that route. -
Ok, lets forget language (old C) specific concerns: Assuming you have exceptions, yes, I agree that you can reasonably handle many cases of premature returns that way. However, my take on exceptions is that code running as expected shouldn't throw one! Finding an element with specific properties in a container does not warrant throwing an exception, whether or not you need clean-up. Look at the following code, ignoring language specific elements:
// find first elementwithin tolerance of a given value
// return element ID
int find(Container c, double value, double tol)
{
int result = INVALID_ID; // some constant that is not a valid element ID
do_some_intialization();
for(size_t index = 0; index < c.size(); ++index)
{
if (element.distance(value) < tol)
{
result = element.ID();
}
}
do_some_clean_up();
return result;
}There are various solutions to short-cut the loop once it finds a fitting element: 1. use some command that breaks out of the innermost loop (in C/C++ you can use
break) 2. attach the check(result==INVALID_ID)to the loop header, so it quits once you assign a valid ID. (not sure how to do that withfor_eachin C#, but a standardforloop lets you add an arbitrary number of stop conditions easily) 3. Introduce a flag variable that indicates when you're done searching. As it would pretty much just store the current value of(result==INVALID_ID)in this case, you might as well go with solution 2 above 4. throwing an exception, catching it with try/finally to ensure proper cleaning up In this example, my suggestion of introducing a flag variable turns out to be unneccesary, as the result variable itself can be used to pretty much the same effect. In my experience, this is often the case, so the effort to use these kind of checks instead of prematurereturns orgotos is often rather low. Using an exception would certainly work, but it conflicts with my understanding of what an exception means. Also, if you do catch actual error cases with exceptions within that same code, you need to make sure to not catch the 'good-case-exceptions' as errors! If you have no problem with that, the more power to you. But I prefer not to go that route.I think you've completely misunderstood what I said. I am in no way advocating introducing new exceptions as a way of prematurely ending a loop. What I am advocating is using language constructs (try...finally or using(...)) specifically designed help ensure safe cleanup regardless of how you exit the function/procedure. It's simply cleaner and easier to get it right than using other methods, especially when there is the possibility of exceptions, which it turns out is almost always when using resources that need to be explicitly cleaned up in languages that provide those constructs.
-
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? :confused: /ravi
My new year resolution: 2048 x 1536 Home | Articles | My .NET bits | Freeware ravib(at)ravib(dot)com
-
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? :confused: /ravi
My new year resolution: 2048 x 1536 Home | Articles | My .NET bits | Freeware ravib(at)ravib(dot)com
-
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
My new year resolution: 2048 x 1536 Home | Articles | My .NET bits | Freeware ravib(at)ravib(dot)com
-
And that's what example #1 does, not #2. /ravi
My new year resolution: 2048 x 1536 Home | Articles | My .NET bits | Freeware ravib(at)ravib(dot)com
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.
-
Code1:
Boolean DoSomething(string\[\] values) { foreach (string s in values) if (s == "ABC") return true; return false; }Code2:
Boolean DoSomething(string[] values)
{
bool retValue = false;
foreach (string s in values)
if (s == "ABC")
retValue=true;
return retValue;
}in the above 2 codes which code you will suggest and why? waiting for your valuable comments. Thanks --RA
Neither; I prefer not to end a
foreachwhen aforwill work just as well (if not better).bool result = false ;
for ( int i = 0 ; !result && i < values.Length ; i++ )
{
result = values [ i ] == "ABC" ;
}return ( result ) ;
This could also lead to a discussion of ifless programming. :-D
-
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
My new year resolution: 2048 x 1536 Home | Articles | My .NET bits | Freeware ravib(at)ravib(dot)com
-
But you're missing the fact that you can exit a loop when you find what you need. You don't have to continue processing.
There are only 10 types of people in the world, those who understand binary and those who don't.
Just because you can doesn't mean you should.
-
Just because you can doesn't mean you should.
-
... until you introduce code that needs clean-up at one point or another. Many of the functions I look at every day are a decade old or more, and consist of several hundred lines of codes with half a dozen levels of nesting or more. Every single one of them allocates stuff, or does something else requiring cleanup. More often than not, this happens before something else happens that necessitates a premature return. Some of the really old functions use
goto exit;to immediately jump to the cleanup code. I use a flag. Sure, not everyone works on such a codebase. But the truth is, the majority of programmers doesn't work on brand-new projects either. 80% work on internal programs designed to improve certain processes inside of a single company. Lots of code, and sometimes with a lifetime higher than that of some of their current programmers. In that context, IME, premature returns are almost always a bad idea.Stefan_Lang wrote:
premature returns are almost always a bad idea
:thumbsup: Definitely a code smell.
-
Code1:
Boolean DoSomething(string\[\] values) { foreach (string s in values) if (s == "ABC") return true; return false; }Code2:
Boolean DoSomething(string[] values)
{
bool retValue = false;
foreach (string s in values)
if (s == "ABC")
retValue=true;
return retValue;
}in the above 2 codes which code you will suggest and why? waiting for your valuable comments. Thanks --RA
I guess, I would try something like:
Boolean DoSomething(string[] values)
{bool retValue = false; if (values != null) for(int i = 0; i < values.Count(); i++) { if (values\[i\] == "ABC") { retValue = true; i = values.Count(); } } return retValue; }I know the example is a simple loop, but thinking in maintenance and performance this will: - Avoid the internal context and memory usage of a FOREACH (FOR is recommended when u do only 1 single access to the object[i]) - Use of a state variable for a return is recommended rater that having a lot of returns. (readability?) - The Return in the for or foreach cause a BREAK, if I'm not wrong that was expensive in the past, not sure with modern languages.