Polish notation interpreter/calculator

Just for the heck of it, I wrote a simple little interpreter that takes in inputs in the form of Polish Notation.

The user will input something like


+ 5 6

And it will then output


11

Here's another sample input:


+ 5 * 2 3

And the output:


11

Users can even define their own variables, like this:


def myVar 10

and like this:


def anotherVar + 5 6

And then of course reuse those variables. So let's say you type


myVar

it will ouput


10

or


anotherVar

which outputs


11

Furthermore, you type in


+ myVar anotherVar

you would then get


21

I'm a total newbie to well-formatted and readable code. What would be a good way to refactor this?

```
#include
#include
#include
#include

using namespace std;

class Variable
{
public:
Variable(const string& name, double val)
{
this->name = name;
this->val = val;
}

inline const string& get_name() const { return name; }
inline double get_val() const { return val; }
private:
string name;
double val;
};

//----------------------------------

double operate(const string& op, istringstream& iss, vector& v);
double perform_addition(istringstream& iss, vector& v);
double perform_subtraction(istringstream& iss, vector& v);
double perform_division(istringstream& iss, vector& v);
double perform_multiplication(istringstream& iss, vector& v);
void define_new_var(vector& v, istringstream& iss);

bool is_number(const string& op)
{
int char_to_int = op[0];
if (char_to_int >= 48 && char_to_int & v)
{
int size = v.size();
for (int i = 0; i v;

string temp;
while (cin)
{
cout ";

getline(cin, temp);

istringstream iss(temp);
string op;
iss >> op;

if (op == "quit")
break;
else if (op == "def")
define_new_var(v, iss);
else if (op == "show_vars")
print_var_list(v);
else
cout & v)
{

#include 
#include 
#include 
#include 
#include  // mmocny: I needed to add this to use atof
#include 

using namespace std;

//----------------------------------

class Variable
{
public:
    Variable(const string& name, double val)
        : name_(name), val_(val) // mmocny: Use initializer lists
    {
    }

    // mmocny: get_* syntax is less common in C++ than in java etc.
    const string& name() const { return name_; } // mmocny: Don't mark as inline (they already are, and its premature optimization)
    double val() const { return val_; } // mmocny: Again, don't mark as inline
private:
    string name_; // mmocny: suggest renaming name_ or _name: Easier to spot member variables in method code, and no naming conflicts with methods
    double val_;
};

//----------------------------------

// mmocny: Replace print_* methods with operator const & v)
{
    for (vector::const_iterator it = v.begin(), end = v.end(); it != end; ++it ) // mmocny: Use iterators rather than index access
    {
        out & v)
{
    // mmocny: instead of using a vector you should be using a map/unordered_map and doing a key lookup here
    int size = v.size();
    for (int i = 0; i = '0' && char_to_int & v);

//----------------------------------
/*
 * mmocny: All of your perform_* functions have identical code other than the operator being used.
 * You can turn all of these functions into a single function template where you pass the operator to be used.
 * Luckily,  already has plus minus multiplies divides function objects (functors)
 */

template
double perform_action(istream& in, vector& v, Operator op)
{
    string left;
    in >> left;

    double result = operate(left, in, v); // mmocny: This is a big one: for correctness, you must calculate result of left BEFORE you read right

    string right;
    in >> right;

    return op(result, operate(right, in, v));
}

//----------------------------------

double operate(const string& op, istream& in, vector& v)
{   
    double value;
    if (op == "+")
        value = perform_action(in, v, plus());
    else if (op == "-")
        value = perform_action(in, v, minus());
    else if(op == "*")
        value = perform_action(in, v, multiplies());
    else if (op == "/")
        value = perform_action(in, v, divides());
    else if (is_number(op))
        value = atof(op.c_str()); // mmocny: consider using boost::lexical_cast<>, or strtod (maybe)
    else
        value = get_variable(op, v);

    return value;
}

//----------------------------------

void define_new_var(vector& v, istream& in)
{
    string name;
    in >> name;

    string temp;
    in >> temp;

    double value = operate(temp, in, v);

    v.push_back(Variable(name, value));
}

//----------------------------------

int main()
{
    cout  v;

    string temp;
    while (cin)
    {
        cout  ";

        getline(cin, temp);

        if (temp.empty()) // mmocny: This also handles the case of CTRL+D
            continue;

        istringstream iss(temp);
        string op;
        iss >> op;

        if (op == "quit")
            break;
        else if (op == "def")
            define_new_var(v, iss);
        else if (op == "show_vars")
            std::cout << v << std::endl;
        else
            cout << endl << operate(op, iss, v) << endl;
    }
}

Those are my changes, with comments inline. I would make more changes, but thats enough for now :)

Notice one BIG change: you have a serious correctness bug in your perform_* functions. Not that I've tested my modified code above for all edge cases, but the original code was flat out always wrong for any nested calculations.

StackExchange Code Review Q#2211, answer score: 5