7
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ (Don't refer to a member using this. but where needed (as in this.size = size;.) \$\endgroup\$ – greybeard Aug 3 '17 at 0:48
  • \$\begingroup\$ Most Stack based languages wrap the stack, are you planning on implementing this, or no? \$\endgroup\$ – tuskiomi Aug 17 '17 at 16:49
8
\$\begingroup\$

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;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ (Not much of a review, but an essential point.) \$\endgroup\$ – greybeard Aug 3 '17 at 0:23
4
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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)

\$\endgroup\$
  • 1
    \$\begingroup\$ I don't fully agree with your "make everything final" mentality. Not saying you're wrong but I feel java is already too verbose as is. So I wouldn't make method parameters or local method variables final. Especially if you follow good coding practices and have only a screenful of lines in that method (and thus you can clearly see at one glance that they don't get changed anyway). I do agree on making class variables final to show the intent of never changing them once created. \$\endgroup\$ – Imus Aug 3 '17 at 7:22
  • 1
    \$\begingroup\$ @Imus What if by mistake, someone change the value of variable? Should a function not operate on the reference provided unless explicitly documented? \$\endgroup\$ – D D Aug 3 '17 at 7:24
  • 1
    \$\begingroup\$ I have never seen someone change a local variable inside a method by accident. And good coding practice suggest to not change method parameters in the first place (except maybe for handling default cases). That's also why I said you're not wrong for making those final, but I don't feel like it adds enough to enforce the use of final as "best practice" (in the scope of a method). \$\endgroup\$ – Imus Aug 3 '17 at 8:18
  • 1
    \$\begingroup\$ On your unless explicitly documented: Code inside a method should normally be self documenting. If you see a method variable being reassigned, it should be clear why that is the case. \$\endgroup\$ – Imus Aug 3 '17 at 8:20
2
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ (be sure to stay consistent with the spacing in your if statements seems to be referring to if(this.top== -1). My favourite approach is All binary operators except . should be separated from their operands by spaces. from Code Conventions for the Java Programming Language.) \$\endgroup\$ – greybeard Aug 3 '17 at 0:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.