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

Implementing a singly LinkedList and IList interface

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

Problem

I have implemented a LinkedList in C#. What would you change/make better? What naming conventions are misused? What would you add? Some of my ideas are to make it thread-safe.

```
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace TestIt.DataStructures
{
public class MyLinkedList : IList
{
private int size;
private MyLinkedListNode head;

public long Size()
{
return size;
}

public void Add(T nodeValue)
{
Add(new MyLinkedListNode(nodeValue));
}

public void Add(MyLinkedListNode newItem)
{
if (size == 0)
{
head = newItem;
}
else
{
var last = head;
while (last.Next != null) { last = last.Next; }
last.Next = newItem;
newItem.Prev = last;
}
size++;
}

public MyLinkedListNode Get(long index)
{
if (index > size)
{
throw new IndexOutOfRangeException(String.Format("List size is {0} but the index {1}", size, index));
}
long c = 0;
var curr = head;
while (c != index) { curr = curr.Next; c++; }
return curr;
}

public bool Remove(long index)
{
if (index > size)
{
throw new IndexOutOfRangeException(String.Format("List size is {0} but the index {1}", size, index));
}

if (size == 0) return false;

long c = 0;
var curr = head;
while (curr.Next != null && c node)
{
if (size == 0)
{
return false;
}
var curr = head;
while (curr != null && curr != node) { curr = curr.Next; }

Solution

Possible problems

-
MyLinkedListNode Get(long index) passing negative index will lead to a NullReferenceException but should lead to an ArgumentOutOfRangeException.

-
Calling CopyTo(T[] array, int arrayIndex) with array == null will lead to an IndexOutOfRangeException but should lead to an ArgumentNullException.

-
Calling CopyTo(T[] array, int arrayIndex) with arrayIndex >= array.Length will lead to an IndexOutOfRangeException but should lead to an ArgumentOutOfRangeException.

-
The guard condition in the this getter should be changed from

if (size < index - 1)
    {
        throw new IndexOutOfRangeException();
    }


to this

if (size-1 < index)
{
    throw new ArgumentOutOfRangeException();
}


otherwise with size == 1 calling with index == 1 or index == 2 would succeed.

-
You should change every guard condition which states

if (index > size)


to

if (index >= size)


-
Insert(int index, T item)

here you need to change the guard condition also and you should throw an ArgumentOutOfRangeException instead of a System.Exception.

Nitpicking

public int Count
{
    get { return (int)size; }
}


The cast isn't necessary.

Update based on comment

public void CopyTo(T[] array, int arrayIndex)
{
    if (size == 0)
    {
        return;
    }
    var curr = head;
    while (curr != null)
    {
        array[arrayIndex] = curr.Value;
        curr = curr.Next;
        arrayIndex++;
    }
}


In the way you have coded the CopyTo() method, you are using the arrayIndex as the indexer of the passed in T[] array. This is differenet than the Array.CopyTo() implementation of the NET framework.

In the NET implementation arrayIndex would be the starting index of the source array. For the NET CopyTo() the arrayIndex needs to be < sourcearray.Length. If you had the intention that your method behaves like the Array.CopyTo() method you would need to rewrite it like

public void CopyTo(T[] array, int arrayIndex)
{
    if (size == 0 || arrayIndex >= size)
    {
        return;
    }

    var current = Get(arrayIndex);

    int destinationCounter = 0;
    int arrayLength = array.Length;

    while (current!= null && destinationCounter < arrayLength)
    {
        array[destinationCounter] = current.Value;
        current= current.Next;
        destinationCounter++;
    }
}

Code Snippets

if (size < index - 1)
    {
        throw new IndexOutOfRangeException();
    }
if (size-1 < index)
{
    throw new ArgumentOutOfRangeException();
}
if (index > size)
if (index >= size)
public int Count
{
    get { return (int)size; }
}

Context

StackExchange Code Review Q#77499, answer score: 3

Revisions (0)

No revisions yet.