Reading from file and storing as Linked List

Question

This is the code I have:

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

typedef struct island{
    char *name;
    struct island *nextIsland;
} island;

void printIslands(island *i){
    printf("%s\n",i->name);
    if(i->nextIsland != NULL){
        printIslands(i->nextIsland);
    }
}

island* fileReader(FILE *file){
    island *i = malloc(sizeof(island));
    char islandName[20];
    int fileRead = fscanf(file,"%s",islandName);
    if(fileRead != EOF){
        i->name = strdup(islandName);
        i->nextIsland = fileReader(file);
    }
    if(fileRead == EOF) {
        return NULL;
    }
    return i;
}

int main(){
    FILE *x = fopen("islands.txt","r");
    island *i = fileReader(x);
    printIslands(i);
    fclose(x);
}

Input file:

 islandone
 islandtwo
 islandthree
 islandfour
 islandfive

The output of the program will be exactly same with the input file, which is desired.

My question is, do you think fileReader is too complicated and fragile? How would you implement fileReader? Would you create a createIsland method and somehow use it in the fileReader?

Any comments welcome on the code.

Answer 1

I think you can simplify this. You test for EOF twice.

island* fileReader(FILE *file){
    island *i = malloc(sizeof(island));
    char islandName[20];
    int fileRead = fscanf(file,"%s",islandName);
    if(fileRead != EOF){
        i->name = strdup(islandName);
        i->nextIsland = fileReader(file);
    }
    if(fileRead == EOF) {
        return NULL;
    }
    return i;
}

If you re-arrange this so you only test once:

island* fileReader(FILE *file){

    island *i = malloc(sizeof(island));
    char islandName[20];
    int fileRead = fscanf(file,"%s",islandName);

    if(fileRead == EOF) {
        return NULL;
    }

    i->name = strdup(islandName);
    i->nextIsland = fileReader(file);
    return i;
}

Now that we have re-arranged it. It becomes more obvious that you are leaking i. You create i in the first half but don't use it till the second half (and there is an early return in there).

island* fileReader(FILE *file){

    char islandName[20];
    int fileRead = fscanf(file,"%s",islandName);

    if(fileRead == EOF) {
        return NULL;
    }

    island *i = malloc(sizeof(island));
    i->name = strdup(islandName);
    i->nextIsland = fileReader(file);

    return i;
}

After this re-arrange. Two things stick out.

1. islandName[20] has a max of 19 characters. This may be OK but you may want to consider not makeing this a limitation. But you should also change your scan statement to make sure you don't exceed this limit.
2. You don't check the return value of malloc

Code

island* fileReader(FILE *file){

    char islandName[20] = {};

    // Note: %*c is there to read the first space character
    //       after the island name. If the island name is
    //       too long then the read is abandoned (because we
    //       limit it to 19 characters) and only the %19s will
    //       have input and thus the returned value will be 1
    //       (rather than 2 on a successful read)
    int fileRead = fscanf(file,"%19s%*c",islandName);

    // If file name exceeds 20 characters then read (but ignore)
    // any remaining characters in the island name
    if (fileRead == 1) {
        fscanf(file,"%*s");  // Reads upto the next space
    }                        // The * means ignore the input.

    if(fileRead == EOF) {
        return NULL;
    }

    island *i = malloc(sizeof(island));
    // Check to make sure we allocated space.
    if (i != NULL) {
        i->name = strdup(islandName);
        if (i->name != NULL) {
            i->nextIsland = fileReader(file);
        }
        else {
            // If the strdup() failed.
            // Then your object is invalid so release and
            // set i to NULL so it becomes the last one.
            free(i);
            i = NULL;
        }
    }

    return i;
}

The other thing you need to watch out for is the recursion here. This may work well for your test cases. But a huge list of millions of islands is going to eat up your memory. Look to using a loop instead of recursion.

island* fileReader(FILE *file){

    island* head = getNextIsland(file);
    island* last = head;

    while(last != NULL) {
        last->next = getNextIsland(file);
        last = last->next;
    }
    return head;
}

Don't assume that the list is not empty!

void printIslands(island *i){
    printf("%s\n",i->name);          // breaks id i is NULL on first call
    if(i->nextIsland != NULL){
        printIslands(i->nextIsland);
    }
}

When writing recursive routines it is usual to check if you have reached the end as the first thing in the recursive function. Then do the body of the action. You don't need to check the next one as soon as you recurse the next call will then check.

void printIslands(island *i){

    if (i == NULL) {
        return;
    }
    printf("%s\n",i->name);
    printIslands(i->nextIsland);
}

Again do not be lulled into false sense of security by recursion prefer to use a loop.

void printIslands(island *i){

    for(;i != NULL; i = i->next) {
        printf("%s\n",i->name);
    }
}