Shuffling different decks give the same deck for both?

Question

I have this shuffling method in the Deck class:

public Card[] deck = new Card[DECK_AMOUNT];
public void shuffleCards()
{
    Random randInt = new Random();

    for (int i = 0; i < 52; i++)
    {
        int firstCard = i;
        int secondCard = randInt.Next(0, 51);

        int tempFirstCard = firstCard;

        deck[i] = deck[secondCard];
        deck[secondCard] = deck[i];

    }
}

And when I make these properties:

private Deck computerCards;
private Deck playerCards;

And use these methods

private void shuffleDecks(){
 computerCards.shuffleCards();
 playerCards.shuffleCards();
}

They both have the same cards on the same positions. Why does this happen and how do I solve it?

If I were to do

private void shuffleDecks(){
 computerCards.shuffleCards();
 playerCards.shuffleCards();
 playerCards.shuffleCards();
}

They are both different.

`deck[i] = deck[secondCard]; deck[secondCard] = deck[i];` produces duplicate - forgot how to exchange two variables? :) — Ivan Stoev, Jun 11 '17 at 17:35
As a remark: There is an error in your first loop. After `deck[i] = deck[secondCard];` both elements of the array will have the same value, so `deck[secondCard] = deck[i];` will change nothing. You need a temporary variable to swap both elements. — Sentry, Jun 11 '17 at 17:35
Yea stupid mistake I forgot to use the tempFirstCard variable — Sinan Samet, Jun 11 '17 at 17:43

score 2 · Accepted Answer · answered Jun 11 '17 at 17:31

From the manual:

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. This problem can be avoided by using a single Random object to generate all random numbers. You can also work around it by modifying the seed value returned by the system clock and then explicitly providing this new seed value to the Random(Int32) constructor. For more information, see the Random(Int32) constructor.

Thanks! I fixed it by adding a parameter to the `shuffleCards()` method — Sinan Samet, Jun 11 '17 at 17:37

Olivier Jacot-Descombes · Answer 2 · 2017-06-12T12:42:51.117

1

Your swapping algorithm is wrong

deck[i] = deck[secondCard];
// Now deck[i] and deck[secondCard] have the same value (i.e. card).

deck[secondCard] = deck[i]; // This doesn't change anything since both are the same now.

Let's make a concrete example and let's call two cards A and B.

// Initialize the example
deck[i] = A;
deck[secondCard] = B;

// Now start swapping
deck[i] = deck[secondCard];
// Here deck[i] is B and deck[secondCard] is B.
// We lost card A!

Save the old value of deck[i] to a temporary variable:

Card temp = deck[i];        // Now temp is A
deck[i] = deck[secondCard]; // Now deck[i] is B
deck[secondCard] = temp;    // Now deck[secondCard] is A

Your statement int tempFirstCard = firstCard; only saves the index, not the card at this index.

Naming can contribute to clearness. Instead of naming the indexes somethingCard, they could be named somethingIndex. But, since it is not important which one is first or second, it would be okay to simply name them i and j. It is very common to use such names in math for indexes.

edited Jun 12 '17 at 12:42

answered Jun 11 '17 at 17:37

Olivier Jacot-Descombes

104,806
13
138
188

Ah you're right I overlooked that. I intended to do `deck[secondCard] = deck[firstCard];`. Or am I missing something there as well? – Sinan Samet Jun 11 '17 at 17:42
It makes no difference, whether you are using `i` or `firstCard`, since both are the same. The problem is that after the first assignment `deck[i] = deck[secondCard];` the old value of `deck[i]` is lost, so `deck[secondCard] = deck[i];` does not what you expect, since now `deck[i]` contains the value of `deck[secondCard]`. – Olivier Jacot-Descombes Jun 11 '17 at 17:48
Ah thank you you are right I didn't know changing i would also change tempFirstCard I think this method did work for me in jQuery and I supposed it would work on C# as well. Sorry I'm new to this language – Sinan Samet Jun 11 '17 at 17:54
1

`tempFirstCard` is an `int` index, not an actual `Card`! You must save a `Card`, i.e. `deck[firstCard]` instead of `firstCard`. – Olivier Jacot-Descombes Jun 11 '17 at 17:58
Ah I get it now I'm sorry it took me a while both the indexes are at that point ofcourse the same card so it doesn't matter if you use index A or B – Sinan Samet Jun 11 '17 at 18:02
No. In my example `A` and `B` are of type `Card`. Not the indexes are the same but 2 items at different indexes are the same. `i` is an index. `secondCard` is an index. `deck[i]` is a card. `deck[secondCard]` is a card. `deck[whatEverIndex]` is a card. Note, my `temp` variable is not if type `int` but of type `Card`. This is the point. – Olivier Jacot-Descombes Jun 11 '17 at 18:08
Yea I said it wrong I meant if you use index i and secondCard the cards on those positions are the same – Sinan Samet Jun 11 '17 at 18:21

Shuffling different decks give the same deck for both?

2 Answers2