HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Simple string repeater program

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
programsimplestringrepeater

Problem

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

//repstring.c
#include 
#include 
#include 

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  \n");
        return 1;
    }

 return 0;
}


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

Solution

-
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.

Context

StackExchange Code Review Q#121217, answer score: 3

Revisions (0)

No revisions yet.