6

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?

11
  • 4
    What should happen if some does call this method on an empty list?
    – Rob
    Jul 22, 2015 at 0:18
  • 1
    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, 2015 at 0:21
  • 2
    @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? Jul 22, 2015 at 0:24
  • 1
    You are right, @user3822370. Next(int) returns anything between 0 and value - 1 inclusive.
    – Andrew
    Jul 22, 2015 at 0:26
  • 1
    Sorry, you're right. I've confused it with another library that had the maximum value inclusive.
    – Rob
    Jul 22, 2015 at 0:26

3 Answers 3

8

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.

3
  • 4
    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). Jul 22, 2015 at 0:28
  • 1
    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? Jul 22, 2015 at 0:54
  • 1
    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, 2015 at 0:59
0

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#?

0

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) { ... }

Your Answer

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

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