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

• delete me: 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.

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

• Andrey Agibalov: Why val1, val2 and addition are global?
• ANeves: @RobinMaben You also forgot the break; in the switch. Please don't be sloppy when proposing improvements.
• delete me: 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.
• Robin Maben: Don't be like that. Keep the OP's code in context.
• delete me: @RobinMaben I'm sorry if I offended you; my goal was to prevent readers from learning bad habits by assuming accepted answers are always correct, and to help you improve your answer. If you'd like help fixing your code, tell me what part you want help with and I'd be glad to write a more thorough explanation. (also, please start your reply with @st-boost, so it appears in my inbox and I don't have to manually check back)

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

• nnnnnn: 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).