RPN calculator in C

I had to write a RPN calculator in C as one of my homework problems, but if someone could critique my code and suggest any improvements, that would be fantastic! I haven't added any overflow errors or input errors, but I will add that eventually.

My code takes operands and operators from the command line (e.g., 3 4 - would return -1), and sends it to the decode function which uses the pop and push functions.

#include 
#include 

void push(float stack[], float value, int *currStack)
{
    int i = *currStack;

    while (i != 0)
    {
        stack[i] = stack[i-1];
        i--;
    }

    stack[0] = value;
    *currStack += 1;
}

void pop(float stack[], char operation, int *currStack)
{
    int i;

    switch (operation)
    {
        case '+':
            stack[0] = stack[1] + stack[0];
            break;
        case '-':
            stack[0] = stack[1] - stack[0];
            break;
        case '*':
            stack[0] = stack[1] * stack[0];
            break;
        case '/':
            stack[0] = stack[1] / stack[0];
            break;
        default:
            printf("Invalid character.");
            break;
    }

    for (i=1;i<*currStack;i++)
    {
        float temp = stack[i];
        stack[i] = stack[i+1];
        stack[i+1] = temp;
    }

    *currStack -= 1;
}

int decode(char **instring, float *outval, int size)
{
    int i=0, currStack=0;
    float stack[size/2];

    for (i=1;i<size;i++)
    {           
        if (atof(instring[i]))
            push(stack, atof(instring[i]), &currStack);
        else
            pop(stack, *instring[i], &currStack);

        *outval = stack[0];
    }
}

int main(int argc, char *argv[])
{
    float result;
    decode(argv, &result, argc);

    printf("The answer is: %.3f\n", result);

    return 0;
}

for (i=1;i<*currStack;i++)
{
    float temp = stack[i];
    stack[i] = stack[i+1];
    stack[i+1] = temp;
}

This is more complicated than it needs to be.

float temp = stack[1];
for ( i = 1; i < *currStack; i++ )
{
    stack[i] = stack[i+1];
}
stack[*currStack] = temp;

This version has the exact same effect but does currStack + 1 assignments where your original does currStack 3 - 3 assignments (although that may drop to currStack * 2 - 2 assignments with good register management by the compiler).

We can do even better since we don't care about the value that is at stack[1] before we start.

for ( i = 1; i < *currStack; i++ )
{
    stack[i] = stack[i+1];
}

The for loop is sufficient. We don't need to muck around with temp at all. It's just junk data at this point.

Note: I'm not disputing vnp's point that the copy is unnecessary. I'm just saying that if you do do the copy, you don't actually have to copy the stack[1] value. And you certainly don't have to copy stack[1] once per element in the stack.

default:
        printf("Invalid character.");
        break;
}

You don't actually need the break; there. It's all right to let it fall out of the default case on its own. It won't hurt anything, but I think it's more readable to set off default slightly by having it not break.

int decode(char **instring, float *outval, int size)

You say that decode is supposed to return an int but you never return anything. You could declare this of type void.

void decode(char **instring, float *outval, int size)

Then no one would expect it to return anything.

float stack[size/2];

for (i=1;i<size;i++)

You should probably comment why this works. Note that it's only because argc is one greater than the number of arguments that it does. This shows up implicitly in the way that you start with i=1 but could be clearer. As a general rule, any time you do something clever, you should comment to explain it. Otherwise, you have to be clever again to read the code.

for (i=1;i<size;i++)
{           
    if (atof(instring[i]))
        push(stack, atof(instring[i]), &currStack);
    else
        pop(stack, *instring[i], &currStack);

    *outval = stack[0];
}

You assign a value to *outval on every iteration of the loop but only use it once. You could do this outside the loop with the same ultimate effect.

for ( i = 1; i < size; ++i ) {
    double temp;
    if ( temp = atof(instring[i]) ) {
        push(stack, temp, &currStack);
    } else {
        pop(stack, *instring[i], &currStack);
    }
}

*outval = stack[0];

You also don't need to calculate atof(instring[i]) twice. Save it the first time.

I added curly braces around the statements in the if/else. The single statement version is susceptible to a class of typo bugs that is rather hard to diagnose.

There is an argument that ++i is faster than i++. It's not a big difference and a good compiler should be able to optimize this out, but it doesn't hurt anything to use the prefix notation.

Your code doesn't allow for values of 0.0 and will display "Invalid character" in that case. Yet that's a valid value. Another possibility would be

if ( temp = atof(instring[i]) || ! strcmp(instring[i], "0.0") ) {

which would allow for 0.0, although it only supports one format. An alternative would be an is_zero function which could do a more exhaustive check.

The string manipulation to check for a number is more expensive (in terms of time) than checking for an operator. Checking for an operator only requires checking two characters to establish that it is an operator.

if ( ! instring[i][1] && is_operator(in_string[i][0] ) {
    pop(stack, *instring[i], &currStack);
} else if ( temp = atof(instring[i]) || ! strcmp(instring[i], "0.0") ) {
    push(stack, temp, &currStack);
} else {
    printf("Invalid argument:  [%s].", in_string[i]);
}

Then you just need an is_operator function:

int is_operator(char c) {
    switch (c) {
        case '+':
        case '-':
        case '*':
        case '/':
            return 1;
        default:
            return 0;
    }
}

The atof statement returns a double, but you are only using float variables. It would be better to make stack, outval, etc. hold double values rather than just float values. That would save the implicit conversions as well. Note that %lf in printf expects a double as well.

StackExchange Code Review Q#79738, answer score: 7