Mixing
Simple C++ calculator
Clash Royale CLAN TAG#URR8PPP Simple C++ calculator
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);
This question came from our site for professional and enthusiast programmers.
3 Answers
3
#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.
#include
std::string
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.
using
using
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?
ch
ch
loop_again
C++ has a boolean (bool) type.
bool
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.
goto
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?
ch
'a'
ch
return 0;
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.
0x
x
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.
fail_if_zero
case '/':
if (b == 0)
cout << "Error: Division by zero.n";
else
cout << a << " / "<< b << " = " << a/b << "n";
break;
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>).
#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:
evaluateSimpleExpression
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.
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.
– Martin York
2 days ago
@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.
– Phil H
yesterday
Some advice on best practice:
"foobar"s with std::literals::string_literals::operator""s() accessible in the global namespace constructs a std::string from "foobar".
"foobar"s
std::literals::string_literals::operator""s()
std::string
"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) 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
while (op != 'a');
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.– user4581301
2 days ago
"+-*x/:a"s
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.– Martin York
2 days ago
op
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.– Martin York
2 days ago
op_pos = "+ - *x/:a "s.find(op)) == std::string::npos
@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.
– Swordfish
2 days ago
By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.
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.
– user4581301
2 days ago