2

I am trying to learn data structure, but I ran into the dreaded NullPointerException and I am not sure how to fix it.

My SinglyLinkedList<E> class implements an interface, LinkedList, where I redefined some methods like, add(), get(), contains(), and more.

The NullPointerException happens when I use the clear() method. It points at the method removeLast() under nodeBefore.setNext(null). It also points to the clear() method under remove(head.getElement()).

Also, if there is anything I can improve upon in my code please let me know.

public class SinglyLinkedList<E> implements LinkedList<E> {

    private class Node<E> {

        public Node<E> next;
        public E element;

        public Node(E element) {

            this.element = element;
        }

        public Node (E element, Node<E> next) {

            this.element = element;
            this.next = next;
        }

        public E getElement() {

            return element;
        }

        public Node<E> getNext() {

            return next;
        }

        public void setElement(E element) {

            this.element = element;
        }

        public void setNext(Node<E> next) {

            this.next = next;
        }

        public String toString() {

            return ("[" + element + "] ");
        }
    }

    public Node<E> head;
    public Node<E> tail;
    public int total;      

    public SinglyLinkedList() {

        this.head = null;
        this.tail = null; 
        this.total = 0;
    }

    public E get(int index) {

        if (index < 0 || index > size()) {
            return null;
        }

        if (index == 0) {
            return head.getElement();
        }

        Node<E> singly = head.getNext();

        for (int i = 1; i < index; i ++) {

            if (singly.getNext() == null) {
              return null;
            }       

            singly = singly.getNext();      
        }

        System.out.println("\n" + singly.getElement());

        return singly.getElement(); 
    }

    public void add(E element) {
        Node<E> singlyAdd = new Node<E>(element);

        if (tail == null) {
            head = singlyAdd;
            tail = singlyAdd;
        } else {
            tail.setNext(singlyAdd);
            tail = singlyAdd;
        }     

        total++;
    }             

    public void display() {
        if (head == null) {
            System.out.println("empty list");
        } else {
            Node<E> current = head;
            while (current != null) {
                System.out.print(current.toString());
                current = current.getNext();
            }
        }

    }

    public boolean contains(E data) {

        if (head == null) {
            return false;
        }

        if (head.getElement() == data) {
            System.out.println(head);
            return true;                                
        }

        while (head.getNext() != null) {
            head = head.getNext();

            if (head.getElement() == data) {
                System.out.println(head);                
                return true;                               
            }             

        } 

        return false;         
    }       

    private Node<E> removeFirst() {
        if (head == null) {
            System.out.println("We cant delete an empty list");
        }    

        Node<E> singly = head;            
        head = head.getNext();
        singly.setNext(null);
        total--;

        return singly;     
    } 

    private Node<E> removeLast() {

        Node<E> nodeBefore;
        Node<E> nodeToRemove;     

        if (size() == 0) {
            System.out.println("Empty list");
        }    

        nodeBefore = head;

        for (int i = 0; i < size() - 2; i++) {
          nodeBefore = nodeBefore.getNext();
        }    

        nodeToRemove = tail;    

        nodeBefore.setNext(null);
        tail = nodeBefore;
        total--;

        return nodeToRemove;
    }       

    public E remove(int index) {      

        E hold = get(index);     

        if (index < 0 || index >= size()) {
            return null;
        } else if (index == 0) { 

            removeFirst();    
            return hold;
        } else {

            Node<E> current = head;
            for (int i = 1; i < index; i++) {                
                current = current.getNext();
            }  

            current.setNext(current.getNext().getNext());
            total--; 
            return hold;
        }       
    }       

    public int size() {
        return getTotal();
    }

    public boolean remove(E data) {      

        Node<E> nodeBefore, currentNode; 

        if (size() == 0) {
            System.out.println("Empty list");
        }            

        currentNode = head;

        if (currentNode.getElement() == data) {
            removeFirst();
        }

        currentNode = tail;
        if (currentNode.getElement() == data) {
            removeLast();
        }

        if (size() - 2 > 0) {
            nodeBefore = head;
            currentNode = head.getNext();
            for (int i = 0; i < size() - 2; i++) {
                if (currentNode.getElement() == data) {

                    nodeBefore.setNext(currentNode.getNext());
                    total--;
                    break;
                }

                nodeBefore = currentNode;
                currentNode = currentNode.getNext();
            } 
        } 

        return true;
    }

