Generating 1000 random numbers

Question

I have got the following C code and I feel it is crazy stupid and there must be a better way to do this, but just can't think of one and have yet to learn algorithms.

/*generates 1000 random numbers, lists frequency*/
#include <stdio.h>
#include <stdlib.h>
#include <time.h>

int main(void)
{
    int count, generated_num ;
    char nums[10] = {0};
    srand(time(NULL));
    for (count = 0; count < 1000; count++) {
        generated_num = rand() % 10 + 1;
        nums[generated_num-1] += 1;
    }

    for (count = 0; count < 10; count++) {
        printf("%d occured %d times\n", count+1, nums[count]);
    }

    return 0;
}

How do I learn to write code that is both efficient in speed but also efficient in the sense it is readable/modifiable/not-a-mess?

Answer 1

Speed and efficiency

Just don't worry about it. You need to generate 1000 random numbers, and do a little bit of accounting work. Any reasonable solution, such as yours, will perform similarly.

Types

Why use an array of char to keep track of the number of occurrences? You're putting 1000 random numbers into 10 bins. What if one of those bins gets more than 127 items? You would get an overflow.

In summary, those bins should be of type int. Don't even think about skimping on a few bytes of storage, if you want your code to be not-a-mess.

Naming

nums actually stores counts. count, as you've used it in the second loop, actually refers to the generated numbers. I suggest the following renaming:

• nums → bins
• count → i

Answer 2

Overall, this code doesn't look too bad, but there are a few places that can be improved.

• This line:

  char nums[10] = {0};
  

  doesn't initialize all the elements of nums (I think some compilers will default-initialize local variables in certain build modes, but you shouldn't rely on that). Use a for loop or the standard library function memset() to do that:

  #include <string.h>
  ...
  memset(nums, 0, sizeof(nums));
  

  As 200_success notes, using a char as the base type of the array leaves you prone to overflow if you happen to randomly generate more than 127 of a given number (assuming your char has 8 bits, which it probably does). For counting things, it's generally a good idea to use unsigned quantities if you know your count will never be negative, so nums should be declared as:

  unsigned int nums[10];
  

• In this code, you're adding 1 to generated_num, then subtracting 1 when you use it on the next line as an index:

  generated_num = rand() % 10 + 1;
  nums[generated_num-1] += 1;
  

  Leave that out:

  generated_num = rand() % 10;
  nums[generated_num] += 1;
  

  You could conceivably collapse the line into one, saving yourself the declaration of generated_num at the start of main:

  nums[rand() % 10] += 1;
  

  You could also use the pre- or post-increment operators to update the count for the generated number:

  nums[rand() % 10]++;
  

• You use the magic number 10 (the size of the nums array) in several places in the code. You should declare that in a constant, one of:

  // Number of bins in the histogram of random numbers.
  static const size_t  NUM_BINS = 10;
  // ... or ...
  #define NUM_BINS  10
  // ...
  char  nums[NUM_BINS]; // etc.
  

  That way, if you decide you want 11 or 12 or 37 random numbers, you only have to change it in one place.

Answer 3

Others have already pointed out the some things about the coding style. Following their advice, you will be on par with more professional C code. Unfortunately, that will still not give you good random numbers.

Now, depending on what you want to do, that might not be too bad, but I still wanted to point it out. The C function rand is (are) a very very bad random number generator. Given only few (full) outputs, one can already deduce some information about the next numbers.

Also srand(time(NULL)); is a relatively bad seed. Running the program on two different computers but at the same time (in seconds!) will result in identical numbers. Running the program in short succession will result in similar numbers.

Furthermore, rand() % 10 will not create evenly distributed numbers. RAND_MAX is usually not divisible by 10, so the lower numbers will be more likely than 8 or 9. This is even more pronounced when you want to draw numbers from a larger range. RAND_MAX is often only of the order of 32k, so drawing numbers in a range of 1-2000 or similar will already be significantly skewed towards the lower numbers.

tl;dr If the random numbers are not important (e.g. a computer game), everything should be fine. If you ever want to do numerics or cryptography, then do not use rand().

---

edit: It is somewhat difficult to get good random numbers in C...

If you want to do cryptography then read /dev/random on linux systems (note that under OSX /dev/random is not cryptographically secure!) or call CryptGenRandom on windows systems. Both will deliver 'true' random numbers. For numerics you can (for example) use a Mersenne Twister (creative-commons code on the wikipedia) and seed it with true random numbers or with (preferably a hash of) the current time in nanoseconds.

In either case you still have to do some work to get a uniform distribution over any given range. A simple but ineffective method simply draws a new random number when needed

/// @arg (*random) - a function providing (pseudo) random unsigned numbers
/// @arg randMax   - the largest number that can be returned by (*random)
/// @arg rangeMax  - the largest desired number
/// @returns a uniformly random number in [0, rangeMax]
unsigned uniform(unsigned (*random)(void), unsigned randMax, unsigned rangeMax) {
    if (rangeMax <= randMax) {
        // the following assumes, that both maxima are smaller than 2^32-1
        randMax += 1;
        rangeMax += 1;
        unsigned cutoff = (randMax / rangeMax) * rangeMax;
        unsigned result = random();
        while (result >= cutoff) {
            result = random();
        }
        return result % rangeMax;
    } else {
        // more complicated code or error
    }
}

If you do not want to throw away any random numbers that you sampled you will have to manage some internal state. This quickly becomes complicated and is beyond the scope of this answer.

Answer 4

This is actually pretty concise for a simple program, but I have two additional things:

1. Declare count right before the loop since it's a loop counter variable. You shouldn't just "list" every variable at the top, as it may make it harder to keep track of them if they're no longer used.

   For loop counters in general, this is applicable to pre-C99. If you have a newer version, thereby allowing you to initialize the counter variable within the loop statement, then do that instead.

2. srand() can be moved to the very top of main() to help keep it separate, although this not really a huge deal. The important thing in general, though, is to only call it in main() in more modular programs (to avoid resetting the seed).