C implementation of a singly linked list

I've done plenty of corrections of the previous code. Dangling pointers seem a fixed problem (correct me If I'm mistaken). The swap/reverse functions are going to be changed soon due to high time complexity. Also the LinkedList_Find() function is not enough specialized for different types of data, such as Person, because it simply compares void pointers: the intention was to write general purpose code, the choice of making this current linked list of type Person was done to show the adaptability of void* to all types (sorry for my influence of java generics :p). Any suggestions and corrections on design patterns and more efficient code are highly appreciated.

LinkedList.h

#ifndef NYO_LINKED_LIST
#define NYO_LINKED_LIST

#include "stdbool.h"
#include "stdio.h"
#include "stdlib.h"

typedef struct Node
{
    void *data;
    struct Node *next;

} Node;

typedef struct LinkedList {
    Node *head, *tail;
    int length;
} LinkedList;

bool LinkedList_Init(LinkedList*);
bool LinkedList_Add(LinkedList*, Node*);
Node* createNode(void*);
bool LinkedList_PushFront(LinkedList*, Node*);
void* LinkedList_GetDataAt(LinkedList*,int);
Node *getNode(LinkedList*,int);
bool LinkedList_DeleteLast(LinkedList*);
bool LinkedList_Find(LinkedList*,void*);
void LinkedList_Reverse(LinkedList*, void(*)(int*,int*));
void LinkedList_Clear(LinkedList*);
bool LinkedList_InsertAt(LinkedList*,int,void*);
bool LinkedList_DeleteAt(LinkedList*,int);
void LinkedList_Free(LinkedList*);

#endif

LinkedList.c

```
#include "LinkedList.h"
#define DEBUG
#undef DEBUG

bool LinkedList_Init(LinkedList *list)
{
list->head = NULL;
list->tail = list->head;
list->length = 0;
return true;

}

bool LinkedList_Add(LinkedList list, Node node)
{
if(list->head!=NULL)
{
list->tail->next = node;
}
else
{
list->head = node;
}

list->tail = node;
++list->length;
return true;
}

Node createNode(void data)
{
Node tmp = (Node

Casting

Generally speaking, you don't want to put explicit casts into your code unless they are actually needed. They usually just add noise to the code / make it harder to change the implementation in the future.

Since malloc returns a void, you don't need to cast it for example. So, this:

Node *tmp = (Node*)malloc(sizeof(Node));

Can just be:

Node *tmp = malloc(sizeof(Node));

You don't need to cast here:

LinkedList_Add(list,createNode((Person*)&defaultUsers[i]));

Because the compile knows what type defaultUsers is, it should simply be:

LinkedList_Add(list,createNode(&defaultUsers[i]));

Init

In its current form init feels dangerous and incomplete. Is it likely that the user is going to want to create a list without initialising it? It doesn't seem likely to me. I'd prefer to see something like this:

LinkedList *LinkedList_Init()
{
    LinkedList *list = malloc(sizeof(LinkedList));

    if(list) 
    {    
        list->tail = list->head = NULL;
        list->length = 0;
    }
    return list;
}

Note: This won't work currently with your code, because of the way you have implemented DeleteLast.

Consider Your API

At the moment, the users of your LinkedList know everything about the structure, all of its members and the data structures that it relies on. You've indicated that you've done Java programming before. This approach is the equivalent of declaring all of your classes members as public. This might be what you want to do, but I would tend to avoid it unless there's a good reason for it.

A better version of your header files definitions might look like this:

typedef struct LinkedList LinkedList;

LinkedList *LinkedList_Create();
void* LinkedList_GetDataAt(LinkedList* list, int index);
bool LinkedList_DeleteLast(LinkedList* list);
bool LinkedList_Contains(LinkedList* list, void* data);
void LinkedList_Reverse(LinkedList* list);
void LinkedList_Clear(LinkedList* list);
bool LinkedList_InsertAt(LinkedList* list, int index, void* data);
bool LinkedList_DeleteAt(LinkedList*list, int index);
void LinkedList_Free(LinkedList* list);  
unsigned LinkedList_Size(LinkedList *list);    
bool LinkedList_Add(LinkedList*, void *data);

Things to notice:

• I've removed the Node datatype and removed it from all function declarations. This is an implementation detail of the list, there's no need for the clients to know about it.

• I've changed the name of LinkedList_Init to LinkedList_Create to reflect the fact that it creates + initialises the list in one step.

• I've added a Size function to return the size of the list. This helps to complete the API, so that the client need access to the contents of the LinkedList struct.

• I've renamed LinkedList_Find to LinkedList_Contains, since it seems to check if the item exists in the list, rather than returns it.

• I've added parameter names to the declarations. Adding a name gives a hint as to what the expected parameter might be for.

• I've removed the swap parameter from Reverse. As has been said by @kamoroso94, swap is unlikely to change and so shouldn't be provided by the client. It is really an implementation detail of the list that should be in the LinkedList.c file.

StackExchange Code Review Q#140822, answer score: 2