Random Number Generation of Combination of six numbers

Question

I am doing a Pseudo Random Number Generator that creates 100 combinations of 6 numbers from the range 1 to 56 with no repetition and then it saves them on a text file. Like this:

33 28 46 7 30 57 
15 29 43 41 16 21 
11 43 7 18 31 25 
36 32 19 42 47 33 
46 13 14 1 28 25 
33 14 55 43 29 13 
30 14 12 45 46 32 
56 31 54 32 20 21 
10 52 40 57 31 14 
28 44 15 47 57 45 
...

This is my code, but I feel that it is a little bit too big, specially the part when is checking no repetitions of the numbers

// Including C Standard Libraries
#include <stdint.h>  
#include<stdio.h> 
#include<stdlib.h> 
#include<time.h> 

int main() 
{ 
    // Initialing Random Generation C Library
    srand(time(NULL)); 

    // Variables for PRNG Loop
    uint16_t i;  
    uint8_t j; 

    uint8_t x[6] = {0,0,0,0,0,0};

    // Opening File to save Results
    FILE *fp;
    fp = fopen("combinations.txt", "w+"); 

    // PRNF Loop
    for(i = 0; i<100; i++)  // Numbers of Combinations 
    {
        for(j = 0; j<6; j++)  // Number of Elements of the Combinations
        {
            x[j] = rand() % 57 + 1;  // Generating Random Number

            // Avoiding Repetition of Numbers
            if ( j==1)
            {
                while(x[1] == x[0])
                {
                    x[1] = rand() % 57 + 1;
                }
            }
            if ( j==2)
            {
                while(x[2] == x[0] || x[2] == x[1])
                {
                    x[2] = rand() % 57 + 1;
                }
            }
            if ( j==3)
            {
                while(x[3] == x[0] || x[3] == x[1] || x[3] == x[2] )
                {
                    x[3] = rand() % 57 + 1;
                }
            }
            if ( j==4)
            {
                while(x[4] == x[0] || x[4] == x[1] || x[4] == x[2] || x[4] == x[3] )
                {
                    x[4] = rand() % 57 + 1;
                }
            }
            if ( j==5)
            {
                while(x[5] == x[0] || x[5] == x[1] || x[5] == x[2] || x[5] == x[3] || x[5] == x[4] )
                {
                    x[5] = rand() % 57 + 1;
                }
            }


            fprintf(fp, "%d", x[j]);  // Saving Random Number in File
            fprintf(fp, " "); 
        } 

        fprintf(fp, "\n");  // Saving Newline  

        for (int i = 0; i < 6; ++i)
        {
            x[i] = 0;
        }    

    }

    fclose(fp);
} 

Is there a way to simplify the code?

Answer 1

You can simplify your uniqueness test by iterating the current set with a loop where for all all preceding values you test for uniqueness from the current value:

            // Generate a unique random number
            bool unique = false ;
            while( !unique )
            {
                x[j] = rand() % RANGE + 1;  // Random 1 to 56

                // Test uniqueness from all preceding values in 
                // the set (x[0] is always unique)
                unique = true ;
                for( int k = 1; unique && k < j; k++ )
                {
                    unique = x[k] != x[j] ;
                }
            }

Note that if the set length is large, then the scan all preceding values approach may be sub-optimal, but in this case seems reasonable and is simple.

Since you (kind of) asked, the code can be further improved by (for example - code incorporating these follows):

