4
\$\begingroup\$

I am trying to implement Stack using a linked list. Looking for code-review, best practices and optimizations.

public class StackUsingLinkedList<T> {

    private Node<T> top;
    int size;

    public StackUsingLinkedList() {
    }

    private static class Node<T> {
        Node<T> next; 
        T item;

        public Node (T item, Node<T> next) {
            this.item = item;
            this.next = next;
        }
    }

    /**
     * Push the item into the stack
     * 
     * @param item  the item to be pushed.
     */
    public void push(T item) {
        final Node<T> node = new Node<T>(item, top);
        top = node;
        size++;
    }

    /**
     * Pops the item from the stack 
     * 
     * @return  the popped item
     * @throws EmptyStackException if attempt is made to access/pop out of an empty stack.
     */
    public T pop() { 
        if (top == null) {
            throw new EmptyStackException();
        }

        T item = top.item;
        top = top.next;
        return item;
    }

    /**
     * Returns the item at top of the stack.
     * 
     * @return  the item at top of stack.
     * @throws EmptyStackException if attempt is made to access empty stack 
     */
    public T peek () {
        if (top == null) {
            throw new EmptyStackException();
        }
        return top.item;
    }


    public int size() {
        return size;
    }


    public boolean isEmpty() {
        return top == null;
    }

    public static void main(String[] args) {
        StackUsingLinkedList<Integer> s = new StackUsingLinkedList<Integer>();
        s.push(10);
        s.push(20);

        // expected 20, 10.
        while (!s.isEmpty()) {
            System.out.println(s.pop());
        }
    }
\$\endgroup\$
2
\$\begingroup\$

This is generally good code, with standards-conforming names and style.

It appears that size is an afterthought variable. It is not scoped as being 'private', and it is not decreased in the pop() method, so it is buggy in two ways.

Additionally, even in the inner private Node class, I would recommend declaring the item and next as private, and also adding getters/setters for them.

Otherwise, this is good code.

\$\endgroup\$
  • \$\begingroup\$ Thanks - just in my opinion adding private modifier and getter/setters for inner classes may be an overkill. \$\endgroup\$ – JavaDeveloper Jun 22 '14 at 21:21
  • \$\begingroup\$ @JavaDeveloper - that's fair. I see it both ways, and it is just a preference. The private is not completely redundant. Oh, the constructor you have is redundant. Still, this code (with the fixed size bug), would pass a review of mine. \$\endgroup\$ – rolfl Jun 22 '14 at 21:24
  • \$\begingroup\$ All fields should be private and/or final, or I would search all the assignments to it to see what's unusual in the way it is used (friend access etc.). (re getters/setters on inner class: Assigning private fields of the inner classes from the enclosing class is also bad style, as it misleads the reader; although it is not the case here.) In this case item, next could be final. \$\endgroup\$ – abuzittin gillifirca Jun 23 '14 at 11:47

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.