HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Remake of Java's doubly LinkedList

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
remakedoublyjavalinkedlist

Problem

I've been learning to program in Java and I decided to implement a doubly linked list as practice and as a bridge project before going on to learn C++. I know Java includes a LinkedList implementation, but I wrote this for practice. I have finished the code and it works correctly. However, I'd appreciate some feedback on style and things that I should do differently or not at all.

Also, note: I'm not really worried about a unified documentation style; I know it's a bit broken. I only added it out of courtesy to the people who may be reading my code. I'm more interested in feedback on my coding style.

MyLinkedList

```
import java.util.NoSuchElementException;

public class MyLinkedList {

protected MyNode firstNode;
protected MyNode lastNode;
protected int size;

/**
* Constructs a new Linked List
*/
public MyLinkedList() {
firstNode = new MyNode();
lastNode = firstNode;
size = 0;

}

/**
* Adds an element to the linked list
*
* @param element
* to be added to linked list
*/
public void add(T content) {
if (size == 0)
firstNode.setContent(content);
else
lastNode = lastNode.spawnNode(content);
size++;
}

/**
* Adds an element to the beginning of the linked list
*
* @param content
* element to be added to linked list
*/
public void addFirst(T content) {
if (size == 0)
firstNode.setContent(content);
else {
MyNode newFirst = new MyNode(content);
MyNode oldFirst = firstNode;
firstNode = newFirst;
newFirst.setNextNode(oldFirst);
oldFirst.setPreviousNode(newFirst);
}
size++;
}

/**
* Returns an arbitrary item from the linked list
*
* @param index
* Index of item to be retrieved from the linked list
* @return Item residing at index
*/
public T get(int index) {
return getNode(index).getContent();
}

/**
* Returns the first item in the linked list
*
* @return First item in the linked list
*/
public T getFir

Solution

Firstly, MyNode isn't going to be used by any class outside of MyLinkedList. It would be better if you implemented it as a private class inside the same file as MyLinkedList. Its only purpose is as data storage for MyLinkedList, its not really an object in its own right.

As part of that, I'd make all of member variables public and get rid of your getter/setter functions. The point of private access is to prevent outside code from mucking with it. But here the only code using MyNode's is MyLinkedList which is supposed to much with it. All those getters/setters just complicate the situation.

if (size == 0)
    firstNode.setContent(content);
else
    lastNode = lastNode.spawnNode(content);


Most people will say that you should always put { and } even in one line blocks.

public boolean isEmpty() {
    if (size == 0)
        return true;
    return false;
}


Just use

return size == 0;


Its simpler and easier to read.

@SuppressWarnings("unused")
    MyNode temp = firstNode;
    firstNode = firstNode.getNextNode();
    firstNode.setPreviousNode(null);
    temp = null;


What you are you doing with temp? The warning is telling you that the code involving temp does nothing.

In your constructor you create one empty node to start with, in several places in your code you have a special case to handle it by storing the first inserted value into that node. As far as I can tell you've managed to find all the case where its a problem. But there doesn't seem to be a good reason to do it that way.

In general, your code is more complicated then it needs to be. For example you have both insertBefore and insertAfter. But insertAfter could have been implemented as insertBefore(index + 1); You have lots of different insert/remove methods when you really only want to have one. The others might be useful from an interface perspective, but they should all call into a single implementation. (Unless they can implement it way more efficiently)

I suggest a method like the following

private link_together(MyNode left, MyNode right)
{
    if(left != null)
    {
        left.next = right;
    }else{
        firstNode = right;
    }

    if(right != null)
    {
        right.previous = left;
    }else{
        lastNode = left;
    }
}


This will link two nodes together. It will also handle the case where either of the nodes is null which means that the current node is either at the beginning or end of the list.

Now insert becomes:

public void insertBefore(int index, T value)
   {
       Node new_node = new Node(value);
       Node node_after = getNode(index);
       // if we are trying to insert at the end of the list
       // node_after will be null
       Node node_before = node_after : node_after.previous : lastNode;
       link_together(node_before, new_node);
       link_together(new_node, node_after);
       size++;
   }


This function can be used to insert nodes anywhere in the list even at the beginning or end or in empty lists. In a similar way, remove also works.

public void remove(int index)
{
    Node node = getNode(index);
    Node node_before = node.previous;
    Node node_after = node.next;
    link_together(node_before, node_after);
    size--;
}

Code Snippets

if (size == 0)
    firstNode.setContent(content);
else
    lastNode = lastNode.spawnNode(content);
public boolean isEmpty() {
    if (size == 0)
        return true;
    return false;
}
return size == 0;
@SuppressWarnings("unused")
    MyNode<T> temp = firstNode;
    firstNode = firstNode.getNextNode();
    firstNode.setPreviousNode(null);
    temp = null;
private link_together(MyNode<T> left, MyNode<T> right)
{
    if(left != null)
    {
        left.next = right;
    }else{
        firstNode = right;
    }

    if(right != null)
    {
        right.previous = left;
    }else{
        lastNode = left;
    }
}

Context

StackExchange Code Review Q#2588, answer score: 10

Revisions (0)

No revisions yet.