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

Singly linked list with generics and comparable data - corrected code

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

Problem

I am currently reading about the thread-safe implementation of a linked list. I would like someone to point out some mistakes that will create issues if I try to make it thread-safe in the future.

I've corrected the buggy code from my original post.

Information:

  • Please review the SinglyLinkedList mainly.



  • You may point out any design issue in classes or suggest another function that should be implemented.



  • I have written the code for practice purpose only. I know its not Java-linked-list compatible.



  • I tried to apply all the test cases. Please don't answer if you found another bug; just mention it in a comment. I will introduced it in test cases then get reviewed.



SinglyLinkedList:

```
package com.atleyvirdee.myDataStructures.linkedlist.implemtations;

import com.atleyvirdee.myDataStructures.linkedlist.ILinkedList;
import com.atleyvirdee.myDataStructures.linkedlist.ILinkedListNode;
import com.atleyvirdee.myDataStructures.linkedlist.LinkedListerTraverser;

public class SinglyLinkedList> implements ILinkedList {

ILinkedListNode root = null;
ILinkedListNode last = null;
int size = 0;
LinkedListerTraverser traverser = new LinkedListerTraverser(root, size);

@Override
public ILinkedListNode getRoot() {
return root;
}

@Override
public LinkedListerTraverser getTraverser() {
traverser.setRoot(root, size);
return traverser;
}

@Override
public void prepend( T data ) {
if ( this.root == null ) {
append(data);
return;
}
size++;
ILinkedListNode node = new NodeImpl(data);
node.setNext(this.root);
this.root = node;
}

@Override
public void append( T data ) {
size++;
ILinkedListNode node = new NodeImpl(data);
if ( this.root == null ) {
this.root = node;
this.last = node;
return;
}
this.last.setNext(node);
this.last = node;
}

@Override
public void remove( T data ) {
ILinkedListNode pare

Solution

I'll just focus on one method:

@Override
  public T find( T data ) {
    ILinkedListNode node = root;
    while (node != null) {
      if ( node.getData().compareTo(data) == 0 ) {
        return node.getData();
      }
      node = node.getNext();
    }
    return (node == null) ? null : node.getData();
  }


As pointed out in my answer to another of your questions, requiring the nodes to be Comparable is an unreasonable burden. If all you want to check is equality, then call node.getData().equals(data) — every Java object has a .equals() method.

Naming interfaces with the I… Hungarian prefix is unconventional for Java; it's more appropriate in C#.

For a linked list, the conventional terminology is "head" instead of "root".

The only way that the last line is reachable is if node is null. You don't need to test the while-loop condition again. I suggest writing it as a for loop instead, to make the pattern more easily recognizable:

@Override
public T find(T data) {
    for (LinkedListNode node = head; node != null; node = node.getNext()) {
        if (node.getData().equals(data)) {
            return node.getData();
        }
    }
    return null;
}

Code Snippets

@Override
  public T find( T data ) {
    ILinkedListNode<T> node = root;
    while (node != null) {
      if ( node.getData().compareTo(data) == 0 ) {
        return node.getData();
      }
      node = node.getNext();
    }
    return (node == null) ? null : node.getData();
  }
@Override
public T find(T data) {
    for (LinkedListNode<T> node = head; node != null; node = node.getNext()) {
        if (node.getData().equals(data)) {
            return node.getData();
        }
    }
    return null;
}

Context

StackExchange Code Review Q#96697, answer score: 3

Revisions (0)

No revisions yet.