Simple string repeater program Round 2

Question

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

#define MAX_LOOP 99999
#define STR_MAXLEN 300
#define USAGE_INFO "Usage: repstring <argument> <string> <amount of times to repeat>\n\nArguments:\n-s silence length of string output\n-a add space inbetween every string\n"
#define SILENT_ARG "-s"
#define SPACE_ARG "-a"

int main(int argc, char **argv)
{

    if(argc == 3 || argc == 4)
    {

        //Initialize variables and pointers
        int i;
        char *deststr;
        int loop;

        //If second argument is space argument, add space inbetween every string
        if(strncmp(argv[1], SPACE_ARG, sizeof(SPACE_ARG)) == 0)
        {
            //Change second argument to integer
            loop = atoi(argv[3]);

            //If loop is equal or under zero, terminate
            if(loop <= 0)
            {
                printf("%s\n", USAGE_INFO);
                return 1;
            }

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



             //Allocate memory
             deststr = malloc(strlen(argv[2]) * loop + 1);

            //If memory allocation failed, terminate
            if(deststr == NULL)
            {
                free(deststr);
                printf("Error allocating memory!\n");
                return 1;
            }



            //Append string in argument 2 to deststr for amount of times loop is set
            for(i = 0; i < loop; i++)
            {
                strncat(deststr, argv[2], sizeof(STR_MAXLEN));
                strncat(deststr, " ", sizeof(STR_MAXLEN));
            }

            //Print deststr and free memory
            printf("%s\n", deststr);
            free(deststr);
            return 0;
         }

        //If second argument is silent option, remove length of string output
        if(strncmp(argv[1], SILENT_ARG, sizeof(SILENT_ARG)) == 0)
        {
             //Change second argument to integer
             loop = atoi(argv[3]);

             //If loop is equal or under zero, terminate
             if(loop <= 0)
             {
                 printf("%s\n", USAGE_INFO);
                 return 1;
             }

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


             //Allocate memory
             deststr = malloc(strlen(argv[2]) * loop + 1);

             //If memory allocation failed, terminate
             if(deststr == NULL)
             {
                 free(deststr);
                 printf("Error allocating memory!\n");
                 return 1;
             }



            //Append string in argument 2 to deststr for amount of times loop is set
            for(i = 0; i < loop; i++)
            {
                 strncat(deststr, argv[2], sizeof(STR_MAXLEN));
            }

            //Print deststr and free memory
            printf("%s\n", deststr);
            free(deststr);
            return 0;
         }

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

        //If loop is equal or under 0, terminate
        if(loop <= 0)
        {
            printf("%s\n", USAGE_INFO);
            return 1;
        }

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



        //Allocate memory
        deststr = malloc(strlen(argv[1]) * loop + 1);

        //If memory allocation failed, terminate
        if(deststr == NULL)
        {
            free(deststr);
            printf("Error allocating memory!\n");
            return 1;
        }

        //Append string in argument 2 to deststr for amount of times loop is set
        for(i = 0; i < loop; i++)
        {
             strncat(deststr, argv[1], sizeof(STR_MAXLEN));
        }


        int destStrLen = strlen(deststr);

        //Print repeated string
        printf("%s\n", deststr);
        printf("\nLength of string: %d\n", destStrLen);

        //Free allocated memory
        free(deststr);
        return 0;

    }
    else
    {
        printf("%s\n", USAGE_INFO);
        return 1;
    }

    return 0;
 }

I realize that I should probably move a lot of the code to seperate functions to uncomplicate things, and that I'm still using strncat instead of strncpy for the loops even though it was suggested that I change that last time, and that you can only use one argument at a time.

What I really want to know is if I've handled everything correctly, if I could make any improvements anywhere, and if the program is secure.

Review would be very appreciated.

Answer 1

By string repeater, I didn't think you meant your code. This should be about 1/3 the size of what it is not because of how non-DRY it is; you are repeating your self in a very bad way.

---

Here's the main theme of what's repeated:

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

//If loop is equal or under 0, terminate
if(loop <= 0)
{
    printf("%s\n", USAGE_INFO);
    return 1;
}

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



//Allocate memory
deststr = malloc(strlen(argv[1]) * loop + 1);

//If memory allocation failed, terminate
if(deststr == NULL)
{
    free(deststr);
    printf("Error allocating memory!\n");
    return 1;
}

//Append string in argument 2 to deststr for amount of times loop is set
for(i = 0; i < loop; i++)
{
     strncat(deststr, argv[1], sizeof(STR_MAXLEN));
}

Really, this construct is repeated three times with very minor differences. The biggest different is which argv you are using, but that's no excuse to repeat.

Let's break down this repetition.

---

