1 of 4
Corbin
  • 10.6k
  • 2
  • 29
  • 51

Welcome to CodeReview! I'm not much of a Java expert, but hopefully I can take a look.

It looks pretty good over all, but I have a few notes.


For learning purposes, Object is fine, but in a real implementation, you would want to use generics. Really, since the generics around this are pretty simple, it might be a good introduction to coding with generics.


for (int i = 0; i < 0; i++){
    curr = curr.getNext();
}

I'm assuming that's a typo? This could also be a good introduction to unit testing if you haven't done it before.


You seem to do the "get the node at index x" operation in a lot of different places. It should be pulled out into a method.


Node is an implementation detail of the List. It shouldn't be a public class that consumers can see.


Instead of traversing the list each time you append an element, you should store a tail reference so it can be constant time instead of linear.


I would consider omitting slow operations from the list. When people use a certain data structure, they tend to assume all of the methods on it are relatively efficient.


To do a linked list right in a high level language, you really need an iterator. That lets you do all kinds of things that are quite nice like a constant time add(Iterator location, Object data) without exposing the Node (the way to do it without an iterator is to just have a getHead() method and then iterate over the nodes directly :/). It also makes travseral of the list a bit cleaner, and, as a bonus, if the list were to implement Iterable, you can use it in for-each loops.


Class contents are usually indented under the class declaration:

public class SomeClass {
    public SomeClass() {}
}

It of course comes down to personal style, but I've never seen class implementation flush with the declaration.

This is an even smaller thing, but people tend to omit the this when accessing properties unless it's required.


public void replace(Object data, int index)

I would expect a replace method to return the old value that was stored in the list.


You should verify that index values are within the range of the list. An IllegalArgumentException is a bit more meaningful than a NullPointerException. (Although, a case could certainly be made for just letting the NPE happen...)

Note: iterators make it a bit more difficult to provide an invalid index.


You should use StringBuilder in toString. Usually it doesn't matter much, but for a container that might be fairly large, it could be a meaningful performance difference.


curr.setNext(curr.getNext().getNext());

This is a null pointer exception if you're removing the last node.


add can be way simplified so that index == 0 doesn't need a special case:

public void add(Object data, int index) {
    Node currentNode = head;
    for (int i = 0; i < index; ++i) {
        currentNode = currentNode.getNext();
    }
    Node newNode = new Node(data, currentNode.getNext());
    currentNode.setNext(newNode);
}

Or, if you add a getNodeByIndex():

public void add(Object data, int index) {
    Node currentNode = getNodeByIndex(index);
    Node newNode = new Node(data, currentNode.getNext());
    currentNode.setNext(newNode);
}

A few other places can have similar simplifications made -- toString, for example.

Corbin
  • 10.6k
  • 2
  • 29
  • 51