HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Card shuffling with Fisher-Yates method

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
methodwithcardshufflingyatesfisher

Problem

The code below takes a specified number of card decks, and shuffles them according to the Fisher-Yates method. Does this implementation have any bias?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace randomShuffle
{
    class Program
    {
        static void Main(string[] args)
        {
            List deck = Cards.startDeck(1);
        }
    }

    class Cards
    {
        public static List startDeck(int numDecks)
        {
            List initial = new List();
            List shuffled = new List();

            //set up list of all cards in order
            while (numDecks > 0)
            {
                for (int i = 2; i <= 14; i++) //2-ace(14)
                {
                    for (int j = 0; j < 4; j++)  //each card in deck 4 times
                    {
                        initial.Add(i);
                    }
                }

                numDecks--;
            }

            //Fisher-Yates method to shuffle
            Random r = new Random(DateTime.Now.Millisecond);
            int count = initial.Count;
            for (int i = 0; i < count; i++) //go through entire unshuffled deck
            {
                //get random number from 0 to new range of unshuffled deck
                int randomDraw = r.Next(0, initial.Count);

                //take whatever is drawn and insert at beginning of shuffled list
                shuffled.Insert(0, initial[randomDraw]);

                //replace the card drawn with the end card
                initial[randomDraw] = initial.Last();

                //remove the end card from initia deck
                initial.RemoveAt(initial.Count - 1);
            }

            return shuffled;
        }
    }
}

Solution

Comments:

about all of your comments add close to no value to your code.

//get random number from 0 to new range of unshuffled deck
int randomDraw = r.Next(0, initial.Count);


Your comment just writes out, what your code does in the next line. This is unneccesary noise and you should not do it. The probably only acceptable comment is this one:

for (int i = 2; i <= 14; i++) //2-ace(14)


This exactly explains why you start on 2 and end with 14, the rest can be removed.

But as @Marc-Andre pointed out correctly, this comment also becomes pure noise, as soon as you extract these magic numbers to named constants:

const int LOWEST_CARD = 2;
const int HIGHEST_CARD = 14;
for (int i = LOWEST_CARD; i <= HIGHEST_CARD; i++)


Extracting methods:

Why not extract your whole shuffle algorithm to a method? This makes the code in your startDeck less cluttered. Example:

public static List startDeck(int numDecks)
{
    List initial = new List();

    prepareDeck();

    return shuffled(initial);
}


You now exactly know, what your startDeck does.

Naming:

Your class Cards. this is definitely not nice, but that name is not OK. What does your class do? Name it after that. Try to keep your classes in SRP, they only should do one thing and nothing more.

Deck might have been better depending on what it does apart from what you posted.

Alternatively use Shuffler, Helper or similar.

Algorithm:

The english wikipedia article on the Fischer-Yates algorithm gives pretty nice pseudocode and tells us, that the algorithm is an in-place algorithm.

This means you will not need a second list to put your shuffled cards in.

//To shuffle an array a of n elements (indices 0..n-1):
for i from n − 1 downto 1 do
      j ← random integer with 0 ≤ j ≤ i
      exchange a[j] and a[i]


this means, if you were strictly following the algorithm your code should look (something) like this:

int temp, randomNumber;
int count = initial.Count;
for (int index = count -1; index > 0; index--){
   randomNumber = r.Next(0, index+1);
   temp = initial [index];
   initial[index] = initial[randomNumber];
   initial[randomNumber] = temp;
}
return initial;

Code Snippets

//get random number from 0 to new range of unshuffled deck
int randomDraw = r.Next(0, initial.Count);
for (int i = 2; i <= 14; i++) //2-ace(14)
const int LOWEST_CARD = 2;
const int HIGHEST_CARD = 14;
for (int i = LOWEST_CARD; i <= HIGHEST_CARD; i++)
public static List<int> startDeck(int numDecks)
{
    List<int> initial = new List<int>();

    prepareDeck();

    return shuffled(initial);
}
//To shuffle an array a of n elements (indices 0..n-1):
for i from n − 1 downto 1 do
      j ← random integer with 0 ≤ j ≤ i
      exchange a[j] and a[i]

Context

StackExchange Code Review Q#44980, answer score: 7

Revisions (0)

No revisions yet.