Infix to postfix Converter implementation

Question

Please suggest some ways to improve my code for calculating a postfix expression from an infix expression entered by the user. The size of the input (Infix string) should not exceed 10 charaters since the maximum size of the input string mentioned in the code is 10.

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

char Infix[10];
char Symbol[10];
char Postfix[10];

void Push();
void Pop();
int Preced(char, char);

void main()
{
   clrscr();
   printf("Enter a Infix Expression:");
   scanf("%s",&Infix);
   Push();
   printf("\nPostfix of Entered Expression: %s",Postfix);
   getch();
}

void Push()
{
   int i=0, j=0, k=0, s=0;
   while(Infix[i]!='\0')
   {
      if(Infix[i]=='('||Infix[i]==')'||Infix[i]=='*'||Infix[i]=='+'||Infix[i]=='-'||Infix[i]=='/'||Infix[i]=='%'||Infix[i]=='^')
      {
     Symbol[j++]=Infix[i];
      }
      else if(isalpha(Infix[i]))
      {
     Postfix[k++]=Infix[i];

      }
      if(Preced(Symbol[j-2],Symbol[j-1])==1)
      {
     Postfix[k++]=Symbol[j-2];
     Symbol[j-2]=Symbol[j-1];
     Symbol[j-1]=NULL;
     j=j-1;
      }
      if(Symbol[j-1]==')')                                         
      {
    s=j-1;
    while(Symbol[--s]!='(')
    {
      printf("%c ",Symbol[s]);
      Postfix[k++]=Symbol[s];
      Symbol[s]=Symbol[s+1];
      Symbol[s+1]=NULL;
    }
      }
      i++;
   }
   while(--j>=0)
   {
     if(Symbol[j]=='^'||Symbol[j]=='*'||Symbol[j]=='/'||Symbol[j]=='%'||Symbol[j]=='+'||Symbol[j]=='-')
       Postfix[k++]=Symbol[j];
   }
}

int Preced(char c1, char c2)
{
  if(c1=='^'&&c2=='^')                                           //equal priorities.
  {
    return 1;
  }
  else if((c1=='+'||c1=='-')&&(c2=='+'||c2=='-'))                      //equal
  {
    return 1;
  }
  else if((c1=='*'||c1=='/'||c1=='%')&&(c2=='*'||c2=='/'||c2=='%'))     //equal
  {
    return 1;
  }
  else if(c1=='^'&&(c2=='*'||c2=='/'||c2=='+'||c2=='-'||c2=='%'))
  {
    return 1;
  }
  else if((c1=='*'||c1=='/'||c1=='%')&&(c2=='+'||c2=='-'))
  {
    return 1;
  }
  else
  {
    return -1;
  }
}

Answer 1

The intent of your preced function is completely unclear. I can't make heads or tails of what it's supposed to be doing as a whole. It appears to return 1 for two operators of equal precedence, but it also returns 1 for some cases when they're not equal--but it's not at all clear what those cases are supposed to be.

As a general rule, my immediate reaction would be to encode operator precedence into a table, and then have a little bit of logic to deal directly with precedence it found for them. So, I'd probably start with some code just to get the precedence:

int get_precedence(char ch) { 

    char const *operators[] = {
        "^",
        "+-",
        "*/%"
    };
    int i;

    for (i=0; i<3; i++)
        if (strchr(operators[i], ch) != NULL)
            return i;
    return -1;
}

Using this, the "return 1 if they're equal" part could look something like this:

int preced(char ch1, char ch2) { 
    if (get_precedence(ch1) == get_precedence(ch2))
        return 1;
}

I'm not quite sure about the rest of the logic, so I won't try to deal with that right now, but I'd almost be surprised if it didn't come out rather clearer after treatment like this as well.

That brings up an obvious second point: preced isn't a particularly great name for the function. It hints at the fact that it's somehow related to precedence, but that's about all we get from the name. Something more descriptive of the real intent would clearly help a lot here.

Use of NULL

NULL is intended to be a null pointer constant--not anything else. It should never be assigned to anything other than a pointer. Right now, you have places where you use it as a char instead, such as:

 Symbol[j-1]=NULL;

Although it's possible for a compiler to define NULL in a way that allows this, it's also possible to define NULL so you'd have to add a cast for this to work, so this affects both style/readability and portability.

Avoid arbitrary limits

If at all reasonable, and I think it probably is, it's generally best to avoid having entirely arbitrary limits on things like the size of the input. If you are going to have an arbitrary limit, at least try to make it large enough that it's unlikely to ever arise in practice (e.g., set it to 8 kilobytes instead of 10 bytes).

Enforce the limits you do have

Any time you see scanf("%s", somestring); you should cringe in fear, bordering on horror. You've just used scanf to re-create gets, a function so terrible it's been removed from recent versions of the standard.

The problem here is that although your string is (in this case) only 10 bytes long, you haven't informed scanf of that fact, so if the user types in more than 9 characters (remember, the string needs a NUL terminator) you'll get undefined behavior. Any time you use the %s conversion with scanf (or cousins like fscanf) you need to include a maximum size allowed:

char buffer[1025];

scanf("%1024s", buffer);

So here we've specified a maximum length of 1024, with a buffer length of 1025, so we have room for the NUL terminator. Result: the code is nice and safe. No matter how much data the user enters, scanf will never write past the end of our buffer.