While loop terminates after one iteration

Question

I am trying to write a function in C++ to evaluate a postfix notation equation. My general strategy is to scan a string (in the proper format, e.g. "10 20 + 30 -").
I am doing this by incrementing an index variable i. At each increment, I check to see if the character is a digit, operator, or neither. If it's a digit, I use the getNextNum() function to get all following digits, convert that to a float, then push it to a stack. I also increment i by the length of the number captured. If the character is an operator, I get the top two elements of the stack, do the operation, then push the result back to the stack.

The trouble is, my while loop only seems to go through once. The function only returns the first number in the string. I can't figure out what's wrong, I would appreciate any help! I inserted cout statements in the while loop, and i is only incrementing to the index after the first number.

EDIT: Ok, I added the getNextNum() function. Also, I updated the evalPostfix() with a cout of strLength, as well as i after each iteration of the while loop. When running the given code, I get this:

Running…
Please enter an expression in postfix notation: 555 666+
3
555
3
Your expression evaluates to: 555

It seems like strLength is being set to less than it should. Why could this be?

#include <iostream>
#include <string>
#include <vector>
#include <deque>
#include <stack>
using namespace std;

string getNextNum(string equation, int i);
float evalPostfix(string postfix);
float doOperation(float x, float y, char op);

float doOperation(float x, float y, char op)
{
    switch (op) {
        case '+':
            return x + y;
        case '-':
            return x - y;
        case '*':
            return x * y;
        case '/':
            return x / y;
        default:
            return 0.0;
    }
}


string getNextNum(string equation, int i)
{
    string num = "";
    const string DELIM = "+-*/%^ ";
    while (i<equation.length()) {
        // Iterate through equation until you hit a delimiter.
        if (DELIM.find(equation[i]) != -1) {
            break;
        }
        num += equation[i];
        i++;
    }
    return num;
}

float evalPostfix(string postfix)
{
    const string OPS = "+-*/%^";
    const string NUMS = "0123456789";
    int strLength = postfix.length();
    stack<float> numStack;
    int i = 0;
    cout << strLength << endl;
    while (i<strLength) {
        if (NUMS.find(postfix[i]) != -1) {
            // If a character is a digit, then you should get the 
            // value and push it to the stack (could be multiple characters long).
            string sNextNum = getNextNum(postfix, i);
            float fNextNum = atof(sNextNum.c_str());

            numStack.push(fNextNum);
            cout << sNextNum << endl;
            i += (sNextNum.length() - 1);
            }
        else if (OPS.find(postfix[i] != -1)) {
            // Otherwise, pop the top two elements of the stack, perform the
            // operation, then push the result back to the stack.
            char op = postfix[i];

            float x = numStack.top();
            numStack.pop();

            float y = numStack.top();
            numStack.pop();

            float z = doOperation(x, y, op);
            numStack.push(z);
            }
        i++;
        cout << i << endl;
    };

    // Once the entire string has been scanned through, there should be a float
    // left in the stack, simply return that.
    return numStack.top();
}

int main ()
{
    cout << "Please enter an expression in postfix notation: ";
    string postfix;
    cin >> postfix;

    float eval = evalPostfix(postfix);
    cout << "Your expression evaluates to: " << eval;
    return 0;
}

Answer 1

One primary problem was that the input cin >> postfix; statement only reads the first word. Echo inputs to ensure that the program is seeing what you think it is seeing.

Shafik Yaghmour points out another problem.

Points to learn:

1. echo inputs to make sure the program is seeing what you think it is seeing;
2. trace key variables with suitable printing messages;
3. post SSCCE (Short, Self-Contained, Correct Example) — code that can be compiled;
4. post example input and the output you're getting from it.

This code works on input 555 666+:

#include <iostream>
#include <string>
#include <stack>
using namespace std;

static float doOperation(float x, float y, char op)
{
    cout << "doOp: x = " << x << ", y = " << y << ", op = " << op << endl;
    if (op == '+')
        x += y;
    return x;
}

string getNextNum(string equation, int i)
{
    string num = "";
    const string DELIM = "+-*/%^ ";
    while (i<equation.length()) {
        // Iterate through equation until you hit a delimiter.
        if (DELIM.find(equation[i]) != -1) {
            break;
        }
        num += equation[i];
        i++;
    }
    return num;
}

float evalPostfix(string postfix)
{
    const string OPS = "+-*/%^";
    const string NUMS = "0123456789";
    int strLength = postfix.length();
    stack<float> numStack;
    int i = 0;
    while (i<strLength) {
        cout << "Top - i: " << i << ", strLength: " << strLength << endl;
        if (NUMS.find(postfix[i]) != -1) {
            // If a character is a digit, then you should get the 
            // value and push it to the stack (could be multiple characters long).
            string sNextNum = getNextNum(postfix, i);
            float fNextNum = atof(sNextNum.c_str());

            numStack.push(fNextNum);
            cout << sNextNum << endl;
            i += (sNextNum.length() - 1);
            }
        else if (OPS.find(postfix[i])!= -1) {
            // Otherwise, pop the top two elements of the stack, perform the
            // operation, then push the result back to the stack.
            char op = postfix[i];

            float x = numStack.top();
            numStack.pop();

            float y = numStack.top();
            numStack.pop();

            float z = doOperation(x, y, op);
            numStack.push(z);
            }
        i++;
        cout << "End - i: " << i << ", strLength: " << strLength << endl;
    }
    cout << "After - i: " << i << ", strLength: " << strLength << endl;

    // Once the entire string has been scanned through, there should be a float
    // left in the stack, simply return that.
    return numStack.top();
}

int main ()
{
    cout << "Please enter an expression in postfix notation: ";
    string postfix;
    //cin >> postfix;
    if (getline(cin, postfix))
    {
        cout << "Evaluating: " << postfix << endl;
        float eval = evalPostfix(postfix);
        cout << "Your expression evaluates to: " << eval << endl;
    }
    return 0;
}

Sample trace:

Please enter an expression in postfix notation: 555 666+
Evaluating: 555 666+
Top - i: 0, strLength: 8
555
End - i: 3, strLength: 8
Top - i: 3, strLength: 8
End - i: 4, strLength: 8
Top - i: 4, strLength: 8
666
End - i: 7, strLength: 8
Top - i: 7, strLength: 8
doOp: x = 666, y = 555, op = +
End - i: 8, strLength: 8
After - i: 8, strLength: 8
Your expression evaluates to: 1221

Clearly, you can lose much of the diagnostic output once the specific problem you are solving is resolved, but being prepared to add it along the lines shown can dramatically speed up the process of solving it.

Answer 2

You have a few issues one of the major one being a typo, you have a misplaced ) this:

else if (OPS.find( postfix[i] != -1 ) ) {
                 ^                  ^ 

should be:

else if (OPS.find( postfix[i] ) != std::string::npos) {
                 ^            ^  

so you are comparing the char at position i to -1 and then doing a find on the boolean result. Next you should be using -1 to compare the results of find but std::string::npos

As Jonathan pointed out:

cin >> postfix ;

only read up to the first black or newline. Using getline will fix that problem:

if (getline(cin, postfix))