Java Stack Implementation

Question

I'm looking for a code review on my Stack Implementation in Java. Specifically I am looking for correctness, style, and efficiency. Thank you!

public class MyStack<T> {
    private int top;
    private T[] stackArray;
    private int size;

    public MyStack(int size) {
        this.size = size;
        this.stackArray = (T[]) new  Object[size];
        this.top = -1;
    }

    public void push(T element) {
        if(isFull()) {
            throw new IndexOutOfBoundsException();
        }
        this.stackArray[++top] = element;
    }

    public T pop() {
        if(isEmpty()) {
            throw new NullPointerException("Stack is empty");
        }
        return this.stackArray[top--];
    }

    public boolean isFull() {
        if(this.top==(size - 1)) {
            return true;
        }
        return false;
    }

    public boolean isEmpty() {
        if(this.top== -1) {
            return true;
        }
        return false;
    }

    public int getTop() {
        return top;
    }

    public T[] getStackArray() {
        return stackArray;
    }

    public void setStackArray(T[] stackArray) {
        this.stackArray = stackArray;
    }

    public int getSize() {
        return size;
    }

    public void setSize(int size) {
        this.size = size;
    }
}

Answer 1

There's a potential bug here:

public T pop() {
    if(isEmpty()) {
        throw new NullPointerException("Stack is empty");
    }
    return this.stackArray[top--];
}

The problem with this code is that a reference to the object stays in the array. You've decremented the index, but the array still has the reference sitting there. This could result in an object not being garbage collected when the user expects it to be. While technically this doesn't affect how your stack works, hanging on to references you don't need is generally considered poor practice.

Consider something like this (not tested):

public T pop() {
    if(isEmpty()) {
        throw new NullPointerException("Stack is empty");
    }
    T retVal = this.stackArray[top];
    this.stackArray[top--] = null;
    return retVal;
}

Answer 2

Lots of good answers already but I just wanted to add to Josh Dawson's point about not exposing private data members.

The only getter that you need to have is getSize()

One thing to watch out for here, when I think "size" such as list.size(), I think current number of elements, not maximum capacity. Maybe a name such as getCapacity or getMaxSize might be more descriptive.

Anybody using the stack shouldn't need to know or care about the top variable, it is just an implementation detail.

However top also acts as a value which represents the number of elements that are in the stack. So I would maybe create a getter not called getTop() but maybe getNumElements() or something along those lines. (or getSize() if you decided to rename your current one)

One pitfall here is that you start top at -1. If you started it at 0 and used top++ instead of ++top it would reliably give back the number of elements without the user needing to know that a value of -1 means "no elements", they would just be left with a 0 instead!

Your current setSize(int size) method can also break your stack. All it's doing is mutating the size variable. It's not actually adjusting the size of the internal array and copying over existing elements (say like an ArrayList does)

If you do want to provide a way of expanding the stack, you could implement something similar.

The setStackArray(T[] stackArray) method is also extremely dangerous. Consider the following code

MyStack<String> myStack = new MyStack<>(10);
String[] updatedArray = new String[5];
myStack.setStackArray(updatedArray); // 'size' is now 10

Code like this could end up corrupting the state of your object. In general I would try to avoid passing references to any objects that are implementation details, and if you have to, clone or copy them as said in the other answers.

If you're looking for ways to add new features, you could consider implementing the Iterable interface, so you could iterate through with a for each loop.

You could also override the equals and toString methods.

Answer 3

One more hint in addition to the other answers:

You can completely remove the size field. It's meant to be the array length, so internally you can refer to stackArray.length instead and avoid the risk of inconsistency between size and array length.

Answer 4

1. Documentation:

Implementing documentation is very good for you, your team mates and developers or API developers using your API (IF) to see what exactly this particular implementation does.

2. Other things:

public MyStack(int size) {
}

Make int size final since you guarantee that value will never be changed.

Rule: final everything which you want to make sure that will not be changed inside the implementation and should not be changed.

        if(this.top== -1) {
            return true;
        }
        return false;

can be changed to:

return (this.top == -1)

Answer 5

Specific exception types

The exceptions you throw are kind of vauge and expose implementation details. Consider making your own exceptions or using prebuilt ones. The EmptyStackException is exactly what you need, and you can easily implement your own FullStackException:

class FullStackException extends Exception {
    public FullStackException() {}
}

Vauge exceptions can lead to collisions in try catch blocks. For example, a NullPointerException could occur from all sorts of situations:

try {
    myStackInstance.pop();
} catch (NullPointerException e) {
    // was `myStackInstance` null or that the stack was empty?
}

A more specific exception allows the user to take advantage of the extra information:

try {
    myStackInstance.pop();
} catch (EmptyStackException e) {
    // no doubts that the stack was empty
} catch (NullPointerException e) {
    // must have been a null pointer
}

Don't expose private data members

A user could mess up your stack's data since you offer them the pointer to the array with getStackArray.

MyStack<Thing> things = new MyStack<Thing>(10);
Things[] array = things.getStackArray();
array[0] = new Thing();
// the stack just got modified externally!!

To avoid this, you can return a clone of the array:

private T[] getStackArray() {
    return stackArray.clone();
}

And do the same when assigning a new stackArray:

private T[] setStackArray(T[] stackArray) {
    if (stackArray.length != size) {
        throw InvalidStackArraySizeException();
    }
    // ??? this.top = ...
    this.stackArray = stackArray.clone();
}

---

Great work overall! Though be sure to stay consistent with the spacing in your if statements.