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->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 elements[ (this->stackSize - 1) - this->storedElements ] = newElement;
this->storedElements++;
}
}

int myStaticIntStack::pop()
{
if( this->storedElements == 0 )
{
cout elements[ (this->stackSize - 1) - this->storedElements ];
}
}

int myStaticIntStack::peek()
{
if( this->storedElements == 0 )
{
cout 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: " << aS

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 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 forbid

StackExchange Code Review Q#23597, answer score: 8