Which code you suggest?
-
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...
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.)
-
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.'"
-
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.'"
-
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
Don't mind.
-
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 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.
-
Yes you can, but...a return is a lot, lot cleaner!
The universe is composed of electrons, neutrons, protons and......morons. (ThePhantomUpvoter)
... 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. -
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 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.
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.
-
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.
-
What if you have two nested loops? It's a lot harder to exit them both...
The universe is composed of electrons, neutrons, protons and......morons. (ThePhantomUpvoter)
throw an exception :laugh:
To alcohol! The cause of, and solution to, all of life's problems - Homer Simpson ---- Our heads are round so our thoughts can change direction - Francis Picabia
-
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.
if (!userGotHisNameRight)
Report It
return
if (!userManganagedHisAddressOK)
Report It
return
if (...)
...
Actual work the method is supposed to doThe alternataive being:
if (!userGotHisNameRight)
Report It
else if (!userManganagedHisAddressOK)
Report It
else if (...)
...
else
{
Actual work the method is supposed to do
}The universe is composed of electrons, neutrons, protons and......morons. (ThePhantomUpvoter)
Exactly. I share the same view - do validation and exit as soon as possible, to avoid crippling the logic down the road.
Interested in Machine Learning in .NET? Check the Accord.NET Framework. See also Sequence Classifiers in C# with Hidden Conditional Random Fields.
-
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.
if (!userGotHisNameRight)
Report It
return
if (!userManganagedHisAddressOK)
Report It
return
if (...)
...
Actual work the method is supposed to doThe alternataive being:
if (!userGotHisNameRight)
Report It
else if (!userManganagedHisAddressOK)
Report It
else if (...)
...
else
{
Actual work the method is supposed to do
}The universe is composed of electrons, neutrons, protons and......morons. (ThePhantomUpvoter)
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.
Interested in Machine Learning in .NET? Check the Accord.NET Framework. See also Sequence Classifiers in C# with Hidden Conditional Random Fields.
-
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
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.
-
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.'"
-
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.
Interested in Machine Learning in .NET? Check the Accord.NET Framework. See also Sequence Classifiers in C# with Hidden Conditional Random Fields.
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
-
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.
Interested in Machine Learning in .NET? Check the Accord.NET Framework. See also Sequence Classifiers in C# with Hidden Conditional Random Fields.
-
... 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. -
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
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.
CEO at: - Rafaga Systems - Para Facturas - Modern Components for the moment...
-
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
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.
