-1

First off I have my "Deck" class here. I've just put in the some basic methods, for testing purposes. It's my first go at creating a "card" program.

    public class Deck
{
    private int deckCounter = 0;
    private List<Card> deckSize = new List<Card>();
    private List<Card> shuffledDeck = new List<Card>();
    private Random random = new Random();

    public Deck()
    {
    }

    public void Build()
    {
        for (int i = 1; i < 5; i++)
        {
            for (int k = 1; k < 14; k++)
            {
                deckSize.Add(new Card(k.ToString(), i));
            }
        }
    }

    public void Add(Card card)
    {
        deckSize.Add(card);
        deckCounter++;
    }

    public Card RemoveCard()
    {
            Card cardToRemove = deckSize.First();
            deckSize.RemoveAt(0);
            return cardToRemove;
    }

    public void ShowContainedCards()
    {
        int cardCount = 0;
        foreach (Card c in deckSize)
        {
            Console.WriteLine(c.ReturnCardInfo());
            cardCount++;
        }
        Console.WriteLine(cardCount);
    }

    public void Shuffle()
    {
        while (deckSize.Count != 0)
        {
            int i = random.Next(deckSize.Count);
            shuffledDeck.Add(deckSize[i]);

            deckSize.RemoveAt(i);
        }
        deckSize = shuffledDeck;
    }

    public bool IsEmpty()
    {
        if (deckSize.Any())
        {
            return false;
        }
        else return true;
    }

    public List<Card> GetCardList()
    {
        return deckSize;
    }
}

Basicly, what I do is, this:

    Deck deck1 = new Deck();
    Deck deck2 = new Deck();

    deck1.Build();
    deck1.Shuffle();
    deck2.Build();
    deck2.Shuffle();

After that, I get the exact same shuffle, for deck1 and deck2. Why is that? Also, I'm a newb at this, if you couldn't tell :)

  • 4
    It says [in the documentation for Random](http://msdn.microsoft.com/en-us/library/h343ddh9(v=vs.110).aspx): "different Random objects that are created in close succession by a call to the default constructor will have identical default seed values and, therefore, will produce identical sets of random numbers." – Raymond Chen Jul 22 '14 at 08:12
  • 2
    As a side note, see the [Fisher-Yates shuffle](http://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle), which is a proper shuffle algorithm. – Lucas Trzesniewski Jul 22 '14 at 08:16
  • 1
    Closed because the underlying issue is the same and the existing question explains it well. – usr Jul 22 '14 at 08:30
  • your `Shuffle` routine is also not very safe - I'd make `shuffledDeck` a local variable rather than a field and always initialize it with a new `List<>` inside Shuffle. At the moment, you can't call `Shuffle` twice in a sane manner because after the first call, `deckSize` and `shuffledDeck` refer to the same list. – Damien_The_Unbeliever Jul 22 '14 at 08:30

3 Answers3

1

Use the same Random class instance in both deck instances:

Random random = new Random();
Deck deck1 = new Deck(random);
Deck deck2 = new Deck(random);

So, in the constructor:

public class Deck
{
    private int deckCounter = 0;
    private List<Card> deckSize = new List<Card>();
    private List<Card> shuffledDeck = new List<Card>();
    private Random random;

    public Deck(Random random)
    {
        this.random = random;
    }

The current problem with your code, is that the two instances of Random that are created are seeded the same. This causes them to produce the same results. Using the same instance of Random means the second shuffle will build on top of the seeded results of the first shuffle.

Davin Tryon
  • 66,517
  • 15
  • 143
  • 132
1

From the docs:

The default seed value is derived from the system clock and has finite resolution. As a result, different Random objects that are created in close succession by a call to the default constructor will have identical default seed values and, therefore, will produce identical sets of random numbers.

The same docs also suggest a solution:

This problem can be avoided by using a single Random object to generate all random numbers.

Hence, one possible solution would be to make your random generator static, so all of your Deck instances share the same Random instance:

private static Random random = new Random();

That way, you would even avoid changing any part of the public interface of your Deck class.

O. R. Mapper
  • 20,083
  • 9
  • 69
  • 114
0

Computers are inherently not random, so any random number generator will actually be using an algorithm to produce output that looks random. The thing with this is that there's always a starting point, and if you know where it' starting from, you can predict the outcome. Random number generators therefore have a "seed" which tells it where to start. The same seed will always give the same sequence of "random" numbers.

Both times, you're using new Random(), which uses the default seed. In some languages, you'd be advised to pass the current time as a seed, but C# does that for you. However, if you create two Random objects close together, they're likely to get the same time, which is what's happening here.

If you made your Random static, then all your random numbers would come from the same source, so the two decks would get successive random numbers, not the same ones in parallel.

anaximander
  • 7,083
  • 3
  • 44
  • 62