The first two places where you are repeating the above theme come under these conditionals:

if(strncmp(argv[1], SPACE_ARG, sizeof(SPACE_ARG)) == 0)

and:

if(strncmp(argv[1], SILENT_ARG, sizeof(SILENT_ARG)) == 0)

Inside these two conditional statements, there is only ONE line that is different, and it is found in the second conditional:

strncat(deststr, " ", sizeof(STR_MAXLEN));

That is it! This is just one line that is added; there is no reason to repeat this entire section of code again just for a single line! To simplify, let's merge the two conditionals and then add another if to see if we run this one extra line:

if(strncmp(argv[1], SPACE_ARG, sizeof(SPACE_ARG)) == 0 || strncmp(argv[1], SILENT_ARG, sizeof(SILENT_ARG)) == 0)
{
    ...
    if(strncmp(argv[1], SPACE_ARG, sizeof(SPACE_ARG)) == 0)
    {
        strncat(deststr, " ", sizeof(STR_MAXLEN));
    }
    ...
}

Hmm. I don't like having to repeat that strncmp operation that many times; it's too expensive. Let's store the results in booleans and use those instead:

bool using_space = strcmp(argv[1], SPACE_ARG)  == 0;
bool using_silent= strcmp(argv[1], SILENT_ARG) == 0;

An edit in the above snippet was suggested by chux. Thanks!

The code will be faster in the future if these booleans are used instead. Also note that strncmp is not needed since strcmp already compares the full strings (you were being redundant before with sizeof(SPAGE_ARG) etc.

As a note, rather than parsing these command-line arguments yourself, I'd recommend using GNU's getopt; it makes things a lot easier.

---

Now that we've simplified part of your code, let's simplify the rest of it. Keep in mind that this next section is almost identical to the first. Again, the major difference is which element of argv is being used.

In fact, let's just get that out of the way be first determining which argv will be used:

// skip over -s or -a
int first_arg = using_space || using_silent ? 2 : 1;

Place this at the top of your code, along with those bools. Now, wherever you use argv[n] or something like that, use first_arg or an offset of this.

---

Now, removing the repetition by using this new variable and by using the tricks we did in the first step, your code looks like this:

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

#define MAX_LOOP 99999
#define STR_MAXLEN 300
#define USAGE_INFO "Usage: repstring <argument> <string> <amount of times to repeat>\n\nArguments:\n-s silence length of string output\n-a add space inbetween every string\n"
#define SILENT_ARG "-s"
#define SPACE_ARG "-a"

int main(int argc, char **argv)
{

    bool using_space = strcmp(argv[1], SPACE_ARG)  == 0;
    bool using_silent= strcmp(argv[1], SILENT_ARG) == 0;
    // thanks again chux!! vv
    int first_arg = using_space + using_silent + 1;

    if(argc == 3 || argc == 4)
    {

        //Initialize variables and pointers
        int i;
        char *deststr;
        int loop;

        //If second argument is space argument, add space inbetween every string
        if(using_space || using_silent)
        {
        //Change second argument to integer
        loop = atoi(argv[first_arg+1]);

        //If loop is equal or under zero, terminate
        if(loop <= 0)
        {
            printf("%s\n", USAGE_INFO);
            return 1;
        }

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

        //Allocate memory
        deststr = malloc(strlen(argv[first_arg]) * loop + 1);

        //If memory allocation failed, terminate
        if(deststr == NULL)
        {
            free(deststr);
            printf("Error allocating memory!\n");
            return 1;
        }

        //Append string in argument 2 to deststr for amount of times loop is set
        for(i = 0; i < loop; i++)
        {
            strncat(deststr, argv[first_arg], sizeof(STR_MAXLEN));
            if(using_space) {
                strncat(deststr, " ", sizeof(STR_MAXLEN));
            }
        }

        if(using_space || using_silent)
        {
            //Print deststr and free memory
            printf("%s\n", deststr);
            free(deststr);
            return 0;
        }

        int destStrLen = strlen(deststr);

        //Print repeated string
        printf("%s\n", deststr);
        printf("\nLength of string: %d\n", destStrLen);

        //Free allocated memory
        free(deststr);
        return 0;

    }
    else
    {
        printf("%s\n", USAGE_INFO);
        return 1;
    }

    return 0;
 }

This code looks much nicer now; there is no repetition. Note that new conditional I introduced at the end if(using_space || using_silent); this handles the different ways the different conditional blocks handled the ending.

However, your code is far from done; everything is not meant to be kept in the main method. Split up your logic into separate, outer functions that your main function calls. I'm not going to handle that in this answer because I've done enough.

---

provided two very helpful comments.

First off, you don't need to do strncmp when comparing