Randomly pick a first name from a file

Question

I have a very easy question for you guys out there. It's been a while since I took my programming courses (switched majors) but am trying to get my feet wet again.

Objective:

I will be creating a program that will output 10 random full names (and have it be different every time the program is run). Before I complete it, I'd like some feedback on the first iteration, which just chooses random first names.

Info:

I have a file called firstnames.txt, containing a list of 10 first names.

Premise:

I'm a beginner and I/O really throws me. I feel this is a basic enough program for me to learn from, and with your advice, I'll be able to copy/paste the same code to get a last name.

Thanks!

Here's what I have so far:

#include <windows.h>
#include <cstdlib>
#include <iostream> 
#include <string> //random headers because it's been awhile :P
#include <fstream>
#include <time.h>

using namespace std;

int main()
{
//Variable declarations
int a = 0;
int randNum;

ifstream myFile;
myFile.open("firstnames.txt");
string names[10]; // holds the 10 names in the file.
string randomNames[10];// holds the randomly generated names FROM the file.

while (myFile.good())
{
    getline(myFile, names[a]); // reads the names from the text file, into an array named "names[]."
    a++;
}
for (int i = 0; i < 10; i++)  // makes this program iterate 10 times; giving you 10 random names.
{
    srand( time(NULL) ); // seed for the random number generator.
    randNum = rand() % 10 + 1; // gets a random number between 1, and 10.
    randomNames[i] = names[randNum];
}
for (int i = 0; i < 10; i++)
{
    cout << randomNames[i] << endl; // outputs all 10 names at once.
}

myFile.close();
return EXIT_SUCCESS;
}

My firstnames.txt looks like:

Ryan
John
Daniel
Gavin
Frank
Danielle
Lucy
Sarah
Katie
Nicole

My lastnames.txt will look similar:

Johnson
Lewis
Walker
Robinson
Wood
Thompson
White    
Watson
Jackson
Wright

(Another question I guess would be is the way I formatted the names (each name on a new line) a bit over complicated in the text file? Like would it be better/shorter to just separate each name by a space or something?)

Thanks again!

Answer 1

1. Well, at least you know what's wrong here:

   #include <string> //random headers because it's been awhile :P
   

   You actually need: <iostream>, <string>, <ctime> and <cstdlib>.

2. You are using using namespace std;.
   That's a bad idea, read "Why is “using namespace std;” considered bad practice?".

3. It's safer and more flexible to use a std::vector, though admittedly insignificantly less efficient.

   std::vector<std::string> names;
   

4. There's no good reason why you first generate all the data and safe it in memory just to dump it all afterwards in one go.

5. There's a reason while(!found_end) {read_some_more; process_it;} doesn't work:
   You only know you have reached the end after you tried to read beyond it.

   for (std::string line; std::getline(myFile, line) && !line.empty(); )
       names.push_back(std::move(line));
   

6. If you extract the code to read names from a file into a function your code does not only get cleaner, you can also reuse it.

7. You are re-seeding the old toy-RNG repeatedly in a relatively tight loop. Can you see where that might not be all that great for randomness?

   Anyway, C++11 provides far better RNGs in <random>. See "Seed std::mt19937 from std::random_device" for how to properly seed one.

8. Ending main with return EXIT_SUCCESS; is equivalent to ending it with return 0; which can be omitted (but only for main).

Following my suggestions lets you loose <ctime> and <cstdlib> but adds <vector>, <utility>, <functional>, <random> and <algorithm>.

#include <iostream>
#include <string>
#include <vector>
#include <utility>
#include <functional>
#include <random>
#include <algorithm>

static std::vector<std::string> read_names(const std::string& filename)
{
    std::vector<std::string> names;
    std::ifstream file(filename);
    for(std::string name; std::getline(file, name) && !name.empty(); )
        names.push_back(std::move(name));
    return names;
}

static const std::string& choose_any(
    std::mt19937& rng,
    const std::vector<std::string>& names
) {
    return names[std::uniform_int_distribution<size_t>(0, names.size() - 1)(rng)];
}

static std::mt19937 ProperlySeededRandomEngine()
{
    std::mt19937::result_type random_data[std::mt19937::state_size];
    std::random_device source;
    std::generate(std::begin(random_data), std::end(random_data), std::ref(source));
    std::seed_seq seeds(std::begin(random_data), std::end(random_data));
    std::mt19937 seededEngine (seeds);
    return seededEngine;
}

