What is the alternate way of using LINQ ForEach loop?

Question

 medicineList.ForEach(x =>
                         {
                             DoctorsOrderViewModel vm = new DoctorsOrderViewModel()
                             {
                                 DrugID = x.PKID,
                                 Name = x.Name,
                                 DrugName = x.Name,
                                 UnitName = x.UnitName,
                                 CategoryID = x.CategoryID,
                                 CategoryName = x.CategoryName,
                                 DosageFormID = x.DosageFormID,
                                 InventoryTypeID = x.InventoryTypeID,
                             };

                             temp.Add(vm);
                             this.DrugItemsComboForSearch.Add(vm);


                             DoctorsOrderViewModel vm2 = new DoctorsOrderViewModel() { CategoryID = x.CategoryID, CategoryName = x.CategoryName, };

                             if (!this.MedicineCategoryItemsCombo.Select(y => y.CategoryID).Contains(x.CategoryID))
                             {
                                 this.MedicineCategoryItemsCombo.Add(vm2);
                             }
                         });

In my Case for 13000 Medicine this code took 8-10 sec to complete but its too lengthy considering performance issue. How can i optimized this?

Linq [does not have a `ForEach` function](https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable?view=netframework-4.7.2). `ForEach` is just [a normal method](https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.list-1.foreach?view=netframework-4.7.2) on the `List` class. — Scott Chamberlain, Sep 05 '18 at 03:46
You can also consider taking advantage of [Parallel ForEach](https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.parallel.foreach?view=netframework-4.7.2) — Dishant, Sep 05 '18 at 04:03
@Dishant He's adding things in the loop - concurrency-safe lists would have to be used. In this case it would be an overkill — tymtam, Sep 05 '18 at 06:36

score 4 · Answer 1 · edited Sep 05 '18 at 03:53

4

What is the alternate way of using LINQ ForEach loop?

A standard foreach.

How can i optimized this

As to performance, its not yourForEach that's the problem, its probably the select and contains ,consider using a ToHashSet once

var set = this.MedicineCategoryItemsCombo.Select(y => y.CategoryID).ToHashSet();

Then you can use in your loop

if (set.Add(x.CategoryID))
{
     this.MedicineCategoryItemsCombo.Add(vm2);
}

However on reading your code, this can probably be optimised with a better query and Where, then do a Select

edited Sep 05 '18 at 03:53

Scott Chamberlain

124,994
33
282
431

answered Sep 05 '18 at 03:43

TheGeneral

79,002
9
103
141

from the author code snipped this.MedicineCategoryItemsCombo.Contains(y => y.CategoryID == x.CategoryID) would be not faster ? – Z.R.T. Sep 05 '18 at 03:49
4

@Z.R.T. no, a HashSet would be faster. The primary use case for HashSets is to do extremely fast contains checks. – Scott Chamberlain Sep 05 '18 at 03:50
Z.R.T what @ScottChamberlain said – TheGeneral Sep 05 '18 at 03:51
3

@TheGeneral I changed your `.Contains` to a `.Add`, it still returns a bool but it will add the new entries to the set as you check them keeping `MedicineCategoryItemsCombo` and `set` in sync. Also, it would have been `if (!set.Contains(x.CategoryID))` for the proper `if` check and you never added new items to the set so you could end up adding duplicates to the combo. Switching to just `Add` solves both of those problems. – Scott Chamberlain Sep 05 '18 at 03:54
@ScottChamberlain if i could up vote you more i would, thanks for the edits! – TheGeneral Sep 05 '18 at 04:04

tymtam · Accepted Answer · 2018-09-05T06:32:58.360

Update: I got some time so I was able to write a complete example:

The results:

10x OPWay for 13000 medicines and 1000 categories: 00:00:03.8986663
10x MyWay for 13000 medicines and 1000 categories: 00:00:00.0879221

Summary

Use AddRange after a transformation with .Select
Use Distinct at the end of the process rather than scanning and adding one by one in each loop.

Solution

    public static List<(string catId, string catName)> MyWay(List<Medicine> medicineList)
    {
        var temp = new List<DoctorsOrderViewModel>();
        var DrugItemsComboForSearch = new List<DoctorsOrderViewModel>();

        var transformed = medicineList.Select(x =>
        {
            return new DoctorsOrderViewModel()
            {
                DrugID = x.PKID,
                Name = x.Name,
                DrugName = x.Name,
                UnitName = x.UnitName,
                CategoryID = x.CategoryID,
                CategoryName = x.CategoryName,
                DosageFormID = x.DosageFormID,
                InventoryTypeID = x.InventoryTypeID,
            };

        }).ToList(); ;

        temp.AddRange(transformed);
        DrugItemsComboForSearch.AddRange(transformed);

        var MedicineCategoryItemsCombo = transformed.Select(m => (catId: m.CategoryID, catName: m.CategoryName)).Distinct().ToList();

        return MedicineCategoryItemsCombo;
    }

Full example:

public static class MainClass
{
    public class Medicine
    {
        public string PKID { get; set; }
        public string Name { get; set; }
        public string UnitName { get; set; }

        public string CategoryID { get; set; }
        public string CategoryName { get; set; }
        public string DosageFormID { get; set; }
        public string InventoryTypeID { get; set; }
    }

