3
\$\begingroup\$

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');
    }
}
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ You and @MikeMcCaughan both gave some really great feedback. My reputation is too low to even vote you up! Please vote up the question so I can vote up your answers. One thing I want to note is that JSLint apparently wants a space between function (n) but not function(). Something about when declaring with parameters it can be mistaken for a name. I don't see how but yeah. \$\endgroup\$ – Justin May 26 '12 at 21:05
3
\$\begingroup\$

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!

\$\endgroup\$
  • \$\begingroup\$ new function = eww... I'd just use MathFactor instead of this and change the function into an object (or declare it as a named type in an IIFE if I really wanted a singleton). Javascript anonymous singleton instantiation syntax is easy to miss and often unexpected to those who aren't aware of what you are doing (then they decide the new is unnecessary and wind up with unintended globals). \$\endgroup\$ – Bill Barry May 26 '12 at 0:13
1
\$\begingroup\$

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

\$\endgroup\$

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.