Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The function asks the user for a value to search for, then deletes all matching values in the list.

The program runs flawlessly, but I can't figure out a simple/elegant way of separating cases for 'first node', 'last node', and 'intermediate node'.

Please help me make it more elegant and readable.

I'll post the relevant code block, followed by the whole program.

In the relevant function, the following code block is part of the switch-case construct I'm using (if user inputs 'v', it means search for a 'value' and delete, as opposed to 'delete the first node' or 'delete the last node').

case 'v':
            printf("\n\tEnter value to delete:\t");
            scanf("%d",&value);
            r=q;
            if(!q)//If linked list is empty
            {
                printf("\n\t\tLinked list is EMPTY\n");
                return;
            }
            else if(!(q->next))//If linked list consists of only one node
            {
                if((q->data)==value)
                {
                    free(q);
                    printf("\nValue found and deleted in position 1\n");
                    count=1;
                }
            }
            else//If linked list consists of more than one node
            {
                for(i=1;q;i++)
                {
                    if(q->data==value)
                    {
                        if(i==1)//for first node
                        {
                            q=q->next;
                            free(r);
                            *pp=q;
                            printf("\nValue found and deleted in position 1\n");
                            count=1;
                        }
                        else//for the rest of the nodes
                        {
                            printf("\nValue found and deleted in position %d\n",i);
                            r->next=q->next;
                            temp=q;
                            q=q->next;
                            free(temp);
                            count++;
                        }
                    }
                    else
                    {
                        r=q;
                        q=q->next;
                    }
                }
            }
            if(count==0)//If no matches are found
                printf("Value not found");
            break;

Complete program:

#include <stdio.h>
#include <stdlib.h>

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

int count(struct node *);
void traverse(struct node *);
void insert(struct node **);
void delete(struct node **);
void deletelist(struct node **);

int main()
{   char choice;
    struct node *head, **tohead;
    head=(struct node *)malloc (sizeof(struct node));
    head=NULL;
    tohead=&head;
    do
    {
        printf("\nChoose operation to perform:\n");
        printf("\tCount number of nodes(c):\n");
        printf("\tInsert a node(i)\n");
        printf("\tDelete a node(d)\n");
        printf("\tDelete the list(D)\n");
        printf("\tShow currenct linked list(s)\n");
        printf("\tQuit without saving changes(q):\t");
        scanf(" %c",&choice);
        switch(choice)
        {
            case 'i':
                insert(tohead);
                break;
            case 'c':
                printf("\nList count is %d\n",count(head));
                break;
            case 's':
                traverse(head);
                break;
            case 'q':
                printf("QUITTING\n");
                break;
            case 'd':
                delete(tohead);
                break;
            case 'D':
                deletelist(tohead);
                break;
            default:
                printf("\n\tINVALID CHOICE\n");
        }
    }while(choice!='q');
}

int count(struct node *p)
{
    int i;
    for(i=0;p;i++)
        p=p->next;
    return i;
}

void traverse(struct node *p)
{
    printf("\nLinked list looks like: ");

    if(!p)
    {
        printf("nothing. It's EMPTY");
    }
    else
    {   while(p)
        {
            printf("%d\t",p->data);
            p=p->next;
        }
    }
    printf("\n");
}

void insert(struct node **pp)
{
    int value,position;
    struct node *q;
    q=*pp;
    struct node *newnode=(struct node *)malloc(sizeof(struct node));
    printf("Insert what number?:\t");
    scanf("%d",&value);
    printf("In what position? Push '0' for last,'1' for first");
    printf("\n\t\tOR\nenter position no.:\t");
    scanf("%d",&position);
    newnode->data=value;
    if(position==1)
    {   newnode->next=q;
        *pp=newnode;
    }
    else if(position==0)
    {
        if(!q)
        {
            newnode->next=q;
            *pp=newnode;
        }
        else
        {
            while(q->next)
                q=q->next;
            q->next=newnode;
            newnode->next=NULL;
        }
    }
    else if((position>1)&&(position<=count(q)+1))
    {
        int i;
        for(i=1;i<position-1;i++)
            q=q->next;
        newnode->next=q->next;
        q->next=newnode;
    }
    else
        printf("\n\t\tINVALID POSITION\n");
}

