Simple static integer stack

Question

This question is about improving my C++ coding skills. I was asked to implement a simple static integer stack in C++ as an assignment. I've come up with the following code:

class myStaticIntStack
{

    int stackSize;
    int storedElements;
    int *elements;

public:

    myStaticIntStack();
    myStaticIntStack( int aNumber );

    ~myStaticIntStack();

    int peek();

    int pop();

    void push( int element );

};

myStaticIntStack::myStaticIntStack()
{
    this->stackSize = 1;
    this->elements = new int(0);
    this->storedElements = 0;
}

myStaticIntStack::myStaticIntStack( int stackSize )
{
    this->stackSize = stackSize;
    this->elements = new int[ stackSize ];
    this->storedElements = 0;
}

myStaticIntStack::~myStaticIntStack()
{
    if( this->elements != NULL )
    {
        if( stackSize > 1 )
            delete[] this->elements;
        else
            delete this->elements;
    }

}

void myStaticIntStack::push( int newElement )
{
    if( this->storedElements == this->stackSize )
        cout << "Stack is full, you must POP an element before PUSHing a new one!" << endl;
    else
    {
        this->elements[ (this->stackSize - 1) - this->storedElements ] = newElement;
        this->storedElements++;
    }
}

int myStaticIntStack::pop()
{
    if( this->storedElements == 0 )
    {
        cout << "Stack is empty, you must PUSH an element before POPping one!" << endl;
        return -1;
    }
    else
    {
        storedElements--;
        return this->elements[ (this->stackSize - 1) - this->storedElements ];
    }
}

int myStaticIntStack::peek()
{
    if( this->storedElements == 0 )
    {
        cout << "Stack is empty, you must PUSH an element before PEEKing one!" << endl;
        return -1;
    }
    else
    {
        return this->elements[ this->stackSize - this->storedElements ];
    }
}

int main()
{
    myStaticIntStack aStack(3);

    cout << "Popped Element: " << aStack.pop() << endl;

    aStack.push(1);
    cout << "Stack Top is: " << aStack.peek() << endl;
    aStack.push(2);
    cout << "Stack Top is: " << aStack.peek() << endl;
    aStack.push(3);
    cout << "Stack Top is: " << aStack.peek() << endl;
    aStack.push(4);
    cout << "Stack Top is: " << aStack.peek() << endl;

    cout << "Popped Element: " << aStack.pop() << endl;
    cout << "Stack Top is: " << aStack.peek() << endl;
    cout << "Popped Element: " << aStack.pop() << endl;
    cout << "Stack Top is: " << aStack.peek() << endl;
    cout << "Popped Element: " << aStack.pop() << endl;
    cout << "Stack Top is: " << aStack.peek() << endl;

    return 0;
}

The code is compiling and running correctly, this is standard output:

Stack is empty, you must PUSH an element before POPping one! 
Popped Element: -1 
Stack Top is: 1 
Stack Top is: 2 
Stack Top is: 3 
Stack is full, you must POP an element before PUSHing a new one! 
Stack Top is: 3 
Popped Element: 3 
Stack Top is: 2 
Popped Element: 2 
Stack Top is: 1 
Popped Element: 1 
Stack is empty, you must PUSH an element before PEEKing one! 
Stack Top is: -1

However I was given a B due to the following reasons:

1. There was a better way of implementing it.
2. I didn't manage well the case in which I'm trying to POP an element from an empty stack, since it's confusing that pop() returns a value even if Stack is empty.

Can you please help me understand how I could improve my code?

Answer 1

First of all, your code has a bug: You did not follow the rule of three. Code such as myStack1 = myStack2; will cause the pointer to be deleted twice - which is undefined behavior.

---

int storedElements;

This seems kind of misleading. This variable doesn't hold the stored elements. It holds the number of stored elements. Perhaps storedElementsCount or something like that would be better?

---

myStaticIntStack( int aNumber );

