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

Singly linked list backed stack

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

Problem

I commented on a question and blindly asserted that a Stack based on a linked list was rather elegant (see here)

I haven't ever written a Stack in C# so I thought I should back up my claim:

public class Stack : IEnumerable
{
    protected class StorageNode
    {
        public T Value { get; set; }

        public StorageNode Next { get; set; }

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

    public int Count { get; private set; }

    private StorageNode head;

    public void Push(T value)
    {
        var newNode = new StorageNode(value);
        if (head != null)
        {
            newNode.Next = head;
        }
        head = newNode;
        ++Count;
    }

    public T Pop()
    {
        if (head == null)
        {
            throw new InvalidOperationException("Cannot pop an empty stack");
        }
        var result = head.Value;
        head = head.Next;
        --Count;
        return result;
    }

    public IEnumerator GetEnumerator()
    {
        var current = head;
        while (current != null)
        {
            yield return current.Value;
            current = current.Next;
        }
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }
}


I didn't use the .Net LinkedList because I thought it was a bit overkill for a singly linked list...

Is there anything I can improve in my code?

Solution

Personally I would change

public void Push(T value)
{
    var newNode = new StorageNode(value);
    if (head != null)
    {
        newNode.Next = head;
    }
    head = newNode;
    ++Count;
}


into

public void Push(T value)
{
    var newNode = new StorageNode(value);
    newNode.Next = head;
    head = newNode;
    ++Count;
}


because I find the control not really useful. I mean, if the head is null then nothing happens, newNode.Next remains null.

Better yet (in my opinion), if you add a constructor:

public StorageNode(T value, StorageNode next)
{
    Value = value;
    Next = next;
}


the Push method can be written as:

public void Push(T value)
{
    head = new StorageNode(value, head);
    ++Count;
}

Code Snippets

public void Push(T value)
{
    var newNode = new StorageNode(value);
    if (head != null)
    {
        newNode.Next = head;
    }
    head = newNode;
    ++Count;
}
public void Push(T value)
{
    var newNode = new StorageNode(value);
    newNode.Next = head;
    head = newNode;
    ++Count;
}
public StorageNode(T value, StorageNode next)
{
    Value = value;
    Next = next;
}
public void Push(T value)
{
    head = new StorageNode(value, head);
    ++Count;
}

Context

StackExchange Code Review Q#106125, answer score: 6

Revisions (0)

No revisions yet.