Help me optimize my calculator code!

Weegee

Go Weegee!
Reaction score
103
I just have a simple request, what are a few ways to optimize my code? This isn't a homework assignment, this is just me trying to learn how to program again after a 2 year lack of C++ (brace yourselves D:)

Code:
#include <iostream>
#include <string>
#include <Windows.h>

using namespace std;

int n1;
int n2;
string mode;

int main()
{
cout << " Please enter a number you would like to calculate: \n";
cin >> n1;
cout << "\n Please enter your second number: \n";
cin >> n2;
cout << "\n Please enter the modefier (+,-,x,/): \n";
cin >> mode;
cout << "\n Your answer is: ";

if (mode == "+")
	cout << n1+n2;
if (mode == "-")
	cout << n1-n2;
if (mode == "x")
	cout << n1*n2;
if (mode == "/")
	cout << n1/n2;

Sleep(5000);
}

I just want to know if there are better programming methods and habits and if I should stay away from some things. All advice or help is appreciated!
 

azareus

And you know it.
Reaction score
63
You misspelled modifier and it might be easier to use a switch statement to find out the mode.

Why are you declaring variables outside the main function?
 

s3rius

Linux is only free if your time is worthless.
Reaction score
130
Disclaimer: Since I don't know how well-versed you were in C++/Programming in general I'll act like you're a beginner.

Well, there is a ton of things you could change, but only a few things stand out for such a small piece of code. Most programming methods / habits are there to help you handle large projects.

#1
You're declaring n1, n2 and mode as global variables when there is no real need. Global variables should be avoided unless doing so would result in a lot of work. In this case you can easily move them into the function to make them local:

