Simple Expression Calculator in C

Question

The calculator evaluates a mathematical expression with binary operators +, -, *, / and ^ in C. I store numbers and operators into two different arrays and process them according to precedence.

Please review my code and suggest me on changes on approaches to make it scalable to add unary operators and parenthesis.

#include <stdio.h>
#include <math.h>

double evaluate (char []);
int precedence(char);
double calculate(char , double , double );
char checknumber(char);

int main(void)
{
    char expression[100];
    double result;

    printf("Enter the Expression: "); scanf("%[^\n]", expression);
    result = evaluate(expression);
    printf("Result = %lf\n", result);
    return 0;
}

double evaluate(char expr[])
{
    double numbers[5]; int nsi = 0;
    char operators[5]; int osi = 0;
    char numbuf[16]; int nbi = 0;
    char ch; int  i = 0;

    while ((ch = expr[i]) != 0) {
        if (checknumber(ch)) 
        {
            numbuf[nbi++] = ch;
            if (!checknumber(expr[i + 1])) 
            {
                numbuf[nbi] = 0; nbi = 0;
                sscanf(numbuf, "%lf", &numbers[nsi++]); 
            }
        }
        else
        {
            while ((osi > 0) && (precedence(ch) <= precedence(operators[osi - 1])))
            {
                numbers[nsi - 2] = calculate(operators[osi - 1], numbers[nsi - 2], numbers[nsi - 1]);
                osi--; nsi--;
            }
            operators[osi++] = ch;          
        }
        i++;
    }
    while (osi > 0) {
        numbers[nsi - 2] = calculate(operators[osi - 1], numbers[nsi - 2], numbers[nsi - 1]);
        osi--; nsi--;
    }
    return numbers[0];
}

char checknumber(char ch) 
{
    if ((ch >= '0' && ch <= '9') || ch == '.') return 1; else return 0;
}
int precedence(char ch)
{
    int precedence;
    switch (ch) 
    {
    case '+':
    case '-':
        precedence = 0;
        break;
    case '*':
    case '/':
        precedence = 1;
        break;
    case '^':
        precedence = 2;
    }
    return precedence;
}

double calculate(char moperator, double num1, double num2) 
{
    double result;
    switch (moperator)
    {
    case '+':
        result = num1 + num2;
        break;
    case '-':
        result = num1 - num2;
        break;
    case '*':
        result = num1 * num2;
        break;
    case '/':
        result = num1 / num2;
        break;
    case '^':
        result = pow(num1, num2);
        break;
    default:
        printf("Invalid Operator\n");
        exit(-1);
    }
    return result;
}

Answer 1

• Returning boolean

  The

      if (condition)
          return true;
      else
          return false;
  

  is a pattern to avoid. An idiomatic

      return condition;
  

  does the same in a much more transparent way.

• Testing for digits

  A standard library provides isdigit(). Use it instead of manually checking the range.

• Reading numbers

  The code accepts numbers with multiple dots, such as 1.2.3, and silently discards everything from the second dot. I recommend to detect a malformed input and issue an error message.

• Reading expressions

  The code accepts multiple operators, as in 1+*-2. I have no idea how this expression is interpreted. What is important, the nsi - 2 becomes negative, and the code accesses numbers beyond bounds. This is UB.

Answer 2

In calculate why not just return directly from the different cases? This would be simpler than setting a temporary value, breaking from the switch and returning that value.

switch (moperator)
{
case '+':
    return num1 + num2;
case '-':
    return num1 - num2;
case '*':
    return num1 * num2;
case '/':
    return num1 / num2;
case '^':
    return pow(num1, num2);
default:
    printf("Invalid Operator\n");
    exit(-1);
}

Answer 3

This function is odd:

char checknumber(char ch) 
{
    if ((ch >= '0' && ch <= '9') || ch == '.') return 1; else return 0;
}

To make a function boolean, this post explains some good options.

Even if you don't change the return type, the implementation can be written simpler:

return ((ch >= '0' && ch <= '9') || ch == '.');

I would also reorder the expressions in the range, as the logic is more clear when the values of the terms are organized in increasing order:

return '0' <= ch && ch <= '9' || ch == '.';

I removed the unnecessary parentheses, but if they help you understand the expression you may leave them in.

Undefined behavior

In this function, if none of the operators are matched, then the precedence variable will be returned, even though it was never assigned.

int precedence(char ch)
{
    int precedence;
    switch (ch) 
    {
    case '+':
    case '-':
        precedence = 0;
        break;
    case '*':
    case '/':
        precedence = 1;
        break;
    case '^':
        precedence = 2;
    }
    return precedence;
}

What will be the value then? 0? Most probably it's indeterminate. It would be better to explicitly define a default value.

Also this function can be simplified as @SuperBiasedMan showed for the other switch block:

int precedence(char ch)
{
    switch (ch) 
    {
    case '+':
    case '-':
        return 0;
    case '*':
    case '/':
        return 1;
    case '^':
        return 2;
    }
    return INT_MAX;
}

To get INT_MAX, you'd need to add #include <limits.h>.

One statement per line

Avoid multiple statements per line like these:

printf("Enter the Expression: "); scanf("%[^\n]", expression);
// ...

double numbers[5]; int nsi = 0;
char operators[5]; int osi = 0;
char numbuf[16]; int nbi = 0;
char ch; int  i = 0;

Code is easiest to read from top to bottom. I suggest to rewrite this to have one statement per line.