Basic arithmetic calculator in JavaScript

Question

Task is basically:

"Write a function which takes two number as the first two parameter. The third parameter is one of the arithmetic operators (+ , - , * , /). Execute the arithmetic operation (number1 operator number2) and return the result of it."

Here's the first idea I've had:

function calculate(num1, num2, operator) {
  num1 = parseFloat(num1);

  if (isNaN(num1)) {
    throw Error('First parameter invalid. Must be a number.');
  }

  num2 = parseFloat(num2);

  if (isNaN(num2)) {
    throw Error('Second parameter invalid. Must be a number.');
  }

  operator = operator.trim(); 

  if (!/^\+$|^\-$|^\*$|^\/$/.test(operator)) {
    throw Error('Third parameter invalid. Valid are + | - | * | /');
  }

  return eval(num1 + operator + num2);
}

Then I've had another idea for the implementation which uses the ES6-syntax:

function calculate2(num1, num2, operator) {
  num1 = parseFloat(num1);

  if (isNaN(num1)) {
    throw Error('First parameter invalid. Must be a number.');
  }

  num2 = parseFloat(num2);

  if (isNaN(num2)) {
    throw Error('Second parameter invalid. Must be a number.');
  }

  operator = operator.trim(); 

  if (!/^\+$|^\-$|^\*$|^\/$/.test(operator)) {
    throw Error('Third parameter invalid. Valid are + | - | * | /');
  }

  var calculations = {
    '+' : (numb1, numb2) => numb1 + numb2,
    '-' : (numb1, numb2) => numb1 - numb2,
    '*' : (numb1, numb2) => numb1 * numb2,
    '/' : (numb1, numb2) => numb1 / numb2
  }

  return calculations[operator](num1, num2);
}

I still like the eval-version a bit better.

Can't see any harm which eval should create here because it's controlled via regular expression what is passed until that point.

What's your opinion?

Which implementation is better one?

Looking forward reading your answers.

Answer 1

I would go with the second one. Not because eval here is insecure -- you're validating the arguments -- but because it's overkill. eval is designed to parse and run any JavaScript code, so it is slow (compared to normal JavaScript code); engines do a lot of optimisation before running code, but they can't optimise if the code is in a dynamic string which changes at runtime. It probably won't maker much difference for you here, but if you're building a larger app, blocking JavaScript can be noticeable.

Here are the changes I would make to calculate2:

'use strict'; // Strict mode === good

function calculate2(num1, num2, operator) {
  // Number.parseFloat is new in ES6
  // (it's identical to window.parseFloat, so you can use either)
  num1 = Number.parseFloat(num1);
  num2 = Number.parseFloat(num2);
  operator = operator.trim();

  // Avoid `var`. ES6 brings `let` and `const`, which:
  //   - are block scoped
  //   - work with destructuring
  //   - prevent redeclaration in the same scope
  //   - aren't hoisted
  // Note that `const` doesn't freeze object properties
  // If you need frozen properties, use Object.freeze
  const calculations = {
    // Shorter names, it's clear from the context what they do.
    '+' : (a, b) => a + b,
    '-' : (a, b) => a - b,
    '*' : (a, b) => a * b,
    '/' : (a, b) => a / b, // You can use trailing commas in object and array literals if you want.
  }; // If you use semicolons, use them all the time.

  // Detailed error messages.
  // 
  // Number.isNan is also new in ES6,
  // however it's not quite the same as window.isNan.
  // Here it won't make a difference which you use.
  // 
  // `throw new Error` rather than `throw Error`
  // as you're using the Error constructor.
  if (Number.isNan(num1)) throw new Error(
    `Expected first argument to calculate2 to be a String valid for Number.parseFloat but got ${num1}.`);
  if (Number.isNan(num2)) throw new Error(
    `Expected second argument to calculate2 to be a String valid for Number.parseFloat but got ${num2}.`);

  // Don't bother maintaining a separate regex of valid operators
  // Just check the calculations object
  if (!calculations[operator]) throw new Error(
    `Expected third argument to calculate2 to be one of ${Object.keys(calculations).join(' ')} but got ${operator}.`);

  return calculations[operator](num1, num2);
}

Answer 2

Adding onto @gcampbell's answer. Especially with ES6, I'd start to move away from objects. ES6 brings us the lovely Map class which is much, much, better than objects.

Objects are evil, they have no proper way to check if a property is actually not there. Looping through them is ugly and you can't use anything as a key.

Here's how to use map in your case:

  const calculations = {
    // Shorter names, it's clear from the context what they do.
    '+' : (a, b) => a + b,
    '-' : (a, b) => a - b,
    '*' : (a, b) => a * b,
    '/' : (a, b) => a / b, // You can use trailing commas in object and array literals if you want.
  }; // If you use semicolons, use them all the time.

You can replace your calculations object with:

  const calculations = new Map([
    ['+' , (a, b) => a + b],
    ['-' , (a, b) => a - b],
    ['*' , (a, b) => a * b],
    ['/' , (a, b) => a / b]
  ]);

---

For checking if it has the key you can use Map#has:

if (!calculations.has(operator))
    throw Error(`Operator was invalid, must be ${Array.from(calculations.keys()).join(" or ")}`);

---

Then for getting the function you can use Map#get:

return calculations.get(operator)(num1, num2);

The second version is much better

Everyone says eval is insecure, but that's not all. It is ridiculously slow. The string which has already been parsed has to become a string, it has to be cast, the engine has to start again, re-tokenize the string, evaluate it, and extract the last value. You should never use eval in production code.