Code:
int main()
{
    int n1, n2;
    string mode;

    cout << " Please enter a number you would like to calculate: \n";
    ....

The reason why globals should be avoided are that they violate the OOP principle of data encapsulation. Also, you have no control when they are created, so for more complex global classes you might end up having weird errors.

#2
You're using sleep(5000) to keep the console window open for another 5 seconds. That's a bit of a hackery. The standard way is to make the console close after the user pressed a key.

Code:
	cout << "\n\nPress the Enter key to continue.";
	cin.ignore( cin.rdbuf()->in_avail() );
	cin.get();

This code replaces the sleep(5000) at the end.
The second line orders the input stream (from what you're reading the user input) to ignore all left-over data that's still in there. That's required because the cin commands don't act like you'd expect them to.
The last line orders the program to wait for you to press Enter.

#3
Your code doesn't prepare for all possible user input. If you enter a non-number when you're asked to set a value for n1 or n2 your program will fail. Same goes for not entering any of the operators when you're asked for it (/ + - *).

Now, checking the input and making sure it's a number is a bit harder, unfortunately. There are easier ways but they are deprecated and faulty.

Code:
#include <sstream>
....
int main(){
        string input;
        int n1;

	cout << " Please enter a number you would like to calculate: \n";
	cin >> input;

	istringstream iss(input);
	iss >> n1;
	bool success = !iss.fail() && iss.eof();
...
}

Now we store the user input in a string directly, not in a number. This way it won't immediately fail if we input anything other than a number.
Next we create a string-stream. These are constructs specifically made to work with strings (that's what we need the <sstream> include for). They can handle the string-to-int conversion for us.
We give our input string to the string-stream and tell it to try and extract a number from input into n1. The syntax is just like the "cin>>x" stuff.

Next, we can check if the extraction of the number failed.

We check if iss.fail() returned false AND if iss.eof() returned true. fail() will be true if the extraction failed and eof() will be false if there is more in the input string than just one number. We don't want both to happen.
If we didn't check for eof() the user could enter "23a" and it would work, because 23 is correctly extracted from the string. The 'a' would be ignored. eof() notices if there is still something left in the string.

Now, if the user entered an invalid input we want him to re-enter it until he gives us a good number. So we wrap everything into a while-do loop:

Code:
	string s;
	bool success;

	do{
		cout << " Please enter a number you would like to calculate: \n";
		cin >> s;
		istringstream iss(s);

		iss >>n1;
		success = !iss.fail() && iss.eof();
	}while( !success );

And the same thing would have to be done with the second number.

The mode input looks similar, but we don't need a stringstream here because we're not doing number conversions:
Code:
	do{
		cout << "\n Please enter the modefier (+,-,x,/): \n";
		cin >> mode;
	}while( mode != "+" && mode != "-" && mode != "/" && mode != "x" );

#4
A small but important issue: If you're entering 0 as the second number and choosing to Divide then you're doing a division-by-zero. Your program goes kaboom.
That's easy to fix though:

Code:
...
	if (mode == "+")
		cout << n1+n2;
	if (mode == "-")
		cout << n1-n2;
	if (mode == "x")
		cout << n1*n2;
	if (mode == "/")
		if( n2 != 0)
			cout << n1/n2;
		else
			cout << "Division-by-zero error!";
...

Tadaaa!
Now we're safe from any user input (I think). Nothing can crash our calculator anymore.
And all that by only ... enlarging our function threefold and making it hardly readable...? Yep.
Good C++ programming is sometimes pretty ugly. Even simply things as this calculator become much more complicated if you're "doing it right".
And it isn't necessary to write your programs like this. But from time to time you might pick up a good idea here or there and you'll improve your methodology and coding style gradually.

The entire program now looks like this:

Code:
#include <iostream>
#include <string>
#include <Windows.h>
#include <sstream>

using namespace std;

int main()
{
	int n1 = 0; //Default initializing all local variables is a good habit too
	int n2 = 0;
	string mode = "";

	string input = "";
	bool success = false;

	//Enter first number
	do{
		cout << " Please enter a number you would like to calculate: \n";
		cin >> input;
		istringstream iss(input);

		iss >>n1;
		success = !iss.fail() && iss.eof();
	}while( !success );

	//Enter second number
	do{
		cout << " Please enter the second number: \n";
		cin >> input;
		istringstream iss(input);

		iss >>n2;
		success = !iss.fail() && iss.eof();
	}while( !success );

	//Enter operator
	do{
		cout << "\n Please enter the modifier (+,-,x,/): \n";
		cin >> mode;
	}while( mode != "+" && mode != "-" && mode != "/" && mode != "x" );


	cout << "\n Your answer is: ";

	if (mode == "+")
		cout << n1+n2;
	if (mode == "-")
		cout << n1-n2;
	if (mode == "x")
		cout << n1*n2;
	if (mode == "/")
		if( n2 != 0)
			cout << n1/n2;
		else
			cout << "Division-by-zero error!";

	//Wait to continue
	cout << "\n\nPress the Enter key to continue.\n";
	cin.ignore( cin.rdbuf()->in_avail() );
	cin.get();
}

Bonus #5
Most of the time it's no big issue, but it might cause compatibility issues on some machines/OSs:
You declare your main function as "int main()" but it should be "int main(int argc, char** args)". It's mostly a OS thing. The passed parameters are for command line arguments. It's a good idea to include them even if you don't have to.
Also, since main() has a return value of type "int" we should make sure that it actually returns a value.
0 is the common value to return when everything went smoothly.

Code:
int main(int argc, char** args){
    ... all the code...

    return 0;
}

Bonus #6
Now another ugly one for beginners:
"using namespace std" sometimes isn't such a good idea. What you're doing is namespace pollution, that means you're telling your program that it may ignore that all std-functions are inside a seperate namespace. That can cause name conflicts, because it might happen that, for example, "std::string" and "sf::string" (if you're using the SFML library, for example) are both handled as "string". Thus the program doesn't know which one you mean.

Correction is simple - remove the "using namespace std" and add "std::" everywhere:

Code:
		std::cout << " Please enter a number you would like to calculate: \n";
		std::cin >> input;
		std::istringstream iss(input);

Basically a lot of parts need the std:: attached. The reason why you see the "using std" in most tutorials and school courses is because it takes the namespace stuff off your back.
And that's a good idea. Less for you to worry about. That's why we want to keep it. However, we don't want to pollute our entire file with this namespace-usage. (That becomes important when you're writing more functions and classes, or when you're starting to #include your own files.)

So instead of removing the "using namespace std" and having to write "std::" like everywhere we're going to move the declaration into the function itself:

Code:
int main(int argc, char** args){
    using namespace std;

    ...all the code...

}

This way we're only "polluting" this function with std. All other functions within this file (or within files that include this one) are kept pure, so they can decide on their own whether they need the namespace-usage or not.

So our final code with all good stuff added looks like this:

Code:
#include <iostream>
#include <string>
#include <Windows.h>
#include <sstream>

int main()
{
	using namespace std;

	int n1 = 0; //Default initializing all local variables is a good habit too
	int n2 = 0;
	string mode = "";

	string input = "";
	bool success = false;

	//Enter first number
	do{
		cout << " Please enter a number you would like to calculate: \n";
		cin >> input;
		istringstream iss(input);

		iss >>n1;
		success = !iss.fail() && iss.eof();
	}while( !success );

	//Enter second number
	do{
		cout << " Please enter the second number: \n";
		cin >> input;
		istringstream iss(input);

		iss >>n2;
		success = !iss.fail() && iss.eof();
	}while( !success );

	//Enter operator
	do{
		cout << "\n Please enter the modifier (+,-,x,/): \n";
		cin >> mode;
	}while( mode != "+" && mode != "-" && mode != "/" && mode != "x" );


	cout << "\n Your answer is: ";

	if (mode == "+")
		cout << n1+n2;
	if (mode == "-")
		cout << n1-n2;
	if (mode == "x")
		cout << n1*n2;
	if (mode == "/")
		if( n2 != 0)
			cout << n1/n2;
		else
			cout << "Division-by-zero error!";

	//Wait to continue
	cout << "\n\nPress the Enter key to continue.\n";
	cin.ignore( cin.rdbuf()->in_avail() );
	cin.get();
}

The "bonus" things should concern you the least. They're really only important when you're working on larger projects or multiplatform stuff. But hey - can't hurt to have heard of it before.

The really important stuff, in my opinion, is only the part about validating user input. And not because it could crash your program, but because it's important to start thinking about everything that could happen in your code, as opposed to the things you want to happen.
You'll often find yourself trying to figure out where a problem/bug lies and the most important skill you need there is to understand how your code would react to input/output that you didn't expect in the first place.



Of course there are even more things that could be done, but we're slowly drifting into the realm of "Personal preference" instead of "Commonly accepted as good practice".

Christ, look at the time. I only wanted to write a few short paragraphs initially ;D

If there are any questions to what I've written (or anything else) feel free to ask away.
 

Weegee

Go Weegee!
Reaction score
103
Wow! What an informative post! This has really helped me shape back up again and really encouraged me! Oh and I rewrote my code using the information you gave me, and then after I was all done compared it to yours; I just wanted to know that when you enter a non-numerical value for the first input, it doesn't crash which is great, but the calculator doesn't go through the whole "please enter your first number" thing it just skips to the second (which is better considering it doesn't crash). Is this intended? for example if I enter the value a for the first one and 2 for the second with the modifier being division, the end result is 0. Is this intended? I can see how it works considering that the input of a would register as 0 since there is no value found via istringstream. So really what I am asking is for the first input, if i put a, it doesnt ask me to put in a real value it just skips to the second one, but if i put in a for the second value, it keeps asking till i put in a number. How should I go about fixing this?

But other than that question thank you so much! Really encouraging post and very informative! +rep :)

I see what I did wrong in my code, i said iss >> input not n1 xD everything works beautifully now thank you!
 

Moridin

Snow Leopard
Reaction score
144
s3rius covered most of it (and kudos to him for that gigantic post), but I wanted to mention two more things that might improve the efficiency of this specific program.

1)

In your program you're using a string variable to store user input when it comes to operations. Since the operation itself is a single character, you could use a char variable instead.

2)

