Feedback on my first "real" program

[Macpon7]

Hello!
I started reading the C++ tutorials on this site a few days ago and finally made it to functions. As a practice exercise I decided to write a simple text based calculator using floating point numerals. I'd love some feedback in the code. :D

I've included two versions, one with and one without my comments.

  //Text based calculator
#include <iostream>
#include <string>
#include <sstream>

using namespace std;

//Function to add x and y (x+y)
double addition (double x, double y) {
	double z;
	z=x+y;
	return z;
}

//Function to subtract y from x (x-y)
double subtraction (double x, double y) {
	double z;
	z=x-y;
	return z;
}

//Function to multiply x by y (x*y)
double multiplication (double x, double y) {
	double z;
	z=x*y;
	return z;
}

//Function to divide x by y (x/y)
double division (double x, double y) {
	double z;
	z=x/y;
	return z;
}

int main() {
	//Declares cmd as a string for input later
	string cmd;
	
	//Initialises interface
	cout << "Welcome to this text based calculator!\n";
	cout << "Here we can do four operations with two numbers at a time.\n";
	
	//Loops until user types "stop"
	do {
		//Instructions to user
		cout << "Type two numbers.\n";
		
		//Gets the first number and makes it a floating point
		string stringx;
		double doublex;
		cout << "X = ";
		getline(cin, stringx);
		stringstream(stringx) >> doublex;
		
		//Gets the second number and makes it a floating point
		string stringy;
		double doubley;
		cout << "Y = ";
		getline (cin, stringy);
		stringstream(stringy) >> doubley;
		
		//Instructions to user
		cout << "Ok, now what do you want to do with these numbers?\n";
		cout << "Your options are: add, subtract, divide, multiply.\n";
		
		//Gets the calculation command form the user
		getline(cin, cmd);
		
		//Declares the result as a floating point of 0.0
		double result = 0.0;
		
		if (cmd == "add")
			//Calls addition function using the two numbers as input
			result = addition(doublex, doubley);
		else if (cmd == "subtract")
			//Calls subtraction function using the two numbers as input
			result = subtraction(doublex, doubley);
		else if (cmd == "multiply")
			//Calls multiplication function using the two numbers as input
			result = multiplication(doublex, doubley);
		else if (cmd =="divide")
			//Calls the division function using the two numbers as input
			result = division(doublex, doubley);
		else
			//If the inputted calculation command isn't recognised, it throwas an error
			cout << "That didn't work.\n";
			
		//Prints the result of whatever calculation was performed
		cout << result << '\n';
		
		//Gives the user the option to stop or continue
		cout << "If you want to stop here, type stop. If not, press enter.\n";
		
		//Gets the stop or contine command from user
		getline(cin, cmd);
	} while (cmd != "stop");
	
	return 0;
}

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

using namespace std;

double addition (double x, double y) {
	double z;
	z=x+y;
	return z;
}

double subtraction (double x, double y) {
	double z;
	z=x-y;
	return z;
}

double multiplication (double x, double y) {
	double z;
	z=x*y;
	return z;
}

double division (double x, double y) {
	double z;
	z=x/y;
	return z;
}

int main() {
	string cmd;
	
	cout << "Welcome to this text based calculator!\n";
	cout << "Here we can do four operations with two numbers at a time.\n";
	
	do {
		cout << "Type two numbers.\n";
		
		string stringx;
		double doublex;
		cout << "X = ";
		getline(cin, stringx);
		stringstream(stringx) >> doublex;
		
		string stringy;
		double doubley;
		cout << "Y = ";
		getline (cin, stringy);
		stringstream(stringy) >> doubley;
		
		cout << "Ok, now what do you want to do with these numbers?\n";
		cout << "Your options are: add, subtract, divide, multiply.\n";
		
		getline(cin, cmd);
		
		double result = 0.0;
		
		if (cmd == "add")
			result = addition(doublex, doubley);
		else if (cmd == "subtract")
			result = subtraction(doublex, doubley);
		else if (cmd == "multiply")
			result = multiplication(doublex, doubley);
		else if (cmd =="divide")
			result = division(doublex, doubley);
		else
			cout << "That didn't work.\n";
			
		cout << result << '\n';
		
		cout << "If you want to stop here, type stop. If not, press enter.\n";
		
		getline(cin, cmd);
	} while (cmd != "stop");
	
	return 0;
}

