patterncsharpMinor
Card shuffling with Fisher-Yates method
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.
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:
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:
Extracting methods:
Why not extract your whole shuffle algorithm to a method? This makes the code in your startDeck less cluttered. Example:
You now exactly know, what your startDeck does.
Naming:
Your class
Alternatively use
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.
this means, if you were strictly following the algorithm your code should look (something) like this:
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.