Skip to main content
1 of 2
Edward
  • 66.6k
  • 4
  • 118
  • 282

Here are some things that may help you improve your code.

Use NULL instead of 0 for pointers

The value 0 is an integer, but the value NULL is an implementation-defined null-pointer constant. It is not guaranteed to have the value 0.

Use include guards

There should be an include guard in the .h file. That is, start the file with:

#ifndef LIST_H
#define LIST_H
// file contents go here
#endif // LIST_H

Simplify your code

I'm not sure why the x_upcast and data_upcast code exists. Maybe the intent was to more cleanly separate the data type (an int here) from the rest of the code. However, consider that it could instead be written like this:

static struct Link *x_upcast(struct X *const x) {
    return (struct Link *)x;
}

Better though, in my opinion, would be to eliminate it entirely. The single place it's used is in ListForEach:

for(x = list->head.next; (next_x = x->next); x = next_x)
        action(&(x_upcast)(x)->data);

This could be more clearly written as:

for(x = list->head.next; (next_x = x->next); x = next_x)
        action(&((struct Link *)x)->data);

This also brings us to the next suggestion.

Use the appropriate data type

The code, as posted, appears to treat pointers to the data value and pointers to a Link identically. This is a problem because it misleads the reader. For example, the get_link code creates and partially initializes a Link but claims to be returning an int *. This would be much more clear if, instead, the code were to actually return a struct Link *. In other words, the interface should guide correct usage rather than encourage incorrect usage. As an example, this code compiles just fine:

int n = 99;
ListPush(&list, &n);

However this is a runtime disaster waiting to happen. We would probably prefer that it not even compile because what ListPush actually requires is a pointer to the data member of an already created struct Link. The next suggestion addresses this problem.

Rethink your interface

If LinkPush really requires a List and a Link, let's declare it that way. Instead of this:

void ListPush(struct List *const list, int *const add);

use this:

void ListPush(struct List *const list, struct Link *const newnode);

Now compiler will actually assist and point out bad usage like the code mentioned previously. This eliminates the type name X and also requires some redefinitions of other things such as List and Action which now look like this:

struct List { struct Link head, tail; };
typedef void (*Action)(struct Link *const);

Use better names

The type name List is good, but the type name X is not. The first name explains something about what the variable means within the context of the code, but the latter is opaque and non-descriptive.

Better describe the responsibilities of the data structure

It is quite important to note that this implementation of a linked list assumes that some other entity is creating (and presumably deleting) its nodes. It gives the user some flexibility, as it would allow the use of statically or dynamically allocated memory, but it's worth explicitly mentioning to the user of the code in a comment.

Edward
  • 66.6k
  • 4
  • 118
  • 282