locked
Removing TreeNode throws null referrence exception RRS feed

  • Question

  • Following code snippet works in 3.5 frameowork

     

            foreach (TreeNode item1 in node.Nodes)

              {

                    item1.Remove();

              }

     

     

    However, if we run the same code under 4.0 framework, it throws a null reference exception.

     

    Can anybody tell me why is so happening?

    Thursday, July 14, 2011 11:16 AM

Answers

  • Hi,

    Removing an item from the source in a FOREACH loop isnot good. For your example you simply can write 'node.Nodes.Clear();' and in general:

    for(int i=0;i<x.Nodes.Count;i++)
    {
     if(condition)
     {
      x.Nodes[i].Remove();
      i--;//i++ will execute after then te next loop will be on index i
     }
    }
    

    Sincerely,

    Yasser.

    • Proposed as answer by Sankarsan Parida Saturday, July 16, 2011 4:42 AM
    • Marked as answer by ssaha Tuesday, July 19, 2011 7:22 AM
    Thursday, July 14, 2011 11:32 AM
  • To add to Yasser:

    Indeed, why not use Nodes.Clear()?

     

    TreeView view = new TreeView();
    view.Nodes.Clear();

     

    And instead of re-adjusting the index pointer, it's easier to iterate backwards:

     

    if (view.Nodes.Count > 0)
    {
        for (int i = view.Nodes.Count - 1; i >= 0; i--)
        {
            view.Nodes.RemoveAt(i);
        }
    }

     

    Or use a Mark-and-sweep technique:

     

    List<TreeNode> toRemove = new List<TreeNode>();
    foreach (TreeNode node in view.Nodes)
    {
        toRemove.Add(node);
    }
    foreach (TreeNode node in toRemove)
    {
        view.Nodes.Remove(node);
    }

     

     


    • Marked as answer by ssaha Tuesday, July 19, 2011 7:22 AM
    Thursday, July 14, 2011 12:54 PM
  • This is a common problem faced by many.

    What you are trying to do and what the computer does is a bit different.

    You are trying to loop through things, so lets say 10 things. The computer internally, generates a loop from 0 to 9.

    Now, when you are deleting one of the items, for you, it wouldnt matter, but for the computer, the list dynamically is shrunk, and its loop from 0 to 9 is all messed up because it is supposed to change to 0 to 8, as well as change its internal linking among the elements etc - which is not done.

    So, its to say, your corrupting up the whole list. I am unsure how it works on 3.5 - I have used 3.5 when I encountered this error. I would request you to recheck on this in 3.5 once more (unless your enumerator method is customized by yourself to handle everything).

    If you are deleting the whole list, then its best to use the Clear() method or ClearAll() method if present. Alternatively, if you are deleting one item based on certain condition, then the usual manner to do is, to store its index or unique identifier into a temporary variable and delete this item from the list after the loop completes itself.

     

    Hope it helps.

    • Marked as answer by ssaha Tuesday, July 19, 2011 7:21 AM
    Friday, July 15, 2011 4:39 PM

