while loop runs even if condition isnt true

Question

i am trying to make a calculator using c++, im trying to implement error handling, so if the user enters a non arithmetic operator, it will tell the user to please enter an operator, using a while loop. the problem is, even when the user enters an operator on the first run through, the while loop still executes.

I have tried not putting a space between while, and the perinthesis, also, i tried not using a variable, and just putting all the conditionals to trigger the loop.

string getop()
{
  string op;
  int check = 1; 
  cout << "Enter an operator (+ - / *): ";
  cin >> op;
  if ((op != "+") || (op != "-") || (op != "/") || (op != "*"))
  {
    check = 0;
  }
  while (check == 0) // while the input is not a valid operator
  {
    cout << "Invalid operator, please enter a valid operator: ";
    cin >> op;
    if ((op == "+") || (op == "-") || (op == "/") || (op == "*"))
      check = 1;
  }

  return op;
}

the problem is, even when the user enters an operator on the first run through, the while loop still executes.

`if ((op != "+") || (op != "-") || (op != "/") || (op != "*"))` this comparison is wrong. At least 3 of these will always be true, so if you `or` them together, you get `true` every time. Do it like your second time instead. Have `check` start out at `0` and do `if ((op == "+") || (op == "-") || (op == "/") || (op == "*"))` to see if you should put it to `1`. — Blaze, Jul 02 '19 at 14:52
@Blaze it's a bummer. This comment alone has more explanation than 2/3 of the answers at the moment. If only you'd left this as answer so it could be recognized as such. — scohe001, Jul 02 '19 at 14:55
`if (!...)` followed by `while (!...)` could be simply replaced by `do { } while (!...);`. This would prevent code duplication. — Scheff's Cat, Jul 02 '19 at 14:57
This doesn't address the question, but the code doesn't need all those parentheses. `if (op == "+" || op == "-" || op == "/" || op == "*")` works just fine. `==` has higher precedence than `||`, which really is the only thing that would make sense. — Pete Becker, Jul 02 '19 at 15:03
@scohe001 thanks for the acknowledgement. I was in a hurry so I didn't have time for an elaborate answer, but it looks like the current answers are good and elaborate now. — Blaze, Jul 03 '19 at 06:08

score 4 · Answer 1 · answered Jul 02 '19 at 14:52

4

Try:

(op != "+") && (op != "-") && (op != "/") && (op != "*")

Operator || is or operator (alternative, one or another is enough). You want operator &&, which forces all conditions to be true together.

answered Jul 02 '19 at 14:52

Radosław Cybulski

2,952
10
21

Thank you so much for responding, i cant believe i missed this – berg95 Jul 02 '19 at 15:01
2

@berg95 - if you keep programming, be assured you will do *much* dumber stuff than that. After the first decade or so, you get used to asking "HOW could I be so stupid?" – Martin Bonner supports Monica Jul 02 '19 at 15:19

Zev Chonoles · Answer 2 · 2019-07-02T14:53:12.927

3

You want to use AND (&&), not OR (||):

if ((op != "+") && (op != "-") && (op != "/") && (op != "*"))
{
  check = 0;
}

edited Jul 02 '19 at 14:53

answered Jul 02 '19 at 14:52

Zev Chonoles

1,063
9
13

3

While this code may solve the problem the answer would be a lot better with an explanation on how/why it does. Remember that your answer is not just for the user that asked the question but also for all the other people that find it. – NathanOliver Jul 02 '19 at 14:52

Stack Danny · Accepted Answer · 2019-07-02T15:04:01.627

3

You have a logic error:

if ((op != "+") || (op != "-") || (op != "/") || (op != "*"))

this will always yield true no matter what op is.

You want:

if ((op != "+") && (op != "-") && (op != "/") && (op != "*"))

A very good practise is to name your logical events. Also, AND chaining negations is rather unintuitive (and also unreadable). So an even better alternative would be:

bool is_valid = (op == "+") || (op == "-") || (op == "/") || (op == "*");

if (!is_valid)
    check = 0;

You also shouldn't use namespace std; - you can read here why.

edited Jul 02 '19 at 15:04

answered Jul 02 '19 at 14:52

Stack Danny

7,754
2
26
55

Why is it better to create another (unneeded) local variable? (also note that you accidentally left the OR's in on that new code) – scohe001 Jul 02 '19 at 14:56
@scohe001 to improve readability drasticly. This is considered good practise. And the OR's are intentional, as this is the positive case. – Stack Danny Jul 02 '19 at 14:58
Ahh my fault, I misread there. Do note that this same conditional statement will be checked again in the while loop, so you'll end up having to duplicate this code again anyways. I still think just a `while(op != "+" && op != .... ) {` without any of the if's and the int variable would be way cleaner. But to each their own. – scohe001 Jul 02 '19 at 15:01
Thank you for responding, now i wont use namespace anymore, and i fixed the issue – berg95 Jul 02 '19 at 15:04
@scohe001 I thought so too, but I frequently received a comment that one shall always name their events. like the comment [here](https://stackoverflow.com/questions/56288821/lambda-expression-to-return-bool-in-if-statement/56288924#56288924) – Stack Danny Jul 02 '19 at 15:07
Meh. Do what you think is most pretty/readable. To me this is not, but I can understand that you may disagree. – scohe001 Jul 02 '19 at 15:19
I think it's one step towards writing expressive code. Code that speaks for itself. – Stack Danny Jul 02 '19 at 15:25

score 1 · Answer 4 · answered Jul 02 '19 at 14:58

Your problem is right here:

if ((op != "+") || (op != "-") || (op != "/") || (op != "*"))
{
    check = 0;
}

Suppose op is assigned a value of "+", that would mean that these conditions all evaluate to true: op != "-", op != "/", op != "*".

Since you are using the OR (||) operator, check will be assigned a value of 0 if any of those conditions are true. In fact, one of those four conditions will always be true no matter what value op has.

You should use AND (&&) instead so that check is assigned 0 when all of the conditions are true:

if ((op != "+") && (op != "-") && (op != "/") && (op != "*"))
{
    check = 0;
}

score 0 · Answer 5 · answered Jul 05 '19 at 20:06

when you have a chain of assertions joined by &&, normally the negation consists in negate all the assertions and change all the && operators by || or viceversa.

You changed all the asserrtions, negating them, but you forgot to change the || by &&.

Look for Demorgan's theorem in Google.

while loop runs even if condition isnt true

5 Answers5