current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite
1

Here's my attempt to replace first occurrence of pattern in file with spaces. Effectively deleting it, and hopefully will allow me to "delete" it in a large file, without rewriting it.

#define MAX_LINE_LENGTH
int removeFirstOccurenceInFile(const char* fileName, const char* pattern) {
  FILE* f = fopen(fileName,"r+");
  if (f == NULL) {
     perror("Can't open input file");
     return 0;
  }
  char buf[MAX_LINE_LENGTH] = {'\0'};
  char spaces[MAX_LINE_LENGTH];

  int unreadBytes = 0;
  char* patInBuf = NULL;
  while (1) {
    if (patInBuf != NULL) break;
    unreadBytes += strlen(buf);
    if (fgets(buf,sizeof(buf),f) == NULL) break;
    patInBuf = strstr(buf,pattern);
  }
  if (patInBuf == NULL) {
    fprintf(stderr,"No '%s' found in '%s'\n",pattern,fileName);
    fclose(f);
    return 0;
  }
  int delFrom = patInBuf-buf;

  memset(spaces,' ',delFrom);
  fseek(f,unreadBytes+delFrom, SEEK_SET);
  fwrite(spaces,1,strlen(pattern),f);

  fclose(f);
  return 1;
}

I'm not sure about the error handling. I considered using goto instead of the usual error handling:

  if (patInBuf == NULL) {
    fprintf(stderr,"No '%s' found in '%s'\n",pattern,fileName);
    retval = -1;
    goto cleanup;
  }
...
cleanup:
  fclose(f);
  return retval;

Are there other problems? Will that indeed be efficient?

share|improve this question
    
Did you test this code before posting it here? Did you test it with an input file consisting of multiple lines with the sought-for string somewhere in the middle? –  Mike Nakis Nov 30 '11 at 18:44
    
@MikeNakis, yes I did. Not too extensively, but it worked. Didn't it work for you? –  Chi-Lan Nov 30 '11 at 18:53

3 Answers 3

up vote 1 down vote

My advice would be to abandon the idea of treating the file as a text file in order to search through it, but then as a binary file in order to write into it. Your calculation of the offset to seek to is flimsy, and guaranteed not to work if the file has non-single-character end-of-line markers, (LF in Unix, but CR+LF in Windows,) if it starts with a unicode BOM, or if it contains any UTF8-encoded characters.

You need to either treat the file as a binary file all the way, in which case you will have to write your own code to read chunks and search through them, or to treat it as a text file all the way, meaning that every time you will be creating a new file and then swapping it with the original.

share|improve this answer
    
Are you sure that plain old C knows about the BOM, or UTF-8? I'm pretty sure many of those things were invented long after C. I'm not sure that it doesn't tuck the whole EOL, whatever it is, to the buffer. –  Chi-Lan Nov 30 '11 at 19:05
    
No, I am not sure, but I do not think that there are any guarantees about these matters, so it is probably not wise to be sure about the opposite, either. And when in doubt, it is a good idea to play it safe. –  Mike Nakis Nov 30 '11 at 19:13
    
If you do not want to play it safe, then I guess that yours seems to be the most efficient way to achieve this bizarre thing that you are trying to achieve. (Writing your own code to read the file as binary has the potential of performing just as well, but it would be quite complicated.) –  Mike Nakis Nov 30 '11 at 19:15
    
I'm actually pretty sure the standard dictate that fgets will read and store the exact number of bytes it read. Your approach is much less efficient, so I guess it worth checking the standard instead of fearing it. –  Chi-Lan Nov 30 '11 at 19:25
    
I do not have "the definitive specification of the standard C library" handy; I just looked at a couple of reference places and they were not specific at all about the question of exactly how many bytes will be read and stored by fgets. Anyway, I do not want to argue about it: I am not suggesting that I have a strong case here, if even I have a case at all. –  Mike Nakis Nov 30 '11 at 19:40
up vote 2 down vote

I think you have the error codes the other way around. It's more common to return 0 on success and 1 (or any non-zero number) on failure, especially for debugging. If you actually need to deviate from this, then comments could help with specification.

As for the goto, it is good to avoid using it whenever possible. Based on that little snippet, it looks like you could've used a function instead, at least if other this were needed in other places.

share|improve this answer
up vote 1 down vote

As @Mike Nakis began with suggesting treating the file as text all the way, OP's code has undefined behavior UB in using fseek()

int fseek(FILE *stream, long int offset, int whence);

For a text stream, either offset shall be zero, or offset shall be a value returned by an earlier successful call to the ftell function on a stream associated with the same file and whence shall be SEEK_SET. C11dr §7.21.9.2 4

This mean performing an ftell() before each fgets() and using that position for fseek() when a pattern is found. And let's use a do while()

  long position;
  do {
    // if (patInBuf != NULL) break;
    // unreadBytes += strlen(buf);
    position = ftell(f);
    if (position == -1L) break;
    if (fgets(buf,sizeof(buf),f) == NULL) break;
    patInBuf = strstr(buf,pattern);
  } while (patInBuf != NULL);
  ...
  // replace pattern with spaces

  // print _whole buffer
  if (fseek(f,position, SEEK_SET)) Handle_Failure();
  fputs(buf, f);

Other mundane things:

  1. #define MAX_LINE_LENGTH certainly needs a number like #define MAX_LINE_LENGTH 1000

  2. Nice use of const in removeFirstOccurenceInFile(const char* fileName, const char* pattern)

  3. Use of goto in error handle is a reasonable exception to the never use goto. Useful when multiple error points. In the end, rather have code do complete error checking with goto than elegant without error checking and without goto.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.