Singly Linked List Implementation using C# - RemoveLast Method

Question

I've implemented Singly linked list using C# . Can anyone please look into the following code and suggest where I'm wrong?

public int RemoveLast()
{
    if (Head != null)
    {
         var curNode = Head;

         while (curNode.Next != null)
         {
              curNode = curNode.Next;
         }

         var lastNodeValue = curNode.Value;

         curNode = null;
         Size--;
         return lastNodeValue;
     }

     return -1;
}

This function does not remove the last node. I'm unable to figure out what's wrong. When while loop ends, we have the reference of node in curNode whose next is null. It means this is the last node. At the end, I'm setting this node to null. But when I use Display function. It displays the last node as well. This is not deleting the last node.

Here is my display function:

    public string Display()
    {
        if (Head == null)
        {
            return string.Empty;
        }

        var curNode = Head;
        var builder = new StringBuilder();

        while (curNode.Next != null)
        {
            builder.Append($"{curNode.Value} ");
            curNode = curNode.Next;
        }

        builder.Append($"{curNode.Value} ");

        return builder.ToString();
    }

You are just clearing your currNode. It doesn't delete anything from the list. Expected bahavior. You'd have to clear the Next property of your second last Node to delete the last node. — Fildor
– Fildor, Commented Feb 20, 2017 at 10:15
You're not changing a .Next property to null. You're just changing a local reference to null. — Matthew Watson
– Matthew Watson, Commented Feb 20, 2017 at 10:15
My guess is that curNode is a local copy of the List Element. Not the actuall list Element. Having curNode being a var makes it ambigious what type it is. You should propably repalce it with explicit typisation. Taht is exactly the kind of case where you should not use var. — Christopher
– Christopher, Commented Feb 20, 2017 at 10:16
try to change your while to check the curNode.Next.Next == null and set curNode.Next = null — Sebastian L
– Sebastian L, Commented Feb 20, 2017 at 10:17
@SebastianL what if size = 1 ? currNode.Next.Next will throw an NPE! — Fildor
– Fildor, Commented Feb 20, 2017 at 10:19

Matthew Watson · Accepted Answer · 2017-02-20 10:31:51Z

2

You need to go to the last-but-one node, and change its Next to null:

public int RemoveLast()
{
    if (Head != null)
    {
        var curNode = Head;

        while (curNode.Next?.Next != null)
        {
            curNode = curNode.Next;
        }

        var lastNodeValue = curNode.Next?.Value ?? -1;
        curNode.Next = null;
        Size--;
        return lastNodeValue;
    }

    return -1;
}

Note that if you also want Head to be set to null if its the only node, then you can do that like so:

public int RemoveLast()
{
    if (Head != null)
    {
        var curNode = Head;

        while (curNode.Next?.Next != null)
        {
            curNode = curNode.Next;
        }

        int lastNodeValue;

        if (Head.Next == null)
        {
            lastNodeValue = Head.Value;
            Head = null;
        }
        else
        {
            lastNodeValue = curNode.Next?.Value ?? -1;
        }
        curNode.Next = null;
        Size--;
        return lastNodeValue;
    }

    return -1;
}

I have to say though, this Head property looks a bit dubious - it should perhaps belong to a different class.

edited Feb 20, 2017 at 10:31

answered Feb 20, 2017 at 10:19

Matthew Watson

111k12 gold badges179 silver badges301 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Fildor Over a year ago

In case of size=1 , Head Node will not be removed, right?

Matthew Watson Over a year ago

@Fildor Yes, this issue here is this strange Head property. We can't really deal with that because we don't know what it is. So Head won't be altered. But I'll add an example saying what to do if it's needed.

Fildor Over a year ago

After edit: Better but still not removing Head. I suspect it is a referecne in the list to the first Node, but you are right, OP should let us know. I'd add a check after the while: if( curNode == Head) Head = null;

Ivan Gritsenko · Accepted Answer · 2017-02-20 10:35:50Z

[x] -> [x] -> [x] -> null
               ^
               curNode (becomes null)
           ^
           this reference still exists

When doing curNode = null you do not change any reference in the list. curNode variable is changed only, it is pointing to the last element before operation and becomes null afterwards.

Try always keep reference to the node before last:

public int RemoveLast()
{
    if (Head != null)
    {
        var curNode = Head;
        // Corner case when there is only one node in the list
        if (Head.Next == null)
        {
            Head = null;
            Size--;
            return curNode.value;
        }

        var beforeLastNode = curNode;
        curNode = curNode.Next;
        while (curNode.Next != null)
        {
            beforeLastNode = curNode;
            curNode = curNode.Next;
        }

        var lastNodeValue = curNode.Value;

        beforeLastNode.Next = null;
        Size--;
        return lastNodeValue;
    }

    return -1;
}

James · Accepted Answer · 2017-02-20 10:32:03Z

0

You need to make the curNode.Next value null on the previous node. 'curNode' is a local variable, setting it null doesn't do anything except maybe extend its GC life.

public int RemoveLast()
{
    if (Head != null)
    {
         var curNode = Head;
         var previousNode = null;

         while (curNode.Next != null)
         {
              previousNode = curNode;
              curNode = curNode.Next;
         }

         var lastNodeValue = curNode.Value;

         if (previousNode == null)
             Head = null;
         else
             previousNode.Next = null;
         Size--;
         return lastNodeValue;
     }

     return -1;
}

edited Feb 20, 2017 at 10:32

answered Feb 20, 2017 at 10:19

James

10k5 gold badges38 silver badges63 bronze badges

1 Comment

Fildor Over a year ago

Here also: What if size == 1?

Muhammad Qasim · Accepted Answer · 2017-02-20 12:27:06Z

okay guys, after your help, I've rewritten this method that meets all the requirements and set to HeadNode null if there is only one node in linked list. So here we go:

    public int RemoveLast()
    {
        if (HeadNode != null)
        {
            var currNode = HeadNode;
            var prevNode = HeadNode;

            if (HeadNode.Next == null)
            {
                HeadNode = null;
                Size--;
                return currNode.Value;
            }

            while (currNode.Next != null)
            {
                prevNode = currNode;
                currNode = currNode.Next;
            }

            prevNode.Next = null;
            Size--;
            return currNode.Value;
        }

        return -1;
    }

Thank you everyone who contributed in this thread. Happy Coding :)

Collectives™ on Stack Overflow

Singly Linked List Implementation using C# - RemoveLast Method

4 Answers 4

3 Comments

Comments

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

3 Comments

Comments

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related