4
\$\begingroup\$

This is a program exercise from The C Programming Language by Kernighan and Ritchie (Chap 5).

It is based on my earlier question here on Stack Overflow.

One user suggested to submit my code here, saying there's a whole lot of bad coding practice in this snippet. Since most of the harmful practice here originates from K&R, you might want to consider a better source for learning C.

What are instances of bad coding practice in this program? How can it be improved or written in more compact form?

If The C Programming Language by Kernighan & Ritchie is not good way to start learning C programming, I'm open to suggestions on an alternative good read.

//Exercise 5-10. Write the program expr, which evaluates a reverse Polish
//expression from the command line, where each operator or operand is a
//separate argument. For example, expr 2 3 4 + *
//evaluates 2 x C+4).

//For multiplication character '*' is not working

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

#define MAXLINE 1000
#define NUMBER 0

int sign = 1;
char s[MAXLINE];

void calc (int type);

int main(int argc, char *argv[])
{   
    if (argc < 4)
        printf("Usage: ./<programName> op1 op2 operator\n");
    else 
    {
        int i, d;
        int c;

        while (--argc > 0 && (c = **++argv) != '\n')
        {   
            i = 0;
            if (c == '+' || c == '-' || c == '*' || c == '/' || c == '=' || c == '\n')
            {
                if ((c == '+' || c == '-') && isdigit(d = *++(argv[0])))
                {   
                    sign = (c == '-') ? -1 : 1;
                    c = d;
                    goto DOWN1;
                }
                else
                {   
                    calc(c);
                    goto DOWN2; //To avoid re-executing calc(Number) which
                                //is outside any loop in main when operator
                                //is read and operation is performed.
                }   
            }

DOWN1:      while (isdigit(c = *argv[0]))
            {
                s[i++] = c;
                c = *++(argv[0]);

                if (**argv == '.')
                {
                    s[i++] =  **argv;
                    while (isdigit(*++(argv[0])))
                        s[i++] = **argv;
                }
                s[i] = '\0';
            }
            calc(NUMBER);   //Outside while to get single push of s[]
                            //after reading the complete number
DOWN2:      ;
        }   
    }
    return 0;
}

void push (double f);
double pop(void);

void calc (int type)
{
    double op2, res;
    switch(type)
    {
        case NUMBER:
            push(sign*atof(s));
            sign = 1;
            break;
        case '+':
            push(pop() + pop());
            break;
        case '-':
            op2 = pop();
            push(pop() - op2);
            break;
        case '*':
            push(pop() * pop());
            break;
        case '/':
            op2 = pop();
            push(pop() / op2);
            break;
        case '=':
            res = pop();
            push(res);
            printf("\t\t\t||Result = %lg||\n", res);
            break;
        case '\n':
            break;
        default:
            printf("\nError: Invalid Operator!\n");
            break;
    }
}

#define STACKSIZE 1000
double val[STACKSIZE];
int sp = 0;

void push(double f)
{
    if (sp >= STACKSIZE)
        printf("\nError: Stack Overflow!\n");
    else
        val[sp++] = f;
}

double pop(void)
{
    if (sp != 0)
    {
        double ret = val[--sp];
        return ret;
    }
    else
    {
        printf("\nError: Stack Empty!\n");
        return 0.0;
    }
}

Output

\$\endgroup\$
4
  • 2
    \$\begingroup\$ Welcome to Code Review! I polished your question a little bit, but that's not something you should expect for further questions. Taking the tour and reading How to Ask can help you to improve the quality of your posts. \$\endgroup\$ – AlexV Sep 13 '19 at 10:09
  • 1
    \$\begingroup\$ Does your code work if * is properly escaped? If not, you misunderstood. The referral said: "Once you got everything up and running". If you haven't, we can't review code that not yet works as expected. Please take a look at the help center. \$\endgroup\$ – Mast Sep 13 '19 at 10:27
  • \$\begingroup\$ @AlexV Thanks.. I will go through it and improve the quality of my posts in future.. \$\endgroup\$ – CrownedEagle Sep 13 '19 at 10:38
  • \$\begingroup\$ @Mast Yes my code works properly if * is properly escaped.. e.g. ./ProgE5-11 +124 -3 '*' = gives -372 which is a correct answer.. \$\endgroup\$ – CrownedEagle Sep 13 '19 at 10:40
2
\$\begingroup\$
  • Early returns are OK.

    if (args < 4) {
        printf(....);
        return;
    }
    ....
    

    emphasizes where the business logic is.

  • The condition (c = **++argv) != '\n' looks sort of strange. It is indeed possible to embed a newline in an argument, but it doesn't warrant a special case. It is just one way to malform an argument, and there are plenty of them.

  • c = d; does nothing. The very first statement after goto DOWN1 overrides c.

  • Avoid gotos. I don't see the compelling reason to have them here. Just move the code under DOWN1 label to where it belongs, and see the gotos disappearing. Better yet, factor it out into a function.

  • There is no reason to copy the rest of the argument into s. You may directly pass it to atof. I understand the desire to sanitize the argument, but the way you do it is incorrect. It allows multiple dots, and misinterprets some well-formed floats (those with exponents, like 1e2). Let atof do its job correctly. Better yet, use strtod, and check where it stopped parsing.

  • Avoid globals. The bullet above eliminates s. To eliminate global sign, don't cramp everything to calc. Just compute the number, and push it. Let calc only deal with operators.

  • All error messages should go to stderr.

\$\endgroup\$
2
  • \$\begingroup\$ Since I am reading the next character after sign (+ or -) to check whether read +/- is sign or operand (if +/- is succeeded by number then it is interpreted as Sign otherwise as Operand for Addition/Subtraction). Since the 1st digit of number is already read and the program is not using getch() or ungetch() to push the extra character to i/p buffer I saved the next char in 'd' and after evaluating value for' sign', the value in 'd' is restored in 'c' as 'c' used for further calculation. \$\endgroup\$ – CrownedEagle Sep 16 '19 at 15:04
  • \$\begingroup\$ Thanks.. I will the improvements you suggested. First I need to understand some of those. But I will read about it and make the changes. \$\endgroup\$ – CrownedEagle Sep 16 '19 at 15:10

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.