Console calculator, takes a string as input (like: 2+3*4-1+3) and outputs the result

Question

I am a beginner programmer(solo learning). I know the basics of c/c++, but I would not consider myself too experienced. Recently I decided to make this calculator which takes an array of characters as input and then decodes the operations. I would like to know if I can improve it in any way and an overall review.

#include <iostream>
#include <string.h>
#include <stdio.h>
using namespace std;
int main()
{
 char v[20];
 int sign, n, i;
 float result, nr, aux, c;
 while (1)
 {
 cout << "insert the expression:" << endl;
 scanf("%s", v);
 n = strlen(v);
 result = 0, nr = 0, aux = 0, c = 0;
 sign = 1, n, i = 0;
 while (i<n)//loops only once through the array
 {
 while (v[i] >= '0'&&v[i] <= '9')//builds the number
 {
 nr = nr * 10 + (v[i] - '0');
 i++;
 }
 switch (v[i])//identifies the operations
 {
 case '+': {
 if (!(v[i - 1] == '*') && !(v[i - 1] == '/'))//if a*-b or a/-b occurs it does nothing
 {
 if (c == 1)// * was found before: 2*2+1
 {
 result += aux*nr*sign;// updates the end result
 aux = 0;//aux is reseted
 }
 else if (c == 2)// / was found before: 2/2+1
 {
 result += aux / nr*sign;
 aux = 0;
 }
 else
 {
 result += nr*sign;
 }
 nr = 0;
 c = 0;
 }
 sign = 1;
 break;
 }
 case '-': {
 if (!(v[i - 1] == '*') && !(v[i - 1] == '/'))
 {
 if (c == 1)
 {
 result += aux*nr*sign;
 aux = 0;
 }
 else if (c == 2)
 {
 result += aux / nr*sign;
 aux = 0;
 }
 else
 {
 result += nr*sign;
 }
 c = 0;
 nr = 0;
 }
 sign = -1;
 break;
 }
 case '*': {
 if (!aux)
 {
 aux = nr*sign; // aux hold the result from * and / operations
 }
 else if (c == 2)
 {
 aux /= nr*sign;
 }
 else
 {
 aux *= nr*sign;
 }
 c = 1;
 nr = 0;
 sign = 1;
 break;
 }
 case '/': {
 if (!aux)
 {
 aux = nr * sign;
 }
 else if (c == 1)
 {
 aux *= nr*sign;
 }
 else
 {
 aux /= nr*sign;
 }
 c = 2;
 nr = 0;
 sign = 1;
 break;
 }
 default:;
 }
 if (i >= (n - 1))// updates the result because there is no + or - at the end of the string.
 {
 if (c == 1)
 {
 result += aux*nr*sign;
 }
 else if (c == 2)
 {
 result += aux / nr*sign;
 }
 else
 {
 result += nr*sign;
 }
 }
 i++;
 }
 cout << "=" << result << endl;
 system("pause");
 system("cls");
 }
}

• Andreas Rejbrand: IIRC, the must-read book on C++ programming, The C++ Programming Language, 4th ed., contains a calculator sample.
• Andreas Rejbrand: I haven't tried your code, but I hope it gets the following expressions right: 2 + 4 * 5, -2 * -5, 5 - -3 (results should be 22, 10, and 8, respectively). Also, it is important that you get correct and helpful error messages on syntax and math errors, like 5 // 8, 4 */ 9, 5 / 0, and 2^3 (unless you support exponentiation as well).
• Andreas Rejbrand: Finally, it is important that the program does not return a (necessarily incorrect) result on an invalid input.
• JDługosz: You might get more attention on your Q if you didn't immediately mark the first answer as Accepted. Especially since it opens with "some quick observations".
• NomadMaker: The line "scanf("%s", v);" is dangerous because if the input is greater than 19 characters, it will overflow v. I would use "scanf("%19s", v)

Answer 1

Don’t write using namespace std;.

You can, however, in a CPP file (not H file) or inside a function put individual using std::string; etc. (See SF.7.)

---

#include <iostream>
#include <string.h>
#include <stdio.h>

Why do you use both iostream and stdio.h in the same function? You should stick with the C++ version. Likewise, why do you need string.h? Even if you did need some special feature from there that is not easier using the C++ stuff, you should use #include <cstring> instead.

---

char v[20];
int sign, n, i;
float result, nr, aux, c;

