# Evaluate mathematical expression in reverse Polish notation in C

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[]
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;
}
}


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

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

• 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 Sep 16 '19 at 15:04
• 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. – CrownedEagle Sep 16 '19 at 15:10