Simple static integer stack 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-&gt;stackSize = 1;
    this-&gt;elements = new int(0);
    this-&gt;storedElements = 0;
}

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

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

}

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

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

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

int main()
{
    myStaticIntStack aStack(3);

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

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

    cout &lt;&lt; "Popped Element: " &lt;&lt; aStack.pop() &lt;&lt; endl;
    cout &lt;&lt; "Stack Top is: " &lt;&lt; aStack.peek() &lt;&lt; endl;
    cout &lt;&lt; "Popped Element: " &lt;&lt; aStack.pop() &lt;&lt; endl;
    cout &lt;&lt; "Stack Top is: " &lt;&lt; aStack.peek() &lt;&lt; endl;
    cout &lt;&lt; "Popped Element: " &lt;&lt; aStack.pop() &lt;&lt; endl;
    cout &lt;&lt; "Stack Top is: " &lt;&lt; aStack.peek() &lt;&lt; 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:


There was a better way of implementing it. 
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?
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-&gt;stackSize = 1;
    this-&gt;elements = new int(0);
    this-&gt;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-&gt;stackSize = stackSize;
    this-&gt;elements = new int[ stackSize ];
    this-&gt;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-&gt;elements != NULL )
    {
        if( stackSize &gt; 1 )
            delete[] this-&gt;elements;
        else
            delete this-&gt;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-&gt;storedElements == this-&gt;stackSize )
        cout &lt;&lt; "Stack is full, you must POP an element before PUSHing a new one!" &lt;&lt; endl;
    else
    {
        this-&gt;elements[ (this-&gt;stackSize - 1) - this-&gt;storedElements ] = newElement;
        this-&gt;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-&gt;elements[ (this-&gt;stackSize - 1) - this-&gt;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-&gt;, beware. There is one case that might cause you trouble.
In the constructor, this-&gt;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.