Home » Programming Best Practice » Avoiding Code Complexity

Avoiding Code Complexity

clip_image001A couple of weeks ago, I talked a bit about how we name things.  Specifically, I talked about the very bad habit of using the variables i, j, and k as variables in our for/next loops.

A few weeks before that, I talked about DRY Programming and the fact that not repeating ourselves extends much farther than most of us normally think when we are thinking about our code.

Today I want to continue on the general theme of code quality by discussing code complexity.

What is it?

Generally, code complexity is anything you introduce into your code that makes it hard to follow.  But here are a few areas you might look for code complexity.

Do you have REALLY long methods?

Here’s the deal.  The longer your method is, the more work it will be to keep track of the overall point of the method.  So, you want to keep the number of lines in your method low.  If you are doing too much, when you come back to your code, even you will be confused.

I remember when I first started programming.  We only had 25 lines for a full screen, and the rule was, you should be able to see the full function or method on one screen full of code in your editor.  The problem with that metric now is that our screens have gotten capable of showing a lot more code.  It’s a lot like telling a new driver, he should be able to still see the license plates of the car in front of him when he stops behind a car.  Some cars exist where my tiny little Civic would be IN the other car and I’d still be able to see the plates.

A better metric would be to use the 7 +/- 2 rule.  Ideally no more than 7 lines of functional code per method.  If you have to, you can fudge it to up to 9, but no more.  This is because the human brain can only deal with about 7 items at a time.

I can think of a few times when you might want to break this rule, like when you have a set of conditions that really need to all be in the same place for them to make sense, but it wouldn’t hurt to try to keep to the rule as often as possible.

Do you have a lot of conditions within one method?

When I was discussing the problem with using i, j, and k as variables, I kind of mentioned this, but I didn’t dwell on the subject a lot.

You see, the story I told when I was telling you all about the i, j, and k problem violated all of the readability rules.  First, it was using the wrong variable names.  Second, the method was MUCH too long.  And third it had too many conditions.

You might think that only if/else statements are conditions.  But so are for/next, while, do/while, and switch statements.  As much as you can, your code should be setup so that you only have one condition per method.  Three at the most.  Again, switch statements might loose their context, but here, I would have one method that handles the switch statement and only one line per case statement.  Each case statement should call a function that does the real work.

There is a tool in Visual Studio 2013 that will help you determine how bad your methods are.  You can calculate metrics for a project or the entire solution and it will generate a table of the Cyclomatic complexity of your code.  Look for methods that have a Cyclomatic complexity of over 10 and try to bring them down.  The closer to  zero you get, the better.  Many people suggest that we keep this number below 10 or 11, but this is just an opinion.  I would rather just say look at what you’ve currently got and strive for better.

If you have Visual Studio, this option is under the Analyze menu option.

Do you have a lot of operations, function calls, parameters to those calls?

What I’m basically talking about here is what’s called Halstead Volume or Halstead Metrics.  What this computes is how complex the code is.

For example, if you have five lines of code and a Cyclomatic complexity of ten as everyone suggest, your code may still be in trouble because each line of code is so complex that no one on earth could possibly understand it.

We call this self obfuscating code.

You probably have some superstar on your team that thinks he’s so hot that he codes an entire function on one line.  The problem with this is that six months from now, neither he nor you will be able to figure out what the code does.  Any fix that will be needed will require an entire rewrite of the code.  That’s exactly what we are trying to prevent.

If you are using Visual Studio 2013 Ultimate, you can get the Microsoft Codelens Code Health Indicator which adds the ability to check for all three of the above problems for each of your methods.  If you pay attention to it, it will help you make code that is easier to understand.

Can You Easily Find Code You Need To Maintain?

This is one that is harder to detect, but I thought I’d mention it here because it is a real issue.

In an ideal world, we shouldn’t need to use a debugger to track down where the code is that we need to modify.  There are cases where this may be the only solution.  But you should know, for example, that all of your validation code is in this one location.  If you have validation code in multiple locations, you are probably thinking about your code incorrectly.

But also cut yourself some slack because this kind of code complexity is not something you are going to notice until late in your project.  You may never see it, but you will certainly recognize it when you run into it in someone else’s code.

Which reminds me of a story.

There was this guy that was working in a cube and he suddenly starts ranting.  “Who wrote this code?!  This is the dumbest code I’ve ever seen.  etc…”  Suddenly he got real quiet and the guy in the next cube asked him, “hey, Joe, what’s wrong?”  Joe replies, “It’s my code.”

So even if you don’t pay attention to these issues for your coworker’s sake.  Do it for yourself.

 

Other post in Programming Best Practice

