C Program - Why is my card shuffling function not working correctly?-CodePudding

I'm creating a card shuffling function which iterates through a passed array, calls another function swap to change each element on pass with another and stores that element into another array called shuffledDeck. Im then returning the shuffled deck from the function. I am getting some shuffled cards on calling the function but most stay in the same place. I'm not sure why the rest of the deck isn't moving like the others. Could anyone point out anything wrong with my functions that would explain the output?

Initialized deck vs output that im getting from my function.

Unshuffled 

[AH] [2H] [3H] [4H] [5H] [6H] [7H] [8H] [9H] [10H] [JH] [QH] [KH] 
[AD] [2D] [3D] [4D] [5D] [6D] [7D] [8D] [9D] [10D] [JD] [QD] [KD] 
[AC] [2C] [3C] [4C] [5C] [6C] [7C] [8C] [9C] [10C] [JC] [QC] [KC] 
[AS] [2S] [3S] [4S] [5S] [6S] [7S] [8S] [9S] [10S] [JS] [QS] [KS] 

Shuffled 

[AD] [AH] [2H] [3H] [4H] [5H] [6H] [7H] [8H] [9H] [10H] [JH] [QH] 
[KH] [KH] [2D] [3D] [4D] [5D] [6D] [7D] [8D] [9D] [10D] [JD] [QD] 
[KD] [AC] [2C] [3C] [4C] [5C] [6C] [7C] [8C] [9C] [10C] [JC] [QC] 
[KC] [AS] [2S] [3S] [4S] [5S] [6S] [7S] [8S] [9S] [10S] [JS] [QS]

My functions

void swap(char *(*deck)[13], int r, int c)
{
    time_t t;
    srand((unsigned) time(&t));

    int rowToSwap = -1;  //will hold generated row and column to swap
    int colToSwap = -1;
    
    while(rowToSwap < 0 || rowToSwap > 3 && colToSwap < 0 || colToSwap > 12){ //while row to swap and col to swap are outside appropriate ranges
        rowToSwap = rand() % (3   1 - 0);
        colToSwap = rand() % (12   1 - 0);
    }
    char *temp = deck[rowToSwap][colToSwap];    //swap by elements using a temp holder
    deck[rowToSwap][colToSwap] = deck[r][c];
    deck[r][c] = temp;
}


char *(*shuffleDeck(char *(*deck)[13]))[13]
{
    char *(*shuffledDeck)[13] = malloc(4 * sizeof(*shuffledDeck));

    for(int i = 0; i < 4; i  ){
        for(int j = 0; j < 13; j  ){
            swap(deck, i, j);
            shuffledDeck[i][j] = deck[i][j];
        }
        
    }
    return shuffledDeck;
}

CodePudding user response：

@chux suggested using a Fisher-Yates shuffle. The "inside out" variant of Fisher-Yates allows the new deck shuffled as it is being copied from the old deck. Fisher-Yates works on a 1-D array, but a little bit of math can be used to modify it for use on a 2-D array:

char *(*shuffleDeck(char *(*deck)[13]))[13]
{
    char *(*shuffledDeck)[13] = malloc(4 * sizeof(*shuffledDeck));

    /* Use "inside out" Fisher-Yates shuffle, modified for 2-D array. */
    for(int i = 0; i < 4; i  ) {
        for(int j = 0; j < 13; j  ) {
            int b = randInt(i * 13   j   1);
            int a = b / 13;
            b -= 13 * a;
            /*
             * Note: a*13 b <= i*13 j,
             * so &shuffledDeck[a][b] <= &shuffledDeck[i][j]
             */
            if (a != i || b != j) {
                shuffledDeck[i][j] = shuffledDeck[a][b];
            }
            shuffledDeck[a][b] = deck[i][j];
        }
    }
    return shuffledDeck;
}

The b = randInt(i * 13 j 1) function call used above returns a random integer less than its parameter value. The argument i * 13 j 1 is one more than the number of cards shuffled so far. The a = b / 13; b -= a * 13; statements convert that random number to row a, column b. Crucially, a * 13 b <= i * 13 j, so the random position (a,b) is less than or equal to the position (i,j).

If the random position (a,b) is less than the current position (i,j), a card in the shuffled deck is copied from position (a,b) to position (i,j). Then the random position (a,b) in the shuffled deck has either never been set (if the random position is the same as the current position) or contains a stale copy. In either case, the card from the current position (i,j) from the unshuffled deck is copied to the random position (a,b) in the shuffled deck.

At the end, all cards from the unshuffled deck have been copied to random but unique positions in the shuffled deck.

Here is an implementation of the randInt function used above:

#include <stdlib.h>

/* Return random integer from 0 to n-1 (for n in range 1 to RAND_MAX 1u) */
int randInt(unsigned int n) {
    unsigned int x = (RAND_MAX   1u) / n;
    unsigned int limit = x * n;
    int s;
    do {
        s = rand();
    } while (s >= limit);
    return s / x;
}

Note that a simple rand() % n usually results in a slightly biased result. The randInt(n) function above uses rand() but produces an unbiased result.

The random number generator needs to be seeded by the srand function. That should be done only once in the program, e.g. from the main function:

#include <stdlib.h>
#include <time.h>

int main(void)
{
    srand(time(NULL));
    /* other stuff */
}

CodePudding user response：

First things first, you can remove the while loop and just call rand % value for row and column once. Second, you should replace those integers with constants. Why put 3 1-0 instead of NUM_ROWS? Third, you should only call srand once, otherwise you reset the seed for your random number generator.

Lastly, the logical error in your code. I think you're missing the function definition for you last tag, but it looks like the issue is that within swap, you swap the cards, and withing your shuffle_deck function you also set shuffled_deck equal to the shuffled value. Either swap the cards in deck in place (i.e. in swap), or make a copy and shuffle your copy.