How to return from Recursive Foreach Function

Question

I am implementing recursion in one of my requirements. My actual requirement is as below:-

There is one master Table called Inventory which has many records like say "Inventory A","Inventory B","Inventory C".

There is one more table called Inventory Bundle which link one Inventory with other. So Inventory Bundle table has two columns :- SI & TI which represent Source Inventory Id and Target Inventory ID.

Record Ex.

SI TI

A B

B C

In my requirement if I click on any inventory then the associated inventory should also be fetched out.

Like here if I click on B then A & C should be fetched out. I use following recursion method to get the requirement:-

List<Guid> vmAllBundle = new List<Guid>();
List<Guid> vmRecursiveBundle = new List<Guid>();
List<Guid> processedList = new List<Guid>();

 public List<Guid> GetAllRecursiveBundle(Guid invId, Guid originalInvId)
        {                       
            List<Guid> vmInvSrcBundleList = GetSourceInventory(invId); //Fetch to get All Related Source Inventories
            List<Guid> vmInvTarBundleList = GetTargetInventory(invId); //Fetch to get All Related Target Inventories

            vmAllBundle.AddRange(vmInvSrcBundleList); 
            vmAllBundle.AddRange(vmInvTarBundleList);           

            if (vmAllBundle.Contains(originalInvId))
                vmAllBundle.Remove(originalInvId);
            vmAllBundle = vmAllBundle.Distinct().ToList();

            vmRecursiveBundle = vmAllBundle.ToList().Except(processedList).ToList();

            foreach (Guid vmInvBundle in vmRecursiveBundle)
            {
                vmRecursiveBundle.Remove(vmInvBundle);
                processedList.Add(vmInvBundle);
                GetAllRecursiveBundle(vmInvBundle, originalInvId);

                if (vmRecursiveBundle.Count == 0)
                    return vmAllBundle;
            }

            return null;
        }

I am able to fetch the data using this method but I am facing problem while returning.

When I am returning it is calling GetAllRecursiveBundle() withing the foreach loop and continue to call until all the items in vmAllBundle gets finished. After this it exits the recursion.

This is something new to me so posting the question to ask if this is normal behavior or some code logic has to be changed.

Modified Code

public List<Guid> GetAllRecursiveBundle(Guid invId, Guid originalInvId)
        {
            if (vmRecursiveBundle.Count > 0)
                vmRecursiveBundle.Remove(invId);

            List<Guid> vmInvSrcBundleList = GetSourceInventory(invId); //Fetch to get All Related Source Inventories
            List<Guid> vmInvTarBundleList = GetTargetInventory(invId); //Fetch to get All Related Target Inventories

            vmAllBundle.AddRange(vmInvSrcBundleList); 
            vmAllBundle.AddRange(vmInvTarBundleList);           

            if (vmAllBundle.Contains(originalInvId))
                vmAllBundle.Remove(originalInvId);
            vmAllBundle = vmAllBundle.Distinct().ToList();

            vmRecursiveBundle = vmAllBundle.ToList().Except(processedList).ToList();

            foreach (Guid vmInvBundle in vmRecursiveBundle)
            {                 
                processedList.Add(vmInvBundle);
                GetAllRecursiveBundle(vmInvBundle, originalInvId);

                if (vmRecursiveBundle.Count == 0)
                    break;
            }

            return vmAllBundle;
        }

Answer 1

I am very surprised that your code runs at all.

You are modifying the list being iterated by a foreach - normally that would throw an exception.

 foreach (Guid vmInvBundle in vmRecursiveBundle)
 {
     vmRecursiveBundle.Remove(vmInvBundle);   // **CRASHES HERE**
 }

Modifying the collection being iterated by the foreach is not allowed, and would be considered bad practice even if it were allowed (because it frequently causes bugs).

You could change to a for loop, which has no such scruples:

for (int i = 0; i < vmRecursiveBundle.Count; i++)
{
     Guid vmInvBundle = vmRecursiveBundle[i];

     vmRecursiveBundle.Remove(vmInvBundle);   // **NO CRASH**

     i--;     // counteracts the i++ so the next Guid is not skipped
}

For further details, see What is the best way to modify a list in a 'foreach' loop?

Answer 2

Normally, recursive method calls need something like a break value, that has to be checked on return, to signal the end of the recursive calls and to stop calling the reursive method. I do not fully understand your code, therefore here is an example:

private string SearchFileRecursive(string directory, string fileToFind)
{
  string filePath = null;

  string[] files = Directory.GetFiles(directory);

  string foundFile = files.FirstOrDefault( file => (0 == string.Compare(Path.GetFileName(file), fileToFind, true)));

  if(string.IsNullOrEmpty(foundFile))
  { // not found
    string[] subDirectories = Directory.GetDirectories(directory);
    foreach(string subDirectory in subDirectories)
    {
      filePath = SearchFileRecursive(subDirectory, fileToFind);
      if(!string.IsNullOrEmpty(filePath)) // found
        break;
    }
  }
  else
  { // found
    filePath = Path.Combine(directory, foundFile);
  }

  return filePath;
}