Thread: code review please :)

Hey all!

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:

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.

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.

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.