Why did you call the variable aNumber here? aNumber is a terrible name. It tells me essentially nothing. Later you used stackSize which is a FAR better name. Why didn't you use it here too?

And, by the way, consider using size_t to store sizes and capacities instead of ints. This would apply to stackSize and storedElements.

---

myStaticIntStack::myStaticIntStack()
{
    this->stackSize = 1;
    this->elements = new int(0);
    this->storedElements = 0;
}

Seriously? The default behavior for the stack is to create a stack with maximum size 1? That is kind of... worthless. It's OK to allow this in the myStaticIntStack(int) constructor, but as a default it just seems odd.

Consider not even allowing a default constructor.

---

myStaticIntStack::myStaticIntStack( int stackSize )
{
    this->stackSize = stackSize;
    this->elements = new int[ stackSize ];
    this->storedElements = 0;
}

Can the stackSize be zero? Can it be negative? Consider adding an assertion. By the way, if you make stackSize be a size_t, the negative case becomes impossible, since size_t is unsigned. But you'll still need to handle the "stackSize == 0" case.

A size of 0 might be acceptable. If that is the case, add a comment stating that and why it is so.

Consider declaring this constructor explicit.

---

myStaticIntStack::~myStaticIntStack()
{
    if( this->elements != NULL )
    {
        if( stackSize > 1 )
            delete[] this->elements;
        else
            delete this->elements;
    }

}

Why do you need a NULL check? Are you expecting the destructor to be called with elements == NULL? If this merely defensive programming and this case is never supposed to happen, then leave it as an assertion.

Also, that whole delete vs delete[] is weird. What if I use the constructor myStaticIntStack(1)? Then you'll be deleteing something created with new[].

I'd change the code (from the constructors) so that new[] - not new - is always used and, therefore, delete[] is always the right thing to do in the destructor.

---

void myStaticIntStack::push( int newElement )
{
    if( this->storedElements == this->stackSize )
        cout << "Stack is full, you must POP an element before PUSHing a new one!" << endl;
    else
    {
        this->elements[ (this->stackSize - 1) - this->storedElements ] = newElement;
        this->storedElements++;
    }
}

This is improper error handling that violates the single responsibility principle. Your function should either throw an exception, merely assert the condition or return an error code. Printing to the command line should be done elsewhere - probably outside this class.

---

this->elements[ (this->stackSize - 1) - this->storedElements ] = newElement;

I'm pretty sure you could simplify this. For a stackSize=16, the first position is at index 15. Why 15? Why not 0? That'd simplify the code to just elements[storedElements] = newElement;. Just remember to also fix the pop/peek code.

Just because the stack is Last-in-first-out doesn't mean you have to fill the last indexes in the internal array first. That's an implementation detail.

---

it's confusing that pop() returns a value even if Stack is empty.

Then don't return a value if the Stack is empty. Throw an exception or abort with an assertion. Problem solved. Next. As I stated before, your couts are in the wrong place. If you fix that the "Pop return" problem might just naturally go away.

If you decide to keep it like that (return -1), then consider making -1 a named constant since magic numbers are bad.

return ERROR_STACK_IS_EMPTY;

---

int myStaticIntStack::peek()

Same thing about cout. Also, peek() doesn't change anything, does it? Then consider making it const.

int myStaticIntStack::peek() const

---

Consider splitting your myStaticIntStack in two files: myStaticIntStack.hpp and myStaticIntStack.cpp. Your main function would be in a third file.

---

Some extra thoughts:

• I suspect the "better way" might be to just use a vector internally. This would painlessly solve your "rule of three" problem. Bonus points since it'd make it easier to "resize" the stack after being created if you ever wanted to add that feature.
• Personally, I'd uppercase the first letter of the class name, but that's just personal preference. Nothing wrong with your particular style.
• Consider adding a bool empty() function, that checks if the stack is empty.
• Consider adding a bool full() function, that checks if the stack is full.
• Those two functions above could make your error detection code easier to understand. If your assignment forbids adding extra functions, consider adding them as private functions.
• Consider adding a int size() function, that returns storedElements.
• Consider adding a int capacity() function, that returns stackSize
• Normally, a class like this would be implemented as a template since it's useful to have stacks for more than just integers. Not sure if you've learned about templates yet.

