4
\$\begingroup\$

Three questions: is there a more performant way, is there a more suscinct, strike that, a way of expressing this where if you read just the body it is immediately apperent what the alorithm does, and should I be returning and IEnumerable of an IEnumerable, I mean what would be the point over and IEnumerable of IList?

public static IEnumerable<IEnumerable<T>> IntoBatches<T>(this IEnumerable<T> list, int size)
{
    if (size < 1)
    {
        yield return list;
    }
    else
    {
        var count = 0;
        var batch = new List<T>();
        foreach (var item in list)
        {
            batch.Add(item);
            if (size == ++count)
            {
                yield return batch;
                batch.Clear();
            }
        }
        if (batch.Count > 0) yield return batch;
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ See: stackoverflow.com/a/419063/2655508 and stackoverflow.com/a/438513/2655508 \$\endgroup\$
    – Heslacher
    Mar 10, 2016 at 11:57
  • \$\begingroup\$ @Heslacher How is it more performant? Or did you mean it was expressed prettier? \$\endgroup\$
    – Marcus
    Mar 10, 2016 at 11:59
  • \$\begingroup\$ The second. I don't know about performance, that would be something to profile. \$\endgroup\$
    – Heslacher
    Mar 10, 2016 at 12:05
  • \$\begingroup\$ @Heslacher Oh because I didn't think so, hehe. But anyway, skip if you will the part about the estetics because that's objective, very apparently. \$\endgroup\$
    – Marcus
    Mar 10, 2016 at 12:07

2 Answers 2

7
\$\begingroup\$

Bug

You have 2 big bugs in your method. The first is that you never ever set the count variable to 0 and the second that you are yielding the List<T>.

If I call your method with a List<int> containing 10000 ints and do a ToList() on the result I get 2 Lists both containing 9997 ints.

Although this is easy to fix like so

public static IEnumerable<IEnumerable<T>> IntoBatches<T>(this IEnumerable<T> list, int size)
{
    if (size < 1)
    {
        yield return list;
    }
    else
    {
        var count = 0;
        foreach (var item in list)
        {
            var batch = new List<T>();                
            batch.Add(item);
            if (size == ++count)
            {
                yield return batch;
                batch = new List<T>();
                count = 0;
            }

        }
        if (batch.Count > 0) yield return batch;
    }
}  

this solution takes for a List<int> having 10000 items with size:

  3: 0.506 ms
 13: 0.505 ms
113: 0.505 ms

whereas an array based solution like this (taken from here)

public static IEnumerable<IEnumerable<T>> Chunkify<T>(this IEnumerable<T> source, int size)
{

    using (var iter = source.GetEnumerator())
    {
        while (iter.MoveNext())
        {
            var chunk = new T[size];
            chunk[0] = iter.Current;
            for (int i = 1; i < size && iter.MoveNext(); i++)
            {
                chunk[i] = iter.Current;
            }
            yield return chunk;
        }
    }
}

takes

  3: 0.270 ms
 13: 0.270 ms
113: 0.270 ms

Edit

That Chunkify() method unfortunately has a bug, which is for a passed in IEnumerable<T> with a size which isn't dividable by the passed in chunk size will produce to many items.

E.g passed in a int[] with values 1,2,3,4 and an size argument of 3 will produce 1,2,3,4,0,0.

Fixed version

public static IEnumerable<IEnumerable<T>> Chunkify<T>(this IEnumerable<T> source, int size)
{
    int count = 0;
    using (var iter = source.GetEnumerator())
    {
        while (iter.MoveNext())
        {
            var chunk = new T[size];
            count = 1;
            chunk[0] = iter.Current;
            for (int i = 1; i < size && iter.MoveNext(); i++)
            {
                chunk[i] = iter.Current;
                count++;
            }
            if (count < size)
            {
                Array.Resize(ref chunk, count);
            }
            yield return chunk;
        }
    }
}
\$\endgroup\$
11
  • \$\begingroup\$ This site is much more fun than unit testing. Thx! \$\endgroup\$
    – Marcus
    Mar 10, 2016 at 13:57
  • \$\begingroup\$ This shall be my new norm: if no answer on codereview, write unit test. \$\endgroup\$
    – Marcus
    Mar 10, 2016 at 13:57
  • 7
    \$\begingroup\$ No, your norm should be first unit tests then code review \$\endgroup\$
    – Heslacher
    Mar 10, 2016 at 13:58
  • \$\begingroup\$ You have proven to be credible, so +1 for the benchmarks. \$\endgroup\$
    – Marcus
    Mar 10, 2016 at 14:00
  • 4
    \$\begingroup\$ Definitely unit test before going to review otherwise you cannot be sure your code will function as expected. \$\endgroup\$
    – Dan
    Mar 10, 2016 at 14:05
3
\$\begingroup\$

Improving readability

Most easy one instead of adding one by one to temporary variable use Enumerable.Take and Enumerable.Skip. Fickle local variables such as loop counters and temporary variable makes code hard to understand.

Also size < 1 part is unnecessary and confusing. Instead of trying to do something, inescapably unexpected, just reject illegal values. This also has the effect of reducing nesting and increasing readability of code.

Here is the result, (not tried though):

public static IEnumerable<IEnumerable<T>> IntoBatches<T>(this IEnumerable<T> list, int size)
{
    if (size < 1)
        throw new ArgumentException();

    var rest = list;

    while (rest.Any()) {
        yield return rest.Take(size);
        rest = rest.Skip(size);
    }
}
\$\endgroup\$
1
  • 3
    \$\begingroup\$ Although this looks neat, it is incredible slow. I couldn't test it with the values of my answer because it seems to take forever. Just for you I tested it using size=3 on a List<int> containing 100 items resulting in 0.278 ms. \$\endgroup\$
    – Heslacher
    Mar 10, 2016 at 14:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.