How can I simplify following C codes?

Question

I am currently working on a library management system project.

I have implemented a hash table for storing book information and a hash table for storing student information.

in two of the modules in this project named as book.h/.c and student.h/.c, I have two identical functions which free the memory.

Is there any way I can rewrite these two functions as one function in my shared_func.h/.c and call it in both module without including header files of both modules in shared_func.h/.c

Any help will be appreciated.

book.h

    typedef struct bnode {
        ...
        some variables
        struct bnode *next;

    } Book;

    void free_all_books();

book.c

#include "book.h"
static Student *book_table[MAX_SIZE] = {NULL};
....

void free_all_books()
    {
       for( i = 0; i < MAX_SIZE; i++ ) {
           Book *head = book_table[i];
           if( head == NULL ) {
               continue;
           } else {
               Book *temp;
               while ( head != NULL ) {
                   temp = head;
                   head = head->next;
                   free(temp);
               }
           }
       }
    }

student.h

       typedef struct snode {
           ...
           some variables
           struct snode *next;

        } Student;

    void free_all_students();

student.c

#include "student.h"
static Student *student_table[MAX_SIZE] = {NULL};
    ....

    void free_all_students()
    {
        for( i = 0; i < MAX_SIZE; i++ ) {
            Student *head = student_table[i];
            if( head == NULL ) {
                continue;
            } else {
                Student *temp;
                while ( head != NULL ) {
                    temp = head;
                    head = head->next;
                    free(temp);
                }
            }
        }
    }

The code isn't identical — one has `Book *` and the other `Student *`. You've not shown the `next` fields in the structures; it probably doesn't matter, but ... Your functions are misnamed: `free_student()` does not free a single student so it should be `free_all_students()` or thereabouts, and similarly for `free_book()`. — Jonathan Leffler, Apr 11 '19 at 21:17
Thank you Jonathan for correcting some of my codes. I have just updated the codes. I was just wondering if there was a way to simplify the code. — G-star, Apr 11 '19 at 21:25
While it would be possible to make the code 'generic', the effort involved in doing so is large enough and the benefit in doing so small enough (if not a negative benefit — it is likely to be harmful) that it is not worth doing, IMO. It might become an issue if you have tens or hundreds of types of hash tables, each with its own structure type and its own 'pointer to next' type. — Jonathan Leffler, Apr 11 '19 at 21:33
It would be useful not to completely elide your structures, in the code you explicitly reference the `next` member; you might at least include that. — Clifford, Apr 11 '19 at 21:34
An alternative uses a more generic hash table, which stores a `void *` to the data, a hash value, and some other information (most notably, offset to next pointer in the lists) — so you store pointers to `void` and free those. That could be better. It gets more complex if the structures being freed need their own custom function to free the memory for embedded pointers (variable length string fields, etc). — Jonathan Leffler, Apr 11 '19 at 21:34
The hoops you have to jump through to make this work may make you wish you were using C++, but it is not so much work to be not worthwhile. Even without that, your code can be simplified in other ways - the _if( null ) continue else remove_ pattern is the same as _if( !null ) remove_ for example; then you will see that you have a needless _if( !null )_ preceding a _while(null)_. And you need ot set the table elements to null. — Clifford, Apr 11 '19 at 21:55
Take a look at [intrusive linked lists](https://stackoverflow.com/questions/3361145/intrusive-lists), which give you both performance (no indirection with `void *` pointers) and code reuse (you can write a single `list_free` method for both books and students), at the cost of some type safety. — Andrew Sun, Apr 11 '19 at 22:43
it is very important to keep your code 'simple'. The changes your requesting makes your code not 'simple' I would recommend not making changes to try to 'simplify' your code — user3629249, Apr 12 '19 at 21:44

Clifford · Accepted Answer · 2019-04-11T22:07:10.950

The address of the first member of a structure is also the address of the structure, so you can exploit that fact by using a common header structure containing the next member and writing a function that will free generic nodes.

Given:

typedef struct node 
{
    struct node* next ;
} Node ;

Then :

typedef struct bnode 
{
    Node node ;
    ...
    some variables
    ...
} Book;

and

typedef struct bnode 
{
    Node node ;
    ...
    some variables
    ...
} Student ;

Then:

void free_all_nodes( Node* table, int max  )
{
    for( int i = 0; i < max; i++ ) 
    {
        Node* head = table[i] ;
        while ( head != NULL ) 
        {
            Node* temp = head ;
            head = head->next;
            free(temp);
        }

        table[i] = NULL ;
    }
}

and:

void free_all_books()
{
    free_all_nodes( (Node*)book_table, MAX_LEN ) ;
}

void free_all_students()
{
    free_all_nodes( (Node*)student_table, MAX_LEN ) ;
}

score 0 · Answer 2 · answered Apr 11 '19 at 21:55

There are a number of ways to approach this. This answer details a few of them.

Use `void *`

After everything is working and tested, one way you could approach this is to change your linked list to hold void * instead of either Book * or Student *. Then the linked list management functions can all be the same (adding nodes, traversing the list, deleting nodes), and it would be up to you, the programmer, to remember what kind of thing is stored in each list.

If you do that, I'd suggest simplifying the function for readability:

void list_delete(list *head) {
    while (head) {
        list *temp = head->next;
        free(head->data);
        free(head);
        head = temp;
    }
}

void free_all_lists()
{
    for( i = 0; i < MAX_SIZE; i++ ) {
        list_delete(table[i]);
        table[i] = NULL;
    }
}

Note also, that each node's data in this scheme is assumed to be a pointer that is separately allocated and therefore separately freed.

Don't worry about it

Another option is just to make sure that the code works and to not worry about the duplication. If the code is relatively simple, the penalty (in terms of complexity, maintenance effort and code size) will also be relatively small.

Use C++

Templated containers are an important part of the C++ language and standard and are very commonly for just this sort of application.

Hello Edward, Thank you for the effort you put in it to answer my question. I have just learnt from your answer that I shouldn't be worried about minimising the functions in this case. — G-star, Apr 12 '19 at 16:59

How can I simplify following C codes?

2 Answers2

Use void *

Don't worry about it

Use C++

Use `void *`