Random string generator

Question

I decided to make a random string generator for fun, and it works for the most part. Since I'm an amateur at programming, I thought I would post the code to get some feedback on it, and hopefully improve any bad habits.

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

typedef unsigned int uint;

std::string randomString(uint l = 15, std::string charIndex = "abcdefghijklmnaoqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890")
{ 
    // l and charIndex can be customized, but I've also initialized them.

    uint length = rand() % l + 1; 
    // length of the string is a random value that can be up to 'l' characters.

    uint ri[15]; 
    /* array of random values that will be used to iterate through random 
      indexes of 'charIndex' */

    for (uint i = 0; i < length; ++i) 
        ri[i] = rand() % charIndex.length();
    // assigns a random number to each index of "ri"

    std::string rs = ""; 
    // random string that will be returned by this function

    for (uint i = 0; i < length; ++i) 
        rs += charIndex[ri[i]];
    // appends a random amount of random characters to "rs"

    if (rs.empty()) randomString(l, charIndex); 
    // if the outcome is empty, then redo the generation (this doesn't work, help?)
    else return rs;
}

int main()
{
    srand(time(NULL));
    std::cout << randomString() << std::endl;
}

Answer 1

Your comments are too long (line length).
Your code is also also seems untidy because of them. I would reduce the comments and have use self documenting code.

This seems to short. What happens if l is 2048?

uint ri[15];

I would use a std::vector<uint> here. You can initalize it with length so that it will be the correct size.

You do the iteration twice. Once to generate the random numbers. The second time to use the random number to access your set of characters. Why not do this in a single loop.

Here is your bug:

    if (rs.empty()) randomString(l, charIndex);
    else return rs;

Can't see it! Add braces. and use more vertical space. You should use {} around all sub blocks. It will help avoid issues in the long run.

    if (rs.empty()) {
        randomString(l, charIndex);
    }
    else {
        return rs;
    }

Do you see it now? You forgot the return in the first branch of the if statement. You only return a value in the else section. This means your class has undefined behavior.

Answer 2

First, the use of srand and rand in new C++ code is generally discouraged (e.g., see Stephan Lavavej's rand() Considered Harmful). You generally want to #include <random> and use the classes it defines instead.

Second, I'd always advise looking at the standard algorithms to see if they can help you out. std::generate_n is well-suited to the task at hand.

It's also worth keeping in mind that a default argument just tells the compiler what argument to pass if you don't specify another where you call the function. That argument still needs to be created and passed. In this case, you're specifying a default argument that may be relatively expensive to create. I'd prefer to avoid creating that string in the process of calling the function.

As far as the if (rs.empty()) part goes, I guess I'm a bit lost. You just wrote code to put data there--if the code works, it won't be empty. If the code doesn't work, calling it again doesn't seem likely to help a lot. If you're going to do something here, I'd (at most) add something like assert(!rs.empty());, so if the code didn't work, it blows up noisily. You might also want to add an assert at the beginning to assure that the length parameter that's passed in is greater than zero.

With these ideas in mind, the code could come out something like this:

#include <random>
#include <algorithm>
#include <iostream>
#include <iterator>
#include <string>

namespace {
std::string const default_chars = 
    "abcdefghijklmnaoqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890";
}

std::string random_string(size_t len = 15, std::string const &allowed_chars = default_chars) {
    std::mt19937_64 gen { std::random_device()() };

    std::uniform_int_distribution<size_t> dist { 0, allowed_chars.length()-1 };

    std::string ret;

    std::generate_n(std::back_inserter(ret), len, [&] { return allowed_chars[dist(gen)]; });
    return ret;
}

int main() {
    std::cout << random_string() << "\n";
    std::cout << random_string(10, "abcdefghijklmnopqrstuvwxyz");
}

If you really value speed highly, you could create two entirely separate overloads of random_string, each with its own code, and the one using the default character set would use it directly instead of passing it as a parameter.

At least in my opinion, however, this is probably going overboard, at least in the vast majority of cases--the expensive part was creating the string for the call, and this avoids that. Passing the string by reference isn't entirely free, but it's very inexpensive.

Answer 3

The code can be easily be improved in a number of ways. A few points:

1. Avoid ambiguous variable names. For example l might be better off named maxLength.
2. The code didn't compile on my machine because I needed #include <stdlib.h> for srand().
3. The usage of curly braces is still widely encouraged for single-line loops and if statements.

Btw, I found the comments useful :P Here's my modified version:

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

typedef unsigned int uint;

std::string randomString(uint maxLength = 15, std::string charIndex = "abcdefghijklmnaoqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890")
{ // maxLength and charIndex can be customized, but I've also initialized them.
    uint length = rand() % maxLength + 1; // length of the string is a random value that can be up to 'l' characters.

    uint indexesOfRandomChars[15]; // array of random values that will be used to iterate through random indexes of 'charIndex'
    for (uint i = 0; i < length; ++i) // assigns a random number to each index of "indexesOfRandomChars"
        indexesOfRandomChars[i] = rand() % charIndex.length();

    std::string randomString = ""; // random string that will be returned by this function
    for (uint i = 0; i < length; ++i)// appends a random amount of random characters to "randomString"
    {
        randomString += charIndex[indexesOfRandomChars[i]];
    } 
    return randomString;
}

int main()
{
    srand(time(NULL));
    std::cout << randomString() << std::endl;
} 

Answer 4

Firstly, as @Loki Astari pointed out, you can do just one iteration to both generate a random number and retrieve the character located at the index specified by the number. It means that you can get rid of the array entirely:

int randomIndex;
std::string rs = "";

for (uint i = 0; i < length; ++i) {
    randomIndex = rand() % charIndex.length();
    rs += charIndex[randomIndex];
}

But if (for whatever reason) you do want to keep a record of all the random indices, take note of @Loki Astari's suggestion that you should initialize a vector with size l instead. The general idea, which is applicable regardless of the programming language you use, is this:

In programming, as far as possible, avoid magic numbers (i.e., values with unexplained meaning) and hard-coded values. It will make refactoring far easier and less bug-prone. E.g., if you ever want to change the default value of l to something other than 15, you will not have to dig inside the method itself to change the array size of ri. It is inconvenient when you remember to do so, and troublesome when you don't. You want to write your code in such a way that you are not required to read through the entire method whenever you want to change your input values.

Second remark: Always favor using informative names over adding comments. E.g., why is the second parameter named charIndex? Would it not be more reflective of the purpose of the method if it is named, say, allowedCharacters instead? If I am a first-time caller of this method who has never read your code or comments, I would find the parameter name charIndex uninformative/misleading. But why should I have to read your code to understand what is going on? I just want to get a randomly generated string -- I don't care how you do it; I simply want to know what information I have to provide, and you should make things easier for me by choosing informative parameter names. Similarly, l would be better named as maxOutputLength, and so on.

Choosing good names will significantly cut down your need for writing comments. As far as possible, try to make your code read like regular English. This is how I would write your code:

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

typedef unsigned int uint;

std::string generateRandomString(uint maxOutputLength = 15, std::string allowedChars = "abcdefghijklmnaoqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890")
{
    uint outputLength = rand() % maxOutputLength + 1; 

    int randomIndex;
    std::string outputString = "";

    for (uint i = 0; i < outputLength; ++i) {
        randomIndex = rand() % allowedChars.length();
        outputString += allowedChars[randomIndex];
    }

    if (outputString.empty()) {
        return generateRandomString(maxOutputLength, allowedChars);
    }
    else {
        return outputString;
    }
}

I hope you agree that it reads better!