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.

Answer 3

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

Answer 4

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

Answer 5

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