Doubly linked list implementation in C I'm quite new to C, and I'm looking for feedback on this (doubly) linked list code.

Once I get to the add and remove methods, I feel like the code goes downhill. I'm looking for any potential errors I haven't spotted, and improvements that could be made for efficiency/tidiness.

struct node {
    int data;
    struct node *next;
    struct node *previous;
};

struct listset {
    struct node *head;
    struct node *tail;
    int current_elements;
};

struct listset * new_listset(){
    struct listset *new_list = malloc(sizeof(struct listset));
    new_list -&gt;head = NULL;
    new_list -&gt;tail = NULL;

    return new_list;
};

struct node *listset_lookup(struct listset *this, int item){
    struct node *curr_elem = this-&gt;head;
    if(!curr_elem) return NULL;
    while(item != curr_elem-&gt;data){
        if(curr_elem-&gt;next==NULL)return NULL;
        curr_elem = curr_elem-&gt;next;
    }
    return curr_elem;
}

void listset_add(struct listset *this, int item){
    struct node *new_node = malloc(sizeof(struct node));
    new_node -&gt; data = item;
    new_node -&gt;next = NULL;
    if(!this-&gt;head) {
        new_node-&gt;previous = NULL;
        this-&gt;head = this-&gt;tail = new_node;
    }
    else{
        this-&gt;tail-&gt;next = new_node;
    }
    this-&gt;current_elements++;
}
void listset_remove(struct listset *this, int item){
    struct node *elem_remove = listset_lookup(this, item);
    if(!elem_remove) return;
    if(elem_remove-&gt;next &amp;&amp; elem_remove-&gt;previous) elem_remove-&gt;previous-&gt;next = elem_remove-&gt;next;
    else if (elem_remove-&gt;previous &amp;&amp; !elem_remove-&gt;previous){
        elem_remove-&gt;previous-&gt;next=NULL;
        this-&gt;tail = elem_remove-&gt;previous;
    }
    else{
        this-&gt;tail = NULL;
        this-&gt;head = NULL;
    }
    free(elem_remove);
}
Your code is actually "off-topic" because it doesn't work.  You should really
include a short main that illustrates its use and allows it to be tested.  All the same, here are some comments.

Be consistent with spacing.  My preference is for no spaces around -&gt; and
for a space after keywords (if, while etc).

Variable naming.  Your names are not to my taste, often too long:


Rename previous as prev, current_elements as n_elements
Rename this globally as list - this means nothing whereas list is
clearly a list.
Rename item as data.  It is called data within the node structure so
why not be consistent?
new_listset would perhaps be more consistent with the other functions if
named listset_new.  It also needs a void parameter list.


Function listset_lookup tests the wrong condition in the loop.  Better to
test for the end of the list:

struct node *listset_lookup(const struct listset *list, int data)
{
    struct node *n = list-&gt;head;
    while (n) {
        if (data == n-&gt;data) {
            break;
        }
        n = n-&gt;next;
    }
    return n;
}


Notice also that the parameter list is const (as you don't alter it).  Also
note that writing this with a short variable name makes it much more readable
compared to the dense text that results from a longer name.  In a ten line
function with only 3 variables there is really nothing wrong (and a lot right)
with using such a short name.

Your listset_add is missing two terms in the else clause and should test
for malloc failure (and return something to show whether it failed).  I
would also rename new_node as simple n - again, it is a short function (as
most should be) and so this is ok.

static struct node* listset_add(struct listset *list, int data)
{
    struct node *n = malloc(sizeof(struct node));
    if (n) {
        n-&gt;data = data;
        n-&gt;next = NULL;
        if (!list-&gt;head) {
            n-&gt;prev = NULL;
            list-&gt;head = list-&gt;tail = n;
        } else {
            n-&gt;prev = list-&gt;tail; // new
            list-&gt;tail-&gt;next = n;
            list-&gt;tail = n;       // new
        }
        list-&gt;n_elements++;
    }
    return n;
}


Your listset_remove is hard to read and wrong.  Again, a shorter variable
name would help.  You should also avoid long lines such as:

if(elem_remove-&gt;next &amp;&amp; elem_remove-&gt;previous) elem_remove-&gt;previous-&gt;next = elem_remove-&gt;next;


This is much clearer as:

if (n-&gt;next &amp;&amp; n-&gt;previous) {
    n-&gt;previous-&gt;next = n-&gt;next;
}


All the same, it is perhaps wrong.  And this:

else if (elem_remove-&gt;previous &amp;&amp; !elem_remove-&gt;previous){


is quite clearly not sensible.  You also don't decrement the element count.
The correct function is:

static void listset_remove(struct listset *list, int data)
{
    struct node *n = listset_lookup(list, data);
    if (!n) return;

    if (n-&gt;prev) {
        n-&gt;prev-&gt;next = n-&gt;next;
    } else {
        list-&gt;head = n-&gt;next;
    }
    if (n-&gt;next) {
        n-&gt;next-&gt;prev = n-&gt;prev;
    } else {
        list-&gt;tail = n-&gt;prev;
    }
    list-&gt;n_elements--;
    free(n);
}


Here's what I used to test it:

static void listset_print(const struct listset *list)
{
    struct node *n = list-&gt;head;
    while (n) {
        printf("%d ", n-&gt;data);
        n = n-&gt;next;
    }
    printf(" : ");

    n = list-&gt;tail;
    while (n) {
        printf("%d ", n-&gt;data);
        n = n-&gt;previous;
    }
    printf("\n");
}

int main(void)
{
    struct listset *list = new_listset();
    listset_add(list, 1);
    listset_add(list, 2);
    listset_add(list, 3);
    listset_add(list, 4);
    listset_add(list, 5);
    listset_print(list);
    listset_remove(list, 6);
    listset_print(list);
    listset_remove(list, 1);
    listset_print(list);
    listset_remove(list, 5);
    listset_print(list);
    listset_remove(list, 3);
    listset_print(list);
    listset_remove(list, 2);
    listset_print(list);
    listset_remove(list, 4);
    listset_print(list);
    return 0;
}