HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Basic modular calculator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
calculatorbasicmodular

Problem

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

Solution

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.

Code Snippets

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
}
calculation = function() {
    if (op in operations)
        operations[op](val1, val2);
    else
        // invalid op requested, so show message, throw exception, whatever
}

Context

StackExchange Code Review Q#14438, answer score: 6

Revisions (0)

No revisions yet.