7
\$\begingroup\$

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;
}
\$\endgroup\$
1
\$\begingroup\$
  • 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.

\$\endgroup\$
1
\$\begingroup\$

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);
}
\$\endgroup\$
  • \$\begingroup\$ Also possibly a check for division by zero error. \$\endgroup\$ – hjpotter92 Dec 2 '15 at 18:15
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.