Calculate the value of an arithmetic term written in Polish Notation I have to create a program which calculates the result of an arithmetic term written in Polish Notation.



var parseNotation = parseNotation || {};

/**
 * Computes the result of an given arithmetic term which is written in
 *  Reverse Polish Notation (https://en.wikipedia.org/wiki/Reverse_Polish_notation).
 * @param { String } termInReversePolish - The operators / operands of
 *  the term have to be separated by colons. See usage example!
 * @return { number } Result of the given term.
 * @throws { Error } Will throw an error in case of no or invalid parameter
 *  is given.
 * 
 * Usage example (will display 37 as result):
 *  console.log(parseNotation.computeFromReversePolish('5,6,2,+,*,12,4,/,-'));
 */

parseNotation.computeFromReversePolish = function(termInReversePolish) {
  var currentChar = '';
  var stack = [];
  var right = 0;

  if (typeof termInReversePolish !== 'string') {
    throw new Error(
      `No valid parameter given. String expected but ${typeof termInReversePolish} found.`);
  }
  
  if (termInReversePolish.search(/[^\d,+*\/-]/g) != -1) {
      throw new Error(
        'Given parameter contains illegal characters. Valid characters are: + - * / , 0-9');
  }

  termInReversePolish = termInReversePolish.split(',');

  termInReversePolish.forEach((currentChar, i) =&gt; {
    if (['+', '-', '*', '/'].indexOf(currentChar) === -1) {
      stack.push(currentChar);
    } else {
      right = stack.pop();

      stack.push(eval(stack.pop() + currentChar + right));
    }
  });
  
  return stack.pop();
}

// ---- Testing ------------------
console.log(parseNotation.computeFromReversePolish( '1,2,+,3,4,+,*' ));
console.log(parseNotation.computeFromReversePolish( '5,6,2,+,*,12,4,/,-' ));
console.log(parseNotation.computeFromReversePolish( '2,7,+3,/,14,3,-,4,*,+,2,/' ));




My first few test with the function have gone well. Nevertheless, I would like to read the opinion of other developer, especially regarding:


Is my documentation done in a proper way? Or does I have to improve it?
Is my parameter validation sufficient? Especially regarding the used regular expression.
Generally: Is everything done in a good way and fashion?


If someone sees an error or flaw then please let me know.
To start at the end:


  Generally: Is everything done in a good way and fashion?


It looks quite good, kudos!

Structurally, there's one thing that's not quite right, though: You're declaring currentChar and right in the function's scope, you really don't need to. The currentChar you've declared never gets used; the argument in the forEach callback shadows it (and it's not necessarily a character - it could be "12312"). And right doesn't have to be in that scope either, it can just be local to the callback.

And you could technically use reduce instead of forEach with a closed-over stack, but that's not so bad.

I also have some more notes, but also I'm nit-picking quite a bit. I had to find something to write, y'know?

Your documentation seems fine. The exact flavor of documentation syntax is more or less up to you (JS doesn't have a strong convention), so the JavaDoc-like syntax is fine. Would also have accepted a regular comment without the @-keywords, so long as the important parts are covered.

You might consider some comments in the function body, though. There isn't much need for it here admittedly, but just to say that it's not just about commenting the API but also the code.

The regex validation seems fine too, but it doesn't catch malformed syntax, only illegal tokens. A string like ",,," wouldn't trigger anything, nor would a string like "3,+".

The latter ("3,+") returns NaN, which is sort of OK. You can evoke the rule of "Garbage in, garbage out" and blame the caller. At least they're getting a Number back (yes, NaN - not a number - is in fact a Number. Go figure.) 

The former input, however, returns an empty string. Since no operations have been performed, the final stack.pop() will be an empty string. So here, you're not even getting a number. That seems less correct.

Ideally, you would complain if the function reaches the end with something other than 1 element in the stack, since that would imply that the RPN expression wasn't "balanced". In fact, your third example suffers from this; the stack ends at [2, 23.1666...], but you report the 23.166... as the answer despite the expression not being fully parsed.

It would also be nice to allow (and ignore) whitespace in the string, just to make input a little flexible. A more readable input string might be beneficial to users (I tested your code, and kept putting in spaces after commas out of habit).

There's another thing your code doesn't handle: Anything with decimals. Since you can't use . in the string, you're limited to integers. That's a bit harsh. You could also consider allowing other legal JS number formats like 0x3 hex, 0b1010 binary, or 3.14e3, but that's more just an exercise for you. eval should already handle them fine, but your regex will complain.

Last bit about validation: I'd throw a TypeError if the input isn't a string. More accurate than a generic Error certainly.

That's mostly smaller stuff, and easily fixed or left as-is. The only thing I'd do very differently would be to not use eval. If you instead define the operators as functions, you:


get rid of eval, which is always nicer in my opinion
you can implement operators that don't take 2 operands such as factorial
you can easily implement more operators (right now, you'll have to change both the regex and the array in the loop)


And if you store your functions in an object, you get quicker lookup than with indexOf (not that that really matters that much).

For instance:

function rpn(expression) {
  // define our operators as functions
  const operators = {
    '+': (a, b) =&gt; a + b,
    '-': (a, b) =&gt; a - b,
    '*': (a, b) =&gt; a * b,
    '/': (a, b) =&gt; a / b,
    '^': Math.pow
  };

  // check input type
  if (typeof expression !== 'string') {
    throw new TypeError(`No valid parameter given. Expected string but got ${typeof expression}.`);
  }

  // evaluate the expression, ignoring whitespace
  var stack = expression.replace(/\s/g, '').split(',').reduce((stack, term) =&gt; {
    if (operators.hasOwnProperty(term)) {
      const op = operators[term];

      // check stack depth
      if (stack.length &lt; op.length) {
        throw new Error("Stack underflow");
      }

      // grab n elements from the stack, where n is the number of arguments
      // that the operator function takes
      var operands = stack.splice(-op.length);

      // perform the operation and push the result
      stack.push(op.apply(null, operands));
    } else {
      var number = term * 1; // coerce the term to a number

      // check that the coercion produced something numeric
      if (Number.isNaN(number)) {
        throw new Error(`The term '${term}' is neither numeric or a recognized operator`);
      }

      stack.push(number);
    }

    return stack;
  }, []);

  if (stack.length !== 1) {
    throw new Error("Incomplete expression"); // find something better to say
  }

  return stack[0];
}