0

I'm trying to access the value of a random element of an list. At the moment my code seems to be returning the element rather than the value.

int x = _randMoveDecider.Count;

//makes sure x is never more than the array size
if(x != 0)
  {
    x = x - 1 ;
  }

Random _r = new Random();

_move = _r.Next(_randMoveDecider[x]);

return _randMoveDecider[_move];

at the moment if _randMoveDecider holds the values 2, 5 and 9 it will return 0, 1 or 2 rather than the values in the list, where am I going wrong?

[edit] I guess I should have said, the length of _randMoveDecider and the values stored in it change with each run through of the program, but they are always integers.

E_S
  • 11
  • 3

4 Answers4

3

How about just this?

// as a field somewhere so it's initialised once only
public Random _r = new Random();

    // later in your code
var _randList = new List<int>{4,5,8,9};
var _move = _r.Next(_randList.Count);
return _randList[_move];

Even better, here's something that will randomise any list:

public static Random _rand = new Random();

public IEnumerable<T> Randomise<T>(IList<T> list)
{
    while(true)
    {
        // we find count every time since list can change
        // between iterations
        yield return list[_rand.Next(list.Count)];
    }
}

One way of using it in your scenario:

// make this a field or something global
public IEnumerbale<int> randomiser = Randomise(_randList);

// then later
return randomiser.First();
yamen
  • 15,390
  • 3
  • 42
  • 52
  • -1. Creating Random every time will traditionally return the same values till time changes... – Alexei Levenkov May 01 '12 at 06:44
  • @AlexeiLevenkov I had that fixed in the second set of code and not the first, updated now. This isn't -1 worthy.. – yamen May 01 '12 at 07:03
  • You did not really fixed the issue - if you create Randomise for 2 lists and compare the resulting "randomized" sequences you'll find that pretty much all the time both are randomized the same way. You are creating Random initialized with likely the same seed (if called at about the same time - i.e. on next line) for each list - as result pseudo-random sequences are identical with high probability. – Alexei Levenkov May 01 '12 at 16:39
  • @AlexeiLevenkov perhaps you should look at how Randomise is built and notice that as long as you only call it once, it only calls `new Random()` once. Every subsequent call is on the enumerator which is just using the `yield` statements. And the first example was also fixed to move the `new Random()` to an instance or static field (whichever is appropriate). – yamen May 01 '12 at 20:32
  • Yes, that why I tried to say that it does not produce random independent ordering when you call it on 2 lists. – Alexei Levenkov May 01 '12 at 21:41
  • OK all clean now. Didn't consider two similarly ordered lists and implications thereof. – yamen May 01 '12 at 21:48
2

Firstly you should initialize Random once. Make it a field:

private Random _rand = new Random();

Then get a random number from the proper range. if(x!=0) is useless - Next() returns numbersform <0, n) range

return _randMoveDecider[_rand.Next(_randMoveDecider.Count)];
Lukasz Madon
  • 14,664
  • 14
  • 64
  • 108
1

Simply add this extension class inside main class:

public static class Extensions
{
    public static int randomOne(this List<int> theList)
    {
        Random rand = new Random(DateTime.Now.Millisecond);
        return theList[rand.Next(0, theList.Count)];
    }
}

and then call it:

int value = mylist.randomOne();

EDIT: This is a test program that demonstrates how one would use the method. Note that due to incorrect usage of Random it produces very unbalanced results with more than 50 "random" numbers out of 100 being the same.

class Program
{
    static void Main(string[] args)
    {
        var myList = Enumerable.Range(0, 100).ToList();
        var myRandoms = myList.Select(v => new { key = v, value = 0 })
                 .ToDictionary(e => e.key, e => e.value);

        for (int i = 0; i < 100; i++)
        {
            var random = myList.RandomOne();
            myRandoms[random]++;
        }

        Console.WriteLine(myRandoms.Values.Max());
        Console.ReadLine();
    }
}

To fix the issue make Random static instance for Extension class or share more broadly in the program. This is discussed in FAQ for Random.

public static class Extensions
{
    static Random rand = new Random();
    public static int randomOne(this List<int> theList)
    {
        return theList[rand.Next(0, theList.Count)];
    }
}
Community
  • 1
  • 1
0
var random = new Random();
var item = list.ElementAt(random.Next(list.Count()));
Sprague
  • 1,610
  • 10
  • 22
  • You can directly call list indexer `list[index]` without calling extension method, which will do several checks, casting, and finally call list indexer. – Sergey Berezovskiy May 01 '12 at 22:22
  • I did this intentionally but can see your point. I did it this way because I was programming to the IEnumerable interface, not a concrete type. http://stackoverflow.com/questions/5326874/why-would-i-use-enumerable-elementat-versus-the-operator. If the poster uses an IList, there's little more overhead than the method call (also note that original poster didn't specify data type.) – Sprague May 02 '12 at 07:12