code review please :)

Hey all! :D

Here's what I want to achieve:

1. Open up a file containing a string.
2. If the string contains the word "lists", insert the word "every" to the right of "lists" into the string.

My logic is to split the original string into 2 strings, append "every" to the first string(which contains "lists"), and then append the second string to the first(modified) string.

Is that the best solution to inserting a string into another string?

Here is the string:

Quote:

---

This page lists toy bus by colour, engine size, transmission, weight, height, and one two, five, ten twenty.

---

Here's the code, which works great:

Code:

---

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

int main(void)
{
        FILE *p_openfile;
        char *p_searchstring = "lists";
        char *p_insertstring = " every ";
        char storestring[300];
        char string1[300];
        char string2[300];
        char *location;
        int cursorposition;
        int counter;
        size_t tail_length;

        if((p_openfile = fopen("testfile.txt", "r")) == NULL)
        {       
                perror("Error opening file");
                exit(EXIT_FAILURE);
        }
        fgets(storestring, 1000, p_openfile);

        if((location = strstr(storestring, p_searchstring)) == NULL)
        {
                        perror("No match was found");
                        exit(EXIT_FAILURE);
        }
        cursorposition = location - storestring;
        tail_length = strlen(storestring);
        memmove(string1, storestring, 14+1);
        string1[15] = '\0';
        strcat(string1, p_insertstring);
        for(counter = cursorposition + 6; storestring[counter] != '\0'; counter++)       
        {        string2[counter - (cursorposition + 6)] = storestring[counter]; }
        string2[counter - (cursorposition + 6)] = '\0';
        strcat(string1, string2);
        printf("%s", string1);
        return 0;
}

---

EDIT: Here's the output:

Code:

---

This page lists every toy bus by colour, engine size, transmission, weight, height, and one two, five, ten twenty.

---

Also, I used memmove(), but I could also have used strncpy(). Is there any real difference between them?

Thanks in advance? :)

Code:

---

        const char *p_searchstring = "lists";
        const char *p_insertstring = " every ";

---

Code:

---

        char storestring[300];
....
        fgets(storestring, 1000, p_openfile);

---

Now then, fgets() is ONLY safe if you do not lie to it about the size of the buffer. Here, I'd suggest using sizeof(storestring) instead of a hard-coded constant, so you can easily change the storestring without having to scan the code.

Code:

---

        memmove(string1, storestring, 14+1);
        string1[15] = '\0';

---

What are those 14, 15 numbers supposed to be?

Code:

---

        for(counter = cursorposition + 6; storestring[counter] != '\0'; counter++)       
        {        string2[counter - (cursorposition + 6)] = storestring[counter]; }
        string2[counter - (cursorposition + 6)] = '\0';

---

What does 6 correspond to? Also, reformat the code so that braces are on separate lines.

Code:

---

        tail_length = strlen(storestring);

---

Calculated, but never used? Although you may find that it is useful to use for the above loop.

--
Mats

Thanks mats. Sorry for the confusion, I should have made all that more clear. :)

Quote:

---

Originally Posted by matsp

Code:

---

        memmove(string1, storestring, 14+1);
        string1[15] = '\0';

---

What are those 14, 15 numbers supposed to be?

---

Copy 14 characters, including "lists", and then insert a \0 to make it into string1.

Quote:

---

Code:

---

        for(counter = cursorposition + 6; storestring[counter] != '\0'; counter++)       
        {        string2[counter - (cursorposition + 6)] = storestring[counter]; }
        string2[counter - (cursorposition + 6)] = '\0';

---

What does 6 correspond to?

---

5 characters of "lists"+ a space, so copy every character after "lists" into string2.

I sort of guessed that, but isn't the code supposed to be the generic, so that if you change "lists" to "provides a list", the code would still work as appropriate.

Edit: At the very least, use a named constant that is positioned next to the string that it refers to. But the 14/15 constant is actually something that is in your INPUT data, so it certainly should not be part of your application - you do, after all, calculate that bit anyways, when you use strstr() - you just don't use it.

Oh, and this:

Code:

---

        for(counter = cursorposition + 6; storestring[counter] != '\0'; counter++)       
        {        string2[counter - (cursorposition + 6)] = storestring[counter]; }
        string2[counter - (cursorposition + 6)] = '\0';

---

is a strcpy in disguise.

--
Mats

Ok, thanks Mats. :)

Quote:

---

Originally Posted by happyclown

Thanks mats. Sorry for the confusion, I should have made all that more clear. :)

---

Not by explaining it directly to us you shouldn't. The code should be self explanatory. Code should be understood just be reading the code and any comments within the code etc.
Use a well named constant.