Suggestions to improve a C ReplaceString function?

Question

Hi, I've just started to get in to C programming and would appreciate criticism on my ReplaceString function. It seems pretty fast (it doesn't allocate any memory other than one malloc for the result string) but it seems awfully verbose and I know it could be done better.

Example usage:

printf("New string: %s\n", ReplaceString("great", "ok", "have a g grea great day and have a great day great"));
printf("New string: %s\n", ReplaceString("great", "fantastic", "have a g grea great day and have a great day great"));

Code:

#ifndef uint
    #define uint unsigned int
 #endif

char *ReplaceString(char *needle, char *replace, char *haystack)
{
    char *newString;
    uint lNeedle = strlen(needle);
    uint lReplace = strlen(replace);
    uint lHaystack = strlen(haystack);
    uint i;
    uint j = 0;
    uint k = 0;
    uint lNew;
    char active = 0;
    uint start = 0;
    uint end = 0;

    /* Calculate new string size */    
    lNew = lHaystack;

    for (i = 0; i < lHaystack; i++)
    {

        if ( (!active) && (haystack[i] == needle[0]))
        {
            /* Start of needle found */
            active = 1;
            start = i;
            end = i;
        }
        else if ( (active) && (i-start == lNeedle) )
        {
            /* End of needle */
            active = 0;
            lNew += lReplace - lNeedle;
        }
        else if ( (active) && (i-start < lNeedle) && (haystack[i] == needle[i-start]) )
        {
            /* Next part of needle found */
            end++;
        }
        else if (active)
        {
            /* Didn't match the entire needle... */
            active = 0;
        }

    }
    active= 0;
    end = 0;


    /* Prepare new string */
    newString = malloc(sizeof(char) * lNew + 1);
    newString[sizeof(char) * lNew] = 0;

    /* Build new string */
    for (i = 0; i < lHaystack; i++)
    {

        if ( (!active) && (haystack[i] == needle[0]))
        {
            /* Start of needle found */
            active = 1;
            start = i;
            end = i;
        }
        else if ( (active) && (i-start == lNeedle) )
        {
            /* End of needle - apply replacement */
            active = 0;

            for (k = 0; k < lReplace; k++)
            {
                newString[j] = replace[k];
                j++;
            }
            newString[j] = haystack[i];
            j++;

        }
        else if ( (active) && (i-start < lNeedle) && (haystack[i] == needle[i-start])
                )
        {
            /* Next part of needle found */
            end++;
        }
        else if (active)
        {
            /* Didn't match the entire needle, so apply skipped chars */
            active = 0;

            for (k = start; k < end+2; k++)
            {
                newString[j] = haystack[k];
                j++;
            }

        }
        else if (!active)
        {
            /* No needle matched */
            newString[j] = haystack[i];
            j++;
        }

    }

    /* If still matching a needle... */
    if ( active && (i-start == lNeedle))
    {
        /* If full needle */
        for (k = 0; k < lReplace; k++)
        {
            newString[j] = replace[k];
            j++;
        }
        newString[j] = haystack[i];
        j++;
    }
    else if (active)
    {
        for (k = start; k < end+2; k++)
        {
            newString[j] = haystack[k];
            j++;
        }
    }

    return newString;
}

Any ideas? Thanks very much!

Answer 1

While looping the first time, you should keep indices on where there need to be replacement and skip those on the strcopy/replace part of the function. This would result in a loop where you only do strncpy from haystack or replacement to new string.

Answer 2

It's possible you are doing this in your own way for practice. If so, you get many points for effort.

If not, you can often save time by using functions that are in the C Runtime Library (CRT) versus coding your own equivalent function. For example, you could use strstr to locate the string that's targeted for replacement. Other string manipulation functions may also be useful to you.

A good exercise would be to complete this example to your satisfaction and then recode using the CRT to see how much faster it is to code and execute.

Answer 3

Don't call strlen(haystack). You are already checking every character in the string, so computing the string length is implicit to your loop, as follows:

for (i = 0; haystack[i] != '\0'; i++)
{
    ...
}
lHaystack = i;

Answer 4

My one suggestion has nothing to do with improving performance, but with improving readability.

"Cute" parameter names are much harder to understand than descriptive ones. Which of the following parameters do you think better convey their purpose?

char *ReplaceString(char *needle, char *replace, char *haystack)
char *ReplaceString(char *oldText, char *newText, char *inString)

With one, you have to consciously map a name to a purpose. With the other, the purpose IS the name. Juggling a bunch of name mappings in your head while trying to understand a piece of code can become difficult, especially as the number of variables increases.

This might not seem so important when you're the only one using your code, but it's paramount when your code is being used or read by someone else. And sometimes, "someone else" is yourself, a year later, looking at your own code, wondering why you're searching through haystacks and trying to replace needles ;)