Find more efficient way to loop through object without 2 foreach loops

Question

I have the following code sample. What I am needing to do is find all the Users that have a combo of 'ABC' anywhere in that order of their Events. So Bob and Jim fit that description because Bob's Events are 'VABCR' and Jim's are 'ABC'. I am wanting to make the DoWork method more efficient by not using 2 foreach loops. What is a better, more modified way to get all the users that have the 'ABC' combo.

   static void Main(string[] args)
    {
        var events = new List<Events>()
        {
            new Events { User = "Bob",Event = 'V'},
            new Events { User = "Jim",Event = 'A'},
            new Events { User = "Bob",Event = 'A'},
            new Events { User = "Bob",Event = 'B'},
            new Events { User = "Sally",Event = 'V'},
            new Events { User = "Jim",Event = 'B'},
            new Events { User = "Sally",Event = 'C'},
            new Events { User = "Bob",Event = 'C'},
            new Events { User = "Jim",Event = 'C'},
            new Events { User = "Bob",Event = 'R'}
        };

        var names = DoWork(events);
        foreach (var a in names)
        {
            Console.WriteLine(a);
        }

        Console.ReadKey();

    }

    public static List<string> DoWork(List<Events> events)
    {
        List<string> names = new List<string>();
        var distinct = events.Select(x => x.User).Distinct();           
        foreach (var name in distinct)
        {
            string e = string.Empty;
            foreach (var evnt in events.Where(y => y.User == name))
            {
                e += evnt.Event;
            }
            if (e.Contains("ABC"))
            {
                names.Add(name);
            }
        }
        return names;
    }

    public class Events
    {
       public string User { get; set; }
       public char Event { get; set; }
    }

StriplingWarrior · Accepted Answer · 2022-11-10 04:44:58Z

In addition to the nested foreach loops, you're constructing your e string with a += pattern that has terrible performance: for Bob's events it has to create separate string objects to represent "V", "VA", "VAB", "VABC", etc., which wastes both CPU time and memory.

A little LINQ can help out here.

public static List<string> DoWork(List<Events> events)
{
    return events.GroupBy(e => e.User)
        .Where(g => string.Concat(g.Select(u => u.Event)).Contains("ABC"))
        .Select(g => g.Key)
        .ToList();
}

GroupBy is O(n) where n is the number of items in your list.
string.Concat and string.Contains are O(m) where m is the number of events per group, so the sum of all the groups is O(n).

I should note that for a small number of events your current code will work just fine. The main advantage of using LINQ here is that its declarative style is better at saying what you're trying to end up with, rather than how you want to end up with it.

Collectives™ on Stack Overflow

Find more efficient way to loop through object without 2 foreach loops

1 Answer 1

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

Comments

Your Answer

Sign up or log in

Post as a guest

Related