Random name generator outputting nothing

[koron123]

I made this random name generator as a practice exercise for arrays but I can't get it to output anything. I'm pretty sure the problem is in the last loop where the name variable is set.

#include <iostream>
#include <ctime>
#include <stdlib.h>

using namespace std;

main()
{
    string syllables[12][5], vowels[5], consonants[12];

    vowels[0] = "a";
    vowels[1] = "e";
    vowels[2] = "i";
    vowels[3] = "o";
    vowels[4] = "u";

    consonants[0] = "d";
    consonants[1] = "f";
    consonants[2] = "g";
    consonants[3] = "h";
    consonants[4] = "j";
    consonants[5] = "k";
    consonants[6] = "r";
    consonants[7] = "s";
    consonants[8] = "t";
    consonants[9] = "v";
    consonants[10] = "w";
    consonants[11] = "z";

    for(int x = 0; x < 60;)
    {
        if(x < 5)
            syllables[0][x] = vowels[x];

        else
            syllables[x / 5][x % 5] = consonants[x / 5] + vowels [x % 5];

        x++;

    }

    srand(time(0));
    int numberOfSyllables = rand() % 6 + 1;
    string chosenSyllables[6], name;

    for(int x = 0; x < numberOfSyllables;)
    {
        string randomSyllable = syllables[(rand() % 12 + 1) - 1][(rand() % 5 + 1) - 1];

        string chosenSyllables[x] = randomSyllable;

        x++;

    }

    for(int x = 0; x < numberOfSyllables;)
    {
        name = name + chosenSyllables[x];

        x++;

    }

    cout << name;

}

[keskiverto]

First a note:

7:6: warning: ISO C++ forbids declaration of 'main' with no type [-Wpedantic]

I'm now disappointed on online compiler, for it says nothing about line 50.

On line 50 you do declare and initialize a string array. It has same name as an another array that was declared on line 44, and thus it hides the line 44 array within the scope of the loop.



PS. Why do you increment the for loop counter inside the body of the loops?


PS2. You do use std::string, but don't include <string>. Do not rely on some library header including it for you.

PS3. C++ code should include <cstdlib>, not the <stdlib.h>.

[icy1]

- the math looks good -- no issues with out of range
- line 50 is straight-up syntax error
- lines 32-33 are a bit awkward -- just set the first row of syllables to the vowels manually and then iterate on indices 5..59
- more idiomatic to use "i" variable for indices and change it in the third section of for
- "name" could be created without needing chosenSyllables array -- chosenSyllables' usefulness is limited
- separate the concerns. main() is getting big -- perhaps one method to build up the syllables table, and another to generate names

Simplified/refactored with generation of 50 names:

#include <ctime>
#include <iostream>
#include <string>

using namespace std;

void BuildTable(string syllables[12][5])
{
    string vowels[5], consonants[12];
    syllables[0][0] = vowels[0] = "a";
    syllables[0][1] = vowels[1] = "e";
    syllables[0][2] = vowels[2] = "i";
    syllables[0][3] = vowels[3] = "o";
    syllables[0][4] = vowels[4] = "u";

    consonants[0] = "d";
    consonants[1] = "f";
    consonants[2] = "g";
    consonants[3] = "h";
    consonants[4] = "j";
    consonants[5] = "k";
    consonants[6] = "r";
    consonants[7] = "s";
    consonants[8] = "t";
    consonants[9] = "v";
    consonants[10] = "w";
    consonants[11] = "z";

    for(int i = 5; i < 60; ++i)
        syllables[i / 5][i % 5] = consonants[i / 5] + vowels [i % 5];
}

string GenerateName(string syllables[12][5])
{
    // 2-6 syllables, or 2-12 characters
    int num_syllables = rand() % 5 + 2;
    string name;

    while(num_syllables--)
        name += syllables[rand() % 12][rand() % 5];

    return name;
}

int main()
{
    srand(time(0));
    string syllables[12][5];
    BuildTable(syllables);

    int total_names = 50;
    while(total_names--)
        cout << GenerateName(syllables) << endl;

    return 0;
}