All replies

  • Hi,

    Removing an item from the source in a FOREACH loop isnot good. For your example you simply can write 'node.Nodes.Clear();' and in general:

    for(int i=0;i<x.Nodes.Count;i++)
    {
     if(condition)
     {
      x.Nodes[i].Remove();
      i--;//i++ will execute after then te next loop will be on index i
     }
    }
    

    Sincerely,

    Yasser.

    • Proposed as answer by Sankarsan Parida Saturday, July 16, 2011 4:42 AM
    • Marked as answer by ssaha Tuesday, July 19, 2011 7:22 AM
    Thursday, July 14, 2011 11:32 AM
  • To add to Yasser:

    Indeed, why not use Nodes.Clear()?

     

    TreeView view = new TreeView();
    view.Nodes.Clear();

     

    And instead of re-adjusting the index pointer, it's easier to iterate backwards:

     

    if (view.Nodes.Count > 0)
    {
        for (int i = view.Nodes.Count - 1; i >= 0; i--)
        {
            view.Nodes.RemoveAt(i);
        }
    }

     

    Or use a Mark-and-sweep technique:

     

    List<TreeNode> toRemove = new List<TreeNode>();
    foreach (TreeNode node in view.Nodes)
    {
        toRemove.Add(node);
    }
    foreach (TreeNode node in toRemove)
    {
        view.Nodes.Remove(node);
    }

     

     


    • Marked as answer by ssaha Tuesday, July 19, 2011 7:22 AM
    Thursday, July 14, 2011 12:54 PM
  • This is a common problem faced by many.

    What you are trying to do and what the computer does is a bit different.

    You are trying to loop through things, so lets say 10 things. The computer internally, generates a loop from 0 to 9.

    Now, when you are deleting one of the items, for you, it wouldnt matter, but for the computer, the list dynamically is shrunk, and its loop from 0 to 9 is all messed up because it is supposed to change to 0 to 8, as well as change its internal linking among the elements etc - which is not done.

    So, its to say, your corrupting up the whole list. I am unsure how it works on 3.5 - I have used 3.5 when I encountered this error. I would request you to recheck on this in 3.5 once more (unless your enumerator method is customized by yourself to handle everything).

    If you are deleting the whole list, then its best to use the Clear() method or ClearAll() method if present. Alternatively, if you are deleting one item based on certain condition, then the usual manner to do is, to store its index or unique identifier into a temporary variable and delete this item from the list after the loop completes itself.

     

    Hope it helps.

    • Marked as answer by ssaha Tuesday, July 19, 2011 7:21 AM
    Friday, July 15, 2011 4:39 PM
  • Thanks a lot for your replies.

    I know it's not a good practice to remove any item from iterator and in this case Clear() would be the best option. But the sample code I had given earlier was surprisingly working fine. So, I just wanted to know how it was working. However, i have changed the code now and it works fine in all frameworks.

     

    Tuesday, July 19, 2011 7:22 AM
  • For each recycle will not let you change the list.
    One day a small demo!
    Tuesday, July 19, 2011 8:30 AM
  • While I agree that removing something from a list you are looping through is bad the original question pointed out the fact that this works in .net 3.5 and doesn't in .net 4.

    I have MANY loops in my code which are designed correctly, using "mark and sweep" but after an upgrade to .net 4 from .net 3.5 I noticed some old legacy code wasn't designed in the "mark and sweep" fashion and therefore broke!

    I'd be curious to know what exactly has changed to break this in the framework upgrade...

    Tuesday, June 12, 2012 12:15 PM
  • Looks like there was a bug in Framework 3.5. Let's say you have 10 nodes and you remove the node at index 5.

    All the nodes from index 6 to 9 are copied to indices 5 to 8. Then the entry at index 9 is set to null.

    In Framework 3.5, the entry at index 9 was not set to null, which could make a memory leak. Since it wasn't set to null, the foreach loop returned the node still present in the internal array. That's why your loop will see nodes 0, 2, 4, 6, 8, 9, 9, 9, 9, 9

    • Edited by Louis.fr Tuesday, June 12, 2012 2:51 PM
    Tuesday, June 12, 2012 2:50 PM
  • Looks like there was a bug in Framework 3.5. Let's say you have 10 nodes and you remove the node at index 5.

    All the nodes from index 6 to 9 are copied to indices 5 to 8. Then the entry at index 9 is set to null.

    In Framework 3.5, the entry at index 9 was not set to null, which could make a memory leak. Since it wasn't set to null, the foreach loop returned the node still present in the internal array. That's why your loop will see nodes 0, 2, 4, 6, 8, 9, 9, 9, 9, 9


    It was in all the earlier versions it seems. Thanks for the reply.
    Tuesday, June 12, 2012 4:35 PM