how could I reduce the cyclomatic complexity?

Question

Whenever I lint a piece of code I'm working on I get the This function's cyclomatic complexity is too high. (7). But I'm a bit confused on how I could rewrite it in such way so it works.

This would be the function that keeps throwing that message:

function () {
  var duration = +new Date() - start.time,
    isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport / 2,
    direction = delta.x < 0;

  if (!isScrolling) {
    if (isPastHalf) {
      if (direction) {
        this.close();
      } else {
        if (this.content.getBoundingClientRect().left > viewport / 2 && pulled === true) {
          this.close();
          return;
        }
        this.open();
      }
    } else {
      if (this.content.getBoundingClientRect().left > viewport / 2) {
        if (this.isEmpty(delta) || delta.x > 0) {
          this.close();
          return;
        }
        this.open();
        return;
      }
      this.close();
    }
  }
}

I would like to hear some advice on how I could structure my code in such way so I avoid this kind of situations.

Seriously? Don't use nested `if`s. Factor out responsibilities according to *Single Responsibility Principle*. One piece of code (a module) should do one and only one thing and it should do it well. Just think about how much possible execution paths do these ugly if-bushes generate... — Powerslave, Jul 29 '13 at 15:21
Some Googling show this is specific to `jshint` so you should tag your question accordingly. — marekful, Jul 29 '13 at 15:23
@Joe I'll gladly tell you tomorrow, but I'm nearly finished for today, so I don't have the time. It is still very possible to pull this thing apart - otherwise there would not be so much exec paths. — Powerslave, Jul 29 '13 at 15:24
Certainly you can refactor a little bit, but it's getting close to one-line-per-function. — Joe, Jul 29 '13 at 15:28
I see, I'll try to refactor it a little bit, but the user scenario ends up in this kind of `if` and `else`. I'll try to see if I can change the breaking point between the main `if` and `else` in such way so it reduces the amount of `if`s I use — Roland, Jul 29 '13 at 15:31
@Joe There are several duplications for example that could be extracted well with a bit of thinking. I'll suggest a cleanup tomorrow if nobody else does so until then. — Powerslave, Jul 29 '13 at 15:33
@rolandjitsu A nice way to avoid the if-elseif-elseif-else hell is, for example configurations and contexts. — Powerslave, Jul 29 '13 at 15:34
Yes I'm sure this can be re-ordered to simplify things. I just don't think pulling out functions is the right answer. — Joe, Jul 29 '13 at 15:35
You may want to consider posting questions like this on http://codereview.stackexchange.com — kojiro, Jul 29 '13 at 15:35
Well, reducing If's on account of &&s and ||s you will not reach lower cyclomatic number. Usually there are limited possibilities to reduce cyclomatic number keeping the only function. Therefore we usually split the function into two in a best way so they do relatively independent tasks. — , Sep 17 '13 at 09:49
I answered this question with what I thought was a helpful answer as did at least 6 other people... however the powers that be determined It should be deleted... go read for yourself if the linked information is still available of course... http://gmetrics.sourceforge.net/gmetrics-CyclomaticComplexityMetric.html — Paul Zahra, Jan 10 '17 at 09:03
Note that the *default* for the jshint `maxcomplexity` setting is *false*, turning this off. Whomever set this to *7* may have picked a rather low default. The [Wikipedia article on Cyclic complexity](https://en.wikipedia.org/wiki/Cyclomatic_complexity#Limiting_complexity_during_development) (linked from the jshint documentation) sites *10* as a better default, with exceptions to be made going up to 15. — Martijn Pieters, Feb 08 '17 at 11:58

score 25 · Accepted Answer · answered Jul 29 '13 at 16:17

25

Well you have only two actions in your code, but much too many conditions. Use a single if-else-statement, and boolean operators in the condition. If that was impossible, you could at least

remove the empty lines to get the full logic on one screen page
add some comments on what the branches are doing (and why)
avoid early returns

Here's your function simplified:

var duration = +new Date() - start.time,
    isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport / 2,
    isFarRight = this.content.getBoundingClientRect().left > viewport / 2, 
    direction = delta.x < 0;

if (!isScrolling) {
    if (isPastHalf) {
        if (direction)
            this.close();
        else {
            if (isFarRight && pulled)
                this.close();
            else
                this.open();
        }
    } else {
        if (isFarRight) {
            // Looks like the opposite of `direction`, is it?
            if (this.isEmpty(delta) || delta.x > 0)
                this.close();
            else
                this.open();
        } else
            this.close();
    }
}

and shortened:

var duration = +new Date() - start.time,
    isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport / 2,
    isFarRight = this.content.getBoundingClientRect().left > viewport / 2, 
    direction = delta.x < 0,
    undirection = this.isEmpty(delta) || delta.x > 0;

if (!isScrolling) {
    if ( isPastHalf && !  direction && !(isFarRight && pulled)
     || !isPastHalf && !undirection &&  isFarRight )
        this.open();
    else
        this.close();
}

answered Jul 29 '13 at 16:17

Bergi

513,640
108
821
1,164

1

What about cyclomatic complexity on `switch` statements, if I'd have something like [this](http://snippi.com/s/nkafrv9) what could I do ? I don't think there's much I can do, or can I ? – Roland Jul 29 '13 at 16:33
There's a lot you can do. First of all, you usually would use else-ifs instead of `switch` when you don't need falltrough. – Bergi Jul 29 '13 at 16:51
2

By the way, if you get brainhurt trying to minimize boolean math, cheat and use [wolframalpha](http://www.wolframalpha.com/input/?i=%28a+%26%26+!+b+%26%26+!%28c+%26%26+d%29+||+!a+%26%26+!e+%26%26+c%29) – kojiro Jul 29 '13 at 16:51
You are almost there :) Just add 'guard clause' at the beggining. if (isScrolling) return. – Piotr Perak Nov 24 '13 at 13:06
1

@Peri: Avoiding early returns does make the code more clear actually. – Bergi Nov 24 '13 at 16:57
Not for me. Because ofthen you have to scroll to see that there is no else. And in this example you do a lot of things even if they are not needed because isScrolling == true! In some cases that can be huge cost that you don't have to pay. Just look at this example and tell me 'early returns' don't make code clearer - http://martinfowler.com/refactoring/catalog/replaceNestedConditionalWithGuardClauses.html – Piotr Perak Nov 24 '13 at 20:09
The lots of things could easily be placed into the if-clause, that's true. And I have to clarify/qualify my statement from above: Early returns should be avoided in deeply nested code and should not be mixed with normal conditionals. They are useful to intercept edge cases that do `return` *things* (like in your example), however it's debatable when those cases are chosen for their side effects and have `void` return type. The early returns in the OP's code are inacceptable, the one you proposed for the outermost special case `isScrolling` is fine (though I don't see much gain). – Bergi Nov 24 '13 at 22:56
2

