Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Here's my implementation of a Stack in Java, all feedback welcome.

import java.util.Iterator;

public class Stack<T> implements Iterable<T> {
    private Node head = null;
    private int size;

    @Override
    public Iterator<T> iterator() {
        return new StackIterator();
    }

    private class Node<T>{

        public Node(T data){
            this.data = data;
            this.next = null;
        }
        T data;
        Node next;
    }

    private class StackIterator implements Iterator<T>{

        private Node current = head;
        @Override
        public boolean hasNext() {
            return current != null;
        }

        @Override
        public T next() {
            T item = (T) current.data;
            current = current.next;
            return item;
        }
    }

    public Stack(){
        size = 0;
        head = null;
    }

    public boolean isEmpty(){
        return head == null;

    }

    public void push(T item){
        Node p = new Node(item);
        if(head == null){
            head = p;
            size++;
            return;
        }
        p.next = head;
        head = p;
        size++;

    }

    public T pop(){
        Node current = head;
        if(current.next == null){
            head = null;
        }else{
            try {

                head = head.next;
                size--;
            }catch (Exception e){
                System.out.println("Popping off an empty stack:" + e);
            }
        }

        return (T)current.data;
    }


    public void trace(){
        Node current = head;
        while(current != null){
            System.out.println(current.data);
            current = current.next;
        }
    }
}
share|improve this question
}catch (Exception e){
    System.out.println("Popping off an empty stack:" + e);
}

Why? You should be able to know if your stack is empty before you even try to pop it. And in that case, any consumer of that Stack class is going to expect an exception to be thrown.

Actually, something's fishy about that catch block.

Stack<int> foo = new Stack<int>();
foo.pop();

What does that do? head is null, you've made sure... twice.

Once here:

private Node head = null;

And another here:

public Stack(){
    size = 0;
    head = null;
}

The constructor seems redundant.

Now, with head being null, foo.pop() will throw a rather surprising NPE, despite that try/catch block seemingly handling the "stack is empty!" case.

You should avoid having that NPE thrown, and instead throw an EmptyStackException, for which your consumer code will be thankful.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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