Loading files into lists

[MiroMage]

I've been working on this for a few days now, and I'm seriously stuck on this function. For this part, I'm supposed to read in a file and create a linked list from the data in the file. I have my program reading in the files, no problem. What I am running into is when it tries to insert the data into the list, the program just crashes. Can someone maybe explain what I am doing wrong?

Here is the code to load the file (it is in my main):

else if(choice == 5)
        {
          printf("Enter load file name.\n");
          scanf("%s", &name2); 
          
          fload = fopen(name2, "r");
          
          while(!feof(fload))
          {
                fscanf(fload, "%s %s %d %s %s", make, model, &year, color, license);
                printf("%s %s %d %s %s \n", make, model, year, color, license);  //To make sure the file is being read properly                
                if(insert(&head, make, model, year, color, license))
                {
                    printf("file loaded\n");
                }
          }
          
          printf("File loaded!\n");
          
          fclose(fload);
        }

And here is my insert function (which will work with the rest of the program):

int insert(struct vehicle** head, char* make, char* model, int year, char* color, char* license)
{
    struct vehicle* newvehicle;
    struct vehicle* crnt = (*head);
    
    newvehicle = malloc(sizeof(struct vehicle));
    newvehicle->make = (char*)calloc(strlen(make)+1, sizeof(char));
    newvehicle->model = (char*)calloc(strlen(model)+1, sizeof(char));
    newvehicle->color = (char*)calloc(strlen(color)+1, sizeof(char));
    newvehicle->license = (char*)calloc(strlen(license)+1, sizeof(char));
    
    strcpy(newvehicle->make,make);
    strcpy(newvehicle->model,model);
    newvehicle->year = year;
    strcpy(newvehicle->color,color);
    strcpy(newvehicle->license,license);
    
    
    
    if ((*head) == NULL || (strcmp((*head)->license , license) > 0))
    {
        newvehicle->next = (*head);
        (*head) = newvehicle;
        return 1;
    }
    
    
    while(crnt->next != NULL && (strcmp(crnt->next->license , license) < 0))
    {
        crnt = crnt->next;
    }
    
    if(strcmp(crnt->next->license, license) == 0)
        return 0;
    
    
    newvehicle->next = crnt->next;
    crnt->next = newvehicle;
    return 1;
}

[vart]

1. scanf("%s", &name2);
& should be removed

2. while(!feof(fload))
do not use feof for control loops - read FAQ

3. you do not check if the head->license is equivalent to the new license

4. if the new license is already present - you have a memory leak - no need to allocate memory

5. no need to cast calloc

[MiroMage]

Alright, I changed the !feof and the &name, but now it is not reading anything from the file at all. I used this line:

while(fscanf(fload, "%s %s %d %s %s", make, model, &year, color, license) == 1)

Should I have used something different?

[matsp]

fscanf() returns the number of elements successfully read.

fscanf(fload, "%s %s %d %s %s", make, model, &year, color, license)

attempts to read 5 elements. The only time that function will return 1 is if ONLY make could be read (which usually would mean that you have a borked input file).

And whilst this is a good excercise in memory allocation, I would think that having some reasonable fixed length fields for make, model, colour, and license would make more sense. Dynamically allocated memory takes up some extra bytes, quite often 16 or 32 bytes on top of the actual memory you allocated plus it is rounded up to the nearest multiple of 8 or more.

So, to make an example, allocating memory for a short name like "Ford" will take up 5 bytes for the name, 3 bytes as it's rounded to 8 bytes, 4 bytes for the pointer, 16 bytes of overhead. That is 28 bytes. If we assume that most car names are less than 20 characters, then we SAVE 8 bytes by having a fixed array of 20 bytes [I don't know of any car manufacturer that has a longer name (although there may be several I haven't heard of), unless you write out BMW as "Bayerische Motoren Werke AG" or something equally silly.]

The models are likely to be about the same, whilst colour and license are shorter (unless you want the colour to be Green And Pink With Red Spots).

It is also a waste of time to call calloc when you are doing a strcpy immediately after, since strcpy will fill every byte of the memory you have allocated in itself. [Of course, if you make the change I just suggested, it would make sense to use calloc to allocate the vehicle struct itself].

--
Mats

[MiroMage]

Alright, I moved the code down into a function, and added some room up at the top such as:

char license[20];
char make[20]; 
char model[20];
char color[20];

This allows me to read the file, but as soon as I try to send it through the insert function, it just crashes again.

[matsp]

    while(crnt->next != NULL && (strcmp(crnt->next->license , license) < 0))
    {
        crnt = crnt->next;
    }
    
    if(strcmp(crnt->next->license, license) == 0)
        return 0;

If crnt->next is NULL, you will try to get license from a NULL-pointer when the loop ends.

Otherwise, add some code to indicate where you are going through the insert function, and print out as many pointers as you can. If that doesn't help, you probably have to post the code again, as it must have changed.

--
Mats

[rmetcalf]

There seems to be a lot of questions about linked lists on this board. This is how I approach them.

First, in order to simplify the explanation I am going to use a very simple structure.
It will only contain the key or search value and the reference to the next list item.

struct list_item {
char key[20];
struct list_item *next;
};

This structure could be expanded to any number of members of various types,
but keep it simple for now. After the list functions correctly you can then add
them.

I am going to assume that the key values in this list are to be unique. That's
kind of a trick statement, because without that constraint the list would be
ambigious. So ...

struct list_item *head;
char token[128];

head = NULL; /* initialize empty list */

if(f3 = fopen("source.txt","r")) != NULL){
  while(fscanf(f3,"%s",token) != EOF){

/* You don't have to pass in a pointer to head. head is already a pointer 
and as such the L value will be passed to the function */

    switch(finsert(head,token)){
      case 0:
         printf("%s inserted\n",token);
         break;
      case 1:
         printf("%s already in list\n",token);
         break;
      default:
         printf("What ????\n");
         break;
    }

  }
  fclose(f3):
}

finsert (head,token)

struct list_item *head;
char *token;

{

  struct list_item *current; /* pointer to the current place in the list */

  /* if haed is equal to NULL (empty list) add element */

  if(head == NULL){

    head = ((struct list_item *) (calloc(1, sizeof(struct list_item))));
    strcpy(head->key,token);
    head->next = NULL;
    return(0);

  } else {

    current = head; /* initialize current to the head of the list */

/* the loop to check if the value is already there. If it is return 1 */

    while(current != NULL){
      if(strcmp(token,current->key) == 0){
        return(1);
      }

/* if current->next equals NULL you are at the end of the list and should add the element otherwise continue to walk the list */

      if(current->next == NULL){
         current->next = ((struct list_item *) (calloc(1, sizeof(struct list_item))));
         strcpy(current->next->key,token);
         current->next->next = NULL;
         return(0);
      } else {
        current = current->next;
      }
    }
  }

}

[tabstop]

Does this work for you? Those pre-C89 compilers must have been something else, being able to modify the value of something passed in (like head in finsert).