Reverse Polish Notation Calculator in C++

Question

I'm trying to code the RPN algorithm in C++, using a string as parameter. I'm using a stack and a string to be read.
I know how the algorithm works, but I don't get a correct result, I always get 0. This is my code:

#include <iostream>
#include <stack>
#include <sstream>
#include <vector>
#include <string>

using namespace std;

float RPN(string s) {
    stack<int> p;
    int n1, n2;

    string values[s.size()];
    for (unsigned int i = 0; i < s.size(); i++)
        values[i] = s.at(i);

    for (unsigned int i = 0; i < s.size(); i++) {
        if (s.at(i) != '+' || s.at(i) != '-' || s.at(i) != '*'
                || s.at(i) != '/') {
            int n;
            istringstream(values[i]) >> n;
            p.push(n);
        }

        else {
            n1 = p.top();
            p.pop();
            n2 = p.top();
            p.pop();
            switch (s.at(i)) {
            case '+':
                p.push(n1 + n2);
                break;
            case '-':
                p.push(n2 - n1);
                break;
            case '*':
                p.push(n1 * n2);
                break;
            case '/':
                p.push(n2 / n1);
                break;
            }
        }
    }

    if (p.size()) {
        int resul = p.top();
        while (p.size())
            p.pop();
        return resul;
    }
    return 0;
}

This is the calling method:

void callRPN() {

    string s1 = "56-";
    string s2 = "65-";
    string s3 = "843+2*-";
    string s4 = "62+83-*4/";

    cout << s1 << " valor: " << RPN(s1) << endl;
    cout << s2 << " valor: " << RPN(s2) << endl;
    cout << s3 << " valor: " << RPN(s3) << endl;
    cout << s4 << " valor: " << RPN(s4) << endl;
}

And the console result:

56- valor: 0
65- valor: 0
843+2*- valor: 0
62+83-*4/ valor: 0

What is the error in my code? If someone could help me I would appreciate it. Thank you.

Answer 1

At least one major problem is that the inputs are invalid (they're not proper Reverse Polish Notation) and you don't check for invalid inputs.

A few points about the algorithm:

int resul = p.top();
// At this point, p.size() should be 1 - it should only contain the answer.
// If it's anything other than 1, the expression you were passed is wrong
// and you should throw an exception.
while (p.size())
    p.pop();

Also, you should check to make sure that there are enough items on the stack when you do this:

n1 = p.top();
p.pop();
n2 = p.top();
p.pop();

If there aren't enough, you should throw an exception because that means that the input is invalid.

At least some of your Reverse Polish Notation inputs are incorrect, depending on how they're interpreted. For example, should 56- be interpreted as 5 - 6 or as 56 -? If it's the latter, then the following is incorrect:

62+83-*4/

This should actually throw an exception for the same reason as above. Try tracing through this; when you do 62 +, for example, 62 plus what? The correct way to do 62 + 83 in Reverse Polish Notation is 62 83 +. At that point, the stack should contain 145 and only 145 (meaning that it's invalid to do - or * at this point). If you're trying to do (62 + 83) / 4, the correct encoding is:

62 83 + 4 /

It's unclear what the -* part is supposed to do for the same reasons I mentioned above.

So, really, this should validate the input.

Perhaps more importantly, the following is incorrect:

if (s.at(i) != '+' || s.at(i) != '-' || s.at(i) != '*'
            || s.at(i) != '/') {
        int n;
        istringstream(values[i]) >> n;
        p.push(n);
    }

You should replace || with && here; you want to push onto the stack if it isn't any of the operators. In this case, it'll only push onto the stack if it's not a + operator (meaning that it'll try to push onto the stack if it's, for example, a - operator).