ESLint rule for Jasmine/Protractor

Question

In Protractor, Jasmine expectations are patched to resolve WebDriver promises implicitly.

For instance, if you need to assert a count of certain elements, you can use count() method (which returns a promise) directly:

expect($$(".myclass").count()).toEqual(5);

But, newcomers to Protractor can miss that and would do the same resolving promises explicitly which leads to a more complex code:

$$(".myclass").then(function (elements) {
    expect(elements.length).toEqual(5);
});

I've developed an ESLint rule to try to catch this kind of explicit "count" value check. The logic behind the rule is:

• It first locates the .length "member expression"
• Iterates over the ancestors and checks if inside expect
• Iterates over the ancestors and checks if inside a "then" callback and there is at least one argument passed to the callback function
• Determines if .then was called on an "element array finder"

'use strict'

/**
 * @fileoverview Recommend using `count()` instead of `then()` and `length`
 * @author Alexander Afanasyev
 */

var isElementArrayFinder = require('../is-element-array-finder')
var isThenCallBack = require('../is-then-callback')
var isExpect = require('../is-expect')

module.exports = {
  meta: {
    schema: []
  },

  create: function (context) {
    return {
      MemberExpression: function (node) {
        if (node.property && node.object && node.property.name === 'length') {
          // remember the variable name the ".length" was used on
          var variableName = node.object.name

          // find out if we are in an expect()
          var expectAncestor
          var thenAncestor
          var ancestors = context.getAncestors(node)

          for (var i = 0; i < ancestors.length; i++) {
            expectAncestor = ancestors[i]
            if (expectAncestor && expectAncestor.type === 'CallExpression' && isExpect(expectAncestor)) {
              // find out if we are inside a then callback
              ancestors = context.getAncestors(expectAncestor)
              for (var j = 0; j < ancestors.length; j++) {
                thenAncestor = ancestors[j]
                if (thenAncestor && thenAncestor.type === 'CallExpression') {
                  var thenCallback = isThenCallBack(thenAncestor)

                  // it has to have at least one argument
                  if (thenCallback && thenCallback.params && thenCallback.params.length > 0) {
                    var thenCallbackArgument = thenCallback.params[0]

                    // the same variable is a "then" callback function argument
                    if (thenCallbackArgument && thenCallbackArgument.name === variableName) {
                      // check that it was an ElementArrayFinder resolution
                      if (thenAncestor.callee && thenAncestor.callee.object) {
                        if (isElementArrayFinder(thenAncestor.callee.object)) {
                          context.report({
                            node: node,
                            message: 'Array.length inside promise resolution function detected. Use count() instead.'
                          })
                          return
                        }
                      }
                    }
                  }
                }
              }
            }
          }
        }
      }
    }
  }
}

Source code is also available on GitHub.

---

The rule is overly complicated, very deep and nested and not readable at all (cyclomatic code complexity is 10). What refactoring methods would you apply to improve on readability?

^{FYI, using standard style.}

Answer 1

Your naming of variables is good, they're mostly descriptive and easy to follow.

You could use reverse conditional to reduce the cyclomatic code complexity, doing that your code would look something like:

module.exports = {
  meta: {
    schema: []
  },

  create(context) {
    return {
      MemberExpression(node) {
        if (!node.property || !node.property.object || node.property.name !== 'length') {
          return
        }

        const variableName = node.object.name

        const expectAncestors = context.getAncestors(node)
        for (const expectAncestor of expectAncestors) {
          if (!expectAncestor || expectAncestor.type !== 'CallExpression' || !isExpect(expectAncestor)) {
            continue
          }

          const thenAncestors = context.getAncestors(expectAncestor)
          for (const thenAncestor of thenAncestors) {
            if (!thenAncestor || thenAncestor.type !== 'CallExpression') {
              continue
            }

            const thenCallback = isThenCallBack(thenAncestor)
            const hasParams = thenCallback && thenCallback.params && thenCallback.params.length > 0

            if (!hasParams) {
              continue
            }

            const [thenCallbackParam] = thenCallback.params
            const paramIsDesiredVariable = thenCallbackParam && thenCallbackParam.name === variableName

            const isElementArrayFinderResolution = thenAncestor.callee && thenAncestor.calle.object && isElementArrayFinder(thenAncestor.callee.object)

            if (!paramIsDesiredVariable || !isElementArrayFinderResolution) {
              continue
            }

            context.report({
              node,
              message: 'Array.length inside promise resolution function detected. Use count() instead.'
            })

            return
          }
        }
      }
    }
  }
}

I've included some extra changes in the above code, such as using some ES6.

