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

Hi Devin, It is not running for infinite time even initially also. I have modified the code. — simple user, Aug 12 '17 at 03:42
Does obtaining the data from the table have to be done within the recursive method? I think the recursive method would be a lot simpler to implement if it just accepted the selected inventory and the relations between them. — Poosh, Aug 12 '17 at 04:34
Did the first code not have any runtime errors removing items from a list you're iterating over in a foreach? I didn't know that was possible. — Poosh, Aug 12 '17 at 05:23
First code was not creating any problem. But based on suggestion I modified the code. There was never any exceptions. — simple user, Aug 16 '17 at 05:26

score 2 · Answer 1 · 2017-08-16T06:13:48.430

2

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?

edited Aug 16 '17 at 06:13

answered Aug 11 '17 at 06:44

Hi buffjape. Thanks for sharing the best practice. I have taken Remove Portion outside Foreach Loop. Please look at the modified code and let me know if it is okay. If you could answer my query for calling GetAllRecursiveBundle() within foreach loop many times before exiting the function then it will be great. I have seen that GetAllRecursiveBundle() inside foreach loop is getting called based on number of items in vmAllBundle List before final exit. – simple user Aug 12 '17 at 03:49
May i suggest that you run the code and see if it works for you. – Aug 12 '17 at 18:12
It was working for me but please let me know if it is correct from coding perspective. – simple user Aug 16 '17 at 05:27
Well normally you would pass everything a recursive function needs as parameters. Using class members (such as vmRecursiveBundle) to keep track of the function's progress complicates things. – Aug 16 '17 at 06:17

score 1 · Answer 2 · answered Aug 11 '17 at 06:27

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

HI KBO, I have modified the code. Please take a look. The addition of break in the foreach loop is not helping. GetAllRecursiveBundle() inside foreach loop is getting called based on number of items in vmAllBundle List before final exit. Any thoughts on this? — simple user, Aug 12 '17 at 03:44

How to return from Recursive Foreach Function

2 Answers2