Self-made LinkedList and Node

I have recently made two kinds of linked lists in Java. I know that there is one with generic and one without. However, what I am really concerned about is which style should be used mostly (or which style should be used as the main style to implement the LinkedList structure). When talking about these two styles, could you ignore the singly or doubly type, but mainly concentrate on the structure?

The first one is a singly LinkedList, but most of the functions are coded inside the Node class, and the linked list class only contains some easy functions which is used to call the functions inside the Node class.

```
package chapter2linkedlist;
import java.util.*;
public class LinkNode {

private Node root;

class Node {
private String data;
private Node nextNode;

public Node() {
}

public Node(String data) {
this();
this.setData(data);
}

public void setData(String data) {
this.data = data;
}

public void add(Node nextNode) {
if (this.nextNode == null) {
this.nextNode = nextNode;
} else {
this.nextNode.add(nextNode);
}
}

public String getData() {
return this.data;
}

public Node getNext() {
return this.nextNode;
}

public void print() {
System.out.println(this.data);
if (this.nextNode != null) {
this.nextNode.print();
}
}

public boolean search(String data) {
if (data == this.data) {
return true;
} else {
if (this.nextNode == null) {
return false;
} else {
return this.nextNode.search(data);
}
}
}

public boolean delete(Node previousNode, String data) {
boolean isDeleted = false;
if (this.data.equals(data)) {
previousNode.nextNode = this.nextNode;
isDeleted = true;
} else {
if (this.nextNode != null)

Singly link or doubly link?

The choice of implementing a linked list as singly-linked or doubly-linked is not a matter of "style", it's a matter of requirements.
A doubly-linked list is useful when you need to manipulate both ends of the list.

And yes, you should use a generic node so that you can store any kind of object in the list.

Cleaning up LinkNode.Node

The class Node nested in LinkNode should be private static.
Private, to hide this implementation detail from the outside,
and static,
because the class doesn't need to access data in the enclosing class.

Since Node.data never changes, it can be final.
To make that work, you'll need to rework the constructors:

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

public Node(String data) {
    this.data = data;
}

The originals were very badly written:

public Node() {
}

public Node(String data) {
    this();
    this.setData(data);
}

public void setData(String data) {
    this.data = data;
}

This is bad because:

• A method with an empty body is always a bad sign

• Calling this() from the other constructor is pointless

• A constructor should not call methods that can be overridden in children

• setData is not used anywhere else, and certainly it should not be public.

The way I rewrote the constructors above makes this method completely unnecessary.

This method is bad in many ways:

public boolean search(String data) {
    if (data == this.data) {
        return true;
    } else {
        if (this.nextNode == null) {
            return false;
        } else {
            return this.nextNode.search(data);
        }
    }
}

Normally you should not compare objects using ==.
This expression will only be true of the instances on the left and the right side are the same instance. You should use .equals() instead.

The else branch can be simplified to this:

return this.nextNode != null && this.nextNode.search(data);

Also, since the method returns a boolean,
the name "contains" would be more natural instead of "search".
Later I noticed the "isContain" method, which is completely unnecessary,
since it simply delegates to "search".

Avoid random access of elements in a linked list

Do not iterate over a linked list like this:

for(int i = 0; i < testList.size(); i++){
    if(testList.get(i).equals("...")) {
        // ...
    }
}

Random access of list elements like this (with .get(i)) is inefficient with lists.
Use a ListIterator instead, like this:

ListIterator iter = testList.listIterator();
while (iter.hasNext()) {
    String item = iter.next();
    if (item.equals("0123456") || item.equals("012345678")) {
        iter.remove();
    }
}

Possible simplifications

Instead of this:

private Node getRootNode() {
    if (null != this.root) {
        return this.root;
    }
    return null;
}

This is exactly the same but much shorter and clearer:

private Node getRootNode() {
    return this.root;
}

Instead of this:

boolean isDeleted = false;
if (this.data.equals(data)) {
    previousNode.nextNode = this.nextNode;
    isDeleted = true;
} else {
    if (this.nextNode != null) {
        this.nextNode.delete(this, data);
    }
}
return isDeleted;

You can simplify by using an early return:

if (this.data.equals(data)) {
    previousNode.nextNode = this.nextNode;
    return true;
}
if (this.nextNode != null) {
    this.nextNode.delete(this, data);
}
return false;

Naming

The method name printNode is misleading:
it suggests printing a single node,
but it prints the entire linked list.

StringBuilder is recommended instead of StringBuffer

Unless you have a specific requirement to use StringBuffer,
use StringBuilder instead.

StackExchange Code Review Q#78679, answer score: 4