Singly linked list backed stack

Question

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<T> : IEnumerable<T>
{
    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<T> 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?

Answer 1

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;
}

Answer 2

I've looked at your code and question (and on the answer by Gentian Kasa), and there has been something troubling which I haven't got my head around, but I do believe the main issues I have with your code is the following:

• Unintuitive to use head and Next – To me it is much more intuitive to use either current or topOfStack instead of head, and similarily to use Prev and not Next. It might be a matter of preference, but your code just looks wrong, and that it is not a good sign
• Replace ++Count with Count += 1 – To me, this reads a lot better with Count += 1 instead of the ++Count which I read as a side effect, and when it is on its own line it looks even worse. This also applies for --Count
• Add underscore to private variables – In most companies I've worked in private variables are named _head, reserving head for locally scoped variables, and Head for public variables/properties

So I would have changed the following (along with correcting names in rest of code):

// In StorageNode class
public StorageNode(T value, StorageNode prev = null)
{
    Value = value;
    Prev = prev;
}

// In Stack class
public Stack() {
    _topOfStack = null;
}

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

---

Added: My little rant on ++Count

Usually one sees this construct in stuff like myArray[++index] or myArray[index++], and I know the syntactic meaning. These reads as Increment index before getting array element at the new index position and Read array element at index, and then increment the index. It has it usages, but can also be rewritten in most cases to something even more readable.

The two usages which I don't get used to seeing are:

++Count;

// Line from answer: http://codereview.stackexchange.com/a/101652/78136
for (int horizontal = 0, vertical = 0; horizontal < board.length; horizontal += 3) {
        if (isWinSet(vertical, vertical + 3, vertical++ + 6)) {

The first I read as: Increment Count before... Ehhh... Nothing.... Awkward, in my opinion. But please note that you are free to your own opinion, and this is not discrediting anyone.

The second is more subtle, but in the loop initialisation both horizontal and vertical is reset. Good. In the loop end statement, horizontal is incremented by 3. Still good. But what about the increment on vertical? It is hidden in the next line, as the third index into the isWinSet( ..., ..., vertical++ + 6)! Not good! This is hard to read: First use vertical + 6 as parameter, and then increment vertical.

The vertical++ + 6 is the side effect at it worst, but after coding for a few years, I tend to avoid using both ++i and i++ to help keep the code easy and understandable. Usually it is much better to use i += 1 on a line of it own.