4
\$\begingroup\$

I tried to make a smart calculator(which does multiplication, division and modulus first) and this is what I ended up with.

I used the following logic : We must get a value, then a sign, then a value, another sign, etc. If we get something else, the expression is invalid so we just exit.

All of the values and operators are in vectors, and calculation goes like this:

0---1----2

7 + 4 * 9 =

---0---1

We scan for multiplication, division and modulus. In the signs vector we get an '*', where i = 1, so we do values.at(i) * values.at(i + 1), we put the result in values.at(i) and we delete values.at(i + 1). Then we scan for the other operators with the same logic, so we will end up with one result.

    #include <iostream>
    #include <vector>
    #include <cmath>
    using namespace std;

    void do_mult_div_mod(vector<double> &values, vector<char> &signs)
    {
        for(int i{0};i < signs.size(); ++i)
        {
            if(signs.size() != 0 && signs.at(i) == '*')
            {
                values.at(i) = values.at(i)*values.at(i+1);
                values.erase(values.begin()+i+1);
                signs.erase(signs.begin()+i);
                --i; // removed an element, so --i, then ++i so i remains at the same value
            }

            if(signs.size() != 0 && signs.at(i) == '/')
            {
                if(values.at(i+1) == 0)
                {
                    cout << "Infinite" << '\n';
                    exit(1);
                }
                values.at(i) = values.at(i)/values.at(i+1);
                values.erase(values.begin()+i+1);
                signs.erase(signs.begin()+i);
                --i;
            }

            if(signs.size() != 0 && signs.at(i) == '%')
            {
               values.at(i) = fmod(values.at(i), values.at(i+1));
               values.erase(values.begin()+i+1);
               signs.erase(signs.begin()+i);
               --i;
            }
        }
     }

double do_add_sub(vector<double> &values, vector<char> &signs)
{
    double result{values.at(0)};
    for(int i{0}; i < signs.size(); ++i)
    {
        if(signs.at(i) == '+')
            result += values.at(i+1);
        if(signs.at(i) == '-')
            result -= values.at(i+1);
    }

    return result;
}

double do_parenthesis()
{
    // get local vectors of values and signs and make a mini-expression
    int nr;
    vector<double> values;

    char op{'0'};
    vector<char> oper;

    while(op != ')')
    {
        if( cin >> nr && cin >> op)
        {
            values.push_back(nr);
            if(op == ')')
                break;

            if(op == '=' || cin.peek() == '=') // if we gan an '=' here, the expression is invalid, as we didn't get an ')' yet
            {
                cout << "Invalid Expression" << '\n';
                exit(2);
            }

            oper.push_back(op);

            char buff;
            if(cin.peek() == '(') // recursion for multiple parenthesis
            {
                cin >> buff;
                values.push_back(do_parenthesis());
                if(cin >> op)
                {
                    if(op == ')')
                        break;
                    if(op == '=')
                    {
                        cout << "Invalid Expression" << '\n';
                        exit(3);
                    }
                    else
                        oper.push_back(op);
                }
                else
                {
                    cout << "Invalid Expression" << '\n';
                    exit(4);
                }
            }
        }
        else
        {
            cout << "Invalid Expression" << '\n';
            exit(5);
        }
    }


    do_mult_div_mod(values, oper);
    double fin{do_add_sub(values, oper)};

    return fin;
}

void get_expression(vector<double> &values, vector<char> &signs)
{
    double num;
    char cha;
    for(;;)  // loop until we get an '='
    {
        if(cin >> num && cin >> cha) // check if the expression we get is valid
        {
            values.push_back(num);
            if(cha == '=')  //stop
                break;
            signs.push_back(cha);

            if(cin.peek() == '(')  // check if the next token is a parenthesis
            {
                char buffer;
                cin >> buffer;
                values.push_back(do_parenthesis()); // if yes, return the result from calculating the parenthesis
                if(cin >> cha) // after a parenthesis, we must get a sign, otherwise we throw an error
                {
                    if(cha == '=')
                        break;
                    else
                        signs.push_back(cha);
                }
                else
                {
                     cout << "Invalid Expression" << '\n';
                     exit(6);
                }
            }

        }
        else
        {
            cout << "Invalid Expression" << '\n';
            exit(7);
        }
    }
}