If you follow QuentinUK's suggestion of removing the this->, beware. There is one case that might cause you trouble. In the constructor, this->stackSize = stackSize; is correct, but stackSize = stackSize; would not be. To work around this issue, you could use something like this:

/*optionally add "explicit" here*/ myStaticIntStack::myStaticIntStack( int stackSize ) :
    stackSize(stackSize)
{
    elements = new int[ stackSize ];
    storedElements = 0;
}

This is all I can think of right now. Hope this helps.

Answer 2

this is not always needed in member functions, you can remove all the

this->

The compiler knows this from the context.

And move brackets, e.g.:

int myStaticIntStack::peek() {
    if( 0 == storedElements ){
        cout << "Stack is empty, you must PUSH an element before PEEKing one!" << endl;
        return -1;
    }
    else{
        return elements[stackSize - storedElements ];
    }
}

In comparisons, the const is safer on the left. This avoids a possible =, which is a common error.

You could improve it by making a vector class. You may have to do this from scratch if it is homework. It would be better than old C arrays.

Answer 3

If you have to implement a stack backwards, ok. :) It's a common thought pattern; i used to do it all the time too. But the truth is, it makes a lot of things easier if you consider the stack to grow upward.

void myStaticIntStack::push( int newElement )
{
    if( storedElements == stackSize ) {
        cout << "Stack is full, you must POP an element before PUSHing a new one!" << endl;
    }
    else {
        // note the lack of index math :P
        elements[ storedElements ] = newElement;
        storedElements++;

        // you could even do this all in one line like:
        //  elements[storedElements++] = newElement;
        // but i assume you're just learning c++, so.  :)
    }
}

int myStaticIntStack::pop()
{
    if( storedElements == 0 )
    {
        // For future reference, you should be throwing exceptions here, rather than
        // returning an int.  You're forgiven this time, cause you're new.  :)
        // But what if i wanted to store -1 in this thing?  I can't reliably do that,
        // now that you've used -1 as an error code.

        cout << "Stack is empty, you must PUSH an element before POPping one!" << endl;
        return -1;
    }
    else
    {
        storedElements--;
        // again, note the indexing is much simpler
        return elements[storedElements];

        // You could likewise make this a one-liner...
        //  return elements[--storedElements];
    }
}

Answer 4

Ok, a few comments:

1. If you are going to support a stack with no capacity, I don't know why you need to allocate memory at all. You could simply have the pointer as NULL.

2. Using cout for error handling is not appropriate. You could either give "undefined behaviour" however may I suggest you throw an exception. You can handle your exceptions and use cout in your exception handler.

3. Implement a deep copy constructor and implement swap(). Then implement assignment in terms of both of these. To swap you would do:

   void MyStaticIntStack::swap( MyStaticIntStack & other )
   {
       // swap each member
   }
   

If you don't like using std::swap, write your own. I am not sure you have to avoid all STL, you probably cannot use STL containers for this because the exercise is to write your own container, but utilities like swap may be permitted (plus you can throw std exceptions).

I don't see why -1 shouldn't be a valid number in your stack. If someone does not know whether your stack is empty and tries a peek() from code, they would get -1 (plus some cout that they cannot handle). Perhaps have a method

bool empty() const;

which tells you if the stack is empty or not. You could also have size() and capacity() access methods.

You might want to be able to resize the capacity of your stack. If you find that difficult, have a private constructor that takes a new capacity plus a reference to an existing stack. That constructor can create the data with the relevant size and copy the data into it.

Your internal method would call this constructor to create a new bigger capacity stack, then invoke the swap() method. The temporary one would now disappear. (Do not create it with new).

On a style issue:

1. Initialize your members in the constructor initialization list, not the body of your constructor.
2. Do not use this-> all over the code.