Simple addition calculator accepting addition expressions

Question

Maybe someone can help me to optimize the calculate method. I think that it can be simplified from that what I have now.

The objective is: grab a string via the incoming argument of the calculate method and get the amount of it or difference (based on the sign standing between numbers). Count of numbers should be 2.

My function, method and constructor are working fine, but I think it is too lengthy/complex for such a simple task.

function Calculator() {
 this.calculate = function(sum) {
 this.summ = 0;
 this.sumArr = sum.split(' '); // make array from string to clear from non-numbers values
 this.newSum = this.sumArr.filter(function(number) {
 return number > 0;
 });
 for (var i = 0; i < this.sumArr.length; i++) { // based on the sign standing between numbers calculate it
 if (this.sumArr[i] == '+') {
 return this.summ = parseInt(this.newSum[0]) + parseInt(this.newSum[1]);
 } else if (this.sumArr[i] == '-') {
 return this.summ = parseInt(this.newSum[0]) - parseInt(this.newSum[1]);
 }
 }
 };
};
var calc = new Calculator;
console.log(calc.calculate("3 + 7")); // 10
console.log(calc.calculate("3 - 7")); // -4

• le_m: You are only interested in the result of the first operation, i.e. expressions 'number operator number', right?

Answer 1

RegExp to the rescue.

First there is the problem for the review. The function is inconsistent and that makes it hard to know what it really must do.

Consider the test below if given the following inputs

"3 + 7" >> 10
"3 - 7" >> -4
"3- 7" >> undefined should be -4
"3 -7" >> undefined should be -4
"3-7" >> undefined should be -4
"3 - 7.9" >> -4 should be -5 as 7.9 is rounded to 8
"3 - - 7" >> -4 should be 10 as 3 - (-7) is 3 + 7
"3 - - 7" >> -4 should be 10 as 3 - (-7) is 3 + 7
"3 a 7" >> undefined correct
"3 - a 7" >> -4 should be undefined

??? not at all what one would expect

---

The review part

You got the first two lines...

function Calculator() {
 this.calculate = function(sum) {

....and the last two...

 }; // the ; not really needed after }
};

...correct. But all the rest is not helping solve the problem.

Step by step.

So lets step through the code

Set result would be undefined

 this.summ = 0;

Split and vet numbers. Could have used isNaN() and use local variables rather than object properties var sumArr and var newSum

 this.sumArr = sum.split(' '); 
 this.newSum = this.sumArr.filter(function(number) {
 return number > 0;
 });

Unneeded loop????

 for (var i = 0; i < this.sumArr.length; i++) {

Check operator use === rather than ==

 if (this.sumArr[i] == '+') {

Parse number and return result. parseInt need a radix 10

 return this.summ = parseInt(this.newSum[0]) + parseInt(this.newSum[1]);
 } else if (this.sumArr[i] == '-') {

Parse number and return result

 return this.summ = parseInt(this.newSum[0]) - parseInt(this.newSum[1]);
 }
 }

---

A better way

Vet

The function must first vet the string and return undefined if it is not a valid calculation.

You can do that with a RegExp.

The simple regExp /[^0-9\-+ .]/

• the [...] mean any of,
• the ^ converts the [...] to means not any of,
• the 0-9 means digits "0", "1", "2", "3", "4", "5", "6", "7", "8", "9"

• the \- for character "-",(because - has special meaning you must add the forward slash)

• the + . means "+", " ", and "."

So the expression will test if not any of "0123456789-+ ." are in the string, and can be used to vet the input and return undefined

if(/[^0-9\-+ .]/.test(sum)) { return undefined } 
// not undefined is the default return so the line above is the same as
if(/[^0-9\-+ .]/.test(sum)) { return } 

We could create an even more complex RegExp that would vet things like "1+1+2" but baby steps first.

Now that we have removed most of the invalide expressions on to parsing the string.

Remove irrelevant content

First remove any irrelevant content. That would be any spaces as they are not needed. Again a RegExp comes in handy. / /g this means find / " " / space and the g means global (all)

 sum = sum.replace(/ /g,""); // remove all spaces

Find the operator

Now we can cut the sum in half to find the two numbers. We know that after the first number we need a "-" or "+".

