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.

Answer 1

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();
}

Answer 2

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.