Simple string repeater program

Question

I'm new to C, want to know if I'm doing this right.

//repstring.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main(int argc, char **argv)
{
    //Initialize variables and pointers
     int i;
     int loop;
     char *deststr;
     char *str;

     //Change second argument to integer
     loop = atoi(argv[2]);

    //If user requests to repeat string more than 9999 times, terminate
    if(loop > 9999)
    {
        printf("Error: Asked to repeat too many times.\n");
        printf("Hardcoded maximum: 9999\n");
        return 1;
    }

    if(argc == 3)
    {

        //Dynamically allocate memory to strings
        deststr = malloc(1024 * 1024 * 1024  * sizeof(char));
        str = malloc(1024 * 1024 * 1024  * sizeof(char));

        if(str == NULL || deststr == NULL)
        {
            printf("Error allocating memory!\n");
            free(deststr);
            free(str);
            return 1;
        }

        //Copy string in first argument to str
        strncpy(str, argv[1], 50);

        //Append str to deststr for amount of times specified in second argument
        for(i = 0; i < loop ; i++)
        {
            strcat(deststr, str);
        }

        //Print repeated string to stdout
        printf("%s\n", deststr);

        //Free allocated memory
        free(deststr);
        free(str);


    }
    else
    {
        printf("Usage: repstring <string> <amount of times to repeat>\n");
        return 1;
    }


 return 0;
}

Is there anything that could be otherwise improved? Am I handling memory correctly? Review would be very appreciated.

Answer 1

• Standard guarantees sizeof(char) to be 1.

• argv[1] is a perfectly good string. There is no need to copy it anywhere, let alone allocate a megabyte for it.

• Length of a destination string is strlen(argv[1]) * loop + 1. Do not hardcode a megabyte.

• strcat has to find the end of destination string over and over again, making the execution time quadratic. Trace the end of the destination string as you go and strcpy to the right location instead.

Answer 2

I don't have much to offer other than I see some magic numbers in there. For example:

if(loop > 9999)

and

strncpy(str, argv[1], 50);

Good example is the 9999. Make that into a variable then you can change either statically or dynamically and you would only have to make the change in one place.

Answer 3

If you just want to print the repeated strings, you do not need to allocate memory, a simple loop can suffice:

for (int i = 0; i < loop; i++) {
    puts(argv[2]);
}