int main()
{
    auto firstnames = read_names("firstnames.txt");
    auto lastnames = read_names("lastnames.txt");
    auto rng = ProperlySeededRandomEngine();
    for(int i = 0; i < 10; ++i)
        std::cout << choose_any(rng, firstnames) << ' '
            << choose_any(rng, lastnames) << std::endl;
}

Answer 2

Bug

Note: I am assuming name[a] is valid in the following discussion.
This is actually a bug:

while (myFile.good())
{
    getline(myFile, names[a]);
    a++;
}

It is also an anti-pattern (in all languages). The problem is that good() is not doing what you think. When you read a file it stays good until AFTER you read past the end of file. The trouble the last successful read will read UP-TO the end of file (but not past it). So when you have read the last line of the file (ie there is no data left to read) good() still returns true because you have not read past the end of the file. This means you enter the loop and try and read a name (this will fail) and set eof to true and thus good() to false but you still increment a.

To fix this you should always test that the read operation worked.

while (myFile.good())
{
    if (getline(myFile, names[a])) {
        a++;
    }
}

But the normal way to do the check is to put the read in the while condition.

while (getline(myFile, names[a]))
{
    a++;
}

But we can improve this even more. Since you are simply loading this into a vector and each name is separated with white space you simply do this in the declaration of the vector (or an array).

std::vector<std::string> names{std::istream_iterator<std::string>(myFile),
                               std::istream_iterator<std::string>()};

This initializes the vector using two iterators. In the above statement we use iterators that read std::string from a stream using operator>>.

Random

First.....

srand( time(NULL) );

You should only seed the random generator once in the application. So do this as you enter main and never again. If your code is quick you seed the generator with the same number again and again. Which means that rand() will return the same random number.

This random number generator is old and should not be used in anything but trivial applications that don't need good randomness.

randNum = rand() % 10 + 1; // gets a random number between 1, and 10.
randomNames[i] = names[randNum];

The modern version of random number generation is detailed here:

And used like this:

// Set this up only once.
std::default_random_engine generator;
std::uniform_int_distribution<int> distribution(1,10); // you want a range 1 -> 10

// You call this many times.
int randomIndex = distribution(generator); 

If you want to keep using your original code. Then you should know that the way you are doing it also biases some numbers slightly more than others.

rand() returns a number in the range [0, RAND_MAX]. If this number is not an exact multiple of your divisor (e.g 10). Then the numbers greater than RAND_MAX % 10 have a slightly lower chance.

Let me illustrate. Assume RAND_MAX is 65534. Then the numbers 1..4 have a slightly higher chance than the numbers 0,5..9 If we count up all the potential results from modding by 10.

1  ->  6554
2  ->  6554
3  ->  6554
4  ->  6554
5  ->  6553
6  ->  6553
7  ->  6553
8  ->  6553
9  ->  6553
0  ->  6553 chances.
      ------
      65534

So chances of a 1 are: 1/6554 while chances of a 9 are 1/6553

To compensate for this you should reject values greater or equal than 65530.

static const maxRand = RAND_MAX / max * max;
int randNum;
do {
    randNum = rand();
}
while (randNum >= maxRand);
randNum = randNum % max + 1;
randomNames[i] = names[randNum];

Array access

Indexing into arrays starts at 0 (not 1). Arrays/Vectors and containers in C all use a 0 based indexing scheme. So if you have an array with 10 elements then the valid indexes are [0..9].

So this is going to be off by one.

randNum = rand() % 10 + 1;
randomNames[i] = names[randNum];

randNum starts at 1 and ends at 10. So its range is off by one.

RAII

That's way to much work.

ifstream myFile;
myFile.open("firstnames.txt");
myFile.close();

The file stream constructor takes a name as a parameter. Also the destructor of the file object will automatically close the file when the object goes out of scope. So we can change the above three lines to.

ifstream myFile("firstnames.txt");

Done.

Answer 3

Headers

I had to remove

#include <windows.h>

as that header doesn't exist on my system. As it still compiled without warnings, I conclude it isn't needed.