The guard return for isscroling could even be done before the assignments. Also, the if is too complex for my taste. I'd introduce two more variables to reveal the actual meanings of isPastHalf && !direction && !(isFarRight && pulled) and !isPastHalf && direction && isFarRight. – realbart Sep 20 '17 at 05:39

score 5 · Answer 2 · answered Jul 29 '13 at 15:58

Firstly, there are three results your function can have: do nothing, call this.close() or call this.open(). So ideally the resulting function will just have one if statement which determines which result is used.

The next step is to extract all boolean code into variables. Eg var leftPastCenter = this.content.getBoundingClientRect().left > viewport / 2.

Finally, use boolean logic to simplify it step by step.

Here is how I did it:

Firstly, extract all boolean variables:

function () {
    var duration = +new Date() - start.time,
      isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport / 2,
      direction = delta.x < 0,
      leftPastCenter = this.content.getBoundingClientRect().left > viewport / 2,
      positiveDelta = this.isEmpty(delta) || delta.x > 0,
      isPulled = pulled === true; // I'll assume the test is needed rather than just using pulled.

    if (!isScrolling) {
        if (isPastHalf) {
            if (direction) {
                this.close();
            } else {
                if (leftPastCenter && isPulled) {
                    this.close();
                    return;
                }
                this.open();
            }
        } else {
            if (leftPastCenter) {
                if (positiveDelta) {
                    this.close();
                    return;
                }
                this.open();
                return;
            }
            this.close();
        }
    }
}

The easiest part to pull out is realizing if isScrolling is true, nothing ever happens. This immediately gets rid of one level of nesting:

    // above same
    if (isScrolling) { return; }

    if (isPastHalf) {
        if (direction) {
            this.close();
        } else {
            if (leftPastCenter && isPulled) {
                this.close();
                return;
            }
            this.open();
        }
    } else {
        if (leftPastCenter) {
            if (positiveDelta) {
                this.close();
                return;
            }
            this.open();
            return;
        }
        this.close();
    }
}

Now look at the cases this.open() are called. If isPastHalf is true, this.open() is only called when !direction and !(leftPastCenter && isPulled). If isPastHalf is false, then this.open() is only called when leftPastCenter and !positiveDelta:

    // above same
    if (isScrolling) { return; }

    if (isPastHalf) {
        if (!direction && !(leftPastCenter && isPulled)) {
            this.open();
        } else {
            this.close();
        }
    } else {
        if (leftPastCenter && !positiveDelta) {
            this.open();
        } else {
            this.close();
        }
    }

