Graph Library in C

Could you guys please review the Graph Library I developed in C? It's also on GitHub

```
#include "graph.h"

/*Function:
* Graph_node_in_adjacency
*
* In this function we verify whether the node is
* present in adjacency List of a vertex
*
* Input:
* Graph_edges_t
* vertex_number_t
*
* Output:
* bool - True - IF Present
* False - IF not Present
*/
bool
Graph_node_in_adjacency(Graph_edges_t *adjacency_list, vertex_number_t node) {

Graph_edges_t *runner;

runner = adjacency_list;

while(runner != NULL) {
if (runner->target == node) {
return TRUE;
}
runner = runner->next;
}

return FALSE;
}

/*
* Function:
* Graph_has_edge
*
* In this function we validate whether
* there is a edge between
* two vetices
*
* Input:
* Graph_t
* vertex_number_t
* vertex_number_t
*
* Output:
* bool True adjacency_list,D);

}

/*
* Function:
* Graph_node_in_priority_list
*
* In this function we verify
* whether node is already present in
* priority_list before appending to
* priority_list
*
* Input
* Graph_priority_t
* vertex_number_t
*
* output
* True (If already present)
* False (If not present)
*/
bool
Graph_node_in_priority_list(Graph_priority_t *prio,
vertex_number_t node) {
Graph_priority_t *S;

S = prio;

while(S != NULL) {
if (S->vertex == node) {
return TRUE;
}
S = S->next;
}

return FALSE;
}

/* Function:
* Graph_dump_prio_list
*
* In this function we dump complete Priority
* List
*
* Input:
Graph_priority_t S
*
* Output:
* none
*/
void
Graph_dump_prio_list(Graph_priority_t *S) {

Graph_priority_t *runner = S;

if (S == NULL) {
LOG_DEBUG("Priority List is NULL");
return;
}

printf("\n");
while(S != NULL) {
printf(" Node -> %d\n",S->vertex);
S = S->next;
}
printf("\n");

return;
}

/*
* Function:
* Graph_

Sorry for the late answer, but I thought I'd provide some feedback anyways. I assume since it's a library that you want other people/projects to use it, so my comments are made with that assumption as a background.

Over all, your code is clear, consistent and well laid out. There's a few things I would change though.

I'll start with the header file:

/* 
 * Basic Includes
 */
#include 
#include 
#include 
#include 

You don't want these in the header file. This means that any source file including your header will also include these headers even if they have no use for it. Include what you must to make your header complete, but no more. Move the includes over to your implementation instead.

/*
 * List of typedef
 */
typedef struct graph_ Graph_t;
typedef struct graph_vertices_ Graph_vertices_t;
typedef struct graph_edges_ Graph_edges_t;
typedef struct graph_priority_ Graph_priority_t;

I'd drop the _tsuffixes. First they don't give you anything of value when reading the code, and second it is reserved by POSIX which may or may not matter to you. In general it's best to drop it.

In fact I would even drop the typedefs for structs, and just use the struct name whenever you need the type. I find it clearer, and it conveys useful information about what kind of type we're dealing with. (Linus seems to agree.) That's more a matter of taste though, and others may disagree.

typedef int bool;

This is bad, include the stdbool.h header instead to use the proper bool defined by your standard library.

struct graph_ {

If you decide to keep the typedefs for the structs, at least keep the name of the stuct and the typedef the same. They are in different namespaces in C, so they will not interfere with each other.

/*
 * Following Defines are to Make life easy
 */
#define LOG_ERR(f_, ...)   printf("%s:%d ERROR: "f_"\n",__FILE__, __LINE__, ## __VA_ARGS__)
#define LOG_INFO(f_, ...)  printf("%s:%d INFO:  "f_"\n",__FILE__, __LINE__, ## __VA_ARGS__)
#ifdef DEBUG
#define LOG_DEBUG(f_, ...) printf("%s:%d DEBUG: "f_"\n",__FILE__, __LINE__, ## __VA_ARGS__)
#else
#define LOG_DEBUG(f_, ...)
#endif /* DEBUG */

Logging from libraries is a bit of a pain. I would however try to find a different solution that this. As a user of the library I would want to control where logs would end up, as well as how they're formatted etc. Just blurting stuff out to stdout (or even stderr) may not work in all environments. In any case, it does not belong in the header file. It's internal to your module, so keep it in the implementation instead.

#define FALSE 0
#define TRUE  1

Again, prefer to use stdbool.h for better portability.

#define NaN     32767

This is a very generic name, I'd probably choose something more specific to your library. How about GRAPH_VALUE_NOT_AVAILABLE?

As for the source file:

Graph_t G = NULL;
G   =   (Graph_t *) malloc(sizeof(Graph_t));
if (G == NULL) {
    LOG_ERR("Unable to allocate memory");
    goto destroy;
}

This construct is repeated a few times. There's a few problems with it:

• Don't cast the result of malloc in C.

• The log doesn't really provide any value, just return NULL instead.

• The goto statement isn't really needed, and just complicates the control flow of the function.

I'd go for this instead:

Graph *
Graph_init_template() {
    struct Graph *g = malloc(sizeof(Graph_t));
    if (g) {
        // initialize variable
    }

    return g;
}

Much more compact code, initializes the variable to it's proper value right away instead of in two steps, no goto's, no unnecessary casts, one point of return.

Apart from that I think it looks good!

And don't worry, while this may seem like a lot of comments, it's all mostly minor things. Most of the problems is with the header, which could cause problems for anyone who wants to include your library into their own project. This again seems to come from the fact that you mix library code and client code into the same source file. The issues with the header file I've pointed out would probably become a lot more obvious if you tried to use the library from a separate project instead.

Still great work! Keep it up!

StackExchange Code Review Q#123810, answer score: 4