For the error handling, I happen to know another way to catch the error, using integers as the input type.

A disclaimer here: I'm not sure which is really more efficient, I'm a beginner on code myself. I just think this might be more efficient because it uses a simple data type, less variables and less included libraries.

Code:
#include <iostream>
#include <Windows.h>

int main()
{
	using namespace std;

	int n1 = 0; //Default initializing all local variables is a good habit too
	int n2 = 0;
	[B]char [/B]mode;

	//Enter first number
	do{
		cout << " Please enter a number you would like to calculate: \n";
		cin >> n1;

	}while([B] std::cin.fail()[/B]);

	//Enter second number
	do{
		cout << " Please enter the second number: \n";
		cin >> n2;

	}while( [B]std::cin.fail()[/B]);

	//Enter operator
	do{
		cout << "\n Please enter the modifier (+,-,x,/): \n";
		cin >> mode;

	}while( mode != '+' && mode != '-' && mode != '/' && mode != 'x' );
       [B] // I should note here that characters are surrounder with ' ', instead of " ".[/B]

	cout << "\n Your answer is: ";

	if (mode == '+')
		cout << n1+n2;
	if (mode == '-')
		cout << n1-n2;
	if (mode == 'x')
		cout << n1*n2;
	if (mode == '/')
		if( n2 != 0)
			cout << n1/n2;
		else
			cout << "Division-by-zero error!";

	//Wait to continue
	cout << "\n\nPress the Enter key to continue.\n";
	cin.ignore( cin.rdbuf()->in_avail() );
	cin.get();
}
 
General chit-chat
Help Users
  • No one is chatting at the moment.

      The Helper Discord

      Members online

      No members online now.

      Affiliates

      Hive Workshop NUON Dome World Editor Tutorials

      Network Sponsors

      Apex Steel Pipe - Buys and sells Steel Pipe.
      Top