Avoiding Closures in C# – Best Practices

cdelegatesnet

I have a data structure in the form of a tree. Each node represents a cardboard box. The boxes can contain child boxes.

public class CardboardBox
{
   public int Id {get; set;}
   public int ParentBoxId {get; set;}

   // [...]
}

I'm storing those objects in a flat list rather than a tree structure:

public class CardboardBoxList : List<CardboardBox>
{
}

I want to be able to traverse the tree so I made a 'for each' delgate like this:

public class CardboardBox
{
   // [...]

   public delegate void ForEachDelegate(int childId);

   public void ForEachChildBox(CardboardBoxList fullListOfBoxes, ForEachDelegate forEachDelegate)
   {
      List childIdList = fullListOfBoxes.GetChildIdList(this.Id);

      foreach(int childId in childIdList)
      {
         forEachDelegate(childId);
      }
   }
}

This is all good because now I can easily recurse through the box-tree with:

someBox.ForEachChild(fullListOfBoxes, childId =>
{
   CardboardBox currentBox = fullListOfBoxes.GetById(childId);

   // [... do something with it ...]
};

The problem is, I have code that wants to go through the boxes and build some other data structure. And I need to have some variables outside the delegate call like this:

NewDataStructure newDataStructure = new NewDataStructure();

int currentParentNodeId = newDataStructure.AddRootNode();

someBox.ForEachChild(fullListOfBoxes, childId =>
{
   CardboardBox currentBox = fullListOfBoxes.GetById(childId);

   NewDataStructureNode newNode = new NewDataStructureNode();
   newNode.Color = currentBox.Color;

   newDataStructure.AddNodeToParent(newNode, currentParentNodeId);
};

So it needs to create a closure for currentParentNodeId as it builds the new tree structure. But I want to try to avoid using closures if possible, just to keep the code simple.

I realize closures just come with the territory when using delegates. In a way it is kind of the point of using delegates. You want to have an anonymous method that interacts with the code around it, rather than having to define a whole new method like the old days.

I just wonder if there is a better way here. The CardboardBox tree data structure will be used extensively in this code and will sometimes need closures on all types of different variables, so I can't just add a new parameter to the delegate type. I would need to create a new delegate and a new ForEachChild method for each delegate type.

I could try adding a generic parameter to the delegate and just pass in a kind of context parameter, but it still doesn't feel clean. In the end I'm trying to avoid having any closures if possible but I am not sure if I am trying to hard.

Please note this code is all paraphrased and the (probably lame) example of CardboardBox is really a container-type of object in the real code. The NewDataStructure is also a tree in the real code but it does not necessarily have the same structure as the tree it is built from.

I posted this question on StackOverflow yesterday but the question was deemed too vague, understandably. I have tried to re-write it more clearly but I admit it is still fairly unclear. I have also done a fair bit of searching over the last few days and reading up about delegates, anonymous methods and closures which has been helpful. But I still don't feel comfortable just using closures willy-nilly. So I'd appreciate any advice or just opinions on all this.

Disclaimer: I have used c# for many years so if there are any syntax errors I don't need you to point those out. I am paraphrasing the real code off the top of my head.

Best Answer

Why don't you design the interface of CardBoardBox utilizing an IEnumerable:

public class CardboardBox
{
   // [...]

   public IEnumerable<int> GetChildBoxIDs(CardboardBoxList fullListOfBoxes)
   {
      List childIdList = fullListOfBoxes.GetChildIdList(this.Id);

      foreach(int childId in childIdList)
      {
         yield return childId;
      }
      // remark: this function could be arbitrarily more complex
   }
}

In your example, the calling code then looks like this:

foreach(var childId in someBox.GetChildBoxIDs(fullListOfBoxes))
{
    CardboardBox currentBox = fullListOfBoxes.GetById(childId);

  // [... do something with it ...]
}

So the caller does not need to provide an anonymous function any more.