Somewhat Advanced C++ Calculator

I am currently in the process of learning C++ from C++ Primer. I have found the exercises in this book to be somewhat dull in that they only test syntax and not reasoning. As such, I have recently written a somewhat advanced C++ calculator. It is advanced in that it can interpret more complex ideas than a simple operation on two numbers. It is only somewhat advanced in that it doesn't understand more complex operations.

It works by asking for an equation and then shows somewhat condensed steps to solve it.

For example, an input of 10242+20484 yields the following steps to solve:

1024*2+2048*4
2048+8192
10240

Entering illegal equations causes the program to crash. Based on my Java knowledge, having the equation parsing function throw and exception on illegal inputs would be best. But, since I have not yet learned the syntax for C++ exceptions, I have decided to simply leave it as is. I would, however, be interested to know about bugs that occur with legal input.

Input should consist of a number and then an operation alternating as many times as the user wants. The only legal operations are addition (+), subtraction (-), multiplication (*), and division (/). Input should also not contain any spaces.

I would like to know if my design decisions (ie. where ranged for and iterators are used) is good. If not, how I can improve upon it and why the other method is better. I would also like to know of any bugs that are not related to parsing illegal input.

The code:

So the code can be read easier (multiple tabs, line numbers, etc.) here is an offsite link to the code: https://gist.github.com/john01dav/44e2afc51d608c84ee45

For those who would prefer to read the code in the question:

main.cpp:

```
#include
#include

#include "term.h"

using std::cout;
using std::endl;
using std::cin;

using std::vector;

using std::string;

int main(){
string equationString;
vector equation;

cout > equationString;

equation = parseEquation(equat

I see a number of things that may help your improve your code.

Fix the bugs

Right now, the program has a very strange notion of mathematics:

Enter an equation: 
10-2*3+2*2-7

10-2*3+2*2-7
10-6+4-7
10-10-7       <== This is not correct!
0-7
-7

There is another problem with operator precedence. I'd expect 1/2*4 to evaluate from left to right and yield an answer of 2 but that's not what the program does:

Enter an equation: 
1/2*4

1/2*4
1/8
0.125

Also, operators that are unknown to the program cause an infinite loop. Try 1z2.

Simplify your code

Within the parseEquation() code, we have this:

if(location <= equationString.size()){
    term.operation = equationString[location];
}else{
    term.operation = '\0';
}

However, this can be simplified to the single line:

term.operation = equationString[location];

This is safe because a std::string is guaranteed to be terminated with a NUL character (that is, '\0').

Don't hide the loop exit condition

The parseEquation() routine currently hides the loop exit condition way down inside a nested if. What we are really trying to do is process the incoming string until there's no more string left to parse. You can write that much more simply like this:

vector parseEquation(string equationString){
    string::size_type location = 0;
    vector equation;

    while(equationString.size()){
        Term term;
        term.number = stod(equationString, &location);
        term.operation = equationString[location++];
        equation.push_back(term);
        equationString.erase(0,location);
    }
    return equation;
}

Return something useful from functions

The current version of the two-parameter simplifyEquation() function doesn't return anything, but each time it's actually used, it's followed by a check to see if the size has changed. Instead, have it return a bool that indicates that the size has changed. If you do that, the code for the single-parameter version can be reduced to this:

void simplifyEquation(vector &equation){
    simplifyEquation(equation, '*') 
        || simplifyEquation(equation, '/')
        || simplifyEquation(equation, '-')
        || simplifyEquation(equation, '+');
}

This relies on "short circuit evaluation"; the operations will be done in the listed order until one of them returns true. (Note however that it still is not correct, as noted in the first comment. The '*' operator should not have precedent over '/'.)

Minimize the public interface

The header file is the representation of the public interface of your code. The two-parameter version of simplifyEquation() appears to me to be just an internal helper function which would not normally be used by programs directly. For that reason, I'd advise making it static within the term.cpp file and omitting it from the header file.

Reconsider your design

Right now, the code has a equation object and a few operations by which it is manipulated. Doesn't that sound a lot like an object? I'd recommend making an Equation class and turning your existing functions into member functions.

StackExchange Code Review Q#117753, answer score: 3