Reputation: 85

STACK in c program. pop operation not working

Guys what is wrong with this program. I am having problems with pop operation, it shows an extra value even after stack is empty. ??

void initstack (struct stack * p, int maxSize) 

void push (struct stack * p, int item) 

int pop (struct stack * p) 

void display (struct stack p) 

 struct stack

{

   int * a;

   int top;

   int maxSize;

};

Note:using d above structure and functions are mandatory..

#include<stdio.h>
#include<malloc.h>
#include<stdlib.h>
struct stack {
    int * a;
    int top;
    int maxSize;
};
void initstack(struct stack * p, int maxSize);
void push(struct stack * p, int item);
int pop(struct stack * p);
void display(struct stack p);
int StackIsEmpty(struct stack * p);
int StackIsFull(struct stack * p);
void printMenu();

int main()  {
    struct stack p;
    int data,ch, data1, m;
    printf("Enter the maximum size of the stack\n");
    scanf("%d",&m);
    initstack(&p,m);
    do {
    printMenu();    
    printf("Enter your choice\n");
    scanf("%d",&ch);
    switch(ch) {
      case 1:
        printf("Enter the element to be pushed\n");
        scanf("%d",&data);
        push(&p, data);
        break;
      case 2:
        data1 = pop(&p);
        if(data1 != -1000)
        printf("The popped element is %d\n",data1);
        break;
      case 3:
        printf("The contents of the stack are");
        display(p);
        printf("\n");
        break;
      default:
        exit(0);
    }
    } while(1);
    return 0;
}

void printMenu()
{
    printf("Choice 1 : Push\n");
    printf("Choice 2 : Pop\n");
    printf("Choice 3 : Display\n");
    printf("Any other choice : Exit\n");
}

void initstack(struct stack * p, int maxSize) {
    int *newContents;
  newContents=(int *)malloc(sizeof(int)*maxSize);
  p->a=newContents;
  p->maxSize=maxSize;
  p->top=-1; 
}

void push(struct stack * p, int item) {
    if(StackIsFull(p))
    {
      printf("Stack is full\n");
    }
  p->a[++p->top]=item;
}

void display(struct stack p) {
  int i;
  struct stack *b=&p;
  if(StackIsEmpty(b))
    printf(" {}");
  for(i=0;i<b->top;i++)
  {
    printf(" %d",b->a[i]);
  }
}

int pop(struct stack * p) {
    if(StackIsEmpty(p))
    {
      printf("Stack is empty\n");
      return -1000;
    }
  else
    return p->a[--p->top];
}

int StackIsEmpty(struct stack *p)
{
  return p->top == -1;          //p->top==-1;
}

int StackIsFull(struct stack *p)
{
  return p->top >= p->maxSize-1;
}

Upvotes: 0

Answers (5)

Subin S V

Reputation: 85

Thanks guys, I fixed it.. works fine. thanks for all ur suggestions.

#include<stdio.h>
#include<malloc.h>
#include<stdlib.h>
struct stack {
    int * a;
    int top;
    int maxSize;
};
void initstack(struct stack * p, int maxSize);
void push(struct stack * p, int item);
int pop(struct stack * p);
void display(struct stack p);
int StackIsEmpty(struct stack * p);
int StackIsFull(struct stack * p);
void printMenu();

int main()  {
    struct stack p;
    int data,ch, data1, m;
    printf("Enter the maximum size of the stack\n");
    scanf("%d",&m);
    initstack(&p,m);
    do {
    printMenu();    
    printf("Enter your choice\n");
    scanf("%d",&ch);
    switch(ch) {
      case 1:
        printf("Enter the element to be pushed\n");
        scanf("%d",&data);
        push(&p, data);
        break;
      case 2:
        data1 = pop(&p);
        if(data1 != -1000)
        printf("The popped element is %d\n",data1);
        break;
      case 3:
        printf("The contents of the stack are");
        display(p);
        printf("\n");
        break;
      default:
        exit(0);
    }
    } while(1);
    return 0;
}

void printMenu()
{
    printf("Choice 1 : Push\n");
    printf("Choice 2 : Pop\n");
    printf("Choice 3 : Display\n");
    printf("Any other choice : Exit\n");
}