Related Post

  • Raking Leaves and Writing CodeRaking Leaves and Writing Code So, today I had the task of removing the leaves from my yard, which gave me a lot of time to think because it is a pretty solitary job, even if you have people helping you, because […]
  • DRY ProgrammingDRY Programming Today I thought I’d talk to you about the programming principle known as DRY.  As you may know, DRY stands for “Don’t Repeat Yourself” and it shows up in a lot more places than you might […]
  • I, J, and K Should DieI, J, and K Should Die One of the hardest things we do as programmers is naming things.  But the easiest thing to name is counter variables and most of us do it wrong several times a day. Of course, I’m […]
  • Limiting Beliefs of ProgrammersLimiting Beliefs of Programmers At the risk of making half of my audience think I’ve gone off the deep end, I’m going to address a topic that I’ve only recently REALLY begun to understand, in part thanks to Soft […]
  • How to Sabotage EstimatesHow to Sabotage Estimates Over the last week, I’ve been helping other programmers estimate the task they’ve been assigned and this has caused me to reflect on how I estimate software.  What works.  What doesn’t.  […]

About Dave Bush

Dave Bush is a Full Stack ASP.NET developer focusing on ASP.NET, C#, Node.js, JavaScript, HTML, CSS, BootStrap, and Angular.JS. Does your team need additional help in any of the above? Contact Dave today.

One Pingback/Trackback

  • Pingback: Dew Drop – June 23, 2014 (#1801) | Morning Dew()

  • Kevin O’Shaughnessy

    I am always encouraging other developers to split up longer methods. Often long methods hide bugs because they are much harder to spot amongst a lot of other code. WET code often hides in long methods and by splitting into smaller methods you can find the repetition and eliminate code.

    I think limiting every method to only a maximum of mine lines is a little extreme though. I think it’s more important that the method name accurately reflects what it does and is easy to understand. This usually means it performs some sort of business function. That is sometimes hard to achieve in 9 lines of OOP code (its a bit easier using a functional language). I try to keep methods to 25 lines or less, including blank lines.

    I have also been teaching other developers that you don’t necessarily need to have separate unit tests for each method. Often it is good to do that, but sometimes it’s better to have tests that exercise two or more methods.

    • As I said in the post, 9 lines should be the norm. There are times when this may not be achievable. But, as you’ve already agreed, the shorter we can get our methods, the easier they are to read, the fewer bugs will be introduced, and the less repetition of code will be introduced.

      I was reviewing a pretty long method in code that I wrote recently, and while there wasn’t any code duplication, I quickly realized that if I had broken the code into smaller chunks, it would be much easier to read and wouldn’t need any comments.

      If you have code that is exercising two or more methods, I would hope that the extra methods you are testing are private and really just broken out for clarity. Otherwise, I would start questioning how well the Single Responsibility principle had been implemented in the code.

      On the other hand, if you are doing integration test, testing multiple methods at a time is a perfectly valid practice.

      • Kevin O’Shaughnessy

        Is there are science to say that more than 9 lines is too many to be readable?

        Much of my work is with ASP.NET MVC. Like most developers I inherited a lot of code written in a different way to how I would have gone about it. Each model inherits an interface with a CreateModel method. Some of these methods are very long. Some of the models are around 1000 lines of code.

        I have been splitting the long methods out. But I wonder if this should all be done in one class? Yes it is all the same model, but there are a lot of different aspects to it.

        • https://en.wikipedia.org/wiki/The_Magical_Number_Seven,_Plus_or_Minus_Two

          1000 lines of code in a model?!! Aren’t models mostly properties that represent fields in a database (or something similar). Sounds like the class could be broken up. Without seeing the code base, it is hard to say. Some questions to reflect on:

          Have they mixed the model with business rules?
          Is the create method doing essentially the same thing over and over again and you might be able to abstract it into the base class using either reflection or generics?

          • Kevin O’Shaughnessy

            I read about Miller’s law in Code Complete, but McConnell reached different conclusions. I might write blog post about it later.

            Are there business rules in the model? What are business rules actually are is a little subjective, but I would say yes. There seem to be two schools of thought on whether there should be any business logic in your model. The way I understand it is the original MVC pattern from the 70s was designed so that the business logic would be in the model and the controller rather than the view. Would you say all the business logic should be in the controller, or in another class outside of the controller.

            The application has actually been developed using an N-tier architecture so the majority of the business rules are in a business logic layer. However, overall, this has added more complexity than it has eliminated.

          • Actually, I tend toward the concept of “services” for business rules. I first saw this in AngularJS, but it would transfer pretty nicely to ASP.NET MVC or MVP.

            MVC just states where the model, the view and the controller live and how they interact with each other. The model, is the resulting data that the view will need to represent and the controller is the traffic cop that gets the data between the view and the model. All of that can, and probably should sit on top of other code that would be more typically represented as tiers with the MVC part being the view tier. I wrote about this a long time ago here: https://blog.dmbcllc.com/aspnet-mvc-model-bll-or-dal/

            In a recent application, my controller instantiates service classes. One for computations, one for enable/disable logic, and another for validations (that were not simple enough to be handled by the view). Doing this made my controller very simple and reduced my model (or view model more accurately) to handling just the state of the view.

            In my case, it also made the code MUCH more testable because the only thing my service classes need is a model. By doing this, I have eliminated the need to test anything other than my service classes.