your suggested refactorings, whilst making the code read better, won’t actually reduce cyclomatic complexity
#1, #2, and #4 definitely will reduce cyclomatic complexity-- less IFs are nested, therefore there are fewer total paths through the code. The total number of IF statements might be the same, but they are no longer nested or they’ve been moved to another function (and no longer nested, at least not in the original function).
The suggestion to combine multiple IF statements on the same line, ala
if (something || somethingelse || somemore) {
Won’t reduce CC at all. But that’s not on my list…
has anyone used those plus some coding guidelines about logging every public method call?
Why not just dump the stack trace at any given time rather than doing explicit logging? The stack trace will give you the current method for sure…
Love this post and the discussions it has brought up. What are your thoughts, in an IF statement, on testing the scenario that is likely to happen most of the time? Does that matter for complexity sake or for readability? I mean this way you can avoid going through the rest of the code most of the time but what if it lessens the readability or checking for a negative versus a positive. Below is a very generic scenario but I think you know what I mean.
Thanks
if(!newsletter.subscribe)
{
// code to unsubscribe
// occurs 90% of the time
}
else
{
// code to subscribe
// occurs 10% of the time
}
What are your thoughts, in an IF statement, on testing the scenario that is likely to happen most of the time? Does that matter for complexity sake or for readability?
Can you implement this check as a guard clause rather than an if…else statement? If so, do that.
I feel very strongly that non-negative if clauses are easier to read, but it’s just a guideline. If there’s some extremely compelling reason to do it the other way, then use your discretion.
Assertions is a lot better solution than refactoring the same code through the same function. Assertions, with tests cases, can guarantee you that your function parameters are good. You then don’t have to code all those nasty ‘if’ and ‘else’ to test the validity of your parameters. As your parameters have always a good value, you can code functionality straight.
Someday we’ll all have huge screens where the entire source code for any major project will all fit on one screen. Have some imagination!
How many acres of screens for the source code to Windows Vista? What new hardware technology do we need to see the whole thing just by moving our eyes and head?
Thank goodness! The experience of practice defeats the supposition of theory.
I’ve written some HUGE procedures (not recommended), A Expression Evaluator routine that creates the Stack. For some reason when I first started, I was insistent on that single exit BS. I soon realized that it was making my code the very opposite of what the method was supposed to do. FAR less readable. My first few iterations were created in a flurry of programming inspiration, and where thus devoid of comments. So When I went back to the code later on I couldn’t figure out why execution remained in the function so long after the return value was decided. Very little clean-up code was necessary, which is to say it would all be done anyway by VB. Needless to say, I reverted to the both more readable and faster get the heck out of there as fast as possible idiom. No problems since then.
A return is effectively:
set_return_pointer(value);
goto where_i_was;
And has one of the problems of gotos: you can’t see where the goto/return comes from until you find the goto/return statement. The improvement on gotos is that you can’t goto the middle of the code block, only come from it.
So, although I see that multiple returns can make for simple code, so do gotos, so the result is similar. If you keep your function simple (as you really, really should) then the gotos will be clear, and you don’t have to worry too much.
Jeff, I completely agree with the approach. Infact I have been religiously encouraging guys around me about this for ages.
Validate the application state in which a particular task cannot be done. All such validations if they fail, they will raise an error or if logically the task cannot be done, just return.
I personally am sick of seeing such ! written in code, why cannot a developer write explicit comparisons with true or false… here I had to think for a while what the statement is trying to do, then I found out that it is actually
The number of times I have had to fix (other peoples) code when they have added a return which then skips code which must be run every time or someone has code which must be run every time but has missed that there is a return somewhere in the method. Even when there is only half adozen lines or so of code. Even when the coder is experienced.
The single exit point is there to reduce complexity. I believe it enhances readibility especially if combined with design by contract principles (see Bertrand Meyer’s work on this). As can be suggested by design by contract, if the method is called in an incorrect state then contract is broken and exceptions can/should be thrown.
Of course, you can argue that this can be checked for by TDD, however, I think that throwing out decades of software engineering practices for a more agile one is being too black and white. As an industry we need to integrate the best practices each (and many other sources) have to offer.
Additionally, if you need to jump out of a method mid-stream to make it clearer then I would assert that the method is not clear enough in the first place and needs refactoring.
BTW, I am completely amused that the Captcha words I am about to enter are ‘abstain’ and ‘backlash’. Surely I will get some backlash for my belief that we should all abstain from implementing point 4!!!
Agree with most of your post but have an issue with rule #3.
=====================================================
if (Attributes[attrVal.AttributeClassId] is ArrayList)
{
// if this is many lines of code …
}
else
{
// … the exceptional circumstance can be easy
// to miss
}
For this reason, I put the exceptions to the rule up front - which is pretty much like your rule #1, except I’m not talking about exceptions in this case - just exceptional circumstances (like boundary conditions)
=====================================================
if (Attributes[attrVal.AttributeClassId] is NOT ArrayList)
{
// deal with the exceptional circumstance
// (nothing to do with exceptions)
}
else
{
// deal with the standard circumstance
}
I originally stormed into the coding world throwing guard clauses and multiple function returns out left and right to cut down on those darn curly braces.
Then I would be forced into debugging a problem. And suddenly those exit points made the ol standby of println() retVal pretty tough to pull off.
So my “rule” looks a bit like this: If the function fits on a less than a dozen lines (and they do, 95% of the time) then multiple returns are A-OK. Once you get into some of the ganglier methods, you can have one or two guard clauses, but everything after than has to have a single return value so you can debug easier.
While I’m rambling about guard clauses, has anyone used those plus some coding guidelines about logging every public method call? Where do you put the log statement? If you enter the method and nothing happens, then it’s a bit misleading to write it to the log on the first line of the method. But if you try to figure out where the log statement goes, you spend extra time figuring out where to actually log that something happened. I guess I could write more statements, but at some point you end up logging every line of code.
Why not just dump the stack trace at any given time rather than doing explicit logging? The stack trace will give you the current method for sure…
Because that requires debugging symbols to be deployed with your assemblies, which isn’t necesarily a bad thing IMO, but it’s a constraint nonetheless. I posted my opinions about deploying pdb’s here: http://jaysonknight.com/blog/archive/2005/11/09/2900.aspx