void initstack(struct stack * p, int maxSize) {
    int *newContents;
  newContents=malloc(sizeof(int)*maxSize);
  p->a=newContents;
  p->maxSize=maxSize;
  p->top=-1; 
}

void push(struct stack * p, int item) {
    if(StackIsFull(p))
    {
      printf("Stack is full\n");
    }
    else
    {
    p->a[++p->top]=item;  //FIXED LINE, ELSE BLOCK ADDED
    }
}

void display(struct stack p) {
  int i;
  struct stack *b=&p;
  if(StackIsEmpty(b))
    printf(" {}");
  for(i=0;i<=b->top;i++)   //FIXED PREVIOUSLY for(i=0;i<b->top;i++)
  {
    printf(" %d",b->a[i]);
  }
}

int pop(struct stack * p) {
    if(StackIsEmpty(p))
    {
      printf("Stack is empty\n");
      return -1000;
    }
  else
    return p->a[p->top--];     //FIXED PREVIOUSLY p->a[--p->top];
}

int StackIsEmpty(struct stack *p)
{
  return p->top < 0;          //FIXED PREVIOUSLY p->top==-1;
}

int StackIsFull(struct stack *p)
{
  return p->top >= p->maxSize-1;
}

Upvotes: 1

John Bode

Reputation: 123458

Let's look at your push and pop operations:

p->a[++p->top]=item; // push
p->a[--p->top];      // pop

Let's assume the stack is empty and top is -1. When you do a push, you increment top to 0 and write your element to p->a[0]. When you pop that element, you first decrement top back to -1, and then try to access the element p->a[-1].

This is a problem. Not only are you popping the wrong element, you're accessing an element outside the range of your array and invoking undefined behavior.

You need to change the stack pointer after you access the element you want, like so:

p->a[++p->top] = item; // push
item = p->a[p->top--]; // pop

For array-based stacks, it's actually a little more natural for the stack to grow "downwards", like so:

p->top = p->maxSize = maxSize; // init

if ( p->top )            // p->top != 0 means room left on the stack
  p->a[--p->top] = item; // push

if ( p->top < p->maxSize ) // p->top < p->maxSize means elements left on stack 
  return p->a[p->top++];   // pop

This way, you don't run the risk of accessing an element outside the range of the array. p->top will always be between 0 and maxSize - 1.

Finally, a style note:

You don't need to cast the result of malloc; it just adds visual clutter, and in some cases can suppress a useful diagnostic. You can clean it up by simply writing:

 /**
  * Whitespace is your friend.  Use it.
  */
 newContents = malloc( sizeof *newContents * maxSize );

sizeof *newContents is the same as sizeof (int); this way, if you ever decide to change the type of the stack array, you don't have to worry about changing the malloc call itself. Saves some maintenance headaches, reads a little easier.

Edit

Here's part of what's causing your headaches:

void push(struct stack * p, int item) {
    if(StackIsFull(p))
    {
      printf("Stack is full\n");
    }
    p->a[++p->top]=item; // DANGER WILL ROBINSON!
}

If the stack is full you print a warning, and then you push the element anyway.

You need an else branch in there

void push(struct stack * p, int item) 
{
  if(StackIsFull(p))
  {
    printf("Stack is full\n");
  }
  else
  {
    p->a[++p->top]=item;
  }
}

Upvotes: 1

mcleod_ideafix

Reputation: 11428

It hink the problem is related to push and pop functions. Both use preincrements, so the last item you push is not the first item you pop.

Your push does: first increment top, then store value in a[top]. So top is pointing to the newly pushed element.

Your pop does: first decrement top, then retrieve the value at a[top]. The value retrieved won't be the last pushed, but the previous-to-last. top keeps pointing at this very same value.

My suggestion: leave push as is, and change pop as this:

int pop(struct stack * p) {
    if(StackIsEmpty(p))
    {
      printf("Stack is empty\n");
      return -1000;
    }
  else
    return p->a[p->top--]; /* post-increment!! */
}

So, push will leave top pointing at the last pushed value. pop will retrieve this value, then decrement top, leaving it pointing at the next value to retrieve.

Upvotes: 0