Is there a better way to do this, maybe replace the for/foreach loop with something LINQ ish?

Question

I have a list of existing Holdings and I want to combine in another list of Holdings. I know using foreach & for loops is a bad way to go, but I can't think of a good way using LINQ to trim this down.

private void CombineHoldings(List<Holding> holdingsToAdd, ref List<Holding> existingHoldings)
{
    foreach (Holding holdingToAdd in holdingsToAdd)
    {
        Boolean found = false;
        for (int i = 0; i < existingHoldings.Count; i++)
        {
            if (existingHoldings[i].Sector == holdingToAdd.Sector)
            {
                found = true;
                existingHoldings[i].Percentage += holdingToAdd.Percentage;
            }
        }
        if (!found)
            existingHoldings.Add(holdingToAdd);
    }
    foreach (Holding holding in existingHoldings)
        holding.Fund = "Combined Funds";
}

If using foreach and for loops is a bad way to go, then steer clear of Linq since Linq uses foreach and for loops under the covers. Btw - you should add a `break` in the inner if loop when `found = true`, otherwise, you'll continue iterating the collection when you've already found your item. — Metro Smurf, Apr 27 '12 at 23:52
possible duplicate of [Merging two IEnumerables](http://stackoverflow.com/questions/590991/merging-two-ienumerablets) — Jason, Apr 27 '12 at 23:57
@Metro Smurf, you're being pedantic. There is nothing here to indicate that the OP is concerned about *performance*. Thus, we're left to conclude that the OP thinks that declaring a `found` variable is highly *unsatisfying*. I'd like to think that's something we can all sympathize with, and in fact LINQ is an *excellent* vehicle for arriving at elegant code that solves this sort of problem to a T. — Kirk Woll, Apr 28 '12 at 00:15
This is my natural, muscle-memory, way of thinking through the solution, I know that there's a better way...just looking for one. I did add the Break as MetroSmurf suggested. Still reading through all of the other answers though. Chris - no, I don't believe one holding can have multiple sectors, but I'm thinking about Mutual Funds and the holdings they have, say V for Visa and it being in the Financial sector. Robaticus - I should be returning the list, but now I remember they should be IEnumerable not lists... — Mark, Apr 28 '12 at 02:17

score 1 · Answer 1 · answered Apr 28 '12 at 01:51

Having the function mutate the original list makes it very non-Linq, so here is a version that treats the two lists as immutable:

private List<Holding> CombineHoldings(
    List<Holding> holdingsToAdd, 
    List<Holding> existingHoldings) 
{
    var holdings = existingHoldings.Concat(holdingsToAdd)
        .GroupBy(h => h.Sector)
        .Select(g => 
        { 
            var result = g.First(); 
            result.Percentage = g.Select(h => h.Percentage).Sum();
            return result; 
        });
    return holdings.ToList();
}

Definintely wouldn't win a performance competition, but I like it for its simplicity. The following would probably be faster, but is more complex and requires you to either override equality on Holdings to compare Sectors or create an IEqualityComparer<Holding>:

private List<Holding> CombineHoldings(
    List<Holding> holdingsToAdd, 
    List<Holding> existingHoldings) 
{
    var holdings = existingHoldings.GroupJoin(holdingsToAdd, h => h, h => h, 
        (h, toAdd) =>
        new Holding(
            h.Sector, 
            /*Other parameters to clone*/, 
            h.Percentage + toAdd.Select(i => i.Percentage).Sum())
        ).ToList();
    holdings.AddRange(holdingsToAdd.Except(holdings));
    return holdings;
};

score 0 · Answer 2 · answered Apr 28 '12 at 00:36

If your calling this method requently on the list then I would suggest putting it into an extension method for the list type, i.e

private static void CombineHoldings(this List<Holding> holdingsToAdd, ref List<Holding> existingHoldings)
{
    foreach (Holding holdingToAdd in holdingsToAdd)
    {
        Boolean found = false;
        for (int i = 0; i < existingHoldings.Count; i++)
        {
            if (existingHoldings[i].Sector == holdingToAdd.Sector)
            {
                found = true;
                existingHoldings[i].Percentage += holdingToAdd.Percentage;
            }
        }
        if (!found)
            existingHoldings.Add(holdingToAdd);
    }
    foreach (Holding holding in existingHoldings)
        holding.Fund = "Combined Funds";
}

this will allow you to, anywhere you have created a list to go

List<Holding> temp1 = new List<Holding>();
List<Holding> temp2 = new List<Holding>();
//add here to temp1 and temp2
//then...
temp1.CombineHoldings(temp2);

making the first method static and putting a 'this' keyword in front of the first parameter means it will extend that type

Looking at the parameters though it would probably make more sense to switch the two, so that it's adding to the list calling the method like so -

private static void CombineHoldings(this List<Holding> existingHoldings, List<Holding> holdingsToAdd)

score 0 · Answer 3 · answered Apr 28 '12 at 01:10

I would probably go with something like this:

private void CombineHoldings(List<Holding> holdingsToAdd, ref List<Holding> existingHoldings)
{
    // group the new holdings by sector
    var groupedHoldings = holdingsToAdd.GroupBy(h => h.Sector);

    // now iterate over the groupings
    foreach(var group in groupedHoldings) {
         // calculate the sum of the percentages in the group
         // we'll need this later
         var sum = group.Sum(h => h.Percentage);

         // get the index of a matching object in existing holdings
         var existingHoldingIndex = existingHoldings.FindIndex(h => h.Sector == group.Key);

         // yay! found one. add the sum of the group and our job's done.
         if(existingHoldingIndex >= 0) {
             existingHoldings[existingHoldingIndex].Percentage += sum;
             continue;
         }

         // didn't find one, so take the first holding in the group, set its percentage to the sum
         // and append that to the existing holdings table
         var newHolding = group[0];
         newHolding.Percentage = sum;

         existingHoldings.Add(newHolding);
    }
}

Performance-wise, I'm not sure how this holds up. But it seems somewhat more elegant.

Trisped · Answer 4 · 2012-04-28T01:57:02.680

You questions is a bit ambiguous, do you want to get rid of the foreach loop because for loops are faster, because you feel you have one too many loops, or because you want better performance?

Assuming this is a question about enhancing performance, I suggest changing existingHoldings from List to a SortedList where T is the type of Holding.Sector. For best performance Sector should be an integer variable type like an int.

private void CombineHoldings(List<Holding> holdingsToAdd, SortedList<int,Holding> existingHoldings) //Remove ref since List and SortedList are reference types and we are not changing the pointer.
{

    for (int i = 0; i < holdingsToAdd.Count; i++)
    {
        if (existingHoldings.ContainsKey(holdingsToAdd[i].Sector))
        {
            existingHoldings[holdingsToAdd[i].Sector].Percentage += holdingsToAdd[i].Percentage;
        }
        else
        {
            existingHoldings.Add(holdingsToAdd[i].Sector, holdingsToAdd[i]);
        }
    }
    for (int i = 0; i < existingHoldings.Count; i++)
    {
        existingHoldings.Values[i].Fund = "Combined Funds";
    }
}

This method would result in an O(m*log n + n) where n is the number of elements in existingHoldings and m is the number of elements in holdingsToAdd. It is unfortunate that all the existingHoldings elements must have their Fund value updated, as it adds an extra pass through that collection.

Note: if you are constantly adding/removing items from existingHoldings then you could use SortedDictionary which should be faster (SortedList is faster for accessing elements but takes longer adding/removing)

Edit: It is important to note that LINQ is for searching collections, not updating them. As such you could use LINQ to find the holdingsToAdd which do and do not exist in existingHoldings and then loop through existingHoldings setting the Fund and if needed setting Percentage, but then both holdingsToAdd and existingHoldings would need to be ordered and you would still be looping through each collection once. It would be something on the order of O(2*m*log n + n). The compiler might be able to combine the two queries into one call, but even then you would be looking at similar performance with less readability.

score 0 · Answer 5 · edited May 23 '17 at 12:11

0

Perhaps this might be useful. A link from MSDN.

How to: Populate Object Collections from Multiple Sources (LINQ)

Found this link from another question https://stackoverflow.com/a/9746336/1278872

edited May 23 '17 at 12:11

Community

1
1

answered May 04 '12 at 05:45

gorengpisang

11
2

Is there a better way to do this, maybe replace the for/foreach loop with something LINQ ish?

5 Answers5