Filling A Linked List With Data From File And Handling User Status Messages

Question

I have completed an university assignment on C. While the code is fully functional based on the specifications of the exercise, I like high-quality code and would like to ask for opinions on how can certain parts of it be improved. Be advised that I cannot change any function signature.

• The first part is the below load() function whose job is to create and fill a linked list based on entries of a file. If the specified file doesn't exist then a new file must be created. Unfortunately, due to the structure of the signature, I am forced to fclose() the file (opening / closing files are expensive operations) as I don't have any way to store the file pointer.

I don't quite like the double fopen() call there. Also, while I've found a way to detect when a file is blank or when it has wrong format, I still have the feeling that there are cases where undefined behavior can occur.

studentList* load(char *filename)
{
    studentList *list = newList();

    FILE *f = fopen(filename, "r");
    if (f == NULL)
    {
        f = fopen(filename, "w");
        fclose(f);
        printf("File %s not found. A new blank file has been created.\n",filename);
        return list;
    }


    fseek(f, 0, SEEK_END);
    if (ftell(f) != 0)
    {
        rewind(f);
        while (!feof(f))
        {
            student st;
            int res = fscanf(f, "%d%s", &st.id, st.name);
            if (res == 2)
                add(list, st);
        }
        fflush(f);
        fclose(f);
    }
    printf("File %s loaded successfully.\n",filename);
    return list;
}

• The second part is more generic and has to do with the way status messages are printed to the console for the user. Excluding load() where I don't have any other option due to the nature of the signature , the other functions that provide basic functionalities of a linked list like add() , delete() , find() etc. don't have any printf() or puts() as I consider that method a dirty code style and besides that, I want to have full control of when a status message is printed.

Instead, these functions return an status code int where then an other function takes that status message and prints the corresponding status message to the console. I have not found a better method to achieve this result. Any ideas in this part would be highly appreciated.

Example of my code:

int res = add(list, st);
printAddStudentResult(res);

Answer 1

The first thing that strikes me is that you're writing error and status messages to the standard output stream, when usually these go to stderr, the standard error stream.

I'm not sure it makes much sense to attempt to create an empty file if the file opening fails - why not leave this until it's time to save? What if the reason it failed is because the file doesn't have read permission for the process? And how do you know whether creating a new file succeeded or not?

I'd argue that it's better to return a null pointer if the file opening fails, so that client code can distinguish this from a successful load of an empty list:

if (f == NULL)
{
    /* NULL return signifies nothing could be read */
    return NULL;
}

studentList *list = newList();

I'm assuming newList() is an allocation function that returns a null pointer if the allocation fails; that means we shouldn't attempt to use it until we've checked it's a valid pointer.

while (!eof) is a common anti-pattern; what we want to do instead is more like while (fscanf(...)==2) - and that removes the need to measure the file size before reading the contents.

This line is clearly wrong

    fflush(f);

f is an input stream, so fflush(f) is Undefined Behaviour. Just remove this call.