Brute force Card shake method in C#

Question

I’m making a bot that can test some card tactics on a game, the bot works but I have one problem. My card shake method is not very good. I shake the cards this way:

        public void ShuffelDeck()
        {
            for (int i = 0; i < 5; i++)
                Cards = ShuffelCards(Cards);
        }

        private ArrayList ShuffelCards(ArrayList Cards)
        {
            ArrayList ShuffedCards = new ArrayList();

            Random SeedCreator = new Random();
            Random RandomNumberCreator = new Random(SeedCreator.Next());

            while (Cards.Count != 0)
            {
                int RandomNumber = RandomNumberCreator.Next(0, Cards.Count);
                ShuffedCards.Insert(ShuffedCards.Count, Cards[RandomNumber]);
                Cards.RemoveAt(RandomNumber);
            }

            return ShuffedCards; 
        }

The problem is when I calculate without a pause between the card shake process , a few players will win a lot of games. For example

Player 1: 8 games
Player 2: 2 games
Player 3: 0 games
Player 4: 0 games

But when I add a dialog before the card shake process the wins will get spread over the players:

Player 1: 3 games
Player 2: 2 games
Player 3: 1 game
Player 4: 4 games

I guess that the Random class is not good enough for a brute force like this. How can I shuffle the cards without a problem like this?

Update: I already tried to use 1 Random class, and I also tried to shake the cards only once.

Any reason you're still using `ArrayList` instead of `List`? — Jon Skeet, Jul 25 '12 at 18:34
Every time someone writes new code with an `ArrayList`, a puppy dies. — pstrjds, Jul 25 '12 at 18:36
@pstrjds: Well, it might be a puppy. It might be a kitten. It might just be a string. There's no way of knowing. If you had a `List` of course, it would be different... — Jon Skeet, Jul 25 '12 at 18:37
Good question, A list is in this case a lot better. I don't know why I used ArrayList.. Thanks for the tip. — Laurence, Jul 25 '12 at 18:39
Does using a different shuffling algorithm work? http://stackoverflow.com/questions/1150646/card-shuffling-in-c-sharp — hometoast, Jul 25 '12 at 18:44
@JonSkeet A List is definitely a bad idea. There's no better way than boxing to make a cat happy! — Kevin Gosse, Jul 25 '12 at 18:47
@hometoast The algorithm is already equivalent to Fisher-Yates, except it's not shuffling in situ — Jonas Høgh, Jul 25 '12 at 18:50
@JonasH, true. I missed the main reason of seeding the random repeatedly. — hometoast, Jul 25 '12 at 18:53

Servy · Accepted Answer · 2012-07-25T18:52:28.360

When you create a new instance of the Random class it is seeded with the current time, but the current time it's seeded with only has precision down to 16 ms, meaning if you create a new Random class within the same 16 millisecond interval (that's a long time for a computer) as another instance you'll get the same random number sequence.

One good solution is to just ensure you aren't creating so many new Randoms. Create one, once, and pass it into the shuffle cards method, or create a static random that is just accessible in a lot of places (just remember that it's not thread safe).

On a side note, for your actual shuffling algorithm it's pretty good. You could make one improvement though. Rather than adding the chosen element to the end and removing it from the middle, you can just swap the element at the end with the one in the middle. This is more efficient since removing from the middle of a List isn't an efficient operation. Additionally, rather than picking a random number between 0 and the size, pick a number between 0 and (size minus the number of the current iteration). See the wikipedia article on the subject for more details on the algorithm.

Already tried that, with 1 random class it also will get a lot of the same values. — Laurence, Jul 25 '12 at 18:47
@user1201889: Because that is no different than what you are currently doing. You are creating a bunch of random objects with the same seed, so they return the same sequence. Create the Random object once and use it until your application exits (or when the class is done with it). Make it a class level variable. — Ed S., Jul 25 '12 at 18:48

score 4 · Answer 2 · answered Jul 25 '12 at 18:46

Having 2 Random classes and producing the seed for one with the other doesn't improve the randomness.

Then, don't create the Random class inside of ShuffelCards. Create it in ShuffelDeck and pass it to ShuffelCards, or make it a member of the class. This should help.

Brute force Card shake method in C#

2 Answers2