Flipping the ifs (so this.close() comes first), makes the code a bit neater, and gives my final version:

    function () {

    var duration = +new Date() - start.time,
      isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport / 2,
      direction = delta.x < 0,
      leftPastCenter = this.content.getBoundingClientRect().left > viewport / 2,
      positiveDelta = this.isEmpty(delta) || delta.x > 0,
      isPulled = pulled === true; // I'll assume the test is needed rather than just using pulled.

    if (isScrolling) { return; }

    if (isPastHalf) {
        if (direction || (leftPastCenter && isPulled)) {
            this.close();
        } else {
            this.open();
        }
    } else {
        if (!leftPastCenter || positiveDelta) {
            this.close();
        } else {
            this.open();
        }
    }
}

It is difficult for me to do more, without knowing your codebase. One thing to note is direction and my new variable positiveDelta are nearly identical - you could possible remove positiveDelta and just use direction. Also, direction isn't a good name for a boolean, something like movingLeft would be better.

score 4 · Answer 3 · answered Jul 29 '13 at 15:49

Actually all those return statements are confusing the issue, but they offer a hint to the solution.

if (direction) {
  this.close();
} else {
  if (this.content.getBoundingClientRect().left > viewport / 2 && pulled === true) {
    this.close();
    return; // We'll never `this.open()` if this is true anyway, so combine the booleans.
  }
  this.open();
}

How about:

if (direction || (this.content.getBoundingClientRect().left > viewport / 2 && pulled === true)) {
  this.close();
} else {
  this.open();
}

And as for:

if (this.content.getBoundingClientRect().left > viewport / 2) {
  if (this.isEmpty(delta) || delta.x > 0) {
    this.close();
    return; // Combine the booleans!
  }
  this.open();
  return;
}

Simplify:

if ((this.isEmpty(delta) || delta.x > 0) || !this.content.getBoundingClientRect().left > viewport / 2) {
  this.close();
} else {
  this.open();
}

(Aside: The original post left out a closing brace. If you (OP) intended that the function continues past your post, then this answer is wrong (but you should've made that clearer))

Result: We've eliminated two (repeated) decisions:

function () {
  var duration = +new Date() - start.time,
    isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport / 2,
    direction = delta.x < 0;

  if (!isScrolling) {
    if (isPastHalf) {
      if (direction || (this.content.getBoundingClientRect().left > viewport / 2 && pulled === true)) {
        this.close();
      } else {
        this.open();
      }
    } else {
      if ((this.isEmpty(delta) || delta.x > 0) || !this.content.getBoundingClientRect().left > viewport / 2) {
        this.close();
      } else {
        this.open();
      }
    }
  }
}

realbart · Answer 4 · 2018-07-26T10:40:19.480

Bergi has already given a correct answer, but it's still too complex for my taste. Since we're not using fortran77 I think we're better off using an early return. Also, the code may be further clarified by introducing extra variables:

function doSomething(isScrolling, start, delta, viewport) {
    if (isScrolling) return;

    var duration = +new Date() - start.time;
    var isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport / 2;
    var isFarRight = this.content.getBoundingClientRect().left > viewport / 2;

    // I'm not sure if my variable names reflect the actual case, but that's
    // exactly the point. By choosing the correct variable names for this,
    // anybody reading the code can immediatly comprehend what's happening.
    var isMovingToLeft = delta.x < 0;
    var isMovedPastEnd = isPastHalf && !isMovingToLeft && !(isFarRight && pulled);
    var isMovedBeforeStart = !isPastHalf && isMovingToLeft && isFarRight;

    if (isMovedPastEnd || isMovedBeforeStart) {
        this.open();
    else
        this.close();
    }
}

I like this. (To be honest I have not checked the correctness of the details.) But I like that enough boolean variables were used so that the calls to open() and close() happen only once. Also the return at the top of methods often helps to remove clutter. One partial solution not shown is to break up code into multiple functions. — user2367418, Nov 01 '18 at 18:18

score -2 · Answer 5 · answered Sep 20 '17 at 05:07

I would prefer a simple and less nested code like below:

function() 
{
    var duration = +new Date() - start.time,
        isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport / 2,
        direction = delta.x < 0;

    if (isScrolling)
    {
        return;
    }
    if (isPastHalf) 
    {
        if (direction) 
        {
            this.close();
            return;
        }
        if (this.content.getBoundingClientRect().left > viewport / 2 && pulled == = true) 
        {
            this.close();
            return;
        }
        this.open();
        return;
    }
    if (this.content.getBoundingClientRect().left > viewport / 2) 
    {
        if (this.isEmpty(delta) || delta.x > 0) 
        {
            this.close();
            return;
        }
        this.open();
        return;
    }
    this.close();
    return;
}

how could I reduce the cyclomatic complexity?

5 Answers5

Linked