Criticize my calculator please.

[Retro94]

Hi, I'm really new to C++, and I just wrote my first calculator. I know there are ways to do it better, but I don't know those ways. Could anyone please point out mistakes and how they'd have done it better? Here is the code:

#include <iostream>
using namespace std;

int main ()
{
float input1, input2;
float result;
char op;
cout << "Input a number: ";
cin >> input1;
cout << "Input +,-,x, or /: ";
cin >> op;
if (op == 'x')
{
cout << "Input number to multiply by: ";
cin >> input2;
result=input1*input2;
cout << "The result is ";
cout << result;
cout << ".";
}
else if (op == '+')
{
cout << "Input number to add by: ";
cin >> input2;
result=input1+input2;
cout << "The result is ";
cout << result;
cout << ".";
}
else if (op == '-')
{
cout << "Input number to subtract by: ";
cin >> input2;
result=input1-input2;
cout << "The result is ";
cout << result;
cout << ".";
}
else if (op == '/')
{
cout << "Input number to divide by: ";
cin >> input2;
result=input1/input2;
cout << "The result is ";
cout << result;
cout << ".";
}

cin.get();
cin.get();
return 0;
}

[johnkravetzki]

you can use switch(op) instead of if-else-if ladder - it's much let's say ergonomic and faster than if-else-if construction.... It won't take long to adjust your code...
Except that you should think about continous workflow of your program (i hope i got myself clear with that - i'm not native). If i see correctly your program ends after the operation is completed.
Finally, I consider you as newcomer in c++ (no offense, I'm newb too), that's why +1 for NOT using system("pause"); :-) But still it's not the most efficient way to end your program. For more info check the forum or wait for the link :-)

[Bazzy]

If you use doubles instead of floats, the results would have a better precision.

cout << "Input a number: ";
cin >> input1;

If the user enters something which isn't a number your program would go crazy ( see http://www.cplusplus.com/forum/articles/6046/ )

result=input1*input2;
cout << "The result is ";
cout << result;
cout << ".";

You can save space in having all this in a single line:
cout << "The result is " << ( result = input1*input2 ) << '.';

Finally, I consider you as newcomer in c++ (no offense, I'm newb too), that's why +1 for NOT using system("pause"); :-) But still it's not the most efficient way to end your program. For more info check the forum or wait for the link

http://www.cplusplus.com/forum/articles/7312/

[kfex]

See how this one uses two phases and splts up the calculation
It works more like a function because it is a function

I think I missed out something but it should work well as long as you don't type any words

#include <iostream>
void calculator (int num1, int num2,char sign);
int main()
{
	using namespace std;

	cout << "Calculator\n";

	char operatorchar;
	int firstnm, secondnm;
	cin >> firstnm;
	cin >> operatorchar;
	cin >> secondnm;

	/*	Long way not in use anymore
	if (operatorchar == '+')
		cout << "Result: " << firstnm << " " << operatorchar << " " << secondnm <<
		" = " << (firstnm + secondnm) << "\n";
	
	if (operatorchar == '-')
		cout << "Result: " << firstnm << " " << operatorchar << " " << secondnm <<
		" = " << (firstnm - secondnm) << "\n";
	if (operatorchar != '+'  && operatorchar != '-')
		cout << "Error operator not found\n";  */
	
	// function at work here
	calculator(firstnm,secondnm,operatorchar);
	return 0;
}

void calculator (int num1, int num2, char sign)
{
	// Do the calculations - stage 1
	// This function works in 2 ways, first it calculates
	int calnum;
	if (sign == '+')
		calnum = num1 + num2;
	if (sign == '-')
		calnum = num1 - num2;
	if (sign == '*')
		calnum = num1 * num2;
	if (sign == '/') {
		if (num1 != 0 && num2 != 0)
			calnum = num1 / num2;
		calnum = 0;
	}

	// print out the result - stage 2
	// Prints out the result if sign match otherwise Error is printed
	if (sign == '+' || sign == '-' || sign == '*' || sign == '/') 
	std::cout << "Result: " << num1 << " " << sign << " " << num2
		<< " = " << calnum << "\n";
	else
		std::cout << "Operator not found!!\n";
}

[Retro94]

Ok, thanks very much everyone. I read up on ending my programs, and am now using a more efficient way. I also changed it to use switch(op) instead if the if-else-if ladder. All the advice and feedback was great, thanks again.