Refactoring a Simple Calculator that is using a series of loops

Question

I am fairly novice when it comes to programming and javascript in general, and I'm running through some example questions to get better over at CodeWars.com

The question I just completed asks the following:

Write a function called calculate that takes 3 values. The first and third values are numbers. The second value is a character. If the character is "+" , "-", "*", or "/", the function will return the result of the corresponding mathematical function on the two numbers. If the string is not one of the specified characters, the function should return null.

It gives you this following code to start out with:

function calculate(num1, operation, num2) {
 //TODO: make a basic calculator.

It checks your code against the following test cases:

Test.assertSimilar(calculate(3.2,"+", 8), 11.2, "3.2 + 8 = 11.2")
Test.assertSimilar(calculate(3.2,"-", 8), -4.8, "3.2 - 8 = -4.8")
Test.assertSimilar(calculate(3.2,"/", 8), 0.4, "3.2 / 8 = .4")
Test.assertSimilar(calculate(3.2,"*", 8), 25.6, "3.2 * 8 = 25.6")
Test.assertSimilar(calculate(-3,"+", 0), -3, "-3 + 0 = -3")
Test.assertSimilar(calculate(-3,"-", 0), -3, "-3 - 0 = -3")
Test.assertSimilar(calculate(-3,"/", 0), null, "-3 / 0 = null")
Test.assertSimilar(calculate(-3,"*", 0), 0, "-3 * 0 = 0")
Test.assertSimilar(calculate(-3,"w", 0), null, "-3 w 0 = null")

I completed all of the test cases by running through a series of if and else if loops ( JSFiddle ):

function calculate(num1, operation, num2) {
 //TODO: make a basic calculator.
 if (operation === '+'){
   console.log(num1 + num2);
 }
 else if (operation === '-'){
   console.log(num1 - num2);
 }
 else if (operation === '/'){
   console.log(num1 / num2);
 }
 else if (operation === '*'){
   console.log(num1 * num2);
 }
 else{
   return null;
 }
}
calculate(3.2,"*", 8);
calculate(-3,"+", 0);
calculate(-3,"-", 0);

I know this isn't the most elegant solution, and I would love to find ways to improve and simplify the code. The hardest part about starting something new is being able to ask the right questions, so I would love if I could get some input on how to better simplify my code!

Thanks

possible duplicate of [jQuery if statement with variable mathematical operator](http://stackoverflow.com/questions/12961085/jquery-if-statement-with-variable-mathematical-operator). You could easily modify my code to support arithmetical instead of comparative operations. — Bergi, Mar 07 '14 at 15:13
You should avoid statements like "Easily", something that is easy for you may not be easy for someone else. — Mathias Rechtzigel, Mar 07 '14 at 16:02
OK, maybe there is not so much triviality as I think. However, it's easier to "just" modify the `validOps` expression than to understand (or even come up with) the whole code :-) — Bergi, Mar 07 '14 at 18:33
The reason I posted was so that I could better understand the entirety of the code, and to find better ways to do it in the future. Thanks for the link, I'll look through it. — Mathias Rechtzigel, Mar 07 '14 at 18:39

Brett Zamir · Accepted Answer · 2014-03-07T15:29:24.870

You could use a switch.

switch (operation) {
    case '+':
        console.log(num1 + num2);
        break;
    case '-':
        console.log(num1 - num2);
        break;
    case '/':
        console.log(num1 / num2);
        break;
    case '*':
        console.log(num1 * num2);
        break;
    default:
        return null;
}

JSFiddle

Some don't like switches as you have to remember to explicitly include break statements (or return or throw where appropriate) to avoid the code falling through to the next case and potentially causing unexpected consequences (though sometimes this ability to fall-through can be a convenience if you are doing it deliberately).

The main advantage of a switch here, besides avoiding repeating the variable name, is that, for those familiar with it, it is more easy to grasp at a glance what is going on than it is to have to check each if condition to see exactly what the condition is about since each if could be checking something different. A switch is always only checking the result of just one expression.

(Btw, it is probably a better idea to throw an exception than to return null since that stops execution rather than keep going with potentially unexpected behavior in case you accidentally allow a mistaken operator into the function.)

And if simplification alone is your goal (and not security, since the following is not safe with untrusted input based on its use of eval), you could do the following:

function calculate(num1, operation, num2) {
    if (!operation.match(/^[+\/*-]$/)) {
        throw 'Unexpected operation ' + operation;
    }
    console.log(eval(num1 + operation + num2));
}

JSFiddle

Awesome! Thanks this is a lot of information to sift through. I'll tinker around with it! — Mathias Rechtzigel, Mar 07 '14 at 16:01

Refactoring a Simple Calculator that is using a series of loops

1 Answers1