Help me optimize my calculator code!

Weegee

Go Weegee!
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.
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.
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!
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
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.
  • Varine Varine:
    Plus I'm confident it's only one of my bosses, the other owner is pretty cool. But I don't think he's the one that has as much control over executive decisions
  • Varine Varine:
    They aren't cutting my responsibilities or interjecting in my management abilities or anything, it's just stupid shit that comes up. And they don't exactly have people lining up for my job, there's not that many people applying for the positions I hire
  • Varine Varine:
    Eh, whatever. Thanks for listening guys
  • jonas jonas:
    Sure :) Let us know how it ends
  • Varine Varine:
    All of these things will end happily, they're just stressful. And I still lack many good friends that I can go to, and the ones I can are preoccupied with similar things. Thus general chit chat, cuz for some reason TH and Ghan and Tom all actively keep it up.
  • Varine Varine:
    Just gotta keep Miss Mazie up through the week until her shock wears off and she realizes that she still has family all around her, and bossman will do whatever he's going to do and I'll respond appropriately when it happens. Thank you all for the support, I do very much appreciate everyone being here for me through the years
    +3
  • vypur85 vypur85:
    Best of luck Varine!
  • vypur85 vypur85:
    I just gotten myself an offer to work in China. The pay quadruples my current one. Damn.... Not really ready to start a new life there in China.
  • The Helper The Helper:
    I have heard that they pay pretty good to English teachers in China - you would be an expat
  • jonas jonas:
    Cool, what kind of job?
  • Accname Accname:
    I would be careful with jobs in China. They can be hit and miss depending on where in China you go. Places like hong kong / Shengzen / Beijing can be neat. Other places not so much.
  • Accname Accname:
    I would recommend searching for some first person experiences for the city you got the offer in. Especially now when the political situation in China is deteriorating.
  • jonas jonas:
    Accname, long time no see
  • jonas jonas:
    What have you been up to
  • tom_mai78101 tom_mai78101:
    Hey Accname, welcome back.
  • Accname Accname:
    Not much. Working in the Renewable Energy Sector as an IT Consultant. Its okay, but I think I preferred working at the university. It was more relaxed and you met all kinds of crazy people there.
  • vypur85 vypur85:
    I gotten a teaching position for Biology in a college in Wuhan (yes, there)... I suppose it should be fine there (I hope). Many of my ex colleagues are teaching in China as well currently (none in Wuhan though)
  • vypur85 vypur85:
    And I signed the contract already. I guess there's no turning back....
  • jonas jonas:
    @Accname how many hours do you work? I heard in some sectors IT consultants rack up insane hours
  • jonas jonas:
    @vypur85 sounds nice, have fun : )
  • Accname Accname:
    I am supposed to work 40 hrs a week, but I can work more if I like and I will be paid for those hours (as long as I don't go too far, there are laws and company policies, etc)
  • Accname Accname:
    In practice its basically work as much as you like, as long as the job gets done in time.
  • jonas jonas:
    Haha, my job is like that as well... that usually means I have a few 70-80 hours weeks a year, and lots of 20 hours weeks...
  • jonas jonas:
    a few weeks ago, one of my friends basically said "jonas, I received an invitation to submit something to conference X but I'm too lazy to do it and also the conference isn't advanced enough for my high level of research*, why don't you write something? Oh by the way, the deadline is in two weeks. Enjoy!" so I got two 80 hour weeks out of that kind offer. (*of course he didn't say those parts, but it's a better story this way)
  • jonas jonas:
    now I'll have next week off to make up for overtime :p and I'll play some good old gothic 2

    Members online

    No members online now.

    Affiliates

    Hive Workshop NUON Dome
    Top