patterncsharpModerate
C# Card Shuffle
Viewed 0 times
cardshufflestackoverflow
Problem
So, I took a stab at the ole C# Card Shuffle. I wanted to create my own genuine solution rather than copy someone else's. Any advice?
public class Program
{
enum Suit
{
Hearts,
Diamonds,
Clubs,
Spades
}
class Deck
{
public List Cards { get; set; }
public Deck()
{
Cards = new List();
foreach (Suit suit in Enum.GetValues(typeof(Suit)))
{
for (int y = 2; y shuffleDeck = new List();
Random r = new Random();
int p = 0;
while (myDeck.Cards.Count > 0)
{
p = r.Next(0, myDeck.Cards.Count);
shuffleDeck.Add(myDeck.Cards[p]);
myDeck.Cards.Remove(myDeck.Cards[p]);
}
myDeck.Cards = shuffleDeck;
}
}Solution
Performance
The shuffle implementation is very inefficient, as removing a random item from a list is inefficient. Searching for the element to remove is an \$O(n)\$ operation, and then removing it from the middle of the list is expensive, as the subsequent elements may have to be shifted (when the list is backed by an array).
A better algorithm is to iterate from the front or back of the list, and swap the current element with a random element in the rest of the list. Here's an example implementation going from the end of the list:
Encapsulation
To preserve its integrity, a deck should not expose the list of cards. You could not pass this deck to Player classes safely, they might manipulate the content to rig the game.
It would be better to provide accessor methods to the necessary operations, for example:
This also implies that the shuffling algorithm should not be just lying around in a main method. As shuffling is such a common task, it should be implemented in its own class, or as a list extension as demonstrated in the answer by @craftworkgames. The deck could call a shuffling implementation it trusts. Or alternatively, the shuffling implementation could be injected using the strategy pattern.
"trivial problems"
In a comment you called this a trivial problem, yet the algorithm has a serious performance issue.
Don't underestimate problems that may seem trivial.
Complex software can be broken down to simple elements.
Solving simple problems contributes to solving more complex problems easier in the future, and also useful for building good habits. Look at how musicians practice. The motions may seem trivial or dull, but actually very important. So goes the saying: "you play how you practice".
The shuffle implementation is very inefficient, as removing a random item from a list is inefficient. Searching for the element to remove is an \$O(n)\$ operation, and then removing it from the middle of the list is expensive, as the subsequent elements may have to be shifted (when the list is backed by an array).
A better algorithm is to iterate from the front or back of the list, and swap the current element with a random element in the rest of the list. Here's an example implementation going from the end of the list:
for (var i = list.Count - 1; i > 0; i--)
{
var temp = list[i];
var index = random.Next(0, i + 1);
list[i] = list[index];
list[index] = temp;
}Encapsulation
To preserve its integrity, a deck should not expose the list of cards. You could not pass this deck to Player classes safely, they might manipulate the content to rig the game.
It would be better to provide accessor methods to the necessary operations, for example:
- take N cards
- shuffle
- reset
This also implies that the shuffling algorithm should not be just lying around in a main method. As shuffling is such a common task, it should be implemented in its own class, or as a list extension as demonstrated in the answer by @craftworkgames. The deck could call a shuffling implementation it trusts. Or alternatively, the shuffling implementation could be injected using the strategy pattern.
"trivial problems"
In a comment you called this a trivial problem, yet the algorithm has a serious performance issue.
Don't underestimate problems that may seem trivial.
Complex software can be broken down to simple elements.
Solving simple problems contributes to solving more complex problems easier in the future, and also useful for building good habits. Look at how musicians practice. The motions may seem trivial or dull, but actually very important. So goes the saying: "you play how you practice".
Code Snippets
for (var i = list.Count - 1; i > 0; i--)
{
var temp = list[i];
var index = random.Next(0, i + 1);
list[i] = list[index];
list[index] = temp;
}Context
StackExchange Code Review Q#131273, answer score: 13
Revisions (0)
No revisions yet.