Thread: Cleaning up the logic on linked list deletion

I have a doubly linked list from which I need to occasionally delete elements (any elements that have a type value different from 0). The function I have seems to work, but looking at it, I think I could probably make the logic cleaner. If anyone has some suggestions for cutting down on redundant code to make this more readable, I would appreciate it.

The structs in questions are declared here, with irrelevent variabled omitted:

Code:

struct Event
{
    //...
    int type;
    //...
    struct Event *next;
    struct Event *prev;
};
typedef struct Event event;

Code:

event *delete_bad_events(event *head)
{
    event *newhead;
    event *current;
    event *tempnext;
    event *tempprev;
    event *temp;
    current = head;
    newhead = head;
    while (current)
    {
        if (current->type != 0)
        {
           tempnext = current->next;
           tempprev = current->prev;
           temp = current;
           current = current->next;
           free_single_event(temp);
           if (tempprev && tempnext)
           {
               tempprev->next = tempnext;
               tempnext->prev = tempprev;
           }
           else if (!tempprev && tempnext) //head of the list
           {
               newhead = tempnext;
               tempnext->prev = NULL;
           }
           else if (tempprev && !tempnext) //end of the list
           {
               tempprev->next = NULL;
           }
           else if (!tempprev && !tempnext) //singleton list, odd case
           {
               newhead = NULL;
           }
        }
        else
        {
            current = current->next;
        }
    }
    return newhead;
}

Why not create a "new" list that does not have any such events? While doing this, you can call free_single_event on those elements found to be bad events.

EDIT:
Oh, but since this is doubly linked, perhaps the idea isn't that great since you need to link to the previous too, and if there are few bad events, the cost could be relatively high.

Yea, you can certainly make that work, but my pseudocode attempt on paper didn't look much cleaner. The special case of finding the new head when the head is a bad event makes it a bit ugly.

I was mostly just wondering if there was some glaring redundancy in my temporary variables, or a way to compress a few of those if cases into one. Seems like there should be, but maybe it's just an ugly problem.

alright, thanks for the suggestions - your suggested changes work as described. It always just bugs me to see code with lots of corner cases, but I guess I'll have to live with this one

Actually, a potential problem with freeing after the if block:

Code:

if (temp->prev && temp->next)
           {
               temp->prev->next = temp->next;
               temp->next->prev = temp->prev;
           }
           else if (!temp->prev && temp->next) //head of the list
           {
               newhead = temp->next;
               temp->next->prev = NULL;
           }
           else if (temp->prev && temp->next) //end of the list
           {
               temp->prev->next = NULL;
           }
           else if (!temp->prev && !temp->next) //singleton list, odd case
           {
               newhead = NULL;
           }
           free_single_event(temp);

that line just sets temp = NULL, which I then call free on, which does nothing. So isn't that a memory leak if I don't free before the block?

Interesting, I had misunderstood something basic about pointer assignment apparently. Thanks for the help!