Evaluate mathematical expression in reverse Polish notation in C

Question

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

• AlexV: 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.
• Mast: 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.
• CrownedEagle: @AlexV Thanks.. I will go through it and improve the quality of my posts in future..
• CrownedEagle: @Mast Yes my code works properly if * is properly escaped.. e.g. ./ProgE5-11 +124 -3 '*' = gives -372 which is a correct answer..

Answer 1

• 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.

• CrownedEagle: 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.
• CrownedEagle: 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.