Java Singly Linked List

Here is a naive version of a singly linked list in Java. It implements only Collection and Iterable interfaces. Any comments and suggestions are welcome.

SinglyLinkedList.java:

```
package com.ciaoshen.thinkinjava.newchapter17;
import java.util.*;

public class SinglyLinkedList implements Collection, Iterable {
private int size = 0;
private Node head = new Node();
// constructor
public SinglyLinkedList() {
head.setNext(head);
}
public SinglyLinkedList(Collection c) {
this();
addAll(c);
}
// unmodifiable collection (except remove() method)
public Iterator iterator() {
return new Iterator() {
Node cursor = head;
Node previous = head;
public boolean hasNext() {
return cursor.getNext() != head;
}
public E next() {
if (! hasNext()) {
throw new IndexOutOfBoundsException("Iterator has reached the end of the list!");
}
Node toReturn = cursor.getNext();
previous = cursor;
cursor = toReturn;
return toReturn.getInfo();
}
public void remove() { // only remove once the last node return by next() method.
if (cursor == previous) {
throw new IndexOutOfBoundsException("Cannot remove current node. Last node returned by next() already removed, or reach the head of the list!");
}
previous.setNext(cursor.getNext());
cursor.setNext(null);
cursor = previous;
size--;
}
};
}
public int size() {
return size;
}
public String toString() {
if (isEmpty()) {
return "[]";
}
StringBuilder sb = new StringBuilder();
Iterator ite = iterator();
sb.append("[");
while (ite.hasNext()) {
sb.app

Advice 1

I would declare Node as the static private inner class in SinglyLinkedList:

public class SinglyLinkedList ... {
    ...
    private static final class Node {
        ...

Advice 2

In Node you have

private T info = null;

By default, Java sets reference members to null by default, so that you can write simply

private T info;

Advice 3

Name info is not the best possible; consider using data or datum.

Advice 4

public int hashCode() {
    if (info == null) {
        return 0;
    }
    return info.hashCode();
}

You can write the above more succintly:

public int hashCode() {
    return Objects.hashCode(info);
}

Advice 5

private int size = 0;

Integer fields are initialized to zero by default; write only

private int size;

Advice 6

private Node head = new Node();

Use diamond inference:

private Node head = new Node<>();

Advice 7

public E next() {
    if (!hasNext()) {
        throw new IndexOutOfBoundsException("Iterator has reached the end of the list!");
    }
    ...

The correct exception is NoSuchElementException.

Advice 8

public void remove() { // only remove once the last node return by next() method.
    if (cursor == previous) {
        throw new IndexOutOfBoundsException("Cannot remove current node. Last node returned by next() already removed, or reach the head of the list!");
    ...

The correct exception here is IllegalStateException.

Advice 9

Your contains(Object o) throws a NullPointerException whenever o is null, java.util.ArrayList does not.

Advice 10

@SuppressWarnings("rawtypes")
public boolean equals(Object o) {
    if (o == null || !(o instanceof SinglyLinkedList)) {
        return false;
    }
    SinglyLinkedList target = (SinglyLinkedList) o;
    if (size() != target.size()) {
        return false;
    }
    return hashCode() == target.hashCode();
}

What??? If lists are equal in type and size you conclude that they are equal if they have same hash code? Unfortunately, there is a small chance that two different lists have same hash value, so do not rely on it. Instead, you could do:

public boolean equals(Object o) {
    if (o == null || !(o instanceof SinglyLinkedList)) {
        return false;
    }
    SinglyLinkedList target = (SinglyLinkedList) o;
    if (size() != target.size()) {
        return false;
    }

    Iterator iter1 = iterator();
    Iterator iter2 = target.iterator();

    while (iter1.hasNext()) {
        if (!Objects.equals(iter1.next(), iter2.next())) {
            return false;
        }
    }

    return true;
}

Advice 11

boolean remove(Object o) has wrong semantics: it removes all occurrences of o; Java lists remove only the first one.

Hope that helps.

StackExchange Code Review Q#153095, answer score: 5