• Property shorthand: { node: node } -> { node }
• Method properties: create: function(context) { -> create(context) {
• For-of: for (var i = 0; i < ancestors.length; i++) { expectAncestor = ancestors[i] -> for (const thenAncestor of thenAncestors), which is nice since the index was only used to access the ith element
• Constants: Using const to denote variables that cannot be re-assigned
• Using param instead of "argument" to be consistent with eslint api
• Creating expectAncestors/thenAncestors instead of re-assigning ancestors

We could further refactor using extract method:

const getExpectAncestor = (context, node) =>
  context.getAncestors(node).find(expectAncestor => expectAncestor && expectAncestor.type === 'CallExpression' && isExpect(expectAncestor))

const getThenAncestor = (context, expectAncestor) =>
  context.getAncestors(expectAncestor).find(thenAncestor => thenAncestor && thenAncestor.type === 'CallExpression')

const getThenCallbackParam = thenAncestor => {
  const thenCallback = isThenCallback(thenAncestor)
  const hasParams = thenCallback && thenCallback.params && thenCallback.params.length > 0

  if (hasParams) {
    return thenCallback.params[0]
  }
}

// Ideally you'd rename this something even more descriptive
const isDesiredVariable = (thenCallbackParam, variableName) => {
  const paramIsDesiredVariable = thenCallbackParam && thenCallbackParam.name === variableName
  const isElementArrayFinderResolution = thenAncestor.callee && thenAncestor.calle.object && isElementArrayFinder(thenAncestor.callee.object)

  return paramIsDesiredVariable && isElementArrayFinder
}

module.exports = {
  meta: {
    schema: []
  },

  create(context) {
    return {
      MemberExpression(node) {
        // You could extract this into a `isLengthVariable` function
        if (!node.property || !node.property.object || node.property.name !== 'length') {
          return
        }

        const expectAncestor = getExpectAncestor(context, node)
        if (!expectAncestor) {
          return
        }

        const thenAncestor = getThenAncestor(context, expectAncestor)
        if (!thenAncestor) {
          return
        }

        const thenCallbackParam = getThenCallbackParam(thenAncestor)
        if (!thenCallbackParam) {
          return
        }

        const variableName = node.object.name

        if (isDesiredVariable(thenCallbackParam, variableName)) {
          context.report({
            node,
            message: 'Array.length inside promise resolution function detected. Use count() instead.'
          })  
        }
      }
    }
  }
}

Here, we have extracted the main logic into seperate methods: getExpectAncestor, getThenAncestor, getThenCallbackParam

These methods are using Array.prototype.find() to find the first element that matches the predicate provided. If no such element is found, it returns undefined and we can exit MemberExpression

Further notes

You could use arrow functions shorthand since you are directly returning:

create = context => ({
  MemberExpression(node) {
    // ...
  }
}

You could declare MemberExpression outside of module.exports to further reduce the indentation level, though since the function relies on context you would need to pass context as an additional parameter. You could achieve this through a higher-order function:

const MemberExpression = context => node => {
  // ...
}

module.exports = {
  // ...

  create: context => ({
    MemberExpression: MemberExpression(context)
  }
}

You'd probably want to refactor this further since there are some especially long lines when checking conditionals, you can use a formatter such as prettier

Prettifying the solution with extracted methods gives the following:

const getExpectAncestor = (context, node) =>
  context
    .getAncestors(node)
    .find(
      expectAncestor =>
        expectAncestor &&
        expectAncestor.type === "CallExpression" &&
        isExpect(expectAncestor)
    );

const getThenAncestor = (context, expectAncestor) =>
  context
    .getAncestors(expectAncestor)
    .find(
      thenAncestor => thenAncestor && thenAncestor.type === "CallExpression"
    );

const getThenCallbackParam = thenAncestor => {
  const thenCallback = isThenCallback(thenAncestor);
  const hasParams =
    thenCallback && thenCallback.params && thenCallback.params.length > 0;

  if (hasParams) {
    return thenCallback.params[0];
  }
};

// Ideally you'd rename this something even more descriptive
const isDesiredVariable = (thenCallbackParam, variableName) => {
  const paramIsDesiredVariable =
    thenCallbackParam && thenCallbackParam.name === variableName;
  const isElementArrayFinderResolution =
    thenAncestor.callee &&
    thenAncestor.calle.object &&
    isElementArrayFinder(thenAncestor.callee.object);

  return paramIsDesiredVariable && isElementArrayFinder;
};

module.exports = {
  meta: {
    schema: []
  },

  create(context) {
    return {
      MemberExpression(node) {
        // You could extract this into a `isLengthVariable` function
        if (
          !node.property ||
          !node.property.object ||
          node.property.name !== "length"
        ) {
          return;
        }

        const expectAncestor = getExpectAncestor(context, node);
        if (!expectAncestor) {
          return;
        }

        const thenAncestor = getThenAncestor(context, expectAncestor);
        if (!thenAncestor) {
          return;
        }

        const thenCallbackParam = getThenCallbackParam(thenAncestor);
        if (!thenCallbackParam) {
          return;
        }

        const variableName = node.object.name;

        if (isDesiredVariable(thenCallbackParam, variableName)) {
          context.report({
            node,
            message: "Array.length inside promise resolution function detected. Use count() instead."
          });
        }
      }
    };
  }
};