logic error? replace characters in a file

user types $./replace i xy data.txt
data.txt contains the words "this is a test file, test file only". Therefore, all i's will be replaced with xy, i.e. thxys xys a test fxyle, test fxyle only

I think I am quite close. However, instead of replacing i with xy, my code just replaces i with x. I think the error is at line 38 strcpy.
However, is the logic correct from line 30 to 40? I am trying to say....

For each element in the first buffer(buf)
copy buf element into another buffer (temp) one character at a time
if buf element == 'i'
copy 'xy' to 'i'

Code:

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


#define BUFFERSIZE 4096

/*replace i xy data.txt */

int main(int ac, char *av[])
{

	int in_fd, out_fd, n_chars, BufElement,j,x;
	ssize_t nread,nwrite;
	off_t newpos;
	char buf[BUFFERSIZE],temp[300];

	
		/*Open file containing original data*/
		if ( (in_fd=open(av[3], O_RDWR)) == -1 )
		{
			printf("Cannot open %s\n", av[3]);
			exit(1);
		}


		/*Read characters from file to buffer*/
		while ( (nread = read(in_fd , buf, BUFFERSIZE)) > 0 )
		{
			for (BufElement=0;BufElement < nread;BufElement++)
			{
				for (j=0; j < strlen(av[1]); j++)
				{
					temp[BufElement] = buf[BufElement];
					if (buf[BufElement] == av[1][j])
						strncpy(temp+BufElement,av[2],strlen(av[2])); /*ERROR*/
								
				}
					
			}
		}
		
		printf("%s\n",buf);
		printf("%s\n",temp);

			newpos = lseek(in_fd, 0, SEEK_SET); 
	
			nwrite = write(in_fd,temp,36); 
			
			close(in_fd);
		}
	
}

Yep, it's a logic error with your for loop.
The problem is that you are trying to use a single marker BufElement to keep track of both the temp string and the original string.

So for the string "This":

Code:

buf[0] = 'T'
buf[1] = 'h'
buf[2] = 'i'
buf[3] = 's'

Your code will modify temp like this:

Code:

temp[0] = 'T'
temp[1] = 'h'
temp[2] = 'i'

Your strncpy call is correct, and sets temp to:

Code:

temp[0] = 'T'
temp[1] = 'h'
temp[2] = 'x'
temp[3] = 'y'

But the next iteration of the loop does:

Code:

temp[3] = buf[3]

and overwrites your 'y'.

You should maintain a separate iterator for the position in the temp buffer, and increment it appropriately.

That'll be enough to get this example working, but from this inner loop:

Code:

for (j=0; j < strlen(av[1]); j++)

It looks like you'd like your code to work with something like "replace is xy data.txt" turning "This" into either "Thxyxy" or "Thxy". It won't do either of those, but that's another problem

General comments:
Turning 'i' into 'xy' means "temp" will end up being longer than "buf", so it's a very bad idea to have buf as 4096 chars and temp as only 300! You can't safely make temp a fixed size at compile time. You have various options here, will be happy to help if you want to know how to make it safer.

You should NULL-terminate "temp" if you're going to try to print it -- your outer 'for' loop will copy the NULL terminator from buf, but your strncpy doesn't copy the terminator from av[2]. If you're going to write it out in buf sized pieces to a file you won't want to NULL terminate the pieces (I suspect that's why you've used strncpy not strcpy -- you might as well just use memcpy though), but for debugging maybe terminate it.

Thanks for your reply. I see where the error is now. I am trying to set a new counter for the temp buffer. But again it seems to be outputting garbage.
If buf runs into character 'i', it copies it into temp and temp counter is incremented by bufelement+1

Code:

  while ( (nread = read(in_fd , buf, BUFFERSIZE)) > 0 )
        {
            for (BufElement=0;BufElement < nread;BufElement++)
            {
                for (j=0; j < strlen(av[1]); j++)
                {
                    temp[x] = buf[BufElement];
                    if (buf[BufElement] == av[1][j]) {
                        strncpy(temp+x,av[2],strlen(av[2])); 
		        x = BufElement + 1;
		     }
                                 
                }
                     
            }
        }

Ok. So x needs to point to the current character to be written in temp.

First problem is that you are only incrementing x when you match 'i'! So x will be 0 until you match i. You still want to increment x every time through the loop. You could put it in here:

Code:

for (BufElement=0;BufElement < nread;BufElement++,x++)

But I would probably just do

Code:

          for (BufElement=0;BufElement < nread;BufElement++)
          {
               x++;

They do the same thing for this loop anyway.

Second problem is

Code:

              x = BufElement + 1;

You don't want to set x based on BufElement.

So say you have a test string "fififi", buf is:

Code:

index   letter
0         f
1         i
2         f
3         i
4         f
5         i

You want temp to be:

Code:

index    letter
0           f
1           x
2           y
3           f
4           x
5           y
6           f
7           x
8           y

When you get to the second 'i', BufElement is 3 and temp is 4. You want to write "xy" to temp and set x to 6, but your increment sets it to 4!

I'd suggest making it independent of the length of string you're writing in too, maybe:

Code:

  x = x + strlen(av[2]) - 1;

The -1 is for the increment you already made for the letter you replaced.