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

5 Answers 5

Reset to default
9
\$\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\$
1
  • 2
    \$\begingroup\$ (Not much of a review, but an essential point.) \$\endgroup\$ Commented Aug 3, 2017 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\$
4
  • 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\$ Commented Aug 3, 2017 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\$ Commented Aug 3, 2017 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\$ Commented Aug 3, 2017 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\$ Commented Aug 3, 2017 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\$
1
  • 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\$ Commented Aug 3, 2017 at 0:41

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.