[Gamer2015]

I think it looks nice regarding the fact, that you are pretty new to programming :)

That being said there still 2 things about your usage of streams:

1 reading the values: you don't need to read them in a string, make a stringstream object and then write it to double, you can just directly write in a double from the standard-input-stream

string stringx;
double doublex;
cout << "X = ";
getline(cin, stringx);
stringstream(stringx) >> doublex;

double doublex;
cout << "X = ";
std::cin >> doublex;

2 use getline only if you have spaces.
This is not really a problem but it is harder to read in my opinion

getline(cin, cmd);
std::cin >> cmd;

3 you could improve it so you can just type "5 - 3" ;)

[andywestken]

Actually, reading using getline() and then converting, etc is better in my opinion as it can be extended to perform error handling more easily than when using operator>> directly.

For the cmd string the main thing to do after reading it with getline would be to trim the leading and trailing white space, to let the user making the benign error of typing excess space.

As written:

Welcome to this text based calculator!
Here we can do four operations with two numbers at a time.
Type two numbers.
X =      45
Y =      11
Ok, now what do you want to do with these numbers?
Your options are: add, subtract, divide, multiply.
     add
That didn't work.
0
If you want to stop here, type stop. If not, press enter.

After adding trim():

Welcome to this text based calculator!
Here we can do four operations with two numbers at a time.
Type two numbers.
X =     42
Y =     11
Ok, now what do you want to do with these numbers?
Your options are: add, subtract, divide, multiply.
     add
53
If you want to stop here, type stop. If not, press enter.

As it stands (with the trim added) the input is quite robust, e.g.

Welcome to this text based calculator!
Here we can do four operations with two numbers at a time.
Type two numbers.
X = 1 2 3
Y = 4 5 6
Ok, now what do you want to do with these numbers?
Your options are: add, subtract, divide, multiply.
add divide
That didn't work.
0
If you want to stop here, type stop. If not, press enter.

Type two numbers.
X = 1 2 3
Y = 4 5 6
Ok, now what do you want to do with these numbers?
Your options are: add, subtract, divide, multiply.
   subtract
-3
If you want to stop here, type stop. If not, press enter.

but altering the code to use cin >> you get

Welcome to this text based calculator!
Here we can do four operations with two numbers at a time.
Type two numbers.
X = 1 2 3
Y = Ok, now what do you want to do with these numbers?
Your options are: add, subtract, divide, multiply.
That didn't work.
0
If you want to stop here, type stop. If not, press enter.
Type two numbers.
X = 1 2 add
Y = Ok, now what do you want to do with these numbers?
Your options are: add, subtract, divide, multiply.
3
If you want to stop here, type stop. If not, press enter.
Type two numbers.
X =

showing it's harder to control what the user inputs in this case.

For how to ensure the user inputs a sensible, numeric value see cire's post in this thread:

Trying to limit input to int type
http://www.cplusplus.com/forum/beginner/108849/#msg592118

and a simplified version of same code here:

how to deny non-numeric input?
http://www.cplusplus.com/forum/beginner/109874/#msg599646

Andy

[andywestken]

Meanwhile...

I think the code looks good.

A few minor points.

1. in C++ you should generally initialize at the point of definition rather than define in one statement and then assign to later on. When working with objects this is more important than with built-in types.

2. The coding standards I've worked with require spaces around binary operators (except ->) but not unary ones. (There is an element of personal preference here, but I have not come across a standard mandated "crunched up" code.) And a quick google later I see WebKit, JUCE, and Google agree on this matter.

double addition (double x, double y) {
	double z = x + y;
	return z;
}

(to be honest I would just code a function like this without a temporary.)

3. using using namespace std; is generally a bad idea.

But I do use it when knocking up small utility programs as I'm a bit lazy. And here on the forums it keeps code a bit terser and easier to fit on the web page.

4. the code on lines 50- and 57- looks like it should be factored out into a function.

(This also fits in with handling the numeric inputs using cire's approach, or something similar.)

5. (quibble?) I would have called the functions add, subtract, divide, multiply -- as I like my function names to be verbs (as a general rule.)

Andy

PS I do agree with Gamer2015's suggestion to rework you code to accept input of the form "5 - 3". As a bridging step between your current approach and handling infix notation you could have a go at postfix notation. i.e. accept inputs like "1 2 add" as well as "stop".