void delete(struct node **pp)
{
    struct node *q,*r,*temp;
    q=*pp;
    int i,count=0,value,position;
    char choice;
    printf("\n\tPush 'v' to delete a specific value");
    printf("\n\t\t\tOR");
    printf("\n\t'1' to delete the first value");
    printf("\n\t'0' to delete the last value:\t");
    scanf(" %c",&choice);
    if(q==NULL)
    {   printf("List is EMPTY\n");
        return;
    }
    switch(choice)
    {
        case '1':
            *pp=q->next;
            free(q);
            break;
        case '0':
            while(q->next!=NULL)
            {
                r=q;
                q=q->next;
            }
            r->next=NULL;
            free(q);
            break;
        case 'v':
            printf("\n\tEnter value to delete:\t");
            scanf("%d",&value);
            r=q;
            if(!q)//If linked list is empty
            {
                printf("\n\t\tLinked list is EMPTY\n");
                return;
            }
            else if(!(q->next))//If linked list consists of only one node
            {
                if((q->data)==value)
                {
                    free(q);
                    printf("\nValue found and deleted in position 1\n");
                    count=1;
                }
            }
            else//If linked list consists of more than one node
            {
                for(i=1;q;i++)
                {
                    if(q->data==value)
                    {
                        if(i==1)//for first node
                        {
                            q=q->next;
                            free(r);
                            *pp=q;
                            printf("\nValue found and deleted in position 1\n");
                            count=1;
                        }
                        else//for the rest of the nodes
                        {
                            printf("\nValue found and deleted in position %d\n",i);
                            r->next=q->next;
                            temp=q;
                            q=q->next;
                            free(temp);
                            count++;
                        }
                    }
                    else
                    {
                        r=q;
                        q=q->next;
                    }
                }
            }
            if(count==0)//If no matches are found
                printf("Value not found");
            break;
        default:
            printf("\nBad choice");
    }return;
}

void deletelist(struct node **pp)
{
    struct node *p,*temp;
    p=*pp;
    while(p)
    {
        temp=p;
        p=p->next;
        free(temp);
    }
    *pp=NULL;
    printf("\nLIST DELETED\n");
}
share|improve this question
    
I'll kick this off by telling you your first two functional lines of code in main() leak memory. Not a good start. –  WhozCraig Jul 16 '14 at 5:05

1 Answer 1

You're making that specific task considerably more difficult than it needs to be. I've taken liberty to post an alternate to the entire function, and it should become obvious why. Regarding the specifics of the code you highlighted:

  • This is the big one: You're already given a pointer-to-pointer that addresses the head node pointer in entrance. Use that pointer to walk the actual pointers in the list; not just their values. Doing this will substantially clean up this code.

  • Make at least a reasonable attempt at validating the input parameter

  • The reporting separate-cases "position 1". This isn't needed. To report both the position of each deletion (1-based) as well as detect if a deletion took place, use two counters: one that maintains a position notice in the original list, the other to note how many items were actually discovered.

Doing the above as well as general cleanup gives you the following. You may find the logic in '0' and '1' handling interesting, as it demonstrates a rare and useful case of fall-through case logic in a switch-case construct:

void delete(struct node **pp)
{
    struct node *temp =  NULL;
    int i, count, value;
    char choice;

    // early exit on empty list
    if(!*pp)
    {
        printf("List is EMPTY\n");
        return;
    }

    printf("\n\tPush 'v' to delete a specific value");
    printf("\n\t\t\tOR");
    printf("\n\t'1' to delete the first value");
    printf("\n\t'0' to delete the last value:\t");
    scanf(" %c", &choice);

    switch(choice)
    {
        case '0':
            // note: intentionally falls through to '1'
            while ((*pp)->next)
                pp = &(*pp)->next;

        case '1':
            temp = *pp;
            *pp = temp->next;
            free(temp);
            break;

        case 'v':
            // we already know the list isn't empty so request value
            printf("Enter value to delete: ");
            if (scanf("%d", &value) != 1)
            {
                printf("Invalid input\n");
                break;
            }

            // apparently the reporting is 1-based, not 0-based
            for (i=1,count=0; *pp; ++i)
            {
                // if we find a match, free it. advance is built-in
                if ((*pp)->data == value)
                {
                    temp = *pp;
                    *pp = temp->next;
                    free(temp);
                    ++count;
                    printf("Value found and deleted from position %d\n", i);
                }
                else
                {   // advance over non-match
                    pp = &(*pp)->next;
                }
            }

            if (count == 0)
                printf("Value NOT found\n");

            break;

        default:
            printf("\nBad choice");
    }
}

It is possible to clean this up further, including writing three different functions and having them exclusively work on the list itself, leaving the UI and console interaction outside of this logic, but that would be well-beyond the scope of your question.

Best of luck

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.