0
\$\begingroup\$

I have no trouble with my code on working part but I still have some worries if there is something missing so I want a review for my code. Also, I wonder if the exceptions are true type for these methods.

public class ArrayListStack<E> implements Stack<E> {

    private ArrayList<E> list;

    public ArrayListStack()
    {
        list = new ArrayList<>();
    }

    public int size()
    {
        return list.size();
    }

    public boolean isEmpty()
    {
        return size() == 0;
    }

    public void push(E e)
    {
        list.add(size(), e);
    }

    public E top()
    {
        if(isEmpty()) throw new IllegalStateException("Array is empty");
        return list.get(size()-1);
    }

    public E pop()
    {
        if(isEmpty()) throw new IllegalStateException("Array is empty");
        return list.remove(size()-1);
    }


}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Small nitpick: list.add(size(), e); can just be list.add(e). \$\endgroup\$ Commented May 28, 2017 at 14:57

2 Answers 2

2
\$\begingroup\$

Error messages should not leak implementation details unnecessarily

ArrayListStack is an implementation of your custom interface Stack. Presumably users would declare their stacks using the interface type, for example:

Stack<String> strings = new ArrayListStack<>();

The stack of strings may get passed to methods expecting a Stack<String>, and then if top or pop get called when the stack is empty, the exception message will say "Array is empty", which is an implementation detail.

Instead of throwing new IllegalStateException("Array is empty"), you could do what the JDK implementation of Stack does, and throw EmptyStackException. No need for a custom message, it's already encapsulated in that exception.

You could get some other hints from the JDK's implementation, such as the method names and return types.

Use final when possible

The list field can be marked final. It's good to mark fields final when possible, as these fields cannot be reassigned, they are easier to read, understand, and trust.

You could also initialize the field on a single line, and drop the explicit constructor:

private final List<E> list = new ArrayList<>();

Don't repeat yourself

This line appears twice in the code:

if (isEmpty()) throw new IllegalStateException("Array is empty");

You could move this to a helper function, you could call it ensureNonEmptyStack.

Appending to a list

The 2-parameter version of list.add inserts an element at the specified position. In your code, your intention is to append at the end of the list. That's what the 1-parameter version of list.add does, and it would be simpler and more appropriate in this code.

Formatting conventions in Java

The convention is to place opening braces on the same line as the statements.

\$\endgroup\$
0
\$\begingroup\$
  1. I tend to instantiate my collection as soon as possibile. Also, using final you are sure that nobody will set it to null (no need to null-check == code more readable and safer)

    private final ArrayList<E> list = new ArrayList<>();

  2. Consider the use of @Override to mark overriden methods so future you (and other developers) understand the origin of your methods at glance.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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