Basic modular calculator

Question

I am new to modular JavaScript code, and after reading an article on the Internet, I wrote a very basic calculator. This works fine, but due to some unknown reason, I feel that this code is not well written. I will appreciate it if someone could improve my code below so that it will be helpful with learning modular JavaScript.

$(function () {
    $('.button').on('click', function() {
        var operator = $(this).attr('name'),
        calc = new Calculator('output', 'valOne', 'valTwo', operator);

        calc.init();
    });
});

calculator.js

var Calculator = function(eq, valone, valtwo, operator) {
    var eqCtl = document.getElementById(eq),
    valone    = document.getElementById(valone),
    valtwo    = document.getElementById(valtwo),
    op        = operator,

    init = function() {
        op   = operator;
        val1 = parseInt($(valone).val());
        val2 = parseInt($(valtwo).val());

        calculation();
    },

    setVal = function(val) {
        eqCtl.innerHTML = val;
    },

    calculation = function() {
        if(op == 'add') {
            addition(val1, val2);
        }
        else if(op == 'sub') {
            subtract(val1, val2);
        }
        else if(op == 'mult') {
            multiply(val1, val2);
        }
        else {
            division(val1, val2);
        }
    },

    addition = function(x,y) {
        return setVal(x + y);
    },

    subtract = function(x,y) {
        return setVal(x - y);
    },

    multiply = function(x,y) {
        return setVal(x * y);
    },

    division = function(x,y) {
        if( y == 0 ) {
                return setVal('cannot divide by 0');
        } else {
                return setVal(x / y);
        }
    };

    return {
        init: init
    };
};

Answer 1

I've done this only for addition to make it more readable -

var Calculator = function() { 

    this.calculate = function(eq, valone, valtwo, op) {

        var val1 = parseInt($(valone).val());
        var val2 = parseInt($(valtwo).val());        

        var eqCtl = $('#' + eq),
        var operation;

        switch(op){
            case 'add' : 
                operation = addition;
            break;
            case 'sub':
                //same here
            break;
            default :
                operation = division;
        }

       this.addition = function(){
            //for those who could not read the question
       }

        return operation.apply(null, [val1, val2]); // to setVal()
    },

    addition = function() {
        return setVal(arguments[0] + arguments[1]);
    }

    return this;
};

And then use as..

var calc = new Calculator();

calc.calculate('output', 'valOne', 'valTwo', operator);
//call as many times as required

Answer 2

The concept of creating a new Calculator() with a specified operator and then calling calc.init() to actually perform the operation seems a bit strange. I'd probably have any initialisation built into the constructor, and then have a calc.calculate() method that takes the operation as a parameter.

I don't have time now for a detailed analysis of your code, but one thing I would probably do is make the different operation functions methods of an object:

var operations = {
    "addition" : function(x,y) { return setVal(x + y); },
    "subtract" : function(x,y) { return setVal(x - y); },
    "multiply" : function(x,y) { return setVal(x * y); },
    // etc
}

Because then you can eliminate the if/else structure that decides what function to call and just do this:

calculation = function() {
    if (op in operations)
        operations[op](val1, val2);
    else
        // invalid op requested, so show message, throw exception, whatever
}

If you add more operations in the future, say a toThePowerOf() operation, you'd add it to the operations object but wouldn't need to change the calculation() function.

Answer 3

You should construct the object only once, and then re-use it. (That, or use a "static" method.)

$(function () {
    "use strict";
    var calculator = new Calculator('output', 'valOne', 'valTwo');
    $('.button').on('click', function() {
        var operator = $(this).attr('name');
        calculator.calculate(operator);
    });
});

Then, inside the Calculator you would initialize the HTML elements for operand1/operand2/output and provide a method that reads the values and outputs the result.

I would just do the operation directly in the switch. Alternatively, you can do something like: var method, result; if(...) {method = add;} ...; result = method(x, y);

var Calculator = function (outputId, firstOperandId, secondOperandId) {
    "use strict";

    var output = document.getElementById(outputId),
        firstOperand = document.getElementById(firstOperandId),
        firstOperand = document.getElementById(secondOperandId);
    return {
        calculate: function (operator) {
            var num1 = +firstOperand.getAttribute('value'),
                num2 = +secondOperand.getAttribute('value'),
                result;
            switch (operator) {
                case 'add':
                    result = x + y;
                    break;
                case 'sub':
                    result = x - y;
                    break;
                case 'mult':
                    result = x * y;
                    break;
                case 'div':
                    if (y === 0) {
                        //throw new Error('cannot divide by 0');
                        result = 'cannot divide by 0';
                    } else {
                        result = x / y;
                    }
                    break;
                default:
                    // throw new Error("invalid operator '" + operator + "'");
                    result = "invalid operator '" + operator + "'";
            }
            output.innerHTML = result;
            return result;
        }
    };
};

Other remarks:

• Use === instead of ==.
• Inside the if-chain (or switch, YMMV) I would just do the operation directly, instead of calling a one-liner method for it - why make it more complicated than it needs to be?
• I wouldn't default to division, but throw an error instead - maybe write error to the output element?
• The + coerces a numeric value. It's not bullet-proof (" " === 0), but it's better than using parseInt, because with parseInt you need to remember to always send a second param 10 - try parseInt('010') and parseInt('008').