    public class DoctorsOrderViewModel
    {
        public string DrugID { get; set; }
        public string Name { get; set; }
        public string DrugName { get; set; }
        public string UnitName { get; set; }

        public string CategoryID { get; set; }
        public string CategoryName { get; set; }
        public string DosageFormID { get; set; }
        public string InventoryTypeID { get; set; }
    }

    class Category
    {
        public string CategoryID { get; set; }
    }

    public static void Main()
    {

        var medicines = new List<Medicine>();

        medicines.AddRange(Enumerable.Range(0, 13000).Select(i => new Medicine()
        {
            PKID = "PKID" + i,
            Name = "Name" + i,
            UnitName = "UnitName" + i,
            CategoryID = "CategoryID" + i%1000,
            CategoryName = "CategoryName for CategoryID" + i%1000,
            DosageFormID = "DosageFormID" + i,
            InventoryTypeID = "InventoryTypeID" + i,
        }));

        Stopwatch sw = new Stopwatch();
        sw.Start();
        List<DoctorsOrderViewModel> comboData = null;
        for (int i = 0; i < 10; i++)
        {
            comboData = OpWay(medicines);
        }
        var elapsed = sw.Elapsed;

        Console.WriteLine($"10x OPWay for {medicines.Count} medicines and {comboData.Count} categories: {elapsed}");


        sw.Restart();
        List<(string catId, string catName)> comboData2 = null;
        for (int i = 0; i < 10; i++)
        {
            comboData2 = MyWay(medicines);
        }
        elapsed = sw.Elapsed;

        Console.WriteLine($"10x MyWay for {medicines.Count} medicines and {comboData2.Count} categories: {elapsed}");

    }

    public static List<DoctorsOrderViewModel> OpWay(List<Medicine> medicineList)
    {
        List<DoctorsOrderViewModel> MedicineCategoryItemsCombo = new List<DoctorsOrderViewModel>();

        var temp = new List<DoctorsOrderViewModel>();
        var DrugItemsComboForSearch = new List<DoctorsOrderViewModel>();

        medicineList.ForEach(x =>
        {
            DoctorsOrderViewModel vm = new DoctorsOrderViewModel()
            {
                DrugID = x.PKID,
                Name = x.Name,
                DrugName = x.Name,
                UnitName = x.UnitName,
                CategoryID = x.CategoryID,
                CategoryName = x.CategoryName,
                DosageFormID = x.DosageFormID,
                InventoryTypeID = x.InventoryTypeID,
            };

            temp.Add(vm);
            DrugItemsComboForSearch.Add(vm);


            DoctorsOrderViewModel vm2 = new DoctorsOrderViewModel() { CategoryID = x.CategoryID, CategoryName = x.CategoryName, };

            if (!MedicineCategoryItemsCombo.Select(y => y.CategoryID).Contains(x.CategoryID))
            {
                MedicineCategoryItemsCombo.Add(vm2);
            }
        });

        return MedicineCategoryItemsCombo;
    }

    public static List<(string catId, string catName)> MyWay(List<Medicine> medicineList)
    {
        var temp = new List<DoctorsOrderViewModel>();
        var DrugItemsComboForSearch = new List<DoctorsOrderViewModel>();

        var transformed = medicineList.Select(x =>
        {
            return new DoctorsOrderViewModel()
            {
                DrugID = x.PKID,
                Name = x.Name,
                DrugName = x.Name,
                UnitName = x.UnitName,
                CategoryID = x.CategoryID,
                CategoryName = x.CategoryName,
                DosageFormID = x.DosageFormID,
                InventoryTypeID = x.InventoryTypeID,
            };

        }).ToList(); ;

        temp.AddRange(transformed);
        DrugItemsComboForSearch.AddRange(transformed);

        var MedicineCategoryItemsCombo = transformed.Select(m => (catId: m.CategoryID, catName: m.CategoryName)).Distinct().ToList();

        return MedicineCategoryItemsCombo;
    }
}

I think this is better, and could be expanded for a complete solution — TheGeneral, Sep 05 '18 at 03:50

score -1 · Answer 3 · edited Sep 19 '18 at 11:07

You can use different approach for foreach, which is better than above, also code could be minimised a bit:

foreach (Medicine medicine in medicineList)
            {
                DoctorsOrderViewModel vm = new DoctorsOrderViewModel()
                {
                    DrugID = x.PKID,
                    Name = x.Name,
                    DrugName = x.Name,
                    UnitName = x.UnitName,
                    CategoryID = x.CategoryID,
                    CategoryName = x.CategoryName,
                    DosageFormID = x.DosageFormID,
                    InventoryTypeID = x.InventoryTypeID,
                };

                temp.Add(vm);
                this.DrugItemsComboForSearch.Add(vm);




                if (!this.MedicineCategoryItemsCombo.Select(y => y.CategoryID == 
                    x.CategoryID).Any())
                {
                    this.MedicineCategoryItemsCombo.Add(new DoctorsOrderViewModel()
                    {
                        CategoryID = x.CategoryID,
                        CategoryName = x.CategoryName,
                    };);
                }
            }

What is the alternate way of using LINQ ForEach loop?

3 Answers3

The results:

Summary

Solution