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.

• 200_success: What's your primary goal — to get repeated output printed, or to play around with memory?

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]);
}