Inside a loop a linked list seems to be ok but outside it is empty-CodePudding

I just started learning C, and I'm lost trying to read cars and their manufacturing years from a text file. The program says it doesn't find any cars when I try it with a .txt file containing the following rows:

Lada 1976
Ferrari 2005
Suzuki 1985
Volvo 1963
Toyota 1993
Honda 2011

I did a similar linked list program which read an integer from a user to be added to the list, instead of a file. I think this is completely identical, except now the program reads from a file. This goes wrong somewhere with the pointer first (my best guess from debugging prints I added to several places) or it could be that I have somehow managed to change the nodes only inside the while loop (another guess). Could someone point out what is wrong here?

#include <stdlib.h>
#include <stdio.h>

typedef struct car {
    char brand[15];
    int year; 
    struct car *prev; 
    struct car *next; 
} car;

void readCars(char *fname, car *newCar, car *first, car *last);
void printCars(car *ptr, car *first);
void freeMemory(car *ptr, car *first);

int main(int argc, char *argv[]) {
    car *first = NULL; // a pointer to the first node of the linked list
    car *last = NULL; // a pointer to the last node of the linked list
    car *newCar; // a pointer for new nodes
    car *ptr; // a pointer for iterating the linked list
    if(argc != 2) {
        printf("No filename provided.\n");
        exit(0);
    } 
    printf("Reading the file %s.\n", argv[1]);
    readCars(argv[1], newCar, first, last);
    printCars(ptr, first);
    freeMemory(ptr, first);
    printf("Program ended.\n");
    return(0);
}

void readCars(char *fname, car *newCar, car *first, car *last) {    
    FILE *tiedosto;
    char rivi[22];
    if ((tiedosto = fopen(fname, "r")) == NULL) {
        printf("Failed to open the file.\nProgram ended.\n");
        exit(0);
    }
    while (fgets(rivi, 22, tiedosto) != NULL) {
        if ((newCar = (car*)malloc(sizeof(car))) == NULL) { // allocate memory for a new node
                perror("Memory allocation failure.\n");
                exit(1);
        }
        sscanf(rivi, "%s %d", newCar->brand, &newCar->year); // set the car brand and age to the new node
        newCar->next = NULL; // set the pointer to next node to NULL as there is no next node
        newCar->prev = last; // set the pointer to the previous node to the previous last node (NULL if there was no previous)
        if (first == NULL) { 
            first = newCar; // the new node is the only node so it is the first node
            last = newCar; // the new node is the only node so it is also the last node
        } else {
            last->next = newCar; // the new node is next node of the previous last node
            last = newCar; // the new node is now the last node
        }
    }
    fclose(tiedosto);
    printf("File read into a linked list.\n");
}

void printCars(car *ptr, car *first) {
    if(first == NULL) {
        printf("No cars found.\n");
    } else {
        ptr = first;
        int count = 1;
        while (ptr != NULL) {
            printf("%d. car: %s from the year %d.\n", count, ptr->brand, ptr->year);
            count  = 1;
            ptr = ptr->next;
        }
    }
}

void freeMemory(car *ptr, car *first) { 
    ptr = first;
    while (ptr != NULL) {
        first = ptr->next;
        free(ptr);
        ptr = first;
    }
    printf("Memory freed.\n");
}

CodePudding user response：

The lines

if (first == NULL) { 
    first = newCar; // the new node is the only node so it is the first node
    last = newCar; // the new node is the only node so it is also the last node
} else {
    last->next = newCar; // the new node is next node of the previous last node
    last = newCar; // the new node is now the last node
}

in the function readCars only modify the variables first and last in the function readCars. They do not modify the variables first and last in the function main.

Therefore, the variable first in the function main will stay NULL, so, as far as that function is concerned, the linked list stays empty.

In order to solve the problem, the function main should not pass copies of the variables first and last to the function readCars, but should rather pass pointers (i.e. references) to these variables. So you should change the parameters of the function readCars to the following:

void readCars(char *fname, car *newCar, car **first, car **last)

You should also change the lines quoted above to the following:

if ( *first == NULL ) { 
    *first = newCar; // the new node is the only node so it is the first node
    *last = newCar; // the new node is the only node so it is also the last node
} else {
    (*last)->next = newCar; // the new node is next node of the previous last node
    (*last) = newCar; // the new node is now the last node
}

Also, the parameter newCar of the function readCars does not serve any purpose as a parameter. You are instead using it as a local variable, so you should declare it as such and remove it from the parameter list of the function.

EDIT: As pointed out in the comment below, you are doing the same thing with the parameter ptr in the functions printCars and freeMemory. You should remove that parameter from these functions too.

CodePudding user response：

My solution is:

#include <stdlib.h>
#include <stdio.h>

typedef struct car {
    char brand[15];
    int year; 
    struct car *prev; 
    struct car *next; 
} car;

void readCars(char *fname, car *newCar, car **first, car **last);
void printCars(car *ptr, car *first);
void freeMemory(car *ptr, car *first);

int main(int argc, char *argv[]) {
    car *first = NULL; // a pointer to the first node of the linked list
    car *last = NULL; // a pointer to the last node of the linked list
    car *newCar; // a pointer for new nodes
    car *ptr; // a pointer for iterating the linked list
    if(argc != 2) {
        printf("No filename provided.\n");
        exit(0);
    } 
    printf("Reading the file %s.\n", argv[1]);
    readCars(argv[1], newCar, &first, &last);
    printCars(ptr, first);
    freeMemory(ptr, first);
    printf("Program ended.\n");
    return(0);
}

void readCars(char *fname, car *newCar, car **first, car **last) {    
    FILE *tiedosto;
    char rivi[22];
    if ((tiedosto = fopen(fname, "r")) == NULL) {
        printf("Failed to open the file.\nProgram ended.\n");
        exit(0);
    }
    while (fgets(rivi, 22, tiedosto) != NULL) {
        if ((newCar = (car*)malloc(sizeof(car))) == NULL) { // allocate memory for a new node
                perror("Memory allocation failure.\n");
                exit(1);
        }
        sscanf(rivi, "%s %d", newCar->brand, &newCar->year); // set the  car brand and age to the new node
        newCar->next = NULL; // set the pointer to next node to NULL as  there is no next node
        newCar->prev = *last; // set the pointer to the previous node to  the previous last node (NULL if there was no previous)
        if (*first == NULL) {
            *first = newCar; // the new node is the only node so it is the  first node
            *last = newCar; // the new node is the only node so it is also  the last node
        } else {
            (*last)->next = newCar; // the new node is next node of the previous last node
            *last = newCar; // the new node is now the last node
        }
    }
    fclose(tiedosto);
    printf("File read into a linked list.\n");
}

void printCars(car *ptr, car *first) {
    if(first == NULL) {
        printf("No cars found.\n");
    } else {
        ptr = first;
        int count = 1;
        while (ptr != NULL) {
            printf("%d. car: %s from the year %d.\n", count, ptr->brand,  ptr->year);
            count  = 1;
            ptr = ptr->next;
        }
    }
}

void freeMemory(car *ptr, car *first) { 
    ptr = first;
    while (ptr != NULL) {
        first = ptr->next;
        free(ptr);
        ptr = first;
    }
    printf("Memory freed.\n");
}

The main difference is that readCars needs a pointer to pointer to first and last. In this way, when the functions ends, the value of first and last does not change to the initial value (in the case NULL). Let me know if you've understood.