3
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Is Sample a real name for something domain specific or is this just meant as an example? \$\endgroup\$ – t3chb0t May 10 '17 at 10:59
  • \$\begingroup\$ It's domain specific: A sample is a subset of a population. \$\endgroup\$ – venerik May 10 '17 at 11:02
  • \$\begingroup\$ Suggest perhaps you rename parameters from 'values' to 'population' to help re-enforce the concept that the return Sample(s) are produced from a Population. \$\endgroup\$ – Rick Davin May 10 '17 at 14:00
  • \$\begingroup\$ Two quick notes: mark _rnd as readonly and then when calling _rnd.Next(...), surround it with a lock(_rnd) { ... } to ensure thread safety (keeping the same value from being returned from two threads calling at the same time). \$\endgroup\$ – Jesse C. Slicer May 10 '17 at 14:50
  • \$\begingroup\$ Oh, one more: instead of return new List<T>();, I would recommend return Enumerable.Empty<T>();. \$\endgroup\$ – Jesse C. Slicer May 10 '17 at 14:52
2
\$\begingroup\$
        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);
        }
    }
\$\endgroup\$
  • \$\begingroup\$ The only exception that should be thrown is the ArgumentNullException everything else should result in an empty collection. \$\endgroup\$ – t3chb0t May 10 '17 at 11:38
  • 1
    \$\begingroup\$ @t3chb0t, I disagree. If you ask me for -3 items from a collection then you've got an error in your logic, and I should throw an exception to help you identify where the error is. If you ask me for five items from a collection of three then again it's an error in your logic and I would expect an exception. IMO Linq's Take is badly named and should be TakeUpTo. \$\endgroup\$ – Peter Taylor May 10 '17 at 11:41
  • 1
    \$\begingroup\$ Take a look at the Take extension. It won't throw for negative numbers and this is what I expect. It's exactly how it should work and I made use of it many time already. \$\endgroup\$ – t3chb0t May 10 '17 at 11:43
  • 2
    \$\begingroup\$ So take-home message for OP: different people expect different things, so make sure you have method-level documentation which makes it very clear what they should expect from your method. \$\endgroup\$ – Peter Taylor May 10 '17 at 11:44
  • 1
    \$\begingroup\$ I find a little bit funny how you like things that I criticize and the other way around ;-) Our reviews are like black and white. Completely different ;-] \$\endgroup\$ – t3chb0t May 10 '17 at 11:51
1
\$\begingroup\$
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
\$\endgroup\$
  • 1
    \$\begingroup\$ The downside of OrderBy is that when selecting a few values from a very large list it does a lot of unnecessary work. \$\endgroup\$ – Peter Taylor May 10 '17 at 11:38
  • \$\begingroup\$ @PeterTaylor it's still much cheaper then new List<T>(values); + source.Remove(value); who knows how many times and each removal is an O(n) ;-) \$\endgroup\$ – t3chb0t May 10 '17 at 11:40
  • \$\begingroup\$ Which is why I addressed that in my answer... \$\endgroup\$ – Peter Taylor May 10 '17 at 11:41
  • \$\begingroup\$ You too, thanks for the feedback. I had to think about your OrderBy solution for a while... nice. The reason I created an extension method though was I do not want to repeat myself. In my solution I have to sample multiple collections. I wanted to stay DRY so I did not want to make the class in need of the samples responsible for the sampling. That's why I moved that responsibility to extension methods. \$\endgroup\$ – venerik May 10 '17 at 13:04
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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