views:

72

answers:

2

Hello.

I'm very new to C and I decided to make a function called str_replace which replaces strings inside strings which have been made using malloc. It appears to work but can anyone find any room for improvements.

Any advice will be appreciated. I'd like to know if people think finding the number of occurrences to calculate the new string size was a good idea and if my use of pointers makes sense.

#include <stdlib.h>
#include <string.h>
#include <stdio.h>

char * str_replace(char * string,char * find,char * replace){
    //Replaces each occurence of a particular string inside a malloc-made string with another string
    char * pos = string;
    size_t replace_size = strlen(replace);
    size_t find_size = strlen(find);
    size_t excess = replace_size - find_size;
    //Get number of occurences
    int x = 0;
    while (1) {
        pos = strstr(pos,find);
        if (pos == NULL){
            break;
        }
        pos++;
        x++;
    }
    if (!x){ //No occurences so return with original string
        return string;
    }
    char * new_string = malloc(sizeof(char)*(strlen(string) + excess*x + 1)); //Plus 1 for null termination
    pos = string; //Reset pointer
    char * string_track = string; //Used to move around string.
    char * new_string_begin = new_string; //Begining of new string to return
    while (1) {
        pos = strstr(pos,find);
        if (pos == NULL){
            strcpy(new_string,string_track); //Fill in remainder
            break;
        }
        pos++;
        size_t seg_len = pos-string_track; //Length between found string and last
        strncpy(new_string,string_track,seg_len); //Copy string to just before pos
        new_string += seg_len - 1; //Go to point for replacement by adding what was added to pointer.
        strncpy(new_string,replace,replace_size);
        new_string += replace_size; //Go to point after what was replaced
        string_track = pos + find_size - 1; //Original string should go to point after found string.
    }
    free(string); //Remove old string
    return new_string_begin; //return new string
}

int main (int argc, const char * argv[]) {
    char * string = malloc(sizeof(char)*21);
    strcpy(string,"No,yes,no,yes,no,yes");
    printf("%s\n",string);
    string = strreplace(string, "no", "nope");
    printf("%s\n",string);
    free(string);
    string = malloc(sizeof(char)*21);
    strcpy(string,"No,yes,no,yes,no,yes");
    printf("%s\n",string);
    string = strreplace(string, "hello", "nope");
    printf("%s\n",string);
    free(string);
    string = malloc(sizeof(char)*21);
    strcpy(string,"No,yes,no,yes,no,yes");
    printf("%s\n",string);
    string = strreplace(string, "yes", "y");
    printf("%s\n",string);
    free(string);
    return 0;
}
+1  A: 

One suggestion: If you intend to call the function with the syntax string = str_replace(string, "no", "nope");, then I would suggest changing the first argument to a char**. That way, you can modify the pointer directly instead of having to assume that the user is using that particular calling notation. Since you free the original string, the following code block is dangerous:

char* str1;
char* str2;
str1 = malloc(SIZE_1);
/* Write something into the buffer here.. */
str2 = str_replace(str1, "from", "to");
free(str1);  // Whoops! Double free!

By changing the first parameter to a char**, the user can safely free the same pointer that they used with malloc regardless of how they called the function. In that case, you can either make your function return void or simply return a copy of the pointer to the original string (*string).

bta
Nice point. I managed to implement it your way and it works, even when the original variable is freed. It does require an ampersand when the user enter the arguments but I suppose this way is safer. If only there was a way to test if a pointer was made with malloc or not. It would be nice if I could make one function to work with memory buffers and normal variables (What's the proper name for the two different types of data?). Thank you very much for your contribution.
Matthew Mitchell
And I also like your idea because I no longer need to re-assign the pointer. I suppose it requires less code to write when using the function.
Matthew Mitchell
+2  A: 

Overall that's pretty solid, some things I would suggest would be

1) don't name the first argument "string" .. I think thats a little risky (doesn't string.h define a "string" symbol?)

2) I wouldn't free the old string in the str_replace function, it wasn't allocated by that function, so it shouldn't free it IMO, not really important for this example but it's generally a good habit. That would also mean theres no need for the "string_track" variable, since the first arg is just a copy of a pointer to a string, you can muck with it and just not care where it ends up cause it gets thrown away when the function exits.

Tom
Thank you for your answer. I made this function for a program I am making and my general use requires no need for the old data. Hence I feel it would be best to remove the old data in the function for convenience. It's a function to replace the string, rather than to make a new one.
Matthew Mitchell
@Matthew: you're written a function that does more than one thing - it performs substring replacement, and it also frees its argument. That seems OK, until you come to a point where you want to do string replacement, but you don't want to free the argument, for example: `str_replace("some fixed string", find, replace);`. Then you have to write another function, to do one of the two things your current function does, and not the other. Code is more re-usable if it does one thing at a time.
Steve Jessop
Thanks for your comment. You are correct but for this particular program I feel it is fine as it does exactly what is needed. I can modify it in the future to have more control over what is done.
Matthew Mitchell