Deleting Duplicates From a Doubly Linked List in C

Question

I am trying to work with a doubly linked list of SAT grades between 200-800. I need to remove from all the duplicates from the list, i.e. make sure each grade appears only once by deleting all its duplicates.

#define HIGHEST_GRADE 800

typedef struct dListNode{
    int* dataPtr;
    struct dListNode* next;
    struct dListNode* prev;
}DListNode;

typedef struct dList

{
    DListNode* head;
    DListNode* tail;
}DList;

void removeDuplicates(DList* lst)
{
    int i;
    int gradesBucket [numOfGrades];
    DListNode* temp;
    temp = lst->head;

    for(i=200 ; i<HIGHEST_GRADE ; i++) /*creating 600 buckets - each bucket for a grade*/
        gradesBucket[i] = FALSE;

    while (temp)
    {
        if ((gradesBucket [*temp->dataPtr]) == TRUE) /*if current grade has already  */
                                                     /* appeared earlier on the list */
        {
            deleteFromList (temp);  /*delete that grade's cell*/
        }
        else
            gradesBucket[*temp->dataPtr] = TRUE; /* mark grade bucket as true, meaning */
                                                 /* the grade already appeared*/
        temp = temp->next; /*moving on to next grade*/
    }
}

void deleteFromList(DListNode*  toRemove)
{
    toRemove->prev->next = toRemove->next;
    toRemove->next->prev = toRemove->prev;

    deAllocateListCell (toRemove);    
}

void deAllocateListCell (DListNode* cell)
{
    free (cell->dataPtr);
    free (cell);
}

Please help me understand what's wrong.

here's the fixed code, which still doesn't work properly. Now it compiles but nothing is shown on screen. And by the way, I dont need to take care of deleting the head, because the first number can never be a duplicate... but I took care of it in case the head was NULL;
I also send the previous cell of the one i want to delete, to the function deleteFromList. It still doesn't work. Any ideas? Thanks!

    void deleteFromList(DList* lst, DListNode*  p)
{

DListNode* del_cell = p->next;   /* cell to delete*/

if (p->next->next == NULL) /*if cell to remove is the tail*/
{
    deAllocateListCell (p->next); /* freeing current tail */
    lst->tail = p;  /* p is the new tail */
    p->next = NULL; /* tail points to NULL */
}
else /* if cell to remove is not the tail (note: can't be head beacuse no duplicates can be found in the first grade) */
{
    p->next = del_cell->next;
    del_cell->next->prev = p;
    deAllocateListCell (del_cell);
    }
}

what exactly does not work ? – giorashc Apr 17 '13 at 12:09 — giorashc, Apr 17 '13 at 12:09

score 2 · Answer 1 · answered Apr 17 '13 at 12:12

2

The code of your function deleteFromList() doesn't account for (literal) edge cases: Deleting the first or last node of a list.

Plus, your code dereferences a pointer to a deallocated node; the pointer can become outright invalid, or the free() function can overwrite its contents (as the Microsoft Debug C RunTime is known to).

answered Apr 17 '13 at 12:12

Medinoc

6,577
20
42

1

Just to expand on this. The first two lines in deleteFromList assume that ->prev and ->next are not null, you should check for this. Also after deleteFromList has been called, temp now points to an invalid deleted node. A better solution is to have deleteFromList return the next node for you, and then use either this value, or advance to the next node manually in your else condition – noggin182 Apr 17 '13 at 12:20
What @noggin182 said, but also, deleting the first element will not modify the `head` pointer. – Medinoc Apr 17 '13 at 12:25
Thank u first of all, I will try to take care the head and tail cases and will let u know – mac-man Apr 17 '13 at 22:31

ArielGro · Answer 2 · 2013-04-17T12:32:31.367

1

Try to be specific - What is it that doesn't work? Does your code compile? Do you get an error during runtime? You don't get the results you expected in a scenario?
Your deleteFromList function should take care of removing the head or tail (That is when toRemove->prev or toRemove->next are null (respectively).
temp = lst->head; and what happens when lst is null? You'll get a run-time error
You are not updating head or tail in case they are deleted

That's what I found in first glance.

edited Apr 17 '13 at 12:32

answered Apr 17 '13 at 12:23

ArielGro

795
1
11
24

1

Thank u first of all. The code does not compile and gets stuck.. I will try to take care the head and tail cases and will let u know – mac-man Apr 17 '13 at 22:31
@user2290597 OK, so can you provide the compilation error you get? Also, if you made changes to the code, I would suggest editing the question with the updated code.. – ArielGro Apr 21 '13 at 05:37

score 0 · Answer 3 · answered Apr 17 '13 at 12:21

0

you should write while(temp->next) for correcting this....and also u can simply deallocate the node using free. and to eliminate dangling pointer problem you should make it NULL after freeing that node

answered Apr 17 '13 at 12:21

umang2203

78
5

No! Changing it to while(temp->next) will just introduce a new problem and not fix anything. while(temp->next) will be exactly the same as it is currently, but miss the last node! – noggin182 Apr 17 '13 at 12:25
when temp->next will be NULL it will stop so it stops for the last node so that will be not the problem i think. and i am not understanding this clearly ` temp = lst->head` – umang2203 Apr 17 '13 at 12:34
when temp->next is NULL, is means there is no node AFTER the current one, the current node (temp) still needs to be processed though. The while condition is evaluated before each iteration – noggin182 Apr 17 '13 at 16:19
we can use do while loop for this i think. – umang2203 Apr 18 '13 at 03:49
Not really, in that case, when would you update the value of temp? it would need to be inside the while loop. so you either going to miss the first one or the last one – noggin182 Apr 18 '13 at 10:52

Deleting Duplicates From a Doubly Linked List in C

3 Answers3