Again a RegExp is used to find the location and value of the first "+" or "-" after the first number. /[0-9\.][+-]/ find and "0-9" or "." followed by "+" or "-"

 const result = /[0-9.][+\-]/.exec(sum);
 // if result is null then we have not found a digit followed by + -
 // This means the string is invalid
 if(result === null){ return } // return undefined
 // the index of the found location is of the number before the + -
 var num1 = sum.substr(0,result.index+1); // get number before
 var operator = sum[result.index + 1]; // get the operator
 var num2 = sum.substr(result.index + 2); // get the number after.

Now we have almost everything. There is still a chance of the two numbers being misinformed (eg "0.0.0" or second number as "0+10") but we can use the function isNaN (is Not a Number) to vet these

Vet again

 if(isNaN(num1) || isNaN(num2)) { return } // return undefined is one or both not numbers

Do the calculation

Now convert to the numbers. You should never use parseInt unless you are converting from a different radix (base). Use Number to parse a number and the use Math.floor, Math.round, Math.ceil, or Math.trunc to do the conversion to integer you want.

 num1 = Math.round(Number(num1));
 num2 = Math.round(Number(num2));

And now the result

 if(operator === "-") { return num1 - num2 }
 return num1 + num2;

And that is the logic.

---

Create the object Calculator

We can put it into an object with the method calculate and then run a good set of tests. You should always test for errors not just for good results.

function Calculator() {
 this.summ = undefined;
 this.calculate = function(str) {
 
 this.summ = undefined;
 // vet out bad characters
 if (/[^0-9\-+ .]/.test(str)) { return } 
 
 // remove spaces
 str = str.replace(/ /g,""); 
 
 // find operator
 const result = /[0-9.][+\-]/.exec(str);
 if (result === null) { return } 
 // get numbers and operator
 var num1 = str.substr(0, result.index + 1); 
 const operator = str[result.index + 1]; 
 var num2 = str.substr(result.index + 2); 
 
 // check numbers
 if (isNaN(num1) || isNaN(num2)) { return } 
 
 // parse and round numbers
 num1 = Math.round(Number(num1));
 num2 = Math.round(Number(num2));
 
 // return result
 if (operator === "-") { return this.summ = num1 - num2 }
 return this.summ = num1 + num2;
 }
}
// and test the results
var calc = new Calculator;
function log(val){
 console.log("Result of " + val + " = " + calc.calculate(val));
}
log("3 + 7");
log("3 - 7");
log("3- 7"); 
log("3 -7"); 
log("3-7"); 
log("-3-7"); 
log("3 - 7.9"); 
log("3 - - 7"); 
log("3 - a 7"); 
log("3 a 7");
log("3-7-");
log("3-7-1");

Links

• Regex 101 is a helpful site to help get your head around regular repressions
• For all things javascript JavaScript MDN is a great resource.

• RoToRa: . is not a special character inside a character class so it does not need to be escaped. However if you want to match a - in a character class it does need to be escaped. /[^0-9\-+ .]/ would be correct.

Answer 2

Instance method vs. class method vs. function:

You declared a constructor function Calculator whose instances all come with their own individual calculate method. Since the calculate method doesn't rely on other instance properties, it should probably be a class method instead.

In JavaScript, a class method is simply a property of the constructor function:

function Calculator() { ... }
Calculator.calculate = function(sum) { ... };

Using class syntax, this is equivalent to:

class Calculator { 
 ...
 static calculate(sum) { ... }
}

However, unless your Calculator will have additional properties later on, a simple function calculate(sum) { ... } instead of a class will do the job just fine.

Instance properties and side effects:

Within your calculate method, you create new instance properties such as this.sumArr, this.newSum and this.summ holding the result of the calculation. This is called a side effect. Pure functions without such side effects are easier to understand and debug.

Also, those property names are not very descriptive.

Consistency:

For different inputs, the values of this.summ and the return values of calculate(sum) are inconsistent:

sum calculate(sum) this.summ 
-------------------------------------
undefined TypeError 0
"" undefined 0
"1 x 1" undefined 0
"1 + x" NaN NaN

Filtering your operands with number > 0 leads to strange effects such as returning NaN for inputs with a zero such as 0 + 1 and returning 1 for input bla - bla + 3 + 2.

Parsing:

Instead of parseInt(str) I recommend using the unary plus operator +str which is "the fastest and preferred way of converting something into a number" according to MDN.

To split an expression into operands and operator, I recommend using robust regular expressions. A regular expression makes it much easier to understand which input strings will be accepted as valid and which input strings will be rejected.

Implementation:

First, the available operations are declared. Then, the input is parsed. For valid input, the result of the operation is returned. For invalid (non-matching) input, undefined is returned:

function calculate(expression) {
 const operations = {
 '+': (left, right) => left + right,
 '-': (left, right) => left - right,
 };
 const [match, left, op, right] = expression.match(/^(\d+) (\+|\-) (\d+)$/) || [];
 return match ? operations[op](+left, +right) : undefined;
}
// Example:
console.log(calculate("3 + 7")); // 10
console.log(calculate("3 - 7")); // -4

Replacing above regular expression with e.g. a much more refined ^\s*([\+\-]?(?:\d*[.])?\d+)\s*(\+|\-)\s*([\+\-]?(?:\d*[.])?\d+)\s*$/ allows you to capture floating point numbers, negative numbers and whitespace. To support more operations, simply append them to the operations object literal, e.g. '*': (left, right) => left * right and add \* to the operator capture group within the regular expression.

Answer 3

Storing value in this.summ

The first line of the method assigns 0 to the property summ (I.e. this.summ = 0;) that property appears to not be used elsewhere, other than the two lines where the value is re-assigned and then returned. Unless there are other methods that would use that property, it could be eleiminated and the lines with return could be simplified by removing the assignment (e.g. return parseInt(this.newSum[0]) + parseInt(this.newSum[1]);).

Useless for loop

It seems that the for isn’t really needed. It could simply be replaced with a check for the operator in the second array index...

Radix for parseInt

If you are going to use parseInt(), it is wise to specify the radix using the second parameter - unless you are using a unique number system like hexidecimal, octal, etc. then specify 10 for decimal numbers.

Always specify this parameter to eliminate reader confusion and to guarantee predictable behavior. Different implementations produce different results when a radix is not specified, usually defaulting the value to 10.¹

return parseInt(this.newSum[0], 10);

That way, if a value like 022 is entered, it isn't interpreted as the octal value (I.e. Decimal number 18).

---

¹_{https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt#Parameters}

Answer 4

First I pretty agree with @le_m: a simple function is absolutely enough for such a need.

Second I pretty agree with @Blindman67: seems good to enlarge the capabilities of the function, i.e.

• accepting more than 2 factors
• accepting more free use of spaces
• possibly accepting floats, not only integers

Then, taking these advantages, I suggest a way far more simpler than the ones proposed above, by merely using the unloved eval() function:

• it's main flaw is that it may be severely unsafe if not carefully used: so here we'll take the necessary steps to avoid that.
• it's generally slower than using alternatives: for the current need it seems that it shouldn't matter, but I admit that it should be accurately evaluated in the case of a massive use of the function.

Here is the simpler version (accepting only integers) of such a solution:

function calc(expr) {
 return /^( *[-+]? *\d+)+ *$/.test(expr) ? eval(expr) : undefined;
}
['3 + 7', '3 - 7', '3- 7', '3 -7', '3-7', '-3-7', '3 - 7.9', '3 - - 7', '3 - a 7', '3 a 7', '3-7-', '3-7-1', ' 37 -25']
.forEach(expr => console.log(expr, '->', calc(expr)));

As stated above, using the regexp ensures to avoid executing any malicious code which might have been put in the incoming expr.
At the same time, it eliminates any expr not-compliant with our need (but allows any number of spaces between operands/operators)

Here I reused the @Blindman67 examples, and you can see that float numbers cause the entire expr to be evaluated as undefined.
In order to accept them we only must slightly change the regexp:

function calc(expr) {
 return /^( *[-+]? *(\d+.)?\d+)+ *$/.test(expr) ? eval(expr) : undefined;
}
['3 + 7', '3 - 7', '3- 7', '3 -7', '3-7', '-3-7', '3 - 7.9', '3 - - 7', '3 - a 7', '3 a 7', '3-7-', '3-7-1', ' 37 -25', '3 + 7..2']
.forEach(expr => console.log(expr, '->', calc(expr)));

In this case I choosed to return a float rather than its rounded value when a float features in the expr. Obviously we can change our option without more effort, like this:

function calc(expr) {
 return /^( *[-+]? *(\d+.)?\d+)+ *$/.test(expr) ? Math.round(eval(expr)) : undefined;
}
['3 + 7', '3 - 7', '3- 7', '3 -7', '3-7', '-3-7', '3 - 7.9', '3 - - 7', '3 - a 7', '3 a 7', '3-7-', '3-7-1', ' 37 -25', '3 + 7..2']
.forEach(expr => console.log(expr, '->', calc(expr)));

In all these different versions the function remains pretty simple.