Multiple returns from methods or clean code flow
-
I have just had a heated argument cage fight lively discussion with some of my team members about ReSharper's suggestion of refactoring code to replace nested ifs with a series of multiple early return statements. This caused horribly messy code that ReSharper actually described in it's help as "more readable"! Using a version of their example code (which is a lot simpler than the actual code in question):
void SomeFunction(SomeThing s)
{
if (s != null)
{
if (s.Thing != null)
{
// Do Some Process with s.Thing
.
.
.
}
}
}becomes:
void SomeFunction(SomeThing s)
{
if (s == null) return
if (s.Thing == null) return;
// Do Some Process with s.Thing
.
.
.
}This makes a complete hash of the natural flow of the code and introduces an execution statement (return) on the same line as the "if" which is bad coding practice, and then does it again, and then drops though to do some actual processing. If I was to refactor the code it would do this:
void SomeFunction(SomeThing s)
{
if (s != null && s.Thing != null)
{
// Do Some Process with s.Thing
.
.
.
}
}or possibly:
void SomeFunction(SomeThing s)
{
if (s?.Thing != null)
{
// Do Some Process with s.Thing
.
.
.
}
}Which is a lot cleaner! ...and this isn't even considering a method that returns a value of some sort. Opinions?
- I would love to change the world, but they won’t give me the source code.
What about 1 return and no braces
void SomeFunction(SomeThing s)
{
if (s == null || s.Thing == null) return;
//... more code
}Just put all your conditions into that 'if' clause. Too many conditions? Use a method to test the conditions and call that method from the 'if' clause Regards GregJF
-
I have just had a heated argument cage fight lively discussion with some of my team members about ReSharper's suggestion of refactoring code to replace nested ifs with a series of multiple early return statements. This caused horribly messy code that ReSharper actually described in it's help as "more readable"! Using a version of their example code (which is a lot simpler than the actual code in question):
void SomeFunction(SomeThing s)
{
if (s != null)
{
if (s.Thing != null)
{
// Do Some Process with s.Thing
.
.
.
}
}
}becomes:
void SomeFunction(SomeThing s)
{
if (s == null) return
if (s.Thing == null) return;
// Do Some Process with s.Thing
.
.
.
}This makes a complete hash of the natural flow of the code and introduces an execution statement (return) on the same line as the "if" which is bad coding practice, and then does it again, and then drops though to do some actual processing. If I was to refactor the code it would do this:
void SomeFunction(SomeThing s)
{
if (s != null && s.Thing != null)
{
// Do Some Process with s.Thing
.
.
.
}
}or possibly:
void SomeFunction(SomeThing s)
{
if (s?.Thing != null)
{
// Do Some Process with s.Thing
.
.
.
}
}Which is a lot cleaner! ...and this isn't even considering a method that returns a value of some sort. Opinions?
- I would love to change the world, but they won’t give me the source code.
Fascinating discussion ! While recognizing the special constraints of device driver, and down-to-the-hardware specific, programming, in a high-level language, like C#, with a powerful optimizing compiler, I favor multiple returns, and "defensive" error checking (throw immediately) wherever possible, asap. For me, "deeply nested" is for the (recursive) birds.
«Where is the Life we have lost in living? Where is the wisdom we have lost in knowledge? Where is the knowledge we have lost in information?» T. S. Elliot
-
If you have dealt with Unix fanboys, you'll now doubt have encountered the stupid "which hacky character shall we use for white space: spaces or tabs?". The correct answer is "whatever the team is using, I format it to that before committing". As for consistency, I only really am a stickler with fucking braces. I've seen too many recurrent bugs because a programmer is cute and makes a braceless if. Then the next one comes in, adds one more call and doesn't realise the braces are gone. Ooops, billion dollar bug introduced. Nasty and no need.
Oh I hear you on the *nix, but as often it's the text editor that mixes spaces with tabs; i.e. 1. inserts a tab ... but then some editors change it to spaces, some don't... 2. <8 spaces>, inserts spaces ... but then some editors change that to a tab, some don't and for more fun if it's an existing source file, some editors change only the edited lines to/from tabs and leave the untouched lines as they were, and some don't. Arrggh (doing some C programming on Linux, pulling down code snippets, getting lots of both tab and space.) So anyway don't blame the *nix fanbois, it's the [history of their] tools. Worked at one company, the system admin decided the standard tab size would be 3 spaces and forced everybody to set the vi editor to honor this - All fine, except when they got source from outside the company set with the standard tab size of 8. Talk about moaning on and on, and when I suggested they really should use the standard size that everyone else on the planet used, "oh no, we won't change, it's everybody else that should change." ... idiots. as to curly braces (and statements on same line as the if ()), as mentioned I'll normally only not use for very simple single statements, i.e. mostly return, break, continue, or perhaps a single method/function call on it's own. i.e. "if (x == 0) return;" it's pretty clear if you want to add a statement before the return that the curlies aren't there. anything longer (even if it's only a single function/method call but with many or long arguments) just like you I'll multi-line and wrap it in curlies for safety.) as said it's my style, not going to claim it's better or worse than another's style.
Message Signature (Click to edit ->)
-
I have just had a heated argument cage fight lively discussion with some of my team members about ReSharper's suggestion of refactoring code to replace nested ifs with a series of multiple early return statements. This caused horribly messy code that ReSharper actually described in it's help as "more readable"! Using a version of their example code (which is a lot simpler than the actual code in question):
void SomeFunction(SomeThing s)
{
if (s != null)
{
if (s.Thing != null)
{
// Do Some Process with s.Thing
.
.
.
}
}
}becomes:
void SomeFunction(SomeThing s)
{
if (s == null) return
if (s.Thing == null) return;
// Do Some Process with s.Thing
.
.
.
}This makes a complete hash of the natural flow of the code and introduces an execution statement (return) on the same line as the "if" which is bad coding practice, and then does it again, and then drops though to do some actual processing. If I was to refactor the code it would do this:
void SomeFunction(SomeThing s)
{
if (s != null && s.Thing != null)
{
// Do Some Process with s.Thing
.
.
.
}
}or possibly:
void SomeFunction(SomeThing s)
{
if (s?.Thing != null)
{
// Do Some Process with s.Thing
.
.
.
}
}Which is a lot cleaner! ...and this isn't even considering a method that returns a value of some sort. Opinions?
- I would love to change the world, but they won’t give me the source code.
Single ``return`` or multiple ``return`` I think is a potential code smell - based on design and code format agreements. I like the concept of having the guard statements and their ``return`` or ``throw`` exception small and tight at the beginning of a function/method. Putting the error handling into an ``else`` block can destroy the readability. We recently found a method in a work project where we were setting a ``results`` variable to ``true``, and then calling ``return false``. The last line of the method was to ``return results``. (Of course, no comments what team member that is no longer with the company was thinking.) This was in the logic and processing, not in guard statements. I think using negation logic can also be a code smell
void doSomething(var a)
{
if (a == null)
{
throw new Exception("a == null");
// or return with no meaningful "why"
}
else
{
//...
}
}is more readable than
void doSomething(var a)
{
if (a != null)
{
//...
// Many lines so that the error handling is separated from the test
//...
}
else
{
throw new Exception("a == null");
// or return with no meaningful "why"
}
} -
Great! The good old times never ended for me, partially because I would see it as a great loss if that old computer ever died, partially because such old tech is the is the only option to tinker around with as a hobby without getting poor. I'm certain you know what kind of equipment and knowledge is needed when you want to play around with the most modern stuff. It's more interesting to overcome the limitations of the old stuff anyway. Modern chipsets do that for you, so where is the point in puzzling them together as intended? Why don't you grab a Z80 on ebay and build a new old computer to bring back the old times?
I have lived with several Zen masters - all of them were cats. His last invention was an evil Lasagna. It didn't kill anyone, and it actually tasted pretty good.
There's actually some modern stuff that's inexpensive. Look at some of TI's Launchpads. They're on the order of $25 and the IDE they provide (Code Composer Studio) is free. It's based on Eclipse so it's pretty powerful. The last project I was working on before I retired (woohoo!!) was based off of one of their Launchpads. I was able to get a lot of the code working before we got our own hardware up and running.
-
There's actually some modern stuff that's inexpensive. Look at some of TI's Launchpads. They're on the order of $25 and the IDE they provide (Code Composer Studio) is free. It's based on Eclipse so it's pretty powerful. The last project I was working on before I retired (woohoo!!) was based off of one of their Launchpads. I was able to get a lot of the code working before we got our own hardware up and running.
sasadler wrote:
Eclipse
So we meet again, my old enemy! :)
I have lived with several Zen masters - all of them were cats. His last invention was an evil Lasagna. It didn't kill anyone, and it actually tasted pretty good.
-
sasadler wrote:
Eclipse
So we meet again, my old enemy! :)
I have lived with several Zen masters - all of them were cats. His last invention was an evil Lasagna. It didn't kill anyone, and it actually tasted pretty good.
Yeah, there is that. So far, I've only found 1 version of Code Composer Studio (CCS) that worked reliably for me (version 7.4), all the later versions I tried would work for a bit and then would stop letting me reliably debug my code through the jtag device. I'd have to close CCS and restart it to be able to reconnect to our hardware. Sometimes had to unplug the (USB based) jtag device and plug it back in to get it to start working again. I only used CCS for compiling and debugging, the rest of the time I used my own favorite editor to write the code.
-
I have just had a heated argument cage fight lively discussion with some of my team members about ReSharper's suggestion of refactoring code to replace nested ifs with a series of multiple early return statements. This caused horribly messy code that ReSharper actually described in it's help as "more readable"! Using a version of their example code (which is a lot simpler than the actual code in question):
void SomeFunction(SomeThing s)
{
if (s != null)
{
if (s.Thing != null)
{
// Do Some Process with s.Thing
.
.
.
}
}
}becomes:
void SomeFunction(SomeThing s)
{
if (s == null) return
if (s.Thing == null) return;
// Do Some Process with s.Thing
.
.
.
}This makes a complete hash of the natural flow of the code and introduces an execution statement (return) on the same line as the "if" which is bad coding practice, and then does it again, and then drops though to do some actual processing. If I was to refactor the code it would do this:
void SomeFunction(SomeThing s)
{
if (s != null && s.Thing != null)
{
// Do Some Process with s.Thing
.
.
.
}
}or possibly:
void SomeFunction(SomeThing s)
{
if (s?.Thing != null)
{
// Do Some Process with s.Thing
.
.
.
}
}Which is a lot cleaner! ...and this isn't even considering a method that returns a value of some sort. Opinions?
- I would love to change the world, but they won’t give me the source code.
I had to write some incredibly complicated business rules. Imagine that we charge a 3% fee. But there are like 19 situations where the fee is either more or less than that fee. The fees are also relative to that 3% fee, which is customer specific. I thought long and hard about how to write this to clarify the businesses intent, and used an infinite loop :-) to avoid the return, but I also had to have a STRING indicating which rule was applied for logging, as well as to generate the invoice.
while (true) {
// Comment Describing Primary Condition 1
if (condition1) {set %; set sMsg; break}// Comment Describing Primary Condition 2
if (condition2) {set %; set sMsg; break}// Calculate complex logic...
break; // prevent a loop
}
logIt("Rate Calculation",sMsg, % as string);
setup return variables
return;
}The upside of this code was that the comments, and the FLOW were ratified 3 times by the business side. The code went into production 20 years ago, and was never modified. Is this similar to the "many returns"? Yes, in a big way. There are many exits from the loop, but it captures the flow and priority perfectly. In fact, there was a decent amount of pre-loop setup code that calculated pieces to make the conditions make sense and be readable to the business people. While I am HUGE on coding standards and code reviews. There are ALWAYS unique coding situations where I will gladly throw them all out the window to produce Highly Provable code that is easy to process and find errors in!
-
It's an old lesson I learned, probably from the days of assembly -- always have one point of return, mainly for consistent stack cleanup. I do rarely make an exception (to that rule) but usually end up making some other change that removes the if. If you're doing parameter checking, as in your example, I tend to think it's better to throw an exception -- why should the function that's being called expect anything but valid parameters? I've seen
returnsprinkled throughout a function as part of the flow control. I hate that. Sometimes I don't see the return, set the breakpoint at the end of the function, and then have to steps through from the top and realize some moron tossed in an early return. I'd almost rather they use agototo the return, haha. Personally, I look at code like that and refactor it into smaller functions that have noifstatements, and the flow control occurs in a higher level function that doesn't do anything but call other functions based on conditions of previous functions or the data values. A lot more readable too when you separate out the flow control from the individual activities of each flow.Latest Article - Slack-Chatting with you rPi Learning to code with python is like learning to swim with those little arm floaties. It gives you undeserved confidence and will eventually drown you. - DangerBunny Artificial intelligence is the only remedy for natural stupidity. - CDP1802
Marc Clifton wrote:
If you're doing parameter checking, as in your example, I tend to think it's better to throw an exception -- why should the function that's being called expect anything but valid parameters?
Right on !
«Where is the Life we have lost in living? Where is the wisdom we have lost in knowledge? Where is the knowledge we have lost in information?» T. S. Elliot
-
One way into a method, one way out (not counting ref and out parameters, which are intuitively obvious). After 40+ years in software development, my tolerance for lazy programmers that just want to hack up some code and get the minimum done with the least time - is very low. Some software shops are starting to use an approach I have used for years when I hired developers. Instead of hiring 10 "full stack" developers with an eye to cheap labor, H1-B and other inexperienced programmers cranking out poor code that just barely works for today, they take a different road. For those 10 positions, they hire 6 experienced software engineers that understand how to code for value engineering, supportability, likely future needs, performance, readability, and meaningful comments. A 7th junior developer is hired who has an attitude and willingness to learn from the senior developers - a person whose character is teachable. No H1-B's. The end result is better code, faster development cycles, fewer bugs, and lower life cycle cost. Experienced software engineers understand how incredibly sophomoric it is to have multiple returns. Now get off my lawn! :)
MSBassSinger wrote:
my tolerance for lazy programmers that just want to hack up some code and get the minimum done with the least time - is very low.
A very strange ad hominem rationale for a personal prejudice applied to a technical issue.
«Where is the Life we have lost in living? Where is the wisdom we have lost in knowledge? Where is the knowledge we have lost in information?» T. S. Elliot
-
MSBassSinger wrote:
my tolerance for lazy programmers that just want to hack up some code and get the minimum done with the least time - is very low.
A very strange ad hominem rationale for a personal prejudice applied to a technical issue.
«Where is the Life we have lost in living? Where is the wisdom we have lost in knowledge? Where is the knowledge we have lost in information?» T. S. Elliot
Not strange if you make the connection between multiple returns and lazy programming. Then use that as an example of a broader problem in software engineering today. After 40 years in the business and Navy Nuc school, such observations are intuitive.