7
\$\begingroup\$

I wrote an RPN shell stack-based interpreter. It supports following operators: +, -, *, /, %, ^. Right now it only works with positive numbers. It features also printing out the top stack element by entering new line character and popping out the result with = operator. I decided to also add one variable available to the user, it is the x variable, which can be used in two states. X (capital X) is used for reading the value of the variable and x (lowercase x) is used for writing the value to the variable.

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

#define CRESET   "\033[0m"
#define CYELLOW  "\033[33m"
#define CRED     "\033[31m"


#define MAXOP 100
#define NUMBER '0'
#define MAXSTACK 100
#define BUFSIZE 100


int getop(char []);
void push(double);
double top(void);
double pop(void);
int getch(void);
void ungetch(int);

int sp = 0;
double stack[MAXSTACK];
char buf[BUFSIZE];
int bufp = 0;
double x = 0;

main () {
    int type;
    double _op;
    char s[MAXOP];

    while ((type = getop(s)) != EOF) {
        switch (type) {
            case NUMBER:
                push(atof(s));
                break;
            case '+':
                push(pop() + pop());
                break;
            case '*':
                push(pop() * pop());
                break;
            case '-':
                _op = pop();
                push(pop() - _op);
                break;
            case '^':
                _op = pop();
                push(pow(pop(), _op));
                break;
            case '/':
                _op = pop();
                if (_op != 0.0)
                    push(pop() / _op);
                else
                    printf(CRED "ERROR: Cannot divide by 0\n" CRESET);
                break;
            case '%':
                _op = pop();
                if (_op != 0.0)
                    push(fmod(pop(), _op));
                else
                    printf(CRED "ERROR: Cannot divide by 0\n" CRESET);
                break;
            case 'x':
                x = pop();
                break;
            case 'X':
                push(x);
                break;
            case '\n':
                printf(CYELLOW "Top: %.8g\n" CRESET, top());
                break;
            case '=':
                printf(CYELLOW "Result: %.8g\n" CRESET, pop());
                break;
            default:
                printf(CRED "ERROR: Unknown command %s\n" CRESET, s);
                break;
        }


    }


}

void push(double f) {
    if (sp < MAXSTACK)
        stack[sp++] = f;
    else
        printf(CRED "ERROR: Stack is full, cannot push %g\n" CRESET, f);
}

double top(void) {
    if (sp > 0)
        return stack[sp - 1];
    else {
        return 0.0;
    }

}

double pop(void) {
    if (sp > 0)
        return stack[--sp];
    else {
        printf(CRED "ERROR: Stack is empty\n" CRESET);
        return 0.0;
    }

}

int getop(char s[]) {
    int i, c;

    while ((s[0] = c = getch()) == ' ' || c == '\t')
        ;

    s[1] = '\0';

    if (!isdigit(c) && c != '.')
        return c;
    i = 0;
    if (isdigit(c))
        while (isdigit(s[++i] = c = getch()))
            ;
    if (c == '.')
        while (isdigit(s[++i] = c = getch()))
            ;
    s[i] = '\0';
    if (c != EOF)
        ungetch(c);
    return NUMBER;

}


int getch(void) {
    return (bufp > 0) ? buf[--bufp] : getchar();
}

void ungetch(int c) {
    if (bufp >= BUFSIZE)
        printf(CRED "UNGETCH: Too many characters\n" CRESET);

  else
        buf[bufp++] = c;

}

Samples

Input:

> 2 2 + =

Output:

Result: 4
Top: 0

Input:

> 3 x
> X 5 +

Output:

Top: 8

Input (x = 3):

> X 3 ^ 2 +

Output:

Top: 29

Could you review my code? Are there any issues I missed regarding the user input?

\$\endgroup\$
  • \$\begingroup\$ Ooh, I can't wait to dig into this! \$\endgroup\$ – cat Jul 9 '17 at 3:44
6
\$\begingroup\$

Here are some ideas for improving your program.

Use the correct prototype for main

Many compilers will default to an int return type when the function return type is undeclared, but it's not a good idea to omit it, especially with main. Instead of this:

