3

I try to take 3 rows at a time from a List of string but its not working as expected. Consider this code...

var listOfStrings = new List<string>
{
    "String 1",
    "String 2",
    "String 3",
    "String 4",
    "String 5",
    "String 6"
};

foreach (var x in listOfStrings.Take(3).ToList())
{
    var currRows = x.ToList();

    // currRows should have 3 items
    foreach (var itm in currRows)
    {

    }
}

The first run I expect currRows to have 3 items (String 1, 2 and 3), the second time I expect to have these 3 items (String 4, 5 and 6). But when I run this currRows only contains for example "String 1" and this is split up character by character?!

What am I missing here?

Yuval Itzchakov
  • 146,575
  • 32
  • 257
  • 321
MTplus
  • 2,077
  • 4
  • 34
  • 51
  • That's not how `Take` works. Or any other LINQ operator, really - they never *modify* the original collection. You need to use e.g. a combination of `Take` and `Skip`. – Luaan Oct 15 '15 at 12:34
  • 5
    Replace your second loop with `foreach(var itm in x)`. Don't call `ToList()` on a string, as it's also an `IEnumerable`, and so `string.ToList()` returns a list of characters. Secondly, remove the `ToList()` call in your first loop, it does nothing, and is a waste of both CPU and memory. And finally, `Take(3)` will only take the ***first 3 items***, not group by 3. – Rob Oct 15 '15 at 12:34

3 Answers3

2
  1. x.ToList(); is calling the method on a string. A string is an IEnumerable<char>, so the result of currRows is a list of characters. You can remove that line completely, and use x in your second loop.
  2. Using ToList() is usually[1] a pointless operation if you're doing it on a collection used in a foreach loop. It'll iterate your collection and construct a new list, which is then iterated. You can remove ToList from your first loop and it'll perform better, but have the same result.
  3. Take simply takes n number of items, it does not group by n items.

Try this:

var listOfStrings = new List<string>
{
    "String 1",
    "String 2",
    "String 3",
    "String 4",
    "String 5",
    "String 6"
};

int step = 0;
const int numTake = 3;
for(var i = 0; i < listOfStrings.Count / numTake; i++)
{
    Console.WriteLine("Starting a group!");
    foreach (var x in listOfStrings.Skip(3 * step++).Take(3))
    {
        Console.WriteLine(x);
    }
}

A huge amount of room to improve, but it should give you an idea of how you should approach this task.

  1. There are cases where you need to do this - but you most likely won't encounter them when starting out with LINQ.
Rob
  • 26,989
  • 16
  • 82
  • 98
2

But when I run this currRows only contains for example "String 1" and this is split up character by character?!

That's because Enumerable.Take will take the requested amount of items from the IEnumerable<T>. This makes your x variable be of type string, which you later call ToList() on, effectively creating a List<char>, which isn't what you want.

You can use MoreLINQ which has a Batch extension method which does exactly what you want. It returns an IEnumerable<IEnumerable<T>>:

foreach (var batch in listOfStrings.Batch(3))
{
    // batch is an IEnumerable<T>, and will have 3 items.
    foreach (var item in batch)
    {

    }
}

Another possibility is to create that extension method yourself. This is taken from this answer:

public static class EnumerableExtensions
{
    public static IEnumerable<IEnumerable<T>> Batch<T>(this IEnumerable<T> items,
                                                       int maxItems)
    {
        return items.Select((item, inx) => new { item, inx })
                    .GroupBy(x => x.inx / maxItems)
                    .Select(g => g.Select(x => x.item));
    }
}
Community
  • 1
  • 1
Yuval Itzchakov
  • 146,575
  • 32
  • 257
  • 321
  • 1
    Thanks, MoreLinq was very handy indeed. And thanks for the explanation of why it did not work as I expected. – MTplus Oct 15 '15 at 12:53
0

Starting with your code:

var listOfStrings = new List<string>
{
    "String 1",
    "String 2",
    "String 3",
    "String 4",
    "String 5",
    "String 6"
};

foreach (var x in listOfStrings.Take(3).ToList())
{
    var currRows = x.ToList();

    // currRows should have 3 items
    foreach (var itm in currRows)
    {

    }
}

Lets simplify your code a little bit by creating an extra variable for what you are looping over:

var listOfStrings = new List<string>
{
    "String 1",
    "String 2",
    "String 3",
    "String 4",
    "String 5",
    "String 6"
};

var thingsYouAreLoopingOver = listOfStrings.Take(3).ToList();

foreach (var x in thingsYouAreLoopingOver)
{
    var currRows = x.ToList();

    // currRows should have 3 items
    foreach (var itm in currRows)
    {

    }
}

but surely this can be simplified again as thingsYouAreLoopingOver is just the first three things of listOfStrings, so we have:

var thingsYouAreLoopingOver = new List<string>
{
    "String 1",
    "String 2",
    "String 3"
};

foreach (var x in thingsYouAreLoopingOver)
{
    var currRows = x.ToList();

    // currRows should have 3 items
    foreach (var itm in currRows)
    {

    }
}

It should be clear now why you get the behaviour you are seeing.

DanL
  • 1,974
  • 14
  • 13