patternjavascriptMinor
Basic modular calculator
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.
calculator.js
$(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
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:
Because then you can eliminate the if/else structure that decides what function to call and just do this:
If you add more operations in the future, say 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.