4
\$\begingroup\$

I'm a programming newbie. In fact, this is my second code ever (after Hello World). I did a lot of reading actually, here on Stack Overflow, and other websites. I would like to know if there's a way to further improve this code.

#include <iostream>
#include <string>
using std::cout; using std::cin;

int main()
{
cout << "Calculator..... \n";
cout << "Type '3a3'to exit... \n";

double a=0 ,b=0;
char op = 'a'; //operator // 'a' is the exit operator, the value of the integers doesn't matter

int ch = 0; //while loop checker

do
{
    cin >> a >> op >> b;
    if (op == '/' && b == 0)
    {
        cout << "Division By Zero, Please Retry \n" ;
        goto retry;
    }
    switch (op)
    {
        case '+':
        cout << a << " + "<< b << " = " << a+b << "\n";
        ch = 0 ;
        break;

        case '-':
        cout << a << " - "<< b << " = " << a-b << "\n";
        ch = 0 ;
        break;

        case 'x':
        case '*':
        cout << a << " * "<< b << " = " << a*b << "\n";
        ch = 0 ;
        break;

        case '/':
        cout << a << " / "<< b << " = " << a/b << "\n";
        ch = 0;
        break;

        case 'a':
        ch = 1;
        break;

        default:
        ch = 0;
        cout << "Invalid operator, Please retry \n";
    }

    retry: ;

}while (ch != 1);

}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Side note: There isn't much you can do to optimize this. Whenever you are taking input from a user, there is no way the user can type fast enough to keep up with the computer. No matter how fast-tastic you make the program you're going to bottleneck at the sack of meat sitting at the keyboard. \$\endgroup\$
    – user4581301
    Jul 29, 2018 at 18:29

3 Answers 3

2
\$\begingroup\$
#include <iostream>
#include <string>

Only #include what you are using. Your code doesn't need to know any of the internal implementation of std::string and it doesn't even use std::string.


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

Prefer avoiding global using declarations and prefix the namespace. Global aliases clutters the global namespace. Readers actually want to know where identifiers are coming from. Finally, you become vulnerable to argument dependent lookup issues. If you want to use using declarations, use them as locally as possible to avoid namespace pollution issues.


    double a=0 ,b=0;
    char op = 'a'; //operator // 'a' is the exit operator, the value of the integers doesn't matter

In C++, we typically have one-declaration per line to avoid mistakes related to the C/C++ grammar rules. That also allows us to have room for descriptive names and additional comments.

Prefer not to introduce a variable before you need to use it. Code becomes safer as variables aren't being used for multiple purposes. The reader also needs to retain less information on variables from higher scopes. There are exceptions, like the cost of tearing down and rebuilding an object is expensive when all you want is a reusable buffer for a loop.