Prefer to include <ctime> rather than <time.h> in new code, as that will give you names in the std namespace. That brings us nicely to:

Avoid using namespace

It's much clearer to import only those names you really need, at the scope you need them. See Why is "using namespace std;" considered bad practice?.

Don't hard-code the number of inputs

Instead of this fixed-size array:

std::string names[10]; // holds the 10 names in the file.

You can use a std::vector to hold the names:

std::vector<std::string> names = read_names("firstnames.txt");

I'm jumping ahead here, by using a function to read the names:

Use a function for repeated code

As we'll want to read a file of first names and a file of last names, let's declare a function that can be called once for each file:

std::vector<std::string> read_names(const char *filename);

We can use a while loop to read the names:

std::vector<std::string> read_names(const char *filename)
{
    std::ifstream in{filename};
    std::vector<std::string> names;

    std::string name;
    while (getline(in, name))
        names.push_back(std::move(name));

    return names;
}

Note that we don't explicitly close in - its destructor will do that for us as it goes out of scope. The std::move() in there is a tiny performance tweak to reduce the amount of copying, as we don't use the contents of name after passing it to push_back().

BUG: Don't add 1 to the array index

We get need a 0-based index into the array of names, so adding 1 will make us read an out-of-range value when we get the highest value:

randNum = rand() % 10 + 1; // gets a random number between 1, and 10.
randomNames[i] = names[randNum]; // but should be 0..9

Don't store the results

There's no reason to store the output strings in randomNames - instead, we can write each one to std::cout as soon as it has been generated:

for (int i = 0;  i < 10;  ++i) {
    int first = rand() % first_names.size();
    int last = rand() % last_names.size();
    std::cout << first_names[first] << " " << last_names[last] << '\n';
}

Use a better random source and a more uniform distribution

The standard <random> header provides the modern facility:

std::mt19937 rng{std::random_device{}()};
std::uniform_int_distribution<size_t> first_dist{0, first_names.size() - 1};
std::uniform_int_distribution<size_t> last_dist{0, last_names.size() - 1};

for (int i = 0;  i < 10;  ++i) {
    int first = first_dist(rng);
    int last = last_dist(rng);
    std::cout << first_names[first] << " " << last_names[last] << '\n';
}

---

Working replacement code

#include <fstream>
#include <iostream>
#include <random>
#include <string>
#include <vector>

std::vector<std::string> read_names(const char *filename)
{
    std::ifstream in{filename};
    std::vector<std::string> names;

    std::string name;
    while (getline(in, name))
        if (!name.empty())
            names.push_back(std::move(name));

    return names;
}

int main()
{
    const auto first_names = read_names("firstnames.txt");
    const auto last_names = read_names("lastnames.txt");

    std::mt19937 rng{std::random_device{}()};
    std::uniform_int_distribution<size_t> first_dist{0, first_names.size() - 1};
    std::uniform_int_distribution<size_t> last_dist{0, last_names.size() - 1};

    for (int i = 0;  i < 100;  ++i) {
        int first = first_dist(rng);
        int last = last_dist(rng);
        std::cout << first_names[first] << " " << last_names[last] << '\n';
    }
}

---

Further enhancements

You might want to consider using std::wfstream for input and std::wcout for output, with std::wstring in between, to support names in more scripts. That will take you into some interesting areas of i18n/l10n - what is a 'first' and 'last' name in cultures other than your own?

You could reduce the remaining duplication by creating a class that encapsulates the random selection from a std::vector.

Instead of hard-coding the number of output lines, you could allow the user to specify the amount of output (e.g. as a command-line argument). Or you could just produce output indefinitely, and expect the user to stream to head or similar to terminate the output, but that's probably a bit rude.

Answer 4

Obviously your example isn't complete, since you're not reading the last names yet. But here's a few observations:

• Instead of arrays of strings, use std::vector<std::string>. After you read in a name, use .push_back(name) to add it to the end of the vector.
• By using a vector, you can index with .at(idx) instead of using the subscripts ([]). The .at() method will range-check and throw an exception when you try to access outside the array (like you could do in your random expression, which doesn't need to add 1.)
• In a real program, you would create a function that returns an array of names from a file. The file would be managed inside the function so it wouldn't be open during the entire operation of your program. Right?