0

I copied a simple "calculator" example from my textbook that accepts 2 values and an operator. However, it does not check if the expression entered is valid. The code should prompt the user until a proper expression is entered.

How should I fix it with my style in mind? What is a better way of doing this?

/*
Input: A simple expression with 2 values and 1 operator
Format: value1 (operator) value2
Ex: 5*5
Output: Answer
*/

#include <stdio.h>

int main (void)
{
float value1, value2;
char operator = ' ';

//Issue Area, the while section does not work
do
{
    printf ("Type in your expression:");
    scanf ("%f %c %f", &value1, &operator, &value2);
}
while ((operator != '+' || operator != '-' || operator != '*' || operator != '/'));

//This is fine, code is for an else...if example in textbook
if (operator == '+')
    printf ("%.2f\n",value1 + value2);
else if (operator == '-')
    printf ("%.2f\n",value1 - value2);
else if (operator == '*')
    printf ("%.2f\n",value1 * value2);
else if (operator == '/')
    printf ("%.2f\n",value1 / value2);

return 0;
}
AGN
  • 61
  • 1
  • 1
  • 9
  • Questions seeking debugging help ("why isn't this code working?") must include the desired behavior, a specific problem or error and the shortest code necessary to reproduce it in the question itself. Questions without a clear problem statement are not useful to other readers. See: How to create a Minimal, Complete, and Verifiable example. 1 – too honest for this site Jul 07 '16 at 19:49
  • 6
    You need `&&` not `||`. Otherwise your logical expression will always succeed. It should be the equivalent of `!(operator == '+' || operator == '-' || operator == '*' || operator == '/')` – lurker Jul 07 '16 at 19:49
  • Read the loop body and condition out loud, speaking `||` as "or". – too honest for this site Jul 07 '16 at 19:50
  • You should learn more logic, and read about [*short-circuit evaluation*](https://en.wikipedia.org/wiki/Short-circuit_evaluation). – Some programmer dude Jul 07 '16 at 19:50
  • @lurker Thanks. Makes more sense. – AGN Jul 07 '16 at 20:03

1 Answers1

3

You have:

do
{
   ... 
}
while ((operator != '+' || operator != '-' || operator != '*' || operator != '/'));

Use of || in the conditional of the while is wrong.

Let's say the value of operator is '+'. Then, the while evaluates to:

while ((false || true || true || true));

which evaluates to

while (true);

No matter what the value of operator is, at least three of those sub-expressions will evaluate to true. Hence, the conditional will always evaluate to true.

You need to use && instead of ||.

do
{
   ... 
}
while ((operator != '+' && operator != '-' && operator != '*' && operator != '/'));

One possible way to make the code clearer is:

do
{
   ... 
}
while (!isValidOperator(operator));

where 

int isValidOperator(char operator)
{
   return (operator == '+' ||
           operator == '-' ||
           operator == '*' ||
           operator == '/' );
}

You can make code shorter in isValidOperator by using:

int isValidOperator(char operator)
{
   return (operator != '\0' && strchr("+-*/", operator) != NULL);
}
R Sahu
  • 204,454
  • 14
  • 159
  • 270