patterncsharpMinor
Implementing a singly LinkedList and IList interface
Viewed 0 times
ilistlinkedlistsinglyinterfaceandimplementing
Problem
I have implemented a
```
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; }
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
-
-
Calling
-
Calling
-
The guard condition in the
to this
otherwise with
-
You should change every guard condition which states
to
-
here you need to change the guard condition also and you should throw an
Nitpicking
The cast isn't necessary.
Update based on comment
In the way you have coded the
In the NET implementation
-
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.