Optomizing my first simple program

[eric Mills]

Hello I am about 3 days into my effort to learn C++. I am learning on my own relying on web based resources like cplusplus.com and my local library.

I recently "finished" a simple calculator program. I am satisfied with the way it works but I feel there are a couple of lines I could clean up or optimize.
I have 2 questions.

1. Line 36 is working the way I want it to but it took several iterations of the same idea to get there and I am not sure it is done in the best way.

2.In order to get my program to run after getting a solution I used a while statement in the main(). I also considered calling main()from printsolution(). Neither of these feel satisfactory to me, I believe my main() will never complete which doesn't seem correct.

My next task will be learning how to enable to user to exit the program at any time, so please don't give that part away.

code below

// simple calculator.cpp : Defines the entry point for the console application.
// simple calculator takes 3 inputs from the user 1.First number 2.Operation symbol 3.Second number
// it then preforms the desired math and returns a solution.

#include <iostream>
#include <string>
#include <sstream>

int domath(float& a,char& b, float& c, float d);

int printsolution(float d);

int getuserinput ( float a, char b, float c)
{
	using namespace std;

	string input="";
	//Gets user input 1 and verifys it is a float number
	while(true)
	{
		cout<<"Enter the first number you want to work with."<<endl;
		getline( cin, input);
		stringstream myStream(input);
		if (myStream >> a)
			break;
		cout<<"That is not a valid number try again."<<endl;
	}

	//Gets user input 2 and verfiys it is a valid operator
	cin.sync();
	while(true)
	{
		cout<<"Now enter a +, -, * or / sign"<<endl;
		getline( cin, input);
		stringstream myStream(input);
		if (myStream >> b, b== '+' || b == '-' || b == '*' || b == '/' )
			break;
		cout<<"That is not a valid operator try again."<<endl;
	}

	//Gets user input 3 and verifys it is a float number
	cin.sync();
	while(true)
	{
		cout<<"Thank you now enter the second number you want to work with."<<endl;
		getline( cin, input);
		stringstream myStream(input);
		if (myStream >> c)
			break;
		cout<<"That is not a valid number try again."<<endl;
	}
	cout<<"You entered "<<a<<b<<c<<endl;
	cout<<"Starting calculation."<<endl;
	domath( a, b, c, 0.0);
	return (0);
}

int domath (float& a, char& b, float& c, float d)
{
	if (b == '+')
		d = a + c;
	if (b == '-')
		d = a - c;
	if (b == '*')
		d = a * c;
	if (b == '/')
		d = a /c;
	printsolution(d);
	return 0;
}


int printsolution (float d)
{
	using namespace std;
	cout<<"Your Soulution is "<<d<<"\n"<<endl;
	return 0;
}


int main()
{
	using namespace std;
	while(true)
	{
		cout<<"Welcome to Eric's Simple Calculator."<<endl;
		cout<<"------------------------------------"<<endl;
		getuserinput(0.0,0,0.0);
	}
	return 0;
}

I have 2 questions.

1. Line 36 is working the way I want it to but it took several iterations of the same idea to get there and I am not sure it is done in the best way.

2.In order to get my program to run after getting a solution I used a while statement in the main(). I also considered calling main()from printsolution(). Neither of these feel satisfactory to me, I believe my main() will never complete which doesn't seem correct.

My next task will be learning how to enable to user to exit the program at any time, so please don't give that part away.

[immore]

I also didnt get it. Is there any one can sort it out.

[ResidentBiscuit]

1. Refer to the 2nd paragraph.

2. C++ doesn't allow you to call main, so the while is the correct way to handle it.


Others:
Also, look into void functions. Another thing, if you place a function BEFORE main, then you don't have to define it. You just write the function. You did it for getuserinput(), but not the others. You only need to define functions if they are anywhere AFTER main. Compiler has no idea what you're function is if it's not defined/implemented before main. You also keep repeating using namespace std;, just write that after your includes and your whole program is good to go with std.

You don't have to mess around with stringstream at all if you don't want. Could just have the input as a float or double, and then use cin to get it. It will raise a flag if you whatever they typed in doesnt fit your variable.

I would recommend having your error statement under an else or else if. It generally won't cause an issue how it is, with your break there, but it could. If you make any changes to your code, and the break for some reason is gone, that error message will run every time, even if they did nothing wrong. Typically, you want your exception handling nice and segregated.

I notice you also like to create variables as parameters, that works fine. But, I recommend declaring variables in their own spot. It makes your code more readable, and trust me, having your code readable is nice when you go to look at it 6 months down the road.

[Galik]

Personally I like where you have your using namespace std;, if you must use it, its better not global.

http://www.parashift.com/c++-faq-lite/coding-standards.html#faq-27.5

Also I think using getline() with std::istringstream() is a good combo for reading/verifying input. It saves you having to clear the error flags and skip the bad data.

However I am not sure that this does everything you want:

if (myStream >> b, b== '+' || b == '-' || b == '*' || b == '/' )

The problem being that I don't think if() will care if myStream >> b fails. It will evaluate the comma from left to right and then take the right most answer. So the if() will fail on b== '+' || b == '-' || b == '*' || b == '/' but not on myStream >> b. That could be a problem if b already contains one of the tested values.

I would do:

if(myStream >> b && (b== '+' || b == '-' || b == '*' || b == '/'))

[Caprico]

In addition to what ResidentBiscuit said, I'd like to add:

1) Make printsolution inline, or get rid of it if you can. Its a fairly short function, used just for cout, and unnecessarily decreases program speed due to to and fro calls... (This is just my opinion. not everyone would agree with this...)

2) Line 19, 31, 43 - Why have infinite loops, and an if to break out of them? Instead, make them such, that while the input is incorrect, keep running. Get rid of the if and break inside.

3) Why are all your functions int and return 0? From wherever they are called, return value is not used at all. Why not have all of them as void?

4) In finer areas, d is an unnecessary variable.. Think about it...

@ ResidentBiscuit, I guess he needs to forward declare the other two functions since he's calling them before they are defined (In his getuserinput() function). Else, I guess he'd get a compiler error... Of course, the simple solution to this would be to define them in reverse order...

Hope this helps... :)

[Galik]

ALSO:

int getuserinput ( float a, char b, float c)
{
    // ... 

You never pass any useful input to that function, so you should not really define your variables as parameters. Just make them local to the function:

int getuserinput() // no parameters used
{
    float a;
    char b;
    float c;
    // ...
}

// ...

int main()
{
	using namespace std;
	while(true)
	{
		cout<<"Welcome to Eric's Simple Calculator."<<endl;
		cout<<"------------------------------------"<<endl;
		getuserinput(); // no parameters
	}
	return 0;
}

[Galik]

FURTHERMORE:
You can be even more pedantic with your error checking using eof():

	while(true)
	{
		cout<<"Enter the first number you want to work with."<<endl;
		if(std::getline(cin, input) && (std::istringstream(input) >> a >> std::ws).eof())
			break;
		cout<<"That is not a valid number try again."<<endl;
	}

Using your method, it is possible to type in values like: 9.07DqX. This works because after reading in 9.07 the input stops, ignoring the fact that the rest is garbage. So testing for eof() ensures that no garbage was included in the number. However using eof() alone will also trap the user if they merely put spaces after the number. This is why I absorb any spaces using std::ws.

[ResidentBiscuit]

In this program, he only uses std, so declaring it global would be just fine. But, if you want to picky, he could just declare globally the members he plans on using. OR, just do std::whatever when he uses it.

EDIT:
And I just read your link Galik, I guess it's pretty much covered haha