int main() {

    vector<double> values;
    vector<char> signs;

    get_expression(values,signs);
    do_mult_div_mod(values, signs);

    double result{do_add_sub(values, signs)};

    cout << result;

    return 0;
}

What mistakes have I made? How could I improve this code?

\$\endgroup\$
  • \$\begingroup\$ Can you explain more what the "0---1----2" and "---0---1" mean? I don't understand what that part is about. \$\endgroup\$ – user1118321 Jan 11 '18 at 15:57
  • \$\begingroup\$ it's the position of the elements in the vectors \$\endgroup\$ – Semetg Jan 11 '18 at 15:57
  • \$\begingroup\$ So is the user supposed to enter that, or are you just showing where they are? I don't understand. \$\endgroup\$ – user1118321 Jan 11 '18 at 16:09
  • \$\begingroup\$ I just thought it would be easier for others to understand my logic if I wrote that part of the question, those numbers are just the indexes of the elements in the vectors, I thought they could help visualise the algorithm for calculating an operation between 2 numbers. The user has to enter just the expression. \$\endgroup\$ – Semetg Jan 11 '18 at 16:12
  • \$\begingroup\$ OK, cool. Just wanted to make sure I understood. Thanks! \$\endgroup\$ – user1118321 Jan 11 '18 at 18:05
2
\$\begingroup\$

Overall, this is fairly straightforward and easy to read. I really like that you broke getting the expression from the user into its own function separate from parsing it. That's a good way to separate things out. Here are some additional thoughts.

Avoid using namespace std

In general, you should avoid writing using namespace std. It's only a few extra characters to write std:: in front of the types that need it.

Naming

Your variable naming could use some work. In do_mult_div_mod() you've named the arguments to the function values and signs. values is kind of a weak name. I'd normally let it go in this case since it's purely a mathematical function and the argument doesn't correspond to a real-world object or concept. However, signs is just wrong. You aren't passing in signs (which would be positive or negative), you are passing in operations you want to perform on the values. So I would recommend renaming them operands instead of values and operators instead of signs.

Likewise in do_parenthesis() you have one variable named nr. I suppose this is supposed to be a number? If so, just call it number. Then you have both op and oper. Are they operator and operand? No, it turns out they're both operators, but you can't have 2 variables of the same name. So I would rename all the variables like this:

int nextOperand;
vector<double> operands;
char nextOperator{'0'};
vector<char> operators;

That would make everything clearer in my opinion.

And again with num and cha in get_expression(). You're basically just repeating the type name as the variable name, and that's not helpful. Names should tell you what the variables represent. Again, they appear to represent operators and operands.

Handling Errors

It's nice to see that you've at least thought about error handling rather than assuming that all inputs will be completely valid and nothing will ever go wrong! However, your handling is a bit extreme. Rather than unceremoniously exiting the application (which is something you'll likely never be allowed to do in production code in the real world), you should return an error to the caller or throw an exception (and of course catch and handle it elsewhere). What this will look like is something like this:

typedef enum CalcErr {
    CE_NoError = 0,
    CE_DivideByZero = 1,
    CE_MissingParens = 2,
    // ... etc.
} CalcErr;

CalcErr do_mult_div_mod(vector<double> &values, vector<char> &signs)
{
    CalcErr result = CE_NoError;

    for (int i{0}; i < signs.size() && result == CE_NoError; ++i)
    {
        // ...etc.
        if (signs.size() != 0 && signs.at(i) == '/')
        {
            if (values.at(i+1) == 0)
            {
                result = CE_DivideByZero;
            }
            else
            {
                //... etc.
            }
        // ... etc.
    }

    return result;
}

If you do it this way, it also helps to separate out the logic for calculations from the display of the results. That's often referred to as "separating the business logic from the presentation logic." It allows you to re-use the calculation code in a GUI app, too, for example. A command-line app will simply print an error message, but a GUI app could use the same code and then display an error alert.

Bugs

I did find some bugs while typing this up. For example, I tried this perfectly valid expression:

(8 + (1 * (3 + (7 * 4) - 1) + 1))=

which should evaluate to 39 (if I've done my math correctly). But instead it generates an Invalid Expression error.

Line Endings

You don't need to print the \n as a character. You can simply include it in your string like this:

std::cout << "Infinite\n";
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.