You should reserve comments for statements of intent (why it is done, not what is supposed to be done). Your comment on the exit condition would be better placed as part of your statement to the user ("'a' is the exit operator. Type any values with 'a' to exit. Example: '3a3'.).


    int ch = 0; //while loop checker

While you meant good in commenting what ch actually does, it would be better if ch was named for what it does. Perhaps loop_again?

C++ has a boolean (bool) type.


        cin >> a >> op >> b;

You should validate that the data was read in correctly. If invalid data is entered, immediately prompt the user telling them the read failed.

        if (!(cin >> a >> op >> b)) 
        {
            cin.clear();
            cin.ignore(numeric_limits<streamsize>::max(), '\n');
            continue;
        }
        // rest of the code that can now knows data was read correctly.

        if (op == '/' && b == 0)
        {
            cout << "Division By Zero, Please Retry \n" ;
            goto retry;
        }

Is this the proper place to check for division by zero? Could this not be checked right before we divide?

Your goto flow control could be replaced by a simple continue.


        switch (op)
        {
            case '+':
            cout << a << " + "<< b << " = " << a+b << "\n";
            ch = 0 ;
            break;

            case '-':
            cout << a << " - "<< b << " = " << a-b << "\n";
            ch = 0 ;
            break;

            case 'x':
            case '*':
            cout << a << " * "<< b << " = " << a*b << "\n";
            ch = 0 ;
            break;

            case '/':
            cout << a << " / "<< b << " = " << a/b << "\n";
            ch = 0;
            break;

            case 'a':
            ch = 1;
            break;

            default:
            ch = 0;
            cout << "Invalid operator, Please retry \n";
        }

ch will always be 0 unless the user enters 'a' for the op code. That should be the only time you update ch. Since you are simply exiting the program, why not just return 0; and avoid the boolean tracking?

If an expression has the suffix 0x, iostreams will treat the value as if it were a hex value. Don't use x for multiplication or be explicit with users that operations should be whitespace delimited from values.

Tell users what operations are supported. You shouldn't expect users to dig through the source code to figure out which operations are supported.

As I mentioned earlier with checking division by zero

            case '/':
            cout << a << " / "<< b << " = " << a / fail_if_zero(b) << "\n";
            break;

and have fail_if_zero throw an exception if b is zero. You can also avoid exceptions and just print out a notice.

            case '/':
            if (b == 0) 
                cout << "Error: Division by zero.\n";
            else
                cout << a << " / "<< b << " = " << a/b << "\n";
            break;
\$\endgroup\$
2
\$\begingroup\$

Structure aids understanding

Variable names, functions, classes... most of the code you actually write gets thrown away by the compiler, and is much more about readability and maintainability than anything else. Well structured code is also easier to spot holes in, easier to debug, etc.

So let's start by isolating the maths bit:

double evaluateSimpleExpression(double a, char op, double b)
{
    switch (op)
    {
    case '+': return a+b;
    case '-': return a-b;
    case 'x': return a*b;
    case '*': return a*b;
    case '/': return a/b;
    default:
        throw std::invalid_argument::invalid_argument("Invalid operator");
    }
}

This collects the logic for the actual expression evaluation given some input values, and also throws an error if the operator isn't valid. We've excluded the logic for getting user inputs in, so we could easily point this at a file of inputs or some other format with minimal effort. Using a standard error like this makes it easier to understand the type of error (just need to #include <stdexcept>).

It's still not quite free of the worries about inputs, which we can improve thus:

enum MathOp
{
    Plus = 0,
    Minus,
    Multiply,
    Divide,
    //...
    MathOp__LAST
};

MathOp interpretOperatorChar(char op)
{
    switch(op)
    {
        case '+': return Plus;
        case '-': return Minus;
        case 'x': return Multiply;
        case '*': return Multiply;
        case '/': return Divide;
        default: return MathOp__LAST;
    }
}

double evaluateSimpleExpression(double a, MathOp mop, double b)
{
    switch(mop)
    {
       case Plus: return a + b;
       case Minus: return a - b;
       case Multiply: return a * b;
       case Divide: return a / b;
       default: 
         throw std::invalid_argument::invalid_argument("Invalid operator");
    }
}

Now we have separated the input-specific part (the characters used for operations, and the fact that there are two ways of entering a multiply) from the logic to perform the operation. If you wanted to add in a power operator '^' then hopefully it would be clear what to add.

Clearly the second sample here is somewhat overkill for a little math processing, but it is the kind of structure you'd expect to see for a more comprehensive application; it would be easy, for example, to change the characters used for operations, or to interpret the expression and store it to evaluation later.

Separation of concerns

We have separated the logic from the specification of the inputs - we could easily run the evaluation function with completely different inputs like '10111PLUS11000'.

You'll notice that the operator interpreting function returns a non-value if it can't find the operator in its switch, and that the evaluation function throws if it can't process the given operator. These are the two different ways of coping with invalid inputs - we will use the interpret method generally, and then only evaluate the expression if we think we have a valid operator; the throw is there in case something goes exceptionally badly.

int main()
{
    cout << "Calculator..... " << endl;
    cout << "Type '3a3' to exit... " << endl;

    while(true)
    {
        double a=0 ,b=0;
        char op = 'a'; 

        bool parsedOK = cin >> a >> op >> b;
        if(!parsedOK)
        {
           cout << "Could not parse expression, expected form a + b" 
                << endl;
           continue;
        }
        // 'a' is the exit operator, the value of the integers doesn't matter
        if(op == 'a')
           break;

        // attempt to parse operator
        MathOp gotOperator = interpretOperatorChar(op);
        if(gotOperator == MathOp__LAST)
        {
            cout << "Operator " << op << " invalid" << endl;
            continue;
        }

        // have a valid operator, attempt operation
        double result = _nan();
        try
        {
            result = evaluateSimpleExpression(a, gotOperator, b);
        }
        catch(std::invalid_argument e)
        {
            cout << "Operation failed: " << e.what();
            continue;
        }

        // print interpreted expression and result
        cout << a << " " << op << " " << b << " = " << result << endl;
    }
    return 0;
}

With this structure, we can progress through parsing and evaluating an expression in stages, and give appropriate errors that should inform the user. Now, we haven't included the divide by zero, but it's easy to see where to put it - it is the job of evaluateSimpleExpression to actually do the maths, so we should put it there:

double evaluateSimpleExpression(double a, MathOp mop, double b)
{
    switch(mop)
    case Plus: return a + b;
    case Minus: return a - b;
    case Multiply: return a * b;
    case Divide: 
    {
      if(b == 0)
         throw std::invalid_argument::invalid_argument("Attempt to divide by zero");
      return a / b;
    }
    default: 
      throw std::invalid_argument::invalid_argument("Invalid operator");
}

If, for example, we added a 'root' operator to get the bth root of a, we could add conditions for valid inputs (like only positive values of b, or only positive values of a when b is even), again providing error messages that give as much contextual information to the user as would help them understand it.

\$\endgroup\$
2
  • \$\begingroup\$ You switch on a a character to get an enum so you can switch on the enum to perform the operation. Try not to over complicate the problem. \$\endgroup\$ Jul 30, 2018 at 16:03
  • \$\begingroup\$ @Martin: But the mapping is not unique. With the above representation you are free to generalise to multi character operators, or to interpret expressions then store them in a tree in an unambiguous format. If you wanted to filter out all the multiplication expressions from a list, your filtering code would need to know about both versions, which were really artefacts of your UI. \$\endgroup\$
    – Phil H
    Jul 31, 2018 at 6:48
0
\$\begingroup\$

Some advice on best practice:

  • Declare (and define) variables as close as possible to the location they're used.
  • Use expressive names for variables, functions and types.
  • Do proper error handling, the user can't be trusted.
  • Use appropriate data types.
  • KISS! Keep it simple, stupid!

"foobar"s with std::literals::string_literals::operator""s() accessible in the global namespace constructs a std::string from "foobar".

#include <limits>
#include <string>
#include <iostream>

using namespace std::literals::string_literals;

int main()
{
    std::cout << "Calculator..... \nType '3a3' to exit...\n";

    char op;
    do
    {
        double lhs, rhs;
        while (std::cout << "> ", // print a promt. due to the comma operator this is not part of the condition
            !(std::cin >> lhs >> op >> rhs) || // input error, cin is in a error state ... !cin
            "+-*x/:a"s.find(op) == std::string::npos || // op is not a valid operation
            "/:"s.find(op) != std::string::npos && rhs == .0) // op is division and the divisor (right-hand-side operand) is zero
        {
            std::cerr << "Input error!\n\n";
            std::cin.clear(); // clear error state of cin
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n'); // ignore everything up to a newline stille in the input buffer
        }
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

        double result;
        switch (op)
        {
        case '+':
            result = lhs + rhs;
            break;
        case '-':
            result = lhs - rhs;
            break;
        case '*':
        case 'x':
            result = lhs * rhs;
            break;
        case '/':
        case ':':
            result = lhs / rhs;
            break;
        }
        if (op != 'a')
            std::cout << lhs << ' ' << op << ' ' << rhs << " = " << result << '\n';

    } while (op != 'a');
}

shorter:

#include <limits>
#include <string>
#include <iostream>
#include <functional>

using namespace std::literals::string_literals;

int main()
{
    std::cout << "Calculator..... \nType '3a3' to exit...\n";
    std::function<double(double, double)> ops[]{
        [&](double lhs, double rhs) { return lhs + rhs; },
        [&](double lhs, double rhs) { return lhs - rhs; },
        [&](double lhs, double rhs) { return lhs * rhs; },
        [&](double lhs, double rhs) { return lhs / rhs; } };
    char op;
    do
    {
        double lhs, rhs;
        std::size_t op_pos;

        while (std::cout << "> ",
               !(std::cin >> lhs >> op >> rhs) ||
               (op_pos = "+ - *x/:a "s.find(op)) == std::string::npos ||
               "/:"s.find(op) != std::string::npos && rhs == .0)
        {
            std::cerr << "Input error!\n\n";
            std::cin.clear();
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        }
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

        if (op != 'a')
            std::cout << lhs << ' ' << op << ' ' << rhs << " = " << ops[op_pos/2](lhs, rhs) << '\n';

    } while (op != 'a');
}
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Tight code, readable, but weak on explanation. For example, I doubt OP has gotten far enough in their text for "+-*x/:a"s, and if they have, it's an easy thing to miss. \$\endgroup\$
    – user4581301
    Jul 29, 2018 at 18:33
  • \$\begingroup\$ Why have an array (now you need to map op to an index). I would use a map of char to function. That means the op can be its own index to find the function. \$\endgroup\$ Jul 30, 2018 at 16:01
  • \$\begingroup\$ This is just unreadable nonsense. op_pos = "+ - *x/:a "s.find(op)) == std::string::npos if you put that in production code I would fire you. Break it out into named variables so we can see what is happening. Do you actually think this makes your code more efficient. NO. It just makes it less readable and therefore bad code. -1. \$\endgroup\$ Jul 30, 2018 at 16:08
  • \$\begingroup\$ @MartinYork "Break it out into named variables so we can see what is happening. Do you actually think this makes your code more efficient. NO. It just makes it less readable and therefore bad code." You don't see on the first glance that the code fragment you mentioned finds op in a string containing operators?? "Do you actually think this makes your code more efficient." No. This resulted out of lazyness keeping me from introducing another variable that would only be used once. \$\endgroup\$
    – Swordfish
    Jul 30, 2018 at 16:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.