• Eliminate magic numbers
• Initialisation of the first member of an array, initialises all remaining members to zero.
• Initialise on instantiation (rather then define and separately assign).
• Instantiate at narrowest possible scope - not all at the top of the function
• Don't open a file for access types you are not using ("w" not "w+")
• Error check I/O functions
• Do not use stdint types unnecessarily - arithmetic operations frequently result in implicit casts to int. Use int unless there is good reason not to to avoid surprises. Good reasons to use stdint include conformance to some file format or comms protocol, or to match a register width in a device driver. Not here.
• Test and fix bugs (rand() % 57 + 1) for example.
• Avoid repetition - use a loop (the uniqueness test in this case).
• Only say something useful in comments. If the code is self documenting, keep quiet. If it is not obvious, explain - do not simply repeat what anyone can see from the code (e.g. "Generating Random Number")
• Do not write comments in present continuous tense (no 'ing verbs). OK, not really an improvement, just something I find annoying ;-). The source code describes something the program will do, not what it is doing.
• Do not write extraneous space after last value on each line.
• Remove unnecessary code - e.g. the loop at the end zeroing x[]
• If you promise to return a value, return a value.

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

#define SET_LENGTH 6    // Number of values in each set
#define SET_COUNT 100   // Number of sets
#define RANGE 56        // Range of each value 1 to RANGE

int main() 
{ 
    // Initialing Random Generation C Library
    srand(time(NULL)); 

    int x[SET_LENGTH] = {0};

    // Open file
    FILE* fp = fopen( "combinations.txt", "w" ) ; 
    if( fp != NULL )
    {
        // For each random number set...
        for( int i = 0; !ferror( fp ) && i < SET_COUNT; i++ )
        {
            // For each number in the set...
            for( int j = 0; j < SET_LENGTH; j++) 
            {
                // Generate a unique random number
                bool unique = false ;
                while( !unique )
                {
                    x[j] = rand() % RANGE + 1;  // Random 1 to 56

                    // Test uniqueness from all preceding values in 
                    // the set (x[0] is always unique)
                    unique = true ;
                    for( int k = 1; unique && k < j; k++ )
                    {
                        unique = x[k] != x[j] ;
                    }
                }

                // Write value to file. 
                // Space separated values with newline end.
                fprintf(fp, "%d%c",x[j], 
                        j < SET_LENGTH - 1? ' ' : '\n' ) ;
            } 
        }
    }

    fclose(fp);

    return 0 ;
} 

Not "improved" here is the issue of "random bias" that occurs when for rand() % n, n is not a factor of RAND_MAX+1. If the randomness is critical, you may want to consider that.

Answer 2

You can simplify by using an array of used values.

For example add:

uint8_t used[56] = {0};

between the first and second for loops

Then select your value this way:

do
{
  x[j] = rand() % 56 + 1;  // Generating Random Number
} while (used[x[j]-1] != 0);
used[x[j]-1] == 1;

When a value is used, we put a 1 at that location in the array. When looking for a value, we loop until we find an empty slot.

To use the shuffle array instead, you would do this.

Create an array with all values before the first for-loop:

uint8_t shuffle[56] = { 1, 2, 3, 4, 5, 6, 7, /* ... */ 54, 55, 56 };

Then choose your values this way:

uint8_t pos = rand() % (56-j);  // Generating Random Number Location
x[j] = shuffle[pos];  // Selecting Random Number
shuffle[pos] = shuffle[55];
shuffle[55] = x[j];

Answer 3

Why not something like:

#define NUM_RANDOM_NUMBERS 56

// Include all possible numbers.
int range[NUM_RANDOM_NUMBERS];

// Record current size of the array (this could be dynamic if we don't know the starting size...
int curSize = NUM_RANDOM_NUMBERS;

int getRandom()
{
    // If we are called with curSize at 0 or less, this is an error condition.  We have no numbers to return.
    assert(curSize > 0);

    // Get a number from our table and place it into a return Value.
    int index = random() % curSize;
    int retValue = range[index];

    // Move all the values above the index over the swapped out value.
    // memmove would probably be more efficient.
    for(int i = index; i < curSize - 1; i++) 
        range[i] = range[i+1];

    // Reduce the size of our pool of numbers.
    curSize--;

    // Return the value;
    return retValue;
}

int main()
{
    // Load the range pool with all the possible numbers.
    for(int i=0; i<NUM_RANDOM_NUMBERS; i++)
    {
        range[i] = i+1;    
    }

    // Now retreive them in random order.
    for(int i=0; i<NUM_RANDOM_NUMBERS; i++)
    {
        printf("%d \n", getRandom());
    }

    return 0;
}

Notice that I am calling random a minimum numbers of times with this. The longest part of this runtime is probably the moving of the memory in the range pool, but it will run a deterministic amount of time.