How do you fill an array with strings in C?

Question

I'm trying to fill an array with names from a file:

Andrew
Andy
Bob
Dan
Derek
Joe
Pete
Richy
Steve
Tyler

Here is the function I wrote...but the program crashes when I run it:

#include <stdio.h>

main(){
  int i=0, size=10;
  char fname[15];
  char** data;
  char* name;
  FILE* fp;

  printf("Enter a filename to read names:\n");
  scanf("%s", fname);

  fp = fopen(fname, "r");
  if(fp == NULL)
    exit();

  data = (char**)malloc(sizeof(char**)*size);

  while(!feof(fp)){
    fscanf(fp, "%s", name);
    data[i] = (char*)malloc(sizeof(name));
    data[i] = name;
    i++;
  }

  fclose(fp);

  printf("\n\n");

  for(i=0; i<size; i++)
    printf("%s ", data[i]);

  free(data);
}

Anyone know what I'm doing wrong? Thanks

Answer 1

You never allocate space for name. But you put stuff in on this line:

 fscanf(fp, "%s", name);

change

 char name[100];

and

fscanf(fp, "%99s", name);

you should limit the imput to filename too:

scanf("%14s", fname);

also you never free the data for each element in the array, if this were a sub-routine in a larger system it would have a memory leak.

Answer 2

You have a couple of errors here:

1) You never allocate memory where name will get stored. You could get around this simply by using:

char name[128];

Then, when you use fscanf, you'd have:

fscanf(fp, "%127s", name); // stores this in name correctly, now...

2) You're inappropriately allocating space for data[i]:

data[i] = (char*)malloc(sizeof(name));

This will allocate space to hold one pointer to char (char*), since name is a char*. You need to be doing:

data[i] = (char*)malloc(sizeof(char) * (strlen(name) + 1 ) );

This will allocate enough space for the data plus a single terminating character.

3) You aren't assigning data[i] correctly. You can't just use = here, but need to use strcpy:

strcpy(data[i], name);

4) You're not freeing the individual pointers in the data[..] elements. You should add, after your printf, a free:

for(i=0; i<size; i++)
{
    printf("%s ", data[i]);
    free(data[i]);  // Free each pointer you allocate
}

Every malloc call should eventually have a matching free call.

Answer 3

There are quite a few problems. Other answer talk about name not being initialized, so I won't repeat that.

A second problem is that you are not handling EOF correctly. feof will only return true once you've tried to read just past the end of the file. So you will get an empty 11'th name.

A final problem is that the data array can only hold 10 names. Because of the second problem you'll overflow the buffer with the blank 11'th name. Furthermore, your code depends on the details of the input file - give it a different input file with more names and you'll still crash even after fixing problem 2.

Answer 4

The usual way to do this is to use a fixed-size buffer to initial read a line of data (hoping that it's large enough for all the data on the line), then allocate enough space for that item, and go on to the next one. If you want to get more elaborate, you can read with fgets, check whether the final character you read in was a new-line, and if not, read again and use-realloc to increase the size of the buffer, repeating as necessary until you run out of memory or see a new-line to let you know that you've read the entire line.

On another note, your loop:

while(!feof(fp)){
    fscanf(fp, "%s", name);
    data[i] = (char*)malloc(sizeof(name));
    data[i] = name;
    i++;
}

is broken -- it won't exit when it should. As it stands, your data will probably look like the last line of the file was read twice. As mentioned above, fgets is generally more suitable for this kind of task. With it, you'd generally want to write your loop something like:

while (fgets(name, sizeof(name), fp)) {
    data[i] = malloc(strlen(name));
    strcpy(data[i++], name);        
}

Not to sound like a proselytizing, but almost any other language will make it much easier to do a much better job of this. Just for example, in C++ you could use:

std::vector<std::string> data;
std::copy(std::istream_iterator<std::string>(infile),
          std::istream_iterator<std::string>(),
          std::back_inserter(data));

...and it would handle automatically resizing the individual strings and the array of strings automatically.