6
\$\begingroup\$

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;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I would use the full error text (the 3rd one) for all cases. \$\endgroup\$ – wOxxOm Sep 18 '17 at 13:09
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Why do you prefer to have variable declarations at the top? Isn't it usual to declare variables right before they're initially used? \$\endgroup\$ – Snowbody Sep 19 '17 at 6:28
  • \$\begingroup\$ Isn't it customary for the RangeError to indicate what the acceptable range is? \$\endgroup\$ – Snowbody Sep 19 '17 at 6:30
  • \$\begingroup\$ @Snowbody vars are hoisted, putting them at the top gives a better indication of their scope. It also makes it easier to know what variables are being used within the function.. You are right about the range error I will add that. \$\endgroup\$ – Blindman67 Sep 19 '17 at 8:16
  • \$\begingroup\$ The OP wants his function to reject any input which is not a positive integer but your solution accepts floats. \$\endgroup\$ – Billal Begueradj Sep 19 '17 at 12:32
  • 1
    \$\begingroup\$ @PeterTaylor I did not know that (gamma function). I think the point is being missed. This is javascript, it has a basic type Number that is a 64bit double. To pass an integer directly to a function is not possible (unless you use a typed array) and to have a function that will throw for the slightest rounding error is anoying (I was going to use Math,round but flooring is more like integer behaviour) The function is going to end up being called with the rounding done in the calling code to ensure the integer and avoid the throw. This is pointless repetition!!! \$\endgroup\$ – Blindman67 Sep 19 '17 at 14:39
3
\$\begingroup\$
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))

\$\endgroup\$
  • 1
    \$\begingroup\$ Your recursive function will throw RangeError when the call stack overflows if the argument is n < 1 \$\endgroup\$ – Blindman67 Sep 18 '17 at 17:21
  • 1
    \$\begingroup\$ When I read this ~9 hours ago I thought the ternary statement looked a bit ugly. Coming back now, I find it is even more so. I find simple if statements for the bounds checking much easier to read. \$\endgroup\$ – Gerrit0 Sep 18 '17 at 23:06
  • \$\begingroup\$ This is a close to perfect implementation of factorial in JS ? The function does not even work in its current version. @Jonah \$\endgroup\$ – Billal Begueradj Sep 19 '17 at 6:44
  • 1
    \$\begingroup\$ The recursion still fails due to call stack overflow if fractions are passed as argument. eg factorial(0.1). The recursion function should be as simple as possible eg function factorial (n) { /* vet n here */ const f = n => n > 1 ? n * f(n - 1) : 1; return f(n | 0); } Adding bounds checking in the recursion means you repeat the vetting each iteration, while it is only needed once. \$\endgroup\$ – Blindman67 Sep 19 '17 at 8:53
  • \$\begingroup\$ @Gerrit0 It's a matter of taste. The issue I have with if statements is the unnecessary visual noise it creates, too much code for something simple. For cases where you 1) only need expressions and 2) the logic is fairly linear and like a lookup, a ternary written this way is much more readable. \$\endgroup\$ – Joseph Sep 19 '17 at 12:06
-1
\$\begingroup\$

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

\$\endgroup\$
  • 2
    \$\begingroup\$ I'm not sure what point you're trying to make with "You forgot to test when the input number is zero". OP's code has the test right here: for (var i = 2; i <= n; i++). It's simpler than your proposed alternative and works just as well. \$\endgroup\$ – Peter Taylor Sep 19 '17 at 13:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.