Factorial in JavaScript

Question

I decided to write a simple factorial() function in JavaScript and ask here for opinions. The idea is to further improve my JavaScript style and learn the best practices. Is there something bad with this code? What can be improved? Any suggestions are welcome.

/**
 * Calculates the factorial of a given non-negative finite integer.
 * @param {Number} n
 * @return {Number} factorial
 * @throws TypeError if n is not a Number
 * @throws TypeError if n is a Number but not an integer
 * @throws RangeError if n is a negative integer
 */
function factorial(n) {
    if (typeof n !== "number") {
        throw new TypeError("factorial() expects a number.");
    }
    if (Math.floor(n) !== n) {
        throw new TypeError("factorial() expects an integer.");
    }
    if (n < 0) {
        throw new RangeError("factorial() expects a non-negative number.");
    }

    // 170 is the biggest integer whose factorial does not become Infinity in JavaScript.
    // Therefore, we can improve performance by checking directly.
    // This if statement also handles the important edge case of n === Infinity, which
    // would cause the function to be stuck forever.
    if (n > 170) {
        return Infinity;
    }

    var accumulator = 1;

    for (var i = 2; i <= n; i++) {
        accumulator *= i;
    }

    return accumulator;
}

Answer 1

Type and vetting

There are a few issues with the code, though minor they would make the function more usable.

• Javascript is loosely typed and therefore you should expect that a function's arguments be so as well. If you are passed a string eg factorial("10") you throw an error. This is not what I would expect from a javascript function.

• Though debatable, Infinity is not the correct result for an input value over 170. It is actually difficult to select an appropriate result for out of range values. I would opt for a RangeError for both n > 170 and n < 0

• The correct numeric return for an argument that can not be coerced into a number is NaN

• You can only get the factorial of integers, I would thus expect that the function floor the argument for me.

• Good code is efficient in both memory and speed. You can improve the function by removing the iterator variable i and use the input argument n in its place and use a while loop.

• Declare var's at the top of the function.

Rewrite

Thus I would rewrite your code to the following

function factorial (n) {
    var accumulator = 1; 

    if (isNaN(n)) {
        return NaN;
    }

    n |= 0; // this will convert a string to type Number

    if (n > 170 || n < 0 ) { 
        throw new RangeError ("Argument out of range. Arguments range is 0 - 170 inclusive.");
    }

    while (n > 1) { 
        accumulator *= n--;
    }

    return accumulator;
}

Though i prefer a different more compact style.

function factorial (n) {
    var accumulator = 1;     
    if (isNaN(n)) { return NaN }
    n |= 0; 
    if (n > 170 || n < 0 ) { throw new RangeError ("Argument out of range.Arguments range is 0 - 170 inclusive.") }
    while (n > 1) { accumulator *= n-- }
    return accumulator;
}

Faster via lookup.

What I don't like about this function is its complexity grows in relationship to the value n. As the range of valid input values is small this function would best be implemented as a lookup table. For speed I would also drop the type checking.

Though you add a slight overhead just after parsing the code all subsequent calls will have the same execution time and be many time faster than calculating the value.

const factorial = (() => {
    const f = new Float64Array(171);
    (()=>{
        var i, a = 1;
        f[0] = a;
        for (i = 1; i < 171; i++) { f[i] = a *= i }
    })();
    return (n) => n < 0 || n > 170 ? NaN : f[n | 0];
})();

Note on recursion.

Though this function is often used as an example of recursion you should avoid recursion in javascript if you are not in control of the calling code. Javascript has a limited call stack, and Tail call optimisation as yet has not been implemented by Chrome, Edge, Firefox. As you can not determine the depth of the call stack at run time any recursive code can indeterminately throw a RangeError.

Answer 2

if (typeof n !== "number") {
    throw new TypeError("factorial() expects a number.");
}
if (Math.floor(n) !== n) {
    throw new TypeError("factorial() expects an integer.");
}
if (n < 0) {
    throw new RangeError("factorial() expects a non-negative number.");
}

First, I discourage manual type and value checks on runtime. They're redundant since the JS engine will emit an error if you operate on the wrong data anyways. It's also runtime overhead as well as unnecessary code.

Often times, developers forward this task to the IDE when writing code. With the right plugins and annotations, the IDE can warn you of incompatible data types and operations. This only prevents writing the API incorrectly though. There's still risk of using the function incorrectly, but that's the consumer's fault, not the API.

var accumulator = 1;

JS has let, the block-scoped version of var. It's a good habit to use let since in most cases, you'll want block-scoped vars. It also prevents ambiguity of where the variable exists in the code, which usually causes problems (like var in for or if). If I remember correctly, it's because of hoisting that developers started defining vars explicitly on the top of the function body to signal that a variable of that name exists in the function.

if (n > 170) {
    return Infinity;
}

var accumulator = 1;

for (var i = 2; i <= n; i++) {
    accumulator *= i;
}

return accumulator;

In the end, the factorial code is just the one above. A shorter version factorials can be done using recursion.

function factorial(n) {
  return n > 170 ? Infinity    // Bail out on 170
    : n == 0 || n == 1 ? 1  // When the answer is just 1
    : n * factorial(n - 1)  // Multiply current with next smallest
}

console.log(factorial(1))
console.log(factorial(2))
console.log(factorial(3))
console.log(factorial(4))
console.log(factorial(5))

Answer 3

One possible way to refactor the different cases you did to check if the input is valid is by relying on Number.isInteger() function which returns true if the argument is an integer.

   // This also test if the integer is positive
   if (Number.isInteger(n) && n > 1){
        var accumulator = 1;
        for (var i = 2; i <= n; i++) {
            accumulator *= i;
        }
        return accumulator;
   } else {
        // Throws an error for ALL other wrong inputs
        throw new Error("Expected a positive integer value");
   }

So your code can be:

function factorial(n) {
   if (n === 0 || n=== 1){ 
        return 1; 
   }
   if (n > 170) {
        return Infinity;
   } 
   if (Number.isInteger(n) && n > 1){
        var accumulator = 1;
        for (var i = 2; i <= n; i++) {
            accumulator *= i;
        }
        return accumulator;
   } else {
        throw new Error("Expected a positive integer value");
   }       
}

// Testing
console.log(factorial(0));
console.log(factorial(1));
console.log(factorial(9));
console.log(factorial(170));
console.log(factorial(171));
// All below test will throw an error:
//console.log(factorial("100"));
//console.log(factorial(a));
//console.log(factorial(2.2));
//console.log(factorial(-2));