Re: Infix to Postfix program not giving correct output
- From: Ben Bacarisse <ben.usenet@xxxxxxxxx>
- Date: Sat, 29 Jan 2011 13:36:11 +0000
Maxx <grungeddd.maxx@xxxxxxxxx> writes:
I'm writing this program which converts a given infix expression to
postfix expression, store the postfix expression in array prints
it ,and then prints the sum. I have used link list to implement the
pushdown stack. The program gets compiled correctly but its not giving
correct output at all. Here is the program i wrote::
There are lots of aspects to programming: you need to code well, know
your algorithms and use the right data structures (these are only the
ones I'll comment on -- there are others).
First, your algorithm is wrong. If you are making this up, well done,
but you have not got it right yet. If you are implementing something
you've read about, you need to do more work on understanding it. What
you have looks like the start of Dijkstra's shunting yard algorithm (I
name it so you can look it up if you want to).
Below, I'll comment on coding and data structures.
#include <stdio.h>
#include <ctype.h>
#define MAX 1000
int rev[MAX];
int *p=rev;
These name are rather small for global variables. p in particular. As
it happens you don't need p to be global -- it should be local to the
function that needs it.
struct node
{
int key;
struct node *next;
};
struct node *head,*z,*t;
Ditto for z and t. 'head' is not a brilliant name either. What is it
the head of?
I'd probably want to use a list for both the value queue (what you call
'rev') and for the operator stack. Limiting one and not the other is
odd. Alternatively, using a fixed size array for both would make the
program very simple, though you should check if you are running out of
space.
void stackinitze()
int pop(void) is the modern way to write this. Ditto for pop() below.
{
head=(struct node *)malloc(sizeof *head);
There is no need for the cast. It just gets in the way of reading the
code. Also (more serious) malloc is not declared. You should to ask for
more warnings from your compiler and you should to follow up on what it
says.
Don't let yourself get into the habit of not checking function return
values.
z=(struct node *)malloc(sizeof *z);
head->next=z;
head->key=0;
z->next=z;
}
I'd expect nothing at all here. I certainly would not expect two
mallocs, one of which is used to make a circular list.
There is an oft-used technique where a list always has a dummy element,
but if you are using this, you have misunderstood it's purpose. With a
global 'head' pointer your list can be much simpler.
void push(int v)
{
t=(struct node *)malloc(sizeof *t);
t->key=v;
t->next=head->next;
head->next=t;
}
't' should be local. You could use 'head' instead of 'head->next'.
int pop()
{
int x;
t=head->next;
head->next=t->next;
x=t->key;
free(t);
return x;
}
Ditto.
int main(int argc, char **argv)
{
I'd expect a function to do the work of converting the expression and
for that function to be called from main. It is a good idea to get into
the habit of doing this early.
int c=*argv[1],i=0,j=1;
What if there is no argc[1]? Also, there is no need for this
initialisation because the code below does it anyway.
for(stackinitze();--argc>0;c=*argv[j++])
This is 'for' loop syntax abuse! Just because you can put any three
expressions into the different parts does not mean it is a good idea.
I'd write:
stackinitze();
for (j = 1; j < argc; j++) {
int c = argv[j];
/* ... */
}
{
if(c>='0' && c<='9')
isdigit is better here.
*p++=(c-'0');
Only single digit numbers?
if(c=='+' || c=='*')
push((char)c);
push takes an int. What this does is convert an int into a char and
then back to an int for the call.
if(c==')')
*p++=pop(c);
}*p='\0';
Odd layout. On that subject, I'd suggest you use a lot more space --
between operators and after keywords for example.
Why '\0'? 'rev' is an int array and '\0' is 0 (but written so it look
like other character literals). You have, though, a data structure
problem here. If zero makes the end of the value queue, how can you
have a zero in the middle?
'p' (if you use it all) should be local to this function. As a general
rule, names should have the smallest possible scope and the larger the
scope the longer the name should be. (I don't mean xxxxxxxxxxxx instead
of x, of course, I mean longer and more helpful: eg: operator_stack,
value_queue, and so on).
while((c=rev[i++])!='\0')
{
if(isdigit(c))
You've turned digits into numbers. I.e. '0' is now 0 in the rev array
and isdigit(0) is 0 (false).
printf("%d",c);
Printing 2 followed by 2 will look like 22. Not a good idea if the
program is ever to handle proper numbers.
else
printf("%c",c);
}
while((c=rev[i++])!='\0')
I think you intended to reset i to 0 before this loop. rev[i] already
beyond the 0 you put in there to mark the end.
{
if(c>='0'&& c<='9')
As before, '2' (for example) will have become 2 and 2 is not between '0'
and '9' (and isdigit is preferable to test the character range).
push(c-'0');
if(c=='+')
push(pop()+pop());
if(c=='*')
push(pop()*pop());
}
printf("Result is %d",pop());
Programs should write whole lines of output. Add a \n at the end.
return 0;
}
<snip>
--
Ben.
.
- References:
- Prev by Date: Re: String: "anti" string oprator
- Next by Date: Re: Infix to Postfix program not giving correct output
- Previous by thread: Infix to Postfix program not giving correct output
- Next by thread: Re: Infix to Postfix program not giving correct output
- Index(es):
Relevant Pages
|
