5
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ What is the benefit of using regexps for testing validity of operators, instead of a simpler solution? (E.g. indexOf in an array of all possible operators.) \$\endgroup\$
    – Attilio
    Jun 26, 2016 at 10:03
  • \$\begingroup\$ @Attilio No actual benefit. It just: I didn't had the indexOf-idea. ;) Thanks a lot for the hint. \$\endgroup\$ Jun 26, 2016 at 11:05
  • \$\begingroup\$ @Attilio or Array.prototype.includes -- looks nicer, is part of ES2016, but may need to be polyfilled. \$\endgroup\$
    – gcampbell
    Jun 26, 2016 at 11:08
  • \$\begingroup\$ @Attilo Awesome. That's even more tight. \$\endgroup\$ Jun 26, 2016 at 11:28
  • \$\begingroup\$ The regex can be shortend to /^[-+*/]$/ \$\endgroup\$
    – t3chb0t
    Jun 27, 2016 at 9:08

2 Answers 2

6
\$\begingroup\$

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);
}
\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ The .has line should be made shorter. \$\endgroup\$ Jul 9, 2016 at 18:51
  • \$\begingroup\$ Note: after tokenisation the engine has to parse the syntax into a syntax tree, then (possibly) check the semantics (I'm not sure how JS engines handle this), and then evaluate the code. You can tell I've been reading too much about compilers recently... :) \$\endgroup\$
    – gcampbell
    Jul 9, 2016 at 19:04
  • \$\begingroup\$ @gcampbell though some engines probably directly interpret a CST from parsing as I assume converting to an AST would be a performance impact. \$\endgroup\$
    – Downgoat
    Jul 9, 2016 at 19:38
  • \$\begingroup\$ @Downgoat You'd have to look inside an engine to see how they do it. But your option sounds more likely. \$\endgroup\$
    – gcampbell
    Jul 9, 2016 at 19:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.