Avoiding out of range issues in generic functions

Question

I come from the Wild Wild West of PHP and Javascript where you can return anything from a function. While I do hate that lack of accountability, I also face a new challenge in my effort to keep my code "perfect".

I made this generic function to pick a random element from a list

public static T PickRandom<T>(this IList<T> list) {
    Random random = new Random();
    int rnd = random.Next(list.Count);
    return list[rnd];
}

But I want to protect myself from using it on a list with 0 values. Obviously I cannot return anything from this function other than T, such as false or -1. I can of course do this

if(myList.Count > 0)
   foo = Utilites.PickRandom(myList);

However there are so many crazy things in C# that I don't know about, and for this app I am creating I very, very often have to choose a random element from a list that could be constantly decrementing in its Count. Is there a better way?

What should happen if some does call this method on an empty list? — Rob, Jul 22 '15 at 00:18
I suppose it should return nothing, but that is not possible? — user3822370, Jul 22 '15 at 00:19
If the list is containing reference types (objects, for example), you can do that. But that doesn't work for things like `int` unless you explicitly say it's null-able. It seems to me that it's **invalid** to call this method on an empty list, so an exception should be fine. Returning null could be unexpected for the caller. I would throw an exception if count == 0 in the PickRandom method. Also, you should be using this: `random.Next(list.Count - 1)`, or you can get an exception trying to access the `last+1` element. — Rob, Jul 22 '15 at 00:21
@Rob I'm confused about your last sentence. Suppose you have a List with 4 elements. If you call random.Next(list.Count) it should only return 0/1/2/3 no? — user3822370, Jul 22 '15 at 00:24
You are right, @user3822370. `Next(int)` returns anything between `0` and `value - 1` inclusive. — Andrew, Jul 22 '15 at 00:26
Sorry, you're right. I've confused it with another library that had the maximum value `inclusive`. — Rob, Jul 22 '15 at 00:26
@JohnSaunders I want to avoid the program crashing. I know this is a generic question, and it will help me answer a lot of other questions along he way. — user3822370, Jul 22 '15 at 00:37
"avoid the program crashing". Definitely a PHP response. There is nothing you can do to prevent the caller of the method from doing something stupid based on the return value of the method. Consider: `if (PickRandom(myList) != null) CrashNow();`. Don't think about whether the program would crash. Think about what should happen in the case of an empty list. — John Saunders, Jul 22 '15 at 00:40
"Think about what should happen in the case of an empty list" - this is what my question truly is. It just happens that the unhandled result is a crash. — user3822370, Jul 22 '15 at 00:46
Side note: make sure to read http://stackoverflow.com/questions/767999/random-number-generator-only-generating-one-random-number for your `new Random()` code. — Alexei Levenkov, Jul 22 '15 at 00:51

score 8 · Accepted Answer · edited May 23 '17 at 12:15

The options you have are

return default(T)

which will be an ambiguous behavior, as that might be a valid element of the list.

Or you can return something like -1 as you said, but that's quite coupled to your code.

Or you can return null, but that can only be done if T is a nullable type, of course.

In all previous cases, if the caller is not aware of this situation, the application may continue with an invalid value, leading to unknown consequences.

So probably the best option is to throw an exception:

throw new InvalidOperationException();

With this approach, you fail fast and you make sure nothing unexpected occurs beyond the caller's intentions.

One reason more to back up this option. Take for example Linq's extension methods. If you call First(), Single() or Last() on an empty list, you will get an InvalidOperationException with the message "Sequence contains no elements". Giving your class a behavior similar to the framework classes' is always a good thing.

I'm adding a side note thanks to Alexei Levenkov's comment in the question. That random generation is not the best approach. Take a look at this question.

Second side note. You are declaring your function as an extension method for IList<T> (you do that by using the this before the first parameter) but then you call it just like a static helper method. An extension method is a syntactic sugar that, instead of doing this:

foo = Utilites.PickRandom(myList);

lets you do this:

foo = myList.PickRandom();

More info on extension methods can be found here.

I'd say this is the correct answer. However, I wish you would emphasize the problems with returning some distinguished value. Namely, the fact that some caller will almost certainly forget to check for the value. Therefore, allowing the indexer to throw `IndexOutOfRangeException` is probably the best move (which you've already stated). — John Saunders, Jul 22 '15 at 00:28
Thanks for this response. Now I am just having trouble seeing how the caller handles this. The PickRandom throws an exception, I'll handle it in some way, but what does the caller do? How does it know to bail? Does it need its own try/catch? — user3822370, Jul 22 '15 at 00:54
Yes, the caller must be aware of this situation, you (as the extension method) are not in charge of deciding what to do in this case. So he should either check for the list count before calling or do a try-catch. Given this situation, the first approach is better, as an empty list is something you might expect and not something unforeseeable (that's where you should use try-catches). — Andrew, Jul 22 '15 at 00:59

score 0 · Answer 2 · edited May 23 '17 at 11:59

Another alternative is the following pair of overloads instead the original. With these, it should be clear to the caller that they are to supply a default random value in the event one cannot be "picked" from the list.

public static T PickRandomOrReturnDefault<T>(this IList<T> list, T defaultRandomValue)
{
    if (list == null || list.Count == 0) return defaultRandomValue;

    Random random = new Random();
    int rnd = random.Next(list.Count);
    return list[rnd];
}

public static T PickRandomOrReturnDefault<T>(this IList<T> list, Func<T> createRandomValue)
{
    if (list == null || list.Count == 0) return createRandomValue();

    Random random = new Random();
    int rnd = random.Next(list.Count);
    return list[rnd];
}

Note: You should probably consider making random a static member field of the class rather than re-instantiating it over and over. See the answer for this post Correct method of a "static" Random.Next in C#?

score 0 · Answer 3 · answered Jul 25 '15 at 01:50

Another option you have is to use the Maybe<T> monad. It's very much like Nullable<T>, but works with reference types.

public class Maybe<T>
{
    public readonly static Maybe<T> Nothing = new Maybe<T>();

    public T Value { get; private set; }
    public bool HasValue { get; private set; }

    public Maybe()
    {
        HasValue = false;
    }

    public Maybe(T value)
    {
        Value = value;
        HasValue = true;
    }

    public static implicit operator Maybe<T>(T v)
    {
        return v.ToMaybe();
    }
}

Your code could then look like this:

private static Random random = new Random();
public static Maybe<T> PickRandom<T>(this IList<T> list)
{
    var result = Maybe<T>.Nothing;
    if (list.Any())
    {
        result = list[random.Next(list.Count)].ToMaybe();
    }
    return result;
}

You would use it like:

var item = list.PickRandom();

if (item.HasValue) { ... }

Personally, I name my maybe methods with Maybe at the end.

It would be more like this:

var itemMaybe = list.PickRandomMaybe();

if (itemMaybe.HasValue) { ... }

Avoiding out of range issues in generic functions

3 Answers3