Learning C++, problem with loop condition

[makken]

hey guys, I'm trying to learn C++, I don't have any prior programming experience except scripting in matlab.

One of the first programs I'm trying to make is a simple calculator that takes 2 inputs and an operator from the user and outputs a result. I got the program working, but I would like to modify it to include a check for a valid operator.

The operator is stored as a char variable, my first thought as to how to implement a check is with a loop:

While(mathoperator !=( '+' || '-' || '*' || '/'))
{
    cout<<mathoperator<<" is not a valid operator, use +, -, *, /. Please try again:"<<endl;
    cin>>mathoperator;
}

However, the condition is always coming up as true, could someone explain why this is happening?

Thanks

 

[iCyborg]

You cannot make comparisons like that:
X ! = (A or B or C) does not mean (X!=A) or (X!=B) or (X!=C) which is what you want.
What you're doing is basically ('+' || '-' || '*' || '/') is evaluated to true/1 and then it checks whether mathoperator is different than 1, which if it's a normal character it always is.

So, do ( (mathoperator != '+') && (mathoperator != '-') ...

 

[makken]

ah, makes sense; I wasn't thinking in terms of 0/1.

thanks a lot.

 

[Markbnj]

ah, makes sense; I wasn't thinking in terms of 0/1.

thanks a lot.

The important thing is to understand precedence in evaluation of expressions (and the correct use of operators in constructing expressions). It's fundamental to C/C++, and most other languages.

Look at the expression:

While(mathoperator !=( '+' || '-' || '*' || '/'))

First, a seemingly minor point but important nonetheless... 'While' is not a keyword in C++... 'while' is.

So the first thing that's going to happen in evaluating that expression is that the terms in the innermost set of parentheses will be evaluated first. You've asked the compiler to evaluate:

( '+' || '-' || '*' || '/')

Each of the literal terms is of type char, which is an unsigned 8-bit integral value. You can view each of those as a non-zero 8-bit integer. After substitution of literals the expression looks like this:

( 43 || 45 || 42 || 47)

The operator || returns true (non-zero) if either of it's terms is true (non-zero). Associativity is left to right, though it doesn't matter in this case. So this innermost expression evaluates to:

(true)

Popping it back into the whole statement you see the problem:

while(mathoperator !=(true))

You could construct a proper while condition for this application, but it would be cumbersome. A more typical implementation of a simple command processor would use a switch statement:

switch (mathoperator)
{
    case '+' :
        // add the terms
        break;

    case '-' : 
        // subtract the terms
        break;

    case '*' :
        // multiply the terms
        break;

    case '/' :
        // divide the terms
        break;

    default:
        // invalid operator
        break;
}

 

[iCyborg]

You could construct a proper while condition for this application, but it would be cumbersome. A more typical implementation of a simple command processor would use a switch statement:

The problem with this approach in his situation is that for an invalid operator he wants to reread it from console and repeat the test - he'd need a goto: out of switch's default block or something to that effect.

 

[Markbnj]

The problem with this approach in his situation is that for an invalid operator he wants to reread it from console and repeat the test - he'd need a goto: out of switch's default block or something to that effect.

I would think you'd just put the switch statement inside a loop that keeps reading from the console.

 

[iCyborg]

Right. Then I've misunderstood the part I quoted, because it seemed to me that it implied using switch instead of while loop.

 

[Markbnj]

Right. Then I've misunderstood the part I quoted, because it seemed to me that it implied using switch instead of while loop.

Yeah I didn't really give the whole picture. The idea was just to show him that the tests moved out of the while condition and into the switch.

 

[makken]

I did use the switch statement to do the actual calculation in the program, it was just with my limited knowledge right now, I couldn't figure out a way to write a default case where it would warn the user of an invalid operator, request a new one, and redo the calculation; so I decided to use the while loop prior to the switch statement to check then.

The actual reference solution was given using if...elseif statements instead of switch

I just read the chapter on associativity and precedence and I'm /facepalming myself over this already. =X

what I ended up doing was

#include "stdafx.h"
#include <iostream>
using namespace std;

float firstinput;
char mathoperator;
float secondinput;
float result;

float GetFirstUserInput()
{
	cout<<"Please enter a number: "<<endl;
	cin>>firstinput;
	return firstinput;
}
char GetMathOperator()
{
	cout<<"Please enter a math operator: "<<endl;
	cin>> mathoperator;
	while((mathoperator !='+')&& (mathoperator != '-')&& (mathoperator !='*')&& (mathoperator !='/'))
	{
		cout<<mathoperator<<" is not a valid operator, use +,-,*,/, please try again: "<<endl;
		cin>>mathoperator;
	}
	return mathoperator;
}
float GetSecondUserInput()
{
	cout<<"Please enter a number: "<<endl;
	cin>>secondinput;
	return secondinput;
}
float DoCalc(float firstinput, char mathoperator, float secondinput)
{
	switch (mathoperator)
	{
	case '+':
		result = firstinput + secondinput;
		break;
	case '-':
		result = firstinput - secondinput;
		break;
	case '*':
		result = firstinput * secondinput;
		break;
	case '/':
		result = firstinput / secondinput;
		break;
	default:
		cout<<"Math operator error";
		break;
	}
	return result;
}
int main()
{
	GetFirstUserInput();
	GetMathOperator();
	GetSecondUserInput();
	DoCalc(firstinput,mathoperator,secondinput);
	cout<<"The Result is: "<<result<<endl;
}

 

[iCyborg]

I don't want to sound like I'm nitpicking, but your code has a number of design issues and C++ 'don't dos':

1. You should avoid using global variables, and they are completely unnecessary here. E.g. it's really bad to have 2 functions that do exactly the same thing, except to a different global variable (your Get(First|Second)Input() functions)
2. IF you are using it, then do not send them as function arguments since they are already accessible in it. Especially if you're passing by value, because
3. Your DoCalc() function is overriding the global firstinput, mathoperator and secondinput variables with local ones. Try modifying them insice DoCalc(), and printing them out in main - no change.
4. DoCalc()'s return value is ignored - you are setting global var result to something, and then returning the value of it, and this return value is never used because you use result directly. IF you're going to use globals (and you shouldn't), make the function void and just use return;
5. What happens if I enter firstinput=3, mathoperator='/' and secondinput=0? 😛
6. A minor note: cout << '\n; and cout << endl; are not identical. std::endl also immediately flushes the output and is thus sometimes slower. It's better to use '\n' unless you explicitly want flushing.

I think the calculator example appears in books often (IIRC Stroustrup's C++ reference book has it) and it's normally done with functions for each token type (number, operation etc.) which recursively call each other, but for simplicity I'll stick to your simple approach with just one operation and give the code without globals:

#include <iostream>
using namespace std;

float GetUserInput()
{
	float input;
	cout<<"Please enter a number: "<<endl;
	cin>>input;
	return input;
}

char GetMathOperator()
{
	cout<<"Please enter a math operator: "<<endl;
	char oper;
	cin>> oper;
	while((oper !='+') && (oper != '-') && (oper !='*') && (oper !='/'))
	{
		cout << oper << " is not a valid operator, use +,-,*,/, please try again: \n";
		cin>>oper;
	}
	return oper;
}

float DoCalc(float firstinput, char mathoperator, float secondinput)
{
	float result;
	switch (mathoperator)
	{
	case '+':
		result = firstinput + secondinput;
		break;
	case '-':
		result = firstinput - secondinput;
		break;
	case '*':
		result = firstinput * secondinput;
		break;
	case '/':
		if (secondinput != 0)
			result = firstinput / secondinput;
		else 
		{
			cout << "Division by 0\n";
			result = 0; // set to some dummy error value, or call abort()..
		}
		break;
	default:
		cout<<"Programming error";
		break;
	}
	return result;
}


int main()
{
	float firstinput = GetUserInput();
	char mathoperator = GetMathOperator();
	float secondinput = GetUserInput();
	
	float result = DoCalc(firstinput,mathoperator,secondinput);
	cout<<"The Result is: "<< result << endl;

	return 0;
}

 

[makken]

I don't want to sound like I'm nitpicking, but your code has a number of design issues and C++ 'don't dos':

1. You should avoid using global variables, and they are completely unnecessary here. E.g. it's really bad to have 2 functions that do exactly the same thing, except to a different global variable (your Get(First|Second)Input() functions)
2. IF you are using it, then do not send them as function arguments since they are already accessible in it. Especially if you're passing by value, because
3. Your DoCalc() function is overriding the global firstinput, mathoperator and secondinput variables with local ones. Try modifying them insice DoCalc(), and printing them out in main - no change.
4. DoCalc()'s return value is ignored - you are setting global var result to something, and then returning the value of it, and this return value is never used because you use result directly. IF you're going to use globals (and you shouldn't), make the function void and just use return;
5. What happens if I enter firstinput=3, mathoperator='/' and secondinput=0? 😛
6. A minor note: cout << '\n; and cout << endl; are not identical. std::endl also immediately flushes the output and is thus sometimes slower. It's better to use '\n' unless you explicitly want flushing.

I think the calculator example appears in books often (IIRC Stroustrup's C++ reference book has it) and it's normally done with functions for each token type (number, operation etc.) which recursively call each other, but for simplicity I'll stick to your simple approach with just one operation and give the code without globals:

thanks for the suggestions, I'm still learning the ropes, so those are definitely helpful.

one of the tenets of this exercise was to write separate functions for getting input and doing the calc itself. Honestly this was a bit counter-intuitive to me; it doesn't seem like doing this with functions was necessary.

I was under the impression that any variables declared inside of a function only existed there (I vaguely remember reading about variables being destroyed once program exited the { } in which they were declared). Thus I thought if I cin to a variable that was declared inside the function, that data would've been lost.

I hadn't considered the division by zero scenario, thanks for pointing that out.

 

[tatteredpotato]

I was under the impression that any variables declared inside of a function only existed there (I vaguely remember reading about variables being destroyed once program exited the { } in which they were declared). Thus I thought if I cin to a variable that was declared inside the function, that data would've been lost.

I hadn't considered the division by zero scenario, thanks for pointing that out.

What he's saying is you have a name collision. Ex: a global variable and a function variable have the same name. In C this is allowed, any references will always refer the the most nearly scoped variable (the function variable), however it's considered bad form.

Two be clear, even though the two variables have the same name, they still represent different locations in memory so changing one won't impact the other.

 

[tatteredpotato]

6. A minor note: cout << '\n; and cout << endl; are not identical. std::endl also immediately flushes the output and is thus sometimes slower. It's better to use '\n' unless you explicitly want flushing.

I wasn't aware of this... nice tip.

 

[iCyborg]

I was under the impression that any variables declared inside of a function only existed there (I vaguely remember reading about variables being destroyed once program exited the { } in which they were declared). Thus I thought if I cin to a variable that was declared inside the function, that data would've been lost.

Yes, they only exist there, which is a good thing. Most functions only deal with a handful of variables; if some does not, it's probably trying to do too much, or the variables could have been grouped inside structs or classes. That's why functions have arguments so we only send to them the variables they really need and we can control whether they are able to modify them. E.g., your GetFirstUserInput() has access to secondinput, result and mathoperator and it has nothing to do with them. Communication back is done with return values. You can see how I did it, cin reads an input and this is what the function returns. You can also send arguments as references (or pointers, though references are preferred in most cases for reasons that would probably confuse you more at this point):

void GetUserInput(float& input)
{
	cout<<"Please enter a number: "<<endl;
	cin>>input;
}


...

int main()
{
	GetUserInput(firstinput);
	char mathoperator = GetMathOperator(); // can do similarly for mathoperator
	GetUserInput(secondinput);
        ...
}

I agree that all these functions aren't necessary in this toy example.