Push and Pop Help

[cjwenigma]

Hello, I'm trying to push and pop a stack with a class and output the results but there seems to be something wrong with my code. Any Ideas? I'm trying to keep it simple. I'm thinking I don't need the showElements but I want some way of display the results being pushed then popped...

#include<iostream>
using namespace std;

class stack{

private:
  int stackArray[10];
  int top;
public:
  void showElements();  
  void push(int x);
  int pop();
  int showTop();
  stack();
  ~stack();
};

stack::stack(){

  top = 0;
  stackArray[10] = 0;
}

stack::~stack(){
}

void stack::push(int x){

  stackArray[top] = x;
  top ++;
}

int stack::pop(){

  int x = stackArray[top];
  return x;

  top--;
  
}

int stack::showTop(){

  return top;

}

void stack::showElements(){

for(int i = 0; i < top; i ++){
    cout<<stackArray[i]<<endl;
 }

}

int main(){

  stack _s;
  int val;
  _s.push(7);
  _s.push(5);
  _s.push(3);
  cout <<"Push: " <<  _s.showTop() << endl;
  _s.showElements();
  val=_s.pop();
  cout <<"Pop: " << val << endl;
   _s.showElements();

  return 0;
}

[Salem]

> stackArray[10] = 0;
This is an out of bound access to the array - who knows what it trashes.
If you want to clear it, use a loop.

> return x;
> top--;
A good compiler would have warned you about unreachable statements.
top-- never happens.

Also, you need to decrement FIRST in pop(), if you increment LAST in push()

[cjwenigma]

hmm.....

[cjwenigma]

i really didn't want a loop in my constructor. I wanted to enable it a different way. I just wanted to use my constructor to basically set them = 0. Is there a different place I could do the loop?

[pheres]

you could build in some rest-like member and call it from the constructor. but whats your reason against looping directly in the constructor?

[matsp]

You could of course avoid a loop by using "unrolling", e.g:

   stackArray[0] = 0; 
   stackArray[1] = 0;
   ...
   stackArray[9] = 0;

It's not too bad wriitng that if you have only 10 items - but of coruse, if you decide to increase your stack to 100 or 1000 elements, then you definitely wouldn't be laughing about the unrolling of the loop.

But why not just use a loop - if the compiler thinks it's better to unroll the loop for best performance, it will do. If not unrolling the loop will give better performance, then it will do that. The compiler may even do a partial unrolling [doing 2 items per loop iteration or some such]. That way, you don't have to worry. And by the way, if you are not creating many stacks very often in your code, what's the reason to NOT use a loop.

There is of course one other option: Don't initialize the stackArray at all - if you use the stack correctly, there shouldn't ever be an access to an entry that hasn't been stored with push.

Finally, I would recommend adding code to check if top is going out of bounds, e.g. top >= 9 before a push, top <= 0 before a pop. Those are invalid situations caused by either pushing too many items or poping more than there was pushed. It's much easier to have a printout saying "attempt to pop too much" than to try to figure out where it got -212189238 from in your calculation.

It's a bad idea to start symbols with _, as many _<something> symbols are "special".

--
Mats

[whiteflags]

You could also take advantage of explicit allocation with new[] and delete[]. new[] will call an int constructor for the array members. If you call the constructor of a built-in type like int it initializes to zero...

Unless you want to call memset or something and pretend that's not a loop. Probably not the best of ideas.

Maybe std::fill instead ...?

[matsp]

You could also take advantage of explicit allocation with new[] and delete[]. new[] will call an int constructor for the array members. If you call the constructor of a built-in type like int it initializes to zero...

Unless you want to call memset or something and pretend that's not a loop. Probably not the best of ideas.

Maybe std::fill instead ...?

Well, I think my suggestion of "not setting it at all" is a fine idea - it never should use a un-set element anyways - if it does, it's because someone has been messing with the top member or something else has gone horribly wrong.

memset(stackArray, 0, sizeof(stackArray)) is of course also fine.

--
Mats

[matsp]

Doh, posted reply to Citizen that then retracted the reply, and I can't delete my post for some reason.

--
Mats

[anon]

You could also take advantage of explicit allocation with new[] and delete[]. new[] will call an int constructor for the array members. If you call the constructor of a built-in type like int it initializes to zero...

A quick check showed that this is not the case (with MingW) and the allocated array contains uninitialized values.

I second the idea that you shouldn't care about unused parts of the stack, and instead you should add some bounds checking so you wouldn't be able to push more numbers than you have room for and report an error if you try to pop or read the top of an empty stack.