CODEWITHSUNDEEP
c#
Rohit Raghuvansi
Rohit Raghuvansi

Reputation: 2864

How to make these foreach loops efficient?

I have List of (PatchFacilityManager) and a List of (Int) facilityManagerId. I want to make the below code efficient. Is there any way to remove these two foreach loop.

 foreach (PatchFacilityManager PM in patchFacilityManager)
 {
     foreach (int FM in facilityManagerId)
     {
         if (PM.FacilityManagerId == FM)
         {
             PM.IsSelected = true;
         }
     }
 }

Upvotes: 4

Views: 454

Answers (7)

this. __curious_geek
this. __curious_geek

Reputation: 43217

Here's one way,

    foreach (PatchFacilityManager PM in patchFacilityManager)
    {
        PM.IsSelected = facilityManagerId.Contains(PM.FacilityManagerId);
    }

EDIT

This solution is efficient in two three ways IMHO as compared to the code given in the question.

First, it does not test for the condition and the result of the expression is straight away assigned into PM.IsSelected. As per LukeH's comment, it is mandatory to not set the PM.IsSelected to false, so the condition is unavoidable. However this improvement is applicable if the asked needs to set it to false. . From question asker's comment, his case seem to go right with this optimization. So no need for conditional assignment.

Second, it does not iterate through whole list, since List.Contains(int), returns true and come out of loop on the first occurrence of the int passed in argument.

Third, when framework gives you the functionality List.Contains(int), then why re-invent the wheel. So from maintenance perspective this is also more efficient.

Upvotes: 7

Adam Houldsworth
Adam Houldsworth

Reputation: 64557

Just for fun and outside the box - an approach when the lists are sorted:

static void Main(string[] args)
        {
            List<int> nums = new List<int>(new int[] { 1, 2, 3, 4, 5, 6, 7, 8 });
            List<int> ids = new List<int>(new int[] { 2, 4, 5 });

            for (int i = 0, j = 0; i < nums.Count && j < ids.Count; i++)
            {
                int num = nums[i];
                int id = ids[j];

                if (num == id)
                {
                    Console.WriteLine("Match = " + id);
                    j++;
                }
            }

            Console.ReadLine();
        }

I presume no knowledge of the performance benefit or detrement of this idea. Of course you can modify this to use a main foreach for the numbers and you manually use the Enumerator of the IDs inside the main foreach if you have allergies to fors.

Upvotes: 0

Adrian Zanescu
Adrian Zanescu

Reputation: 8008

Another variant using LINQ syntax:

var match = for PM in patchFacilityManager
            join FM in facilityManager on PM.FacilityManagerId equals FM
            select PM;
foreach(var PM in match)
{
   PM.IsSelected = true;
}

Upvotes: 0

Matteo Mosca
Matteo Mosca

Reputation: 7458

patchFacilityManager
.Where(c => facilityManagerId.Contains(c.FacilityManagerId))
.ForEach(c => c.IsSelected = true);

Upvotes: 1

goofballLogic
goofballLogic

Reputation: 40559

patchFacilityManager
   .Where(m => facilityManagerId.Contains(m.FacilityManagerId))
   .ToList()
   .ForEach(m => m.IsSelected = true);

or

patchFacilityManager
   .Join(facilityManagerId, m => m.FacilityManagerId, f => f, (m,f) => m)
   .ToList()
   .ForEach(m => m.IsSelected = true);

Upvotes: 0

LukeH
LukeH

Reputation: 269658

var ids = new HashSet<int>(facilityManagerId);
foreach (PatchFacilityManager pfm in patchFacilityManager)
{
    if (ids.Contains(pfm.FacilityManagerId))
        pfm.IsSelected = true;
}

Upvotes: 1

Stephen Cleary
Stephen Cleary

Reputation: 457472

You could store the facility manager id's in an array in sorted order, and then look them up using BinarySearch instead of a foreach.

Upvotes: 0

Related Questions