Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am new to modular javascript code and read some article on 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 if someone improve my code below so that it will be helpful to learn modular javascript.

Thanks in advance

here is my code

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

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

here is 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
    };
};
share|improve this question
Why are you checking for y == 0 in division? Javascript's Numbers can have a value of Infinity (or -Infinity), which is returned when you divide by 0 and works as you'd expect it to work. It doesn't throw an error. – st-boost Aug 10 '12 at 3:17

3 Answers

up vote 0 down vote accepted

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;
            case 'sub':
                //same here
            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
share|improve this answer
calc.calculate(...) have a typo I think, calculate doesn't exist – ajax333221 Aug 8 '12 at 18:26
@ajax333221: Thanks. You're right :) Fixed. Method names should always (as much possible) be verbs. – Robin Maben Aug 8 '12 at 19:02
3  
Why val1, val2 and addition are global? – loki2302 Aug 8 '12 at 19:17
2  
@RobinMaben You also forgot the break; in the switch. Please don't be sloppy when proposing improvements. – ANeves Aug 9 '12 at 9:44
This post introduces a staggering number of errors. Some lines before var statements are terminated with commas, which is invalid syntax. addition is a leaked global variable. parseInt, as written, will fail for input like "010". The switch statement is broken. addition is much slower due to an entirely unnecessary use of arguments. operator is a reserved word, so the entire program won't run on some engines. The entire structure is needlessly complex. And the spacing is off in a few places. – st-boost Aug 10 '12 at 3:28
show 3 more comments

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').
share|improve this answer
Regarding your comments about parseInt(), it's worse than that. Even when you remember the radix with parseInt(val,10) it still ignores any non-numeric characters at the end of the string, and obviously it doesn't do decimals. So if the user tries to calculate 2.8 + 2.8 they get an answer of 4 because parseInt("2.8",10) returns 2 (ignoring everything from the decimal point). If the user enters "2abc" I think it would be better to explicitly report an error than to just use the 2 returned from parseInt("2abc",10). – nnnnnn Aug 10 '12 at 5:41

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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