Checking for a string in an array of strings in C

Question

I have a function that takes in a filename (string) and I want to have an array of filenames (strings) to check to see if the filename taht was passed in is in the array and if not add it to it. Here is what I got so far.

bool read_file(char * filename, bool warnings){
  //Check to see if filename is in array files
  static char * files[MAXFILES];
  static int counter = 0;
  for(int i=0;i<counter;i++){
    if(strcmp(filename,files[i]) == 0){
      fprintf(stdout, "Error\n");
      return 0;
    }
    else{
      files[counter]=filename;
      counter++;
    }
  }
  FILE * fp = fopen(filename,"r");
  if(fp == NULL){
    if(warnings){
      fprintf(stderr, "Can't open the file %s\n",filename);
      return 0;
    }
    else{
      return 0;
    }
  }
  else{
    fclose(fp);
    fp = NULL;
    return 1;
  }
}

For some reason it wont add filenames to files[] any help would be appricated.

The for loop is never entered, you are initializing the counter to 0 and using i<counter in the for loop. Is that the problem you are looking for? — Naveen
– Naveen, Commented Feb 21, 2012 at 5:49

Seth · Accepted Answer · 2012-02-21 05:54:30Z

1

Take a look at the first time through your loop (when i == 0 and counter == 0):

static int counter = 0;
for(int i = 0; i < counter; i++) {
    // this never runs
}

i < counter is always false, because counter is incremented in the body of the for loop.

You'll need to rethink your logic. This function is doing a lot of stuff. Maybe something like this infront of the loop will help:

if (counter == 0) {
    // first time in the function, just add the filename
    files[counter]=filename;
    counter++;
}
else {
    for (int i = 0; i < counter; i++) {
        ...
    }
}

However, you might have better luck splitting this into a couple functions. One to add a filename to the array, another to check if it's already there, a third to open the file, etc.

answered Feb 21, 2012 at 5:54

Seth

46.8k10 gold badges87 silver badges123 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Timothy Jones Over a year ago

+1 for splitting the function - when the first comment in a function doesn't seem to match with the name of the function, it's probably a good time to split it up :)

Jonathan Leffler Over a year ago

Special casing the first iteration isn't very neat and tidy. But I agree with the suggestion to split the function into smaller pieces. A +1 and a -1 leaves a net no change.

Seth Over a year ago

@JonathanLeffler - Yeah, the main problem is that the scope of the function is way too big (and this really wants to be a class). Special-casing the counter variable is the quick hack to allow time for a proper refactoring :D

Dave Over a year ago

@Seth -1 special-casing does nothing to fix the actual problem that the code should append the filename after checking not during.

Jonathan Leffler · Accepted Answer · 2012-02-21 06:48:58Z

Look at the else clause of this loop:

  for(int i=0;i<counter;i++){
    if(strcmp(filename,files[i]) == 0){
      fprintf(stdout, "Error\n");
      return 0;
    }
    else{
      files[counter]=filename;
      counter++;
    }
  }

You want to add the file to the list only once you know it is not in the list, and you won't know that until you have gone through the entire list. So, that code should probably look like:

for (int i = 0; i < counter; i++)
{
    if (strcmp(filename, files[i]) == 0)
    {
      fprintf(stdout, "Duplicate file %s found at %d\n", filename, i);
      return 0;
    }
}
files[counter++] = filename;

Notice the better error message than just 'Error'. You will need to know what went wrong because this won't be the only place in the program where there could be errors. You can debate whether the number is helpful; the name most certainly is, but having the index number might help you too.

Dave · Accepted Answer · 2012-02-21 09:35:48Z

Your else is misplaced. This:

 for(int i=0;i<counter;i++){
    if(strcmp(filename,files[i]) == 0){
      fprintf(stdout, "Error\n");
      return 0;
   }
   else{
      files[counter]=filename;
      counter++;
   }
}

Will never be entered, and were it to be, it would always think that the file was in the list, because you append it before you finish checking. You would then find the one you appended, and think it was already there.

Instead, wait until you've finished checking the whole list until you append the filename:

for(int i=0;i<counter;i++){
   if(strcmp(filename,files[i]) == 0){
     fprintf(stdout, "Error\n");
     return 0;
   }
}
files[counter]=filename;
counter++;

The key is that the return 0 redirects the flow out of your function, rather than an else clause. You know that as long as the loop finishes and you're still in the function that the filename isn't there yet.

vireshas · Accepted Answer · 2012-02-21 06:13:28Z

0

change your for loop to

 for (i=0;i<=counter;i++)

answered Feb 21, 2012 at 6:13

vireshas

8166 silver badges19 bronze badges

1 Comment

Jonathan Leffler Over a year ago

No; that is occasionally, but only very occasionally, a good way to write a loop in C. The for (i = 0; i < MAX; i++) loop is almost invariably better. In this case, your change would trigger a comparison with NULL, which is going to cause a crash. Even if we got past that problem (empty names instead of null pointers), you'd add the first file the first time the function is called; thereafter, you'd add each file to the list as many times as there are other files already in the list, which is going to lead to quadratic growth in your file list.

Collectives™ on Stack Overflow

Checking for a string in an array of strings in C

4 Answers 4

4 Comments

Comments

Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

4 Comments

Comments

Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related