3
\$\begingroup\$

I have to solve an exercise from ANSI C book:

Add the commands to print the top elements of the stack without popping, to duplicate it, and to swap the top two elements. Add a command to clear the stack.

Here is my implementation in RPN Calculator:

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

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

int getop(char []);
void push(double);
double pop(void);
double top(void);
void clear(void);
void swap(void);
void duplicate(void);

int getch(void);
void ungetch(int);

int sp = 0;
double val[MAXVAL];
char buf[BUFSIZE];
int bufp = 0;

int main(void) {
    int type;
    double op2;
    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 '-':
            op2 = pop();
            push(pop() - op2);
            break;
        case '/':
            op2 = pop();
            if (op2 != 0.0)
                push(pop() / op2);
            else
                printf("error: zero divisor\n");
            break;
        case '%':
            op2 = pop();
            if (op2 != 0)
                push((int) pop() % (int) op2);
            else
                printf("error: zero divisor\n");
            break;
        case 'c':
            clear();
            printf("stack empty!\n");
            break;
        case 'd':
            duplicate();
            printf("duplicate element\n");
            break;
        case 's':
            swap();
            printf("swap elements\n");
            break;
        case '?':
            printf("top element on the stack: %f\n", top());
            break;
        case '\n':
            printf("\t%.8g\n", pop());
            break;
        default:
            printf("error: unknown command %s\n", s);
            break;
        }
    }
    return 0;
}

void push(double f) {
    if (sp < MAXVAL)
        val[sp++] = f;
    else
        printf("error: stack full, can't push %g\n", f);
}

double pop(void) {
    if (sp > 0)
        return val[--sp];
    else {
        printf("error: stack empty\n");
        return 0.0;
    }
}

double top(void) {
    if (sp > 0)
        return val[sp - 1];
    else {
        printf("error: stack empty\n");
        return 0.0;
    }
}

void clear(void) {
    if (sp > 0) {
        while (--sp > 0);
    } else {
        printf("error: stack empty\n");
    }
}

void duplicate(void) {
    if (sp > 0) {
        int elem = val[sp - 1];
        push(elem);
    } else {
        printf("error: stack empty\n");
    }
}

void swap(void) {
    if (sp > 0 && (sp - 1) > 0) {
        int elem1 = val[sp - 1];
        int elem2 = val[sp - 2];
        val[sp - 1] = elem2;
        val[sp - 2] = elem1;
    } else {
        printf("error: stack empty\n");
    }
}

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

    while ((s[0] = c = getch()) == ' ' || c == '\t')
        ;
    s[1] = '\0';
    if (!isdigit(c) && c != '.' && c != '-')
        return c;
    i = 0;
    if (c == '-')
        s[++i] = c = getch();
    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("ungetch: too many characters\n");
    else
        buf[bufp++] = c;
}

What are your opinions about it? Is it proper and optimal way to implement these functions?

\$\endgroup\$
2
\$\begingroup\$
  1. Your clear function uses while (--sp > 0); when sp = 0; does the same thing in faster and clearer way. also consider not throwing an error when clearing an empty stack, because it still behaves as expected.

  2. The condition sp > 0 && (sp - 1) > 0 is esentially just sp > 1 or sp >= 2, which I would prefer, but that is just a matter of personal style.

  3. Instead of using val[sp-1] to get the top element of the array (in swap and duplicate) use the top() function, as it is a good idea to express your stack operations in terms of more primitive stack operations.

  4. Follwing the concept of the defining operation in terms of primitive operations, duplicate can be written as:

    void duplicate(void) {
        push(top());
    }
    

    and swap can be written as:

    void swap(void) {
        int elem1 = pop();
        int elem2 = pop();
        push(elem1);
        push(elem2);
    }
    

    Notice how there is no error checking in these functions, because push(), pop(), and top() handle the full or empty stack for us.

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

Your code looks really good.

I doubt it is simple homework because you took care about the evaluation order for the - and / operators and wrote the code for + and * in the unspecified order because these operations are commutative. This level of knowledge is not typical for homework.

One thing you overlooked is calling isdigit with a possibly signed char, which invokes undefined behavior. You have to cast the argument to unsigned char.

Other than that, the code looks carefully written.

If you want to make your code reusable, you should convert the global variables into a struct stack and then define the operations on this stack. This also makes the code easily testable since you will need a function called stack_init that initializes all variables needed for the stack.

\$\endgroup\$
  • \$\begingroup\$ Note that he only wrote duplicate, swap and clear, as well as a command to print the top of the stack. \$\endgroup\$ – Peter Jun 23 '18 at 21:36

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.