Recently I was part of a conversation debating the value of sticking to MISRA guidelines for C development in automotive applications. First off I’d like to submit that the notion of following any particular guideline exactly and without exception is usually not a good idea. The particular rule I’d like to discuss is 17.4.(No more than one return statement)
Multiple returns are bad m’kay. MISRA rules and others like it can make us feel like we’re being told how to code because we’re not smart enough to do it right. We have to understand that in an embedded environment there is no recovery from errors and resets are not acceptable, ever. Rule sets like MISRA help us cover the last mile of quality.
Let’s look at a simple example of multiple return statements.
multiRet(){ if(a){ if(b){ if(c){ return 2; } }else{ return 1; } } return 0; } |
That’s pretty simple, but it can be even simpler, and simplicity is the path to zen.
singleRet(){ int rt=0; if(a){ if(b&&c){ rt=2; }else{ rt=1; } } return rt; } |
That’s a lot simpler isn’t it? One trick on getting from there to here is to make a truth table based on requirements:
| input | output | ||
| a | b | c | x |
| 0 | 0 | 0 | 0 |
| 0 | 0 | 1 | 0 |
| 0 | 1 | 1 | 0 |
| 1 | 0 | 0 | 1 |
| 1 | 0 | 1 | 1 |
| 1 | 1 | 0 | 0 |
| 1 | 1 | 1 | 2 |
I can’t tell you how many times a truth table has helped me identify issues in my own code and in code reviews. This is also a smashing example of why you should write tests first. Writing test first will lead directly to the simplest solution. “Let the code tell you what it wants to be” – Mike Karlesky
Ignoring MISRA rules is like riding a motorcycle without a helmet on. I know I can ride my motorcycle safely, but wear a helmet because it’s everyone else I don’t trust…and I’m not perfect anyway. Many of the MISRA rules are there for code readability and maintainability. They are there to help us avoid a situation where a bug is injected. If we all follow the rules it makes the overall project safer to edit and maintain.
Often times in supplied code I find MISRA violations with copious comments about how a particular violation is “ok this time”. It’s not enough to inspect your compiled code to see that a MISRA violation is safe. The violation stands even if the violation itself does not cause a bug today. This is because a bug might be created tomorrow when someone else changes the code.
Sometimes multiple returns are used as a parachute to jump out of a function before doing any further processing (can be called a guard statement). Guard statements are fine outside of the embedded world, like when you’re checking an argument list. In an embedded environment I just don’t see the utility. If multiple returns are being added to a function that is complex and not well understood, it’s time to refactor.
Some rules are made to be broken, but for many MISRA rules I believe it’s a rare case and a case that can be hard to justify.
Taking an immutable solution and turning it into a solution that requires a variable to mutate seems to be a downgrade in robustness to me. Mutability is the new “global” or “magic constant” or name-your-own bad practice in today’s multithreaded world. Functions where everything are constant are easier to follow and are less susceptible to breaking changes. A nice, non-mutating solution with one return statement:
f(a, b, c) {
return a ? b && c ? 2 : 1 : 0;
}
That’s an interesting code snippet, but I’m not convinced that my example has thread safety issues. The local variable I introduced is thread safe. I would expect an optimizing compiler to produce the same code in either case. While I’m not totally against the use of conditional operators I do think they are less readable under these circumstances.
Since a thread has it’s own stack a mutable local variable is thread safe. If for some strange reason you used a global mutable variable then it would not be thread safe.
Nick’s example clearly uses a local variable.
Yes this case is thread-safe, but even threadsafe local mutations in a larger function will still cause headaches when you try to introduce new functionality. Keeping things constant in my experience almost always leads to more maintainable code. I also agree the chained ternary operator is a bit ugly, but I guess that’s the best you can do in C. Ultimately I simply think that the MISRA guidelines in this case are simply wrong: the original version with the multiple return statements is the most legible and the most maintainable.
As I see it, 17.4 was simply made before the recent recognition of mutability as a source of many problems. These days it should be qualified by “unless it forces you to mutate a variable”.
Regardless, I like your extrapolation of the example into a truth table. Since you like this approach you should check out a variant of the ML family of languages; it makes the truth table super-obvious:
match (a, b, c) with
| (true, true, true) -> 2
| (true, _, _) -> 1
| (false, _, _) -> 0
Finding embedded ML compilers can be hit-and-miss, but I’ve found that ML in general leads to much safer and more legible code. My thought is that the MISRA C guidelines should be short and sweet: “Don’t use C”. :)
The value of this particular MISRA C rule became apparent to me when I went back to add a feature to a previously written module. When there are multiple return statements each path has to be evaluated and the new feature may require new code in multiple paths. In the example above, with the single return statement, new logic can be added at the end of the module and, if needed, the return value can be easily checked by the new code.
@Nick
Your original multiRet example was convoluted to start with:
multiRet(){
if(a){
if(b&&c){
return 2;
}
return 1;
}
return 0;
}
It’s 2 line shorter than your example….
Or have you never seen a 10-level deep nesting because someone was trying to have a single return point?
> Guard statements are fine outside of the embedded world, like when you’re checking an argument list. In an embedded environment I just don’t see the utility.
That’s not even funny: take a look at Linux kernel which powers lots of embedded devices out there…Apparently, linux hackers do see their utility..
@Craig
Indeed, single return statements make some things simpler (like adding code at the end). They also make other things harder (like add an early bail-out)… Ultimately, to use single-return, programmers invariably either use deep nesting (which is far more disgusting IMHO) or status variables (not as disgusting but still clumsy)..
@ilya
I think that if you have 10-levels of nesting, it would serve you well to break up your function into smaller more manageable pieces.
It’s true that the use of guard statements in linux code is a common, and useful pattern. However, I define an embedded device as one that does not have an MMU and cannot run “user” programs. The MISRA rules only apply to these types of embedded devices.
If you refactor relentlessly (which tests enable!) you will find that functions rarely get as long as 15 lines. Once you reach this point the function is so simple you can understand it no matter how many returns. (I’m assuming standard formating where there is one statement per line).
When I have to deal with 30,000 line functions in a class with 250 poorly named member variables (I wish I wasn’t making this up) nd no unit tests – I need all the help I can get. There single return helps. (I left that job before I was able to fix that one, but nearly everyone has code nearly that bad someplace)
Write better code and that MISRA rule could go away. However when you are writing better code you look at the example given and refactor it. At that point it doesn’t matter much.
The 2 programs that you have given dont get the same output when you use try them with a=1,b=1 and c=0
The first program gives it as 0 where as the second one returns 1.
Regards,
Shiva Komuravelly.