main () {

write this:

int main() {

Eliminate global variables where practical

The code declares and uses 5 global variables. Global variables obfuscate the actual dependencies within code and make maintainance and understanding of the code that much more difficult. It also makes the code harder to reuse. For all of these reasons, it's generally far preferable to eliminate global variables and to instead pass pointers to them. That way the linkage is explicit and may be altered more easily if needed. In this case, I'd suggest gathering everything together into a structure within main's scope and then passing that structure to each of the functions. For example, it might be defined like this:

typedef struct stack_s {
    int sp;
    int bufp;
    double x;
    double stack[MAXSTACK];
    char buf[BUFSIZE];
} StackMachine;

And then within main it could be defined like this:

StackMachine sm = { .sp = 0, .bufp = 0 };

Then push could be reimplemented like this:

void push(StackMachine *sm, double f) {
    if (sm->sp < MAXSTACK)
        sm->stack[sm->sp++] = f;
    else
        printf(CRED "ERROR: Stack is full, cannot push %g\n" CRESET, f);
}

Be consistent with the user interface

When the user uses an empty line to see the top of the stack, it reports 0 instead of STACK EMPTY when the stack is empty. This is inconsistent with the other operations.

\$\endgroup\$
  • \$\begingroup\$ int main() is still not a prototype \$\endgroup\$ – cat Jul 9 '17 at 18:04
3
\$\begingroup\$

Don't use the function name getch

The function getch isn't reserved by ISO C, but it's defined by the widely used conio library on Windows and by curses on Unix. (curses defines ungetch as well.) Choose other names for your functions to avoid conflicts in case you ever decide to add more terminal functionality to your program.

Even if you plan to work exclusively within ISO C, getch isn't very descriptive: the name suggests that it gets a character somehow, but how can the reader guess how it differs from getc and getchar?

Actually, why not just use the standard functions getc and ungetc in the first place? ungetc is only guaranteed to be able to un-get one character, but as far as I can tell, that's all you do. You're creating another layer of buffering on top of input that's already buffered by the standard library, in a manner that's probably better tuned than yours.

Similarly:

Don't tie yourself to stdin

Inevitably, someday you will want your code to be able to read from files other than standard input. (For instance, maybe you want the user to be able to give a list of script files on the command line.) Functions that do input (e.g. getop) should accept a FILE * argument and read from that file. This is a little more typing, but much easier than retrofitting later. For now, you can just pass stdin as the file, but it will be easy to add code later to open other files.

\$\endgroup\$
2
\$\begingroup\$

This review is unfinished, but already quite long. I'll probably write the rest in the next few hours.


Well, this is pretty decent code, and a good attempt at making a simple RPN calculator. Let me take a more thorough look at it, with my compiler options that should be enabled by default:

-Wall -Wextra -Wfloat-equal -Winline -Wundef -Werror -fverbose-asm -Wint-to-pointer-cast \
-Wshadow -Wpointer-arith -Wcast-align  -Wcast-qual -Wunreachable-code -Wstrict-overflow=5 \
-Wwrite-strings -Wconversion --pedantic-errors -ggdb -Wredundant-decls \
-Wstrict-prototypes -Wmissing-prototypes

With -Werror=maybe-uninitialized for GCC and -Werror=uninitialized for Clang.

This code, however, does not compile with a sane configuration.

main.c:30:1: error: return type defaults to ‘int’ [-Wimplicit-int]
 main () {
 ^~~~
main.c:30:1: error: function declaration isn’t a prototype [-Werror=strict-prototypes]
main.c: In function ‘main’:
main.c:56:25: error: comparing floating point with == or != is unsafe [-Werror=float-equal]
                 if (_op != 0.0)
                         ^~
main.c:63:25: error: comparing floating point with == or != is unsafe [-Werror=float-equal]
                 if (_op != 0.0)
                         ^~
main.c: In function ‘getop’:
main.c:120:20: error: conversion to ‘char’ from ‘int’ may alter its value [-Werror=conversion]
     while ((s[0] = c = getch()) == ' ' || c == '\t')
                    ^
In file included from main.c:3:0:
main.c:129:33: error: conversion to ‘char’ from ‘int’ may alter its value [-Werror=conversion]
         while (isdigit(s[++i] = c = getch()))
                                 ^
main.c:132:33: error: conversion to ‘char’ from ‘int’ may alter its value [-Werror=conversion]
         while (isdigit(s[++i] = c = getch()))
                                 ^
main.c: In function ‘ungetch’:
main.c:151:23: error: conversion to ‘char’ from ‘int’ may alter its value [-Werror=conversion]
         buf[bufp++] = c;
                       ^
cc1: all warnings being treated as errors
makefile:2: recipe for target 'main' failed
make: *** [main] Error 1

We'll go over the code and look at what's causing this to not compile.


#define NUMBER '0'

A comment explaining this name would be helpful, or just a better name.


int sp = 0;
double stack[MAXSTACK];
char buf[BUFSIZE];
int bufp = 0;
double x = 0;

Avoid global variables, as implicit global state should be avoided whenever possible, especially in small programs that have no need of it anyways.

Additionally, the descriptiveness of a variable name should scale with its scope. i, j, k, etc are good for loop counters (especially since programmers know i and j will be loop counters); for function scope, depending on the inteded lifetime of the variable, you should use at least a word or two, perhaps shortened.

For global variables, especially those globals that have little cause to exist, you should justify their existence with better names. buf could at least be input_buf, but that's still a little vague for a global. I'd go with input_buf_ptr (bufp) and stack_ptr (sp).

Finally, x is a god awful variable name for any scope or lifetime longer than a couple lines.


main.c:30:1: error: return type defaults to ‘int’ [-Wimplicit-int]
 main () {
 ^~~~
main.c:30:1: error: function declaration isn’t a prototype [-Werror=strict-prototypes]

Two errors for one simple declaration, because...

main () { ... }

This does not declare a function, except in C89.

int main () { ... }

Neither does this. function declaration isn’t a prototype [-Werror=strict-prototypes]

int main (void) { ... }

This does declare and define a function in C99 and C11, which I hope you are using. See Is it better to use C void arguments “void foo(void)” or not “void foo()”?

main must be declared int except if its return type is omitted, in which case it's inferred to be int by the compiler. Compliant programs declare int main.

Since C99, main need not explicitly return int -- if you omit a return value, the compiler will insert return EXIT_SUCCESS; at the end.


int type;
double _op;
char s[MAXOP];

These are poor variable names -- you are not limited to 6 characters, like assemblers of old! Seriously, there's no excuse for these to be named this way.

I can only guess that type is the determined type of the input, op is the determined resultant operator, and s is the raw input string, but I shouldn't have to guess. Use better names, and then use comments for more complex ideas.


while ((type = getop(s)) != EOF) {

C is very cool in that you can do this in one line, but that doesn't mean you should do this if you don't have to -- and you don't have to. Here's one possibility:

char s[MAXOP];
int type;
double op;

do {
  type = getop(s);
  // ...
} while (EOF != type);

It's still a little messy which means the whole flow deserves refactoring, in my opinion.


switch (type) {
    case NUMBER:

Ahh, maybe it should be called OP_NUMBER or so? And why does '0' get a name while '+' and the other operators don't? No apparent reason, so far.


case '/':
    _op = pop();
    if (_op != 0.0)
        push(pop() / _op);
    else
        printf(CRED "ERROR: Cannot divide by 0\n" CRESET);
    break;

Fact: you cannot reliably compare floating point values with == or !=. Instead, you need to use an epsilon and compare the difference, like:

abs(a - b) < 1e-8

This is what I'm used to using, but apparently the following is better:

abs(( a - b ) / b) < 1e-8

I've never actually seen this before, but apparently it's not good enough for the real pedants either. You can, however, use the first, simpler one, and satisfy most input cases as well as the compiler, who is right to warn.

See What Every Programmer Should Know About Floating Point Math, and The Floating Point Guide.

Opinion: omitting braces around control statements like if is dangerous and ugly. The example oft provided is to suppose that someone, be it you or not, comes along and changes the else branch:

else
    printf(CRED "ERROR: Cannot divide by 0\n" CRESET);
    abort();

Well, now the program aborts 100% of the time, and it will take you a couple minutes to figure out why because of the tricky indentation.

If you had used braces, this would have been avoided:

else {
    printf(CRED "ERROR: Cannot divide by 0\n" CRESET);
    abort();
}

Futhermore, and perhaps even more extreme, I always use block-style case:

case j: {
  printf("%s\n", x);
  break;
}

It looks better to me, and feels much less haphazard than braceless case.


case 'x':
    x = pop();
    break;
case 'X':

Whaaat? Why the letter x? Why does letter case matter? Comments would be helpful.


void push(double f) { ... }

f is not returned or modified, nor should it be. As such, it should be declared const, so that the compiler may make optimizations and assumptions around the fact it will not change:

void push (const double f) { ... }

if (sp < MAXSTACK)
    stack[sp++] = f;
else
    printf(CRED "ERROR: Stack is full, cannot push %g\n" CRESET, f);

In these four lines, six names are used. A total of eight references are made to names from this translation unit, but only one of these names was declared anywhere near this code.

I know what f is; it's a mutable double. For the other names, I have to think a little bit, to remember where they came from -- this is bad, because it should be explicit and obvious where they came from.

I still can't think why those need to be globals, but nevermind that.

Ditto on the braces, too.


double top(void) {

Alright, now there's a declaration!


if (sp > 0)
    return stack[sp - 1];
else {
    return 0.0;
}

I semantically disagree with making an empty stack return 0.0 forever, but that is just personal style.


double pop(void) {
    if (sp > 0)
        return stack[--sp];
    else {
        printf(CRED "ERROR: Stack is empty\n" CRESET);
        return 0.0;
    }

}

Rather oddly and inconsistently, getting the top of an empty stack is fine and gives 0.0 but popping from an empty stack is an error, and gives 0.0.

\$\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.