Exception when deleting element from linked list

Question

I try to delete a word (char* value) from a linked list but I am getting an exception when I compare my word to the links list node word.

EXC_BAD_ACCESS (code=1, address=0xa00000002)

I'm not sure why this is happening, I would appreciate to get any solution to the problem.

LinkedList* DeleteWordElement(LinkedList* head, char* value){

    LinkedList *previous=head, *current=head->next;
    if (head == NULL)
        return head;
    if (head->data == value) 
    {
        LinkedList *temp = head;
        head = head->next;
        free(temp);
        return head;
    }
    while (previous!=NULL)
    {
        if (previous->data == value) // Exception
            break;
        current = previous;
        previous = previous->next;
    }
    if (previous != NULL)
        current->next = previous->next;
    free(previous);
    return head;
}

Answer 1

Before doing:

LinkedList *previous=head, *current=head->next;

you need to check if the head is NULL. Then you should use strcmp, instead of ==, to compare the C-string value:

strcmp compares the actual C-string content, while using == between two C-string is asking if these two char pointers point to the same position. (source)

Finally, you do not need to keep the previous pointer, the current->next pointer is enough:

LinkedList* DeleteWordElement(LinkedList* head, char* value){
    if (head == NULL)
        return head;

    if (strcmp(value, head->data) == 0){
       LinkedList *next = head->next;
       free(head);
       return next;
    }
    LinkedList *current=head;
    while (current->next != NULL){
        if (strcmp(value, current->next->data) == 0){
           LinkedList *next = current->next;
           current->next = current->next->next;
           free(next);
           break;
        }
        current = current->next;
    }
    return head;
}

The code can be shorten if you use the recursive version, albeit at the cost of performance:

LinkedList* DeleteWordElement(LinkedList* current, char* value){
    if(current != NULL){
       if (strcmp(value, current->data) == 0){
          LinkedList *result = current->next;
          free(current);
          return result;
       }
       current->next = DeleteWordElement(current->next, data);
    }
    return current;
}

Answer 2

As has already been pointed in the other answer and in the comments section, the expression

current=head->next

will cause undefined behavior when head == NULL.

Another problem is that the expression

head->data == value

does not compare the actual string contents, but instead it compares the pointers themselves (i.e. the memory addresses). This is not what you want, because the pointers will probably always be different, even when the string contents are identical. In order to compare the actual string contents, you must use the function strcmp instead.

The other answer already contains a solution to the problem. However, I would like to provide an alternate solution, which is shorter and more efficient for the computer, but is maybe harder to understand for a programmer, because it uses double pointers (i.e. pointers to pointers):

LinkedList* DeleteWordElement( LinkedList *head, char* value )
{
    LinkedList **pp = &head, *p;

    while ( (p=*pp) != NULL )
    {
        if ( strcmp( p->data, value ) == 0 )
        {
            //found node, so unlink and remove it
            *pp = p->next;
            free( p );
            break;
        }

        pp = &p->next;
    }

    return head;
}

As you can see, this solution only requires a single if statement, in contrast to the code in your answer which requires 4 if statements and the other answer, which requires 3 if statements. These additional if statements are not necessary when using double pointers, because the same code can handle all cases. Therefore, you don't need to introduce additional code paths for every single case. As a consequence, this solution also has no need for code duplication.

It is also worth mentioning that the function signature

LinkedList* DeleteWordElement(LinkedList* head, char* value)

is a bit inefficient. The return value is the new head, so the code that calls the function must update the list head based on the return value. It would be simpler if the code that calls the function simply passes the address of the list head pointer, so the function can update the list head itself, when necessary. That way, the code which calls the function won't have to do any additional work.

In order to accomplish this, you can change the function signature to the following:

void DeleteWordElement( LinkedList** pp_head, char* value )

Because the address of a pointer is a pointer to a pointer (i.e. a double pointer), you must now use ** instead of only *.

Also, now that you are no longer using the return value of the function, you may want to use it for something else. For example, you may want the function to return whether the value was found or not. Therefore, you may want to change the signature to the following:

bool DeleteWordElement( LinkedList** pp_head, char* value )

Now you can make the function return true when the value was found, otherwise return false to indicate that the value was not found. Note that you must #include <stdbool.h> to have access to bool, true and false.

Although we have made the function more powerful by adding the boolean return value, it is just as simple as what we had before.

bool DeleteWordElement( LinkedList **pp_head, char* value )
{
    LinkedList **pp = pp_head, *p;

    while ( (p=*pp) != NULL )
    {
        if ( strcmp( p->data, value ) == 0 )
        {
            //found node, so unlink and remove it, and then return true
            *pp = p->next;
            free( p );
            return true;
        }

        pp = &p->next;
    }

    //return false because we did not find any matching node in the list
    return false;
}