Linked List Delete Method

Question

The following is a class definition for my Linked List. I run a test program that creates a new LinkedList and inserts the numbers "3, 2, 1" and then prints the list. This works fine. However, when I try and delete "3" or "2," the delete method never completes. When I try and delete "1," it just prints out the complete list as if nothing had been deleted.

public class LinkedListTest implements LinkedList {
private Node head;

public LinkedListTest(){
    head = new Node();
}

public void insert(Object x){
    if (lookup(x).equals(false)){

        if (head.data == null)
            head.data = x;

        else{
            //InsertLast
            Node temp = head;

            while (temp.next != null){
                temp = temp.next;
            }

            Node NewNode = new Node();
            NewNode.data = x;
            NewNode.next = null;

            temp.next = NewNode;

        }
    }
    //Runtime of insert method will be n, where n is the number of nodes
}

public void delete(Object x){
    if (lookup(x).equals(true)){
        if (head.data == x)
            head = head.next;

        else{
            Node temp = head;
            while (temp.next != null){
                if ((temp.next).data == x)
                    temp.next = (temp.next).next;
                else
                    temp = temp.next;
            }
        }

    }
}

public Object lookup(Object x){
    Node temp = head;
    Boolean search = false;

    if (head.data == x)
        search = true;

    while (temp.next != null){
        if (temp.data == x){
            search = true;
        }

        else{
            temp = temp.next;
        }
    }

    return search;
}

public boolean isEmpty(){
    if (head.next == null && head.data == null)
        return true;
    else
        return false;
}

public void printList(){
    Node temp = head;
    System.out.print(temp.data + " ");

    while (temp.next != null){
        temp = temp.next;
        System.out.print(temp.data + " ");
    }

}
}

EDIT: Here is the node class:

public class Node {
public Object data;
public Node next;

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

Answer 1

There are a few issues here.

The first big issue is that in your lookup() and your delete() methods, you don't break out of your loops when a successful condition occurs. This is why your program is hanging; it's in an infinite loop.

It's also worth noting a this point is that it's an incredibly bad practice not to use curly braces with all if/else statements. There's no reason not to do so, and it can introduce bugs easily when you don't.

in lookup() you should have:

if (head.data == x) {
    search = true;
} else {
    while (temp.next != null){
        if (temp.data == x){
            search = true;
            break;
        } else {
            temp = temp.next;
        }
    }
}

and in delete():

if (head.data == x) {
    head = head.next;
} else {
    Node temp = head;
    while (temp.next != null) {
        if (temp.next.data.equals(x)) {
            temp.next = temp.next.next;
            break;
        } else {
            temp = temp.next;
        }
    }
}

Now this will produce what you expect:

public static void main( String[] args ) 
{
   LinkedListTest llt = new LinkedListTest();

   llt.insert(1);
   llt.insert(2);
   llt.insert(3);

   llt.printList();
   System.out.println();

   llt.delete(2);
   llt.printList();
}

Output:

1 2 3
1 3

However, that doesn't expose your second, larger issue. You're comparing reference values using == when looking at the node's data.

This "works" at the moment because of a side-effect of auto-boxing small integer values; you're getting the same object references. (String literals would also "work" because of the string pool). For more info on this, look at How do I compare Strings in Java and When comparing two integers in java does auto-unboxing occur

Let's look at this:

public static void main( String[] args )
{
   LinkedListTest llt = new LinkedListTest();

   llt.insert(1000);
   llt.insert(2000);
   llt.insert(2000);
   llt.insert(3000);

   llt.printList();
   System.out.println();

   llt.delete(2000);
   llt.printList();
}

Output:

1000 2000 2000 3000
1000 2000 2000 3000

lookup() stopped working, allowing a duplicate to be inserted. delete() stopped working as well.

This is because int values over 127 auto-box to unique Integer objects rather than cached ones (see the linked SO question above for a full explanation).

Anywhere you're using == to compare the value held by data needs to be changed to use .equals() instead.

if (temp.data.equals(x)) {

With those technical issues solved, your program will work. There's other things that you should consider though. The two that jump right out are:

• lookup should return a boolean.
• There's no need to call lookup() in delete()
• lookup itself is a fairly inefficient approach as a separate method; An insert iterates through the entire list twice.

Answer 2

First of all why does lookup return an object? change it to boolean. also your "while" loop in lookup() doesn't advance. you need to remove the "else". your delete function seems fine though.

Answer 3

I think you got quite some things wrong.

First of all, when you use LinkedList all of the functionality you tried to implement already exists.

Furthermore your style is relatively bad. For example, why do you use the WrapperClass Boolean for lookup?

Combining functionality like contains with something like get in one method is not a good idea. Split this up in two methods, let contains just return a boolean and test if an element exists in the list. Let get search and return for an element.

In addition to this you try to compare the Objects via equal. If you have not overwritten equals you can not delete anything ever, because equals needs reference equality which is not given most times.

I would highly recommend that you buy a java book or something to improve your overall knowledge..