Return to Answer

Edit July 31 23:07UTC

Why would I write the function to take a buffer instead of allocating the string inside the function?

Returning dynamically allocated strings works fine. And if the memory is later freed there is no problem. But writing this sort of function is a great way to leak memory, for example if the caller doesn't know the memory must be freed or forgets to free memory, or even if he frees it in the main path but forgets to free it in other paths etc.

Memory leaks in a desktop applications might not be fatal, but leaks in an embedded system will lead eventually to failure. This can be serious, depending upon the system involved. In many embedded systems, dynamic allocation is often not allowed or is at least best avoided.

Although it certainly is common not to know the size of strings or buffers at compile time, the opposite is also often true. It is often possible to write code with fixed buffer sizes. I always prefer this option if possible so I would be reluctant to use your allocating function. Perhaps it is better to add a wrapper to a non-allocating function for those cases where you really must allocate dynamically (for example when the random string has to outlive the calling context):

char* rand_string_alloc(size_t size)
{
     char *s = malloc(size + 1);
     if (s) {
         rand_string(s, size);
     }
     return s;
}

Your function is nice but has a few issues, the main one being that it should not call srand. srand should be called elsewhere (eg in main) just once. This seeds the random number generator, which need only be done once.

A minor issue is that string is badly named - charset might be better. It should be const and you then need not call strlen to find its length sizeof charset -1 is enough. For me, randomString is an unnecessarily long name.

On failing to allocate memory for the string, I would prefer to see a NULL return than an exit. If you want an error message, use perror, but perhaps in the caller, not here. I would be inclined to avoid the possibility of such an error but passing in the buffer and its length instead of allocating.

Some minor points: sizeof(char) is 1 by definition and using short for key is pointless - just use int. Also key should be defined where it is used and I would leave a space after the ; in the for loop definition.

Note also that using rand() % n assumes that the modulo division is random - that is not what rand promises.

Here is how I might do it:

static char *rand_string(char *str, size_t size)
{
    const char *charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJK...";
    if (size) {
        --size;
        for (size_t n = 0; n < size; n++) {
            int key = rand() % (int) (sizeof charset - 1);
            str[n] = charset[key];
        }
        str[size] = '\0';
    }
    return str;
}

Your function is nice but has a few issues, the main one being that it should not call srand. srand should be called elsewhere (eg in main) just once. This seeds the random number generator, which need only be done once.

A minor issue is that string is badly named - charset might be better. It should be const and you then need not call strlen to find its length sizeof charset -1 is enough. For me, randomString is an unnecessarily long name.

On failing to allocate memory for the string, I would prefer to see a NULL return than an exit. If you want an error message, use perror, but perhaps in the caller, not here. I would be inclined to avoid the possibility of such an error but passing in the buffer and its length instead of allocating.

Some minor points: sizeof(char) is 1 by definition and using short for key is pointless - just use int. Also key should be defined where it is used and I would leave a space after the ; in the for loop definition.

Note also that using rand() % n assumes that the modulo division is random - that is not what rand promises.

Here is how I might do it:

static char *rand_string(char *str, size_t size)
{
    const char charset[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJK...";
    if (size) {
        --size;
        for (size_t n = 0; n < size; n++) {
            int key = rand() % (int) (sizeof charset - 1);
            str[n] = charset[key];
        }
        str[size] = '\0';
    }
    return str;
}

Your function is nice but has a few issues, the main one being that it should not call srand. srand should be called elsewhere (eg in main) just once. This seeds the random number generator, which need only be done once.

A minor issue is that string is badly named - charset might be better. It should be const and you then need not call strlen to find its length sizeof charset -1 is enough. For me, randomString is an unnecessarily long name.

On failing to allocate memory for the string, I would prefer to see a NULL return than an exit. If you want an error message, use perror, but perhaps in the caller, not here. I would be inclined to avoid the possibility of such an error but passing in the buffer and its length instead of allocating.

Some minor points: sizeof(char) is 1 by definition and using short for key is pointless - just use int. Also key should be defined where it is used and I would leave a space after the ; in the for loop definition.

Note also that using rand() % n assumes that the modulo division is random - that is not what rand promises.

Your function is nice but has a few issues, the main one being that it should not call srand. srand should be called elsewhere (eg in main) just once. This seeds the random number generator, which need only be done once.

A minor issue is that string is badly named - charset might be better. It should be const and you then need not call strlen to find its length sizeof charset -1 is enough. For me, randomString is an unnecessarily long name.

On failing to allocate memory for the string, I would prefer to see a NULL return than an exit. If you want an error message, use perror, but perhaps in the caller, not here. I would be inclined to avoid the possibility of such an error but passing in the buffer and its length instead of allocating.

Some minor points: sizeof(char) is 1 by definition and using short for key is pointless - just use int. Also key should be defined where it is used and I would leave a space after the ; in the for loop definition.

Note also that using rand() % n assumes that the modulo division is random - that is not what rand promises.

Here is how I might do it:

static char *rand_string(char *str, size_t size)
{
    const char *charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJK...";
    if (size) {
        --size;
        for (size_t n = 0; n < size; n++) {
            int key = rand() % (int) (sizeof charset - 1);
            str[n] = charset[key];
        }
        str[size] = '\0';
    }
    return str;
}