Extension method to retrieve a sample from a collection

Question

I wrote a little extension method to retrieve a random sample of items from a collection. I've tried to write clean code. What do you think?

• is it clean?
• does it work as you would expect?

I'll be glad to hear your comments.

using System;
using System.Collections.Generic;
using System.Linq;

namespace Extensions
{
    public static class IEnumerableExtensions
    {
        private static Random _rnd = new Random();

        public static T Sample<T>(this IEnumerable<T> values)
        {
            return Sample(values, 1).First();
        }

        public static IEnumerable<T> Sample<T>(this IEnumerable<T> values, int size)
        {
            if(!values.Any())
            {
                throw new InvalidOperationException("Collection empty.");
            }

            if (size < 1)
            {
                return new List<T>();
            }

            if (size >= values.Count())
            {
                return values;
            }

            return GetSamples(values, size);
        }
        private static IEnumerable<T> GetSamples<T>(IEnumerable<T> values, int size)
        {
            var samples = new List<T>();
            var source = new List<T>(values);
            while (samples.Count != size)
            {
                var index = _rnd.Next(source.Count());
                var value = source[index];
                samples.Add(value);
                source.Remove(value);
            }
            return samples;
        }
    }
}

Answer 1

        public static T Sample<T>(this IEnumerable<T> values)
        {
            return Sample(values, 1).First();
        }

Nice helper method.

---

            if(!values.Any())
            {
                throw new InvalidOperationException("Collection empty.");
            }

I would have used an ArgumentOutOfRangeException, but that's a matter of personal preference. Should it throw an exception if size == 0 or return an empty list? That's a tricky edge case which should be clearly documented.

---

        public static IEnumerable<T> Sample<T>(this IEnumerable<T> values, int size)

I would prefer the Random to be an argument too, even if it's as Random rnd = null with a fallback inside the method. Being able to seed your random selection is important for testing the code and for reproducing results if it's a scientific project.

---

            if (size < 1)
            {
                return new List<T>();
            }

I would definitely prefer to throw an ArgumentOutOfRangeException if size < 0.

---

            if (size >= values.Count())
            {
                return values;
            }

If size > values.Count() then I think that should be ArgumentOutOfRangeException or InvalidOperationException. Again, whichever way you finally decide, it should be clearly documented.

Note that we've now called two methods which iterate partially or fully through values. If it was a non-deterministic iterator we have problems. An example of a non-deterministic iterator would be

public IEnumerable<int> Demo()
{
    var rnd = new Random();
    for (int i = rnd.Next(10); i > 0; i--) yield return rnd.Next();
}

If you call this once and then loop through the resulting iterator multiple times then it will not consistently return the same values. You may think that in real usage you won't come across iterators like that, but sometimes the non-determinism is a lot more subtle.

---

            var samples = new List<T>();
            var source = new List<T>(values);

Since we have to convert source to a list here, there's an argument for doing it earlier and ensuring that we only iterate through values once, solving the problem of non-deterministic iterators.

---

                source.Remove(value);

This is expensive. It's asymptotically more efficient to replace this with

                source[index] = source[source.Count - 1];
                source.RemoveAt(source.Count - 1);

---

Finally, on the names. In your context it might be obvious that Sample means "sample without replacement", but in some contexts it might be important to distinguish between sampling with and without replacement. Consider whether you might need to distinguish the cases in future usage of the library.

---

Postscript: t3chb0t's suggestion of a Randomise method which can then be combined with Take is compatible with a rewrite of your GetSamples method, and in fact this is what I have kicking around in my library of utility methods (although I call it Shuffle). As a modification of your code:

    public static IEnumerable<T> Randomise<T>(IEnumerable<T> values, Random rnd = null)
    {
        if (rnd == null) rnd = _rnd;

        var source = new List<T>(values);
        while (source.Count > 0)
        {
            var index = rnd.Next(source.Count);
            yield return source[index];
            source[index] = source[source.Count - 1];
            source.RemoveAt(source.Count - 1);
        }
    }

Answer 2

public static T Sample<T>(this IEnumerable<T> values)
{
    return Sample(values, 1).First();
}

For someone who knows how to use LINQ this extension will be difficult to use and rather unintuitive because you have more options without it. It uses the First extension but what if I wanted to use FirstOrDefault in certain situations?

You've written an extension for something that's already there and limited the options. Now you need another one, which would be SampleOrDefault.

---

Let's take a look at the last one now.

private static IEnumerable<T> GetSamples<T>(IEnumerable<T> values, int size)
{
    var samples = new List<T>();
    var source = new List<T>(values);
    while (samples.Count != size)
    {
        var index = _rnd.Next(source.Count());
        var value = source[index];
        samples.Add(value);
        source.Remove(value);
    }
    return samples;
}

You've written another extension for something that already exists namely the OrderBy and Take extensions. One can do exactly the same with:

values.OrderBy(x => _rnd.Next()).Take(5);

---

The second extension isn't that useful either and it does a few things that is shouldn't.

if(!values.Any())
{
    throw new InvalidOperationException("Collection empty.");
}

Empty collections aren't considered an error. Most queries should work with empty collections just fine and return empty collections as a result. I wouldn't expect it to throw. You should check values for null only.

if (size < 1)
{
    return new List<T>();
}

Here you might want to return Enumerable.Empty<T>() instead.

if (size >= values.Count())
{
    return values;
}

This might cost you a lot of performnace because if values is a real deferred enumerable Count will enumerate it alredy for the first time. Then you'll enumerate it again consuming the result.

---

As I said above. All three extensions are not really necessary and with direct LINQ you'd be better off.

This means I'd rather do

var randomSamples =
    values
       .OrderBy(x => _rnd.Next())
       .Take(5); // or First or FirstOrDefault

then use any of the above extensions.

---

The only extension that might make sense would be

public static IEnumerable<T> Randomize(this IEnumerable<T> values) 
{
    return values.OrderBy(x => _rnd.Next());
}

so that you don't have to repeat yourself

var randomSamples =
    values
       .Randomize()
       .Take(5); // or First or FirstOrDefault

Answer 3

I don't like this

if (size >= values.Count())
{
      return values;
}

If size is > count I don't get an random order?

This has some waste

private static IEnumerable<T> GetSamples<T>(IEnumerable<T> values, int size)
{
    var samples = new List<T>();
    var source = new List<T>(values);
    while (samples.Count != size)
    {
        var index = _rnd.Next(source.Count());
        var value = source[index];
        samples.Add(value);
        source.Remove(value);
    }
    return samples;
}

You don't need samples at all. Just use return yield.

You could use an index and get rid of the O(n) source.Remove(value);.

If GetSamples is not used any where else I would inline it.