    public void clear() {

        while (head.getNext() != null) {    
            remove(head.getElement());    
        }

        remove(head.getElement());    
    }

    private int getTotal() {
        return total;
    } 
}
code
  • 77
  • 6

3 Answers3

2

For your clear method, I don't see that you do any per element cleanup, and the return type is void, so all you want is an empty list. The easiest way is to simply clear everything, like in the constructor:

public void clear() {
    this.head = null;
    this.tail = null; 
    this.total = 0;
}

Another comment:

in contains, you do

while (head.getNext() != null) {
        head = head.getNext();

        if (head.getElement() == data) {
            System.out.println(head);                
            return true;                               
        }             
    } 

which may have two problems (where the first applies to the entire class),

  1. you compare with == data which compares references, where you probably want to compare values with .equals(data)

Edit: I.e. n.getElement().equals(data) instead of n.getElement() == data.

(Or, if n and data may be null, something like (data != null ? data.equals(n.getElement()) : data == n.getElement())

  1. you use the attribute head as the scan variable which modifies the state of the list. Do you really want that?
drRobertz
  • 3,490
  • 1
  • 12
  • 23
  • Thank you very much! Can you elaborate on what I need to change on the contains method? Are you saying that I need to override the equals method? – code Apr 18 '15 at 07:51
  • 1
    @code check my answer for a link which elaborates the difference between `==` and `equals`. – Turing85 Apr 18 '15 at 07:53
  • @code no, simply use `.equals` instead of `==` if you want value comparison. Answer edited. – drRobertz Apr 18 '15 at 08:01
2

The problem arises when you delete the last element within clear: remove(head.getElement());. For some reason, you first remove the head and then the tail. But when calling removeLast, you use the head (which is already null). Within removeLast this is the line, which causes the NullPointerException: nodeBefore.setNext(null);. My advice would be to write the clear() method as @bali182 has suggested:

public void clear() {
    this.head = null;
    this.tail = head;
    this.total = 0;
}

One advice: if you are writing methods to search or delete entries, you should never use == when dealing with objects (or even better: don't use == at all when dealing with objects). You may want to read this thread.

Community
  • 1
  • 1
Turing85
  • 18,217
  • 7
  • 33
  • 58
0

From within clear method, you are calling remove(head.getElement()); meaning you are trying to call LinkedList's remove method. And since you are overriding each functionality and so is add, you don't maintain internal state of LinkedList and hence you get exception. Code in remove is:

    public boolean remove(Object o) {
    if (o==null) {
        for (Entry<E> e = header.next; e != header; e = e.next) {
            if (e.element==null) {

So here since you are not using functionality of LinkedList, header would be null and doing header.next would return NullPointerException.

SMA
  • 36,381
  • 8
  • 49
  • 73
  • OP is implementing a custum interface `LinkedList`. OP is not extending `LinkedList`. Therefore, OP does not have a method `remove(E)` – Turing85 Apr 18 '15 at 07:28
  • I don't think I understand? I already overridden my interface class method remove() and it works properly. – code Apr 18 '15 at 07:29
  • You implemented remove without any argument, but in reality you are calling it with one element `remove(head.getElement());`. So either you redefine remove method to accept one parameter or you change the call to remove without any argument. – SMA Apr 18 '15 at 07:32
  • @SMA `remove(int)` has an argument, but the wrong type. `removeFirst()` and `removeLast()` have no arguments. There is no method `remove()` – Turing85 Apr 18 '15 at 07:35
  • Your getElement returns a generic type element and not int `public E getElement() { return element; }` – SMA Apr 18 '15 at 07:37
  • You have tick below vote count on left side of my answer. If this has helped, tick it, so others can benefit of it. – SMA Apr 18 '15 at 07:46