Thread: logic error? replace characters in a file

  1. #1
    Registered User
    Join Date
    Jan 2011
    Posts
    144

    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);
    		}
    	
    }
    Last edited by bos1234; 04-20-2012 at 05:30 AM.

  2. #2
    Registered User
    Join Date
    Mar 2010
    Posts
    583
    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.

  3. #3
    Registered User
    Join Date
    Jan 2011
    Posts
    144
    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;
    		     }
                                     
                    }
                         
                }
            }

  4. #4
    Registered User
    Join Date
    Mar 2010
    Posts
    583
    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.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 13
    Last Post: 04-02-2012, 05:51 PM
  2. Replies: 2
    Last Post: 02-06-2010, 11:45 AM
  3. Replies: 1
    Last Post: 02-05-2010, 02:59 PM
  4. Logic Error Help
    By Taka in forum C++ Programming
    Replies: 4
    Last Post: 04-02-2009, 10:13 PM
  5. Replace AlphaNumeric Characters
    By Hexxx in forum C++ Programming
    Replies: 3
    Last Post: 01-01-2006, 06:12 PM