Do not declare all the variables at the top! I just saw a presentation by Kevlen Hennelly where he quipped, "Are they scared of the code?" and "Is it so cold, they are huddling together like penguins!"

Declare the variables where they are first needed and you are ready to initialize them. Put them in the smallest scope possible.

The char array is ominous. Why not a string or somesuch? Why 20?

---

while (1)

I think maybe you don’t know about the bool type.

---

Your program is one huge function. First, separate I/O from the actual work. This is not just good organization, but is more realistic of real-world code. Imagine you want to move it to a GUI — your console calls are mingling with the code that does the real work!

So, write a function:

ValueType calculate (std::string_view input) { ⋯

and a function

void interaction()
{
 for (;;) {
 std::cout << "expression: ";
 std::string input;
 std::getline (std::cin, input);
 auto result= calculate (input);
 std::cout << "result might possibly be " << result;
 }
}

And you can have another function which does automated testing. You don’t have to type a bunch of samples manually each time you try it!

void test()
{
auto y= calculate ("1+2*3");
VERIFY (y, 7);
y= calculate ("0*42 + 17");
 ⋮
}

---

Notice I used ValueType? What data type are you using for the calculator values? Declare that at the top and use the alias when referring to this purpose.

using ValueType = float;

but I wonder why you intentionally limit things to single-precision? It doesn’t make sense to not use double here. And, you might want to use an extended or arbitrary precision type (e.g. Boost.Multiprecision) or do BCD math or whatnot. It should be as easy to change as changing this one line.

---

 while (i<n)//loops only once through the array

Don’t go through the string using index values. Use iterators. Most of the time you can just use a range-for loop, but here you often refer back to the previous character.

 const auto End= end(input); 
 for (auto it=begin(input); it != End; ++it) {

---

 while (v[i] >= '0'&&v[i] <= '9')//builds the number

There is a library function isdigit.

You don’t seem to be able to read the decimal point or exponent.

What I do is separate the lexing and digesting steps: scan a "word" (token) that is a numeric value, but you just have the range of characters making it up. Then, pass that to another function to produce the value from the string. There are library functions for doing this, so you don’t have to write a parser for floating point literals yourself. (But if you wanted to, you still have it in a function that does only that.)

---

The body here is way too long, because each case is large itself.

How about the body of the loop being:

 auto nr= read_number (it);
 char op= read_op (it);
 process (nr, op);

That is, break it up into logical pieces. This is a very important lesson.

---

 if (!aux)
 {
 aux = nr * sign;
 }
 else if (c == 1)
 {
 aux *= nr*sign;
 }
 else
 {
 aux /= nr*sign;
 }

You have nr*sign repeated just about everywhere. I think maybe you should have applied the sign to the number read right away and then nr holds the right value, positive or negative. You don’t have to do this step everywhere; just once.

Some people will say to always put braces around the body even if it is only one line. I’m with Kevlen who says, "make it a goal to get it down to just one line instead" Puffing up the code, when it is simple code like this, just makes it harder to see and makes less fit on the screen.

---

A big concern is that your calculator does not seem to be based on an understanding of grammar, but is ad-hoc logic based on know the exact form of the input. I suppose aux might considered an operand precedence stack of depth one, but the code doesn’t reflect this understanding.

As an autodidact, you might not always know what is there to be known and what order is good to explore things in. Terms to search on to get a handle on this subject: parsing and pushdown automata, "context free grammar", and "pumping lemmings" (when you grasp the joke, you are going down the paved path).

Keep it up! And best of luck to you.

• Loki Astari: Please re-phrase that first sentence. It starts off implying that using namespace std; is OK. It took some deciphering to get its actual meaning.
• JDługosz: @MartinYork What would you suggest that would be clearer?
• Loki Astari: I would drop the first part. Just start the sentence: Inside a function **STUFF** put individual using std::string; **STUFF**. I just mainly would drop You can, however, in a CPP file as it feels like I can use (with a cpp file) the dreded using namespace XXX;

Answer 2

Some quick observations:

• Don't use using namespace std

• Prefer using \n over std::endl

• Prefer prefix over postfix

• You're mixing C and C++ here. Use <cstring>/<string> (depending on what you need) instead of string.h. For input have a look at the C++ I/O library

• Some of your code is windows only ( system("pause") and system("cls") )

• Don't define more than one variable per line

• Comparing floats with ==/!= might be unsafe

• There are several (probably unwanted) type conversions which you can catch by enabling warnings in your compiler of choice

• Putting all the code into the main function makes this hard to read. It's better to split it up into functions

• Try to avoid overly short variable names. They should always express intent to make reading the code easier.

• sneakysnake: "Comparing floats with ==/!= might be unsafe", What do you mean by this?
• yuri: @sneakysnake See this and that for some in-depth information.
• Loki Astari: @sneakysnake What Every Programer Should know about Floating Point Numbers The problem comes down to rounding errors. So rather than test for floating point numbers being an exact value you test if they are within your error limits of the value. x == y should be abs(x-y) < 0.0000001 (or whatever your error limit is).
• Lord Farquaad: @sneakysnake Just to tack on, here's a real-world example of float arithmetic going very, very wrong: www-users.math.umn.edu/~arnold/disasters/patriot.html

Answer 3

if (!(v[i - 1] == '*') && !(v[i - 1] == '/'))

would read more naturally as:

if (v[i - 1] != '*' && v[i - 1] != '/')

as you can see, this removes parenthesis and negation, so is much more readable.

---

You are accessing v[i-1] when i might be zero. Therefor you are accessing v[-1], which is usually a no-no.

---

What would your algorithm do with "5 6" as the input? Ideally this would be an error (as there is no operator between 5 & 6).

---

You are using aux both as a boolean and as an integer. This means you won't be able to tell the difference between an integer valued 0 and false meaning not set.

---

The proper use and meaning of the c variable requires a comprehensive understanding of the algorithm. Better variable naming is indicated here.

---

This is a fairly unusual parsing algorithm. I'm pretty sure there are lots of corner cases this won't handle. Try, for example, "-2*-2+-4*--5".

---

Don't be disheartened, though, parsing arbitrarily complex expressions is not trivial. Even the famous Shunting Yard algorithm is incomplete and fails on various inputs &8212; fails to detect bad input and/or doesn't properly parse (such as we try to enhance the grammar, such as having unary negation and subtraction together in the same grammar).

---

Infix expressions naturally have two states at the start of any given token: unary and binary — hence, I prefer to model the two states explicitly.&nbsp To track any prior state I prefer to use two stacks, one for operands, and one for operators. The states are implemented by two different sections of code (i.e. there's no need for a state variable as we know we're in unary state if we're in the unary section of code, and vice versa).

The parse starts in the unary state where (, -, +, numbers (and variables) are accepted (things we don't recognize should error). These are grouping paren, unary negation, unary plus, etc... After a number or variable push it to operand stack and switch to binary state; otherwise, push the operator and stay in unary state.

In binary state, we can accept ), +, -, *, / (things we don't recognize should error). After ) stay in binary state and reduce operators on on the operator stack until reaching a matching ( (if it's not there there's an unbalanced ).; for the others: reconcile the newest operator with the top operator on the operator stack (here operator precedence is handled, like * higher than +), then push that newest operator, and switch back to unary state.

Reconciling a higher precedence operator already on the stack means consuming it (from the operator stack) and consuming its operands from the operand stack, and then, returning to the operand stack a representation of the intermediate computation.

For example, in 4*5+6, when we get to the + operator, we have 4 and 5 on the operand stack, and * on the operator stack. Since * is higher precedence than +, the * is "executed", by taking * off the operator stack, and taking 4 and 5 off the operand stack, and multiplying the two (popped) operands, and then pushing the result (either 20, or if you're doing a compiler, then a tree/AST node that representing the multiplication of 4*5) back onto the operand stack.

By treating the unary and binary states separately, not only can we distinguish between unary - and subtraction -, if we see, for example, ( in the binary state, then that means we have a function call parenthesis (e.g. a(b)), instead of a grouping paren as when same seen in the unary state(a+b). It is pretty hard to parse infix expressions well without separating these states.

"End of input" is allowed in the binary state (we could allow it in unary state also if you wanted to allow the empty expression). On EOInput we have to reconcile the operator stack against, well the lowest precedence operator "nothing". if we encounter an ( grouping paren on the stack, then that is an input error of unbalanced ( paren. When all is said and done, if the parse succeeds, the operator stack is empty and the result is the top and only item on the operand stack.

• JDługosz: I think zero for aux "works" because he is adding a phantom term.
• Erik Eidt: @JDługosz, ok, despite testing if(!aux)... in several places?
• sneakysnake: "-2*-2+-4*--5" this results in -1 because --5.If it was -5 instead, then the result would have been 24. "5 6" outputs 5