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

Simple Linked List

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

Problem

I've implemented a simple linked list in C# using generic T for values. I did not inherit from any of .Net's fun and helpful classes like IEnumerable, or IList :D. This is just after an hours worth of work. I've tested everything and I believe it all works well enough, but before I go on to add more methods, or make the actual data-structure more complex I'm looking for some feedback on what I have. Critiques on everything are welcome, perhaps there is some glaring issue that I have not addressed.

As far as my nodes having a previous and next value, that is because I did intend to have quicker searching iterating backwards through, and some other stuff that is not yet implemented. While right now do use both properties of the node, I know I could avoid it. But I have kept it in and use it because I do plan to implement more stuff later.

This is supposed to emulate the List class. I've called mine Jist to not confuse anyone. I also have considered taking away the JNode constructor, because all the properties are public anyway, but I've decided against that, because I don't want any accidentally null values... while default values are fine, I don't want any in there by accident.

```
class JNode
{
public T Value { get; set; }
public JNode Next { get; set; }
public JNode Previous { get; set; }

public JNode(T value) { Value = value; }
}

public class Jist
{
#region Private Members
private JNode _first;
private JNode _last;

#endregion

#region Public Properties
public UInt64 Count { get; private set; }

public T this[UInt64 index]
{
get { return NodeAtIndex(index).Value; }
set { NodeAtIndex(index).Value = value; }
}

#endregion

#region Constructor
public Jist()
{
Count = 0;
_first = new JNode(default(T));
_last = new JNode(default(T));
LinkNodes(_first, _last);
}

#endregion

#region Public Methods
public void Add(T value)

Solution

Overall, this isn't bad. I do see a few things that bug me though.

  • Regions - I find they clutter up the code and don't add much value. I would not use them.



-
Code blocks, regardless how many lines they are should be surrounded by {}. This makes it much easier for your eyes to line code up. There are a few cases where I think this can be ignored (see point 3)

if (node != null)
{
    LinkNodes(node.Previous, node.Next);
}

for (UInt64 i = 0; i < index; i++)
{
    node = node.Next;
}


-
The else statement in the following code is redundant because of the return. This is one of those cases I'm on the fence about having {} around code blocks.

for (UInt64 i = 0; i .Default.Equals(node.Value, value)) return node;

    node = node.Next;
}


-
Add UL to numbers to avoid having to use the type declaration. This will allow you to use var instead.

for (var i = 0UL; i .Default.Equals(node.Value, value)) return node;

    node = node.Next;
}


-
The variable nn is not named very well. Try something like newNode to make the intent more visible.

Code Snippets

if (node != null)
{
    LinkNodes(node.Previous, node.Next);
}

for (UInt64 i = 0; i < index; i++)
{
    node = node.Next;
}
for (UInt64 i = 0; i < Count; i++)
{
    if (EqualityComparer<T>.Default.Equals(node.Value, value)) return node;

    node = node.Next;
}
for (var i = 0UL; i < Count; i++)
{
    if (EqualityComparer<T>.Default.Equals(node.Value, value)) return node;

    node = node.Next;
}

Context

StackExchange Code Review Q#44693, answer score: 10

Revisions (0)

No revisions yet.