Edit: added <ctime>, needed by Windows to see time() function -- see comments below. Also changed the loops that don't need any indexing to while with decrement, which seems cleaner

Some of the sample output (50 lines total), running at https://repl.it/@icy_1/IdealWoozyButton

goje
vasa
sie
ugehazota
zekowige
ruwireuri
rirurojovofa
rozufaheheo
fiehesa
goage

[Ganado]

@icy1
You should add #include <cstdlib> and #include <ctime> to your program, for completeness.

[icy1]

@Ganado -- it compiled for me in two online compilers (repl.it and the one from this site)

Out of curiosity, I opened up VS2015 and, indeed, with these two headers included it liked srand() but had no love for time() function. I began opening the include documents to put together the inclusion chain:

both <string> and <iostream> include <istream>
<istream> includes <ostream>
<ostream> includes <ios>
<ios> includes <xlocnum>
<xlocnum> includes <climits>, <cmath>, <cstdio>, <cstdlib>, <streambuf>

So since <cstdlib> gets pulled in, I can see why the srand() works, but I can't explain how the online compilers got the time() function working.

Edit: appears explicit inclusion of <ctime> may be necessary on windows, but for linux it gets automagically included with <iostream> to help with pthreads, as per this answer https://stackoverflow.com/questions/8016646/why-does-iostream-include-time-h

Updated my answer to #include <ctime> .

[tpb]

@icy1, What the implementations do is not the point. The standard doesn't require that <cstdlib> be included by <string> or <iostream>, so if you are using something from <cstdlib> you should include it.

Also, the time_t value returned by time() should be cast to unsigned for srand:

srand(unsigned(time(nullptr));

Better yet, use the <random> facilities.

[icy1]

@tpb Guess I got away with a finesse there. For correctness/readability, I suppose including the header would help in some situations. If I'm using a std::pair in concert with other structures (vector, map, etc), however, in practice I'm rarely going to explicitly include <utility>, because that header gets pulled in almost everywhere.

tpb wrote:

Also, the time_t value returned by time() should be cast to unsigned for srand:

Politely disagree. As far as I know, the implicit casting of int to unsigned int is well-defined, so this shouldn't be an issue. The function could return a (time_t)(-1) on error, for example, but a function that takes unsigned would have no problem converting it to 2³² - 1 or so.

<random> is recommended, for sure. A random_device 's output is very similar to that of rand(), so the user experience need not suffer ;D

[tpb]

icy1 wrote:

As far as I know, the implicit casting of int to unsigned int is well-defined, so this shouldn't be an issue.

My point was that the type of time_t is unspecified (technically it doesn't even need to be an integer). On modern systems it's almost certainly a 64-bit integer (signed or unsigned). But srand takes an unsigned int, which is likely 32-bits. I don't know why compilers don't complain about that.

(EDIT: removed my stupid "could even be a struct" comment. Duh!)

Also, I believe that random_device is best used to seed a PRNG since the random device is not guaranteed to be able to provide values at the rate you need them (or does it fall back on a PRNG itself?).

Here's a simple way of using <random>:

#include <random>
namespace RND {
    std::default_random_engine re(std::random_device{}());
    std::uniform_int_distribution<> dist;
    void set(int lo, int hi) {
        std::uniform_int_distribution<>::param_type pt(lo, hi);
        dist.param(pt);
    }
    void set(int hi) { set(0, hi); }
    int rnd() { return dist(re); }
    int rnd(int lo, int hi) { set(lo, hi); return rnd(); }
    int rnd(int hi) { set(hi); return rnd(); }
}

#include <iostream>
#include <iomanip>

int main() {
    using namespace std;    

    RND::set(1,6);
    for (int i = 0; i < 30; i++)
        cout << RND::rnd() << ' ';    
    cout << "\n\n";

    RND::set(99);  // 0 to 99, inclusive
    for (int i = 0; i < 100; i++)
        cout << setw(2) << RND::rnd() << ' ';
    cout << '\n';
}