JSLint says "unexpected function" doCalc

Question

See the full calculator with HTML -> https://gist.github.com/1861120 You should be able to drag that html file from github into a browser and start using it. I'm only putting the js below. The calculator works fine but JS Lint is telling me I have an unexpected 'function' doCalc line 9 character 1.

Also just looking for some general tips on best practices and how this might be refactored. Any help is appreciated. Thank you.

var type, n, r, formula;
Math.factorial = function (n) {
    if (n < 2) {
        return 1;
    }
    return (n * Math.factorial(n - 1));
}

function doCalc(type) {
    switch (type) {
        case 'permutation':
            n = document.permutation.T1.value;
            r = document.permutation.T2.value;
            formula = function() {
                document.permutation.T3.value = Math.factorial(n) / Math.factorial(n - r);
            }
            break;
        case 'combination':
            n = document.combination.T1.value;
            r = document.combination.T2.value;
            formula = function() {
                document.combination.T3.value = Math.factorial(n) / (Math.factorial(r) * Math.factorial(n - r));
            }
            break;
    }
    n = Number(n);
    r = Number(r);
    if (isNaN(n) || isNaN(r)) {
        alert("Factorial requires a numberic argument.");
        return null;
    }
    formula();
}

window.onload = function() {
    document.getElementById('calculate-p').onclick = function() {
        doCalc('permutation');
    }
    document.getElementById('calculate-c').onclick = function() {
        doCalc('combination');
    }
}

Answer 1

You are getting jslint errors because of missing semicolons (the function expressions) and there are a few space issues (function() -> function ()). I would also make a few other changes:

1. Wrap with an IIFE and pass in globals [see also]
2. Add a "use strict"; line to use strict es5 syntax
3. Change factorial to be iterative so it works on numbers > 3000 (the Firefox recursion limit)
4. Get rid of the unused type variable
5. Move the variables local to doCalc to actually be local
6. Extract methods for the permutation and combination functions (and since we are already extending Math, why not put them there? - edit: I would prefer not doing it in the first place but didn't feel like suggesting that)
7. Since the switch statement is now almost the same, simplify the code by replacing the string type parameter to be a function parameter
8. Inline the repetitive assignments to n and r

Results

(function (Math, window, document) {
    "use strict";

    Math.factorial = function (n) {
        var r = n;
        while (n > 1) {
            r = r * n;
            n -= 1;
        }
        return r;
    };

    Math.permutation = function (n, r) {
        return Math.factorial(n) / Math.factorial(n - r);
    };

    Math.combination = function (n, r) {
        return Math.factorial(n) / (Math.factorial(r) *
                Math.factorial(n - r));
    };

    function doCalc(formula) {
        var n = Number(document.permutation.T1.value),
            r = Number(document.permutation.T2.value);
        if (isNaN(n) || isNaN(r)) {
            window.alert("Factorial requires a numberic argument.");
            return null;
        }
        document.permutation.T3.value = formula(n, r);
    }

    window.onload = function () {
        document.getElementById('calculate-p').onclick = function () {
            doCalc(Math.permutation);
        };
        document.getElementById('calculate-c').onclick = function () {
            doCalc(Math.combination);
        };
    };
}(Math, window, document));

There is still more to do with this code. For example you shouldn't use the on* properties to attach events because it overrides existing ones.

Answer 2

Well, you asked for general tips, so here goes ;)

I would almost always refrain from extending built-in types. I will "shim" to provide standard-compliant functions which are not available (e.g. "forEach" on Array.prototype), but adding new methods to existing types is a recipe for problems. You could simply have a factorial function without the Math and it would work just as well.

In fact, you could do yourself a favor and create your own Math object (named something different, of course) and add your formula functions to that, so:

var MathFactor = new function () {
    this.factorial = function (n) {
        return n < 2 ? 1 : n * this.factorial(n - 1);
    };
    this.permutate = function (n, r) {
        return this.factorial(n) / this.factorial(n - r);
    };
    this.combine = function (n, r) {
        return this.factorial(n) / (this.factorial(r) * this.factorial(n - r));
    };
};

Then doCalc is just a matter of a simple numeric check, then setting your T3.value to the output of a call to MathFactor.permutate() or MathFactor.combine(). Hope that helps!

Answer 3

It's complaining that you haven't ended your assignment statment, Math.factorial = ..., with a semi-colon. (This is the case for just about every error jslint will throw with your code.)