function to delete first node of linked list leaves weird text (c )-CodePudding

#include <iostream>
#include <string>
using namespace std;

struct book
{
    string title;
    book* next;
};

void printList(book* n){
    while (n != NULL){
        cout << n->title << endl;
        n=n->next;
    }
}

void delete_book(book *head, string title){
    cout << "TITLE TO DELETE:";
    getline(cin,title);

    // list is not empty
    if (head != NULL){
        book *temp1 = head;

        // the book to delete is the first one
        if (head->title == title){
            head = head->next;
            delete temp1;
        }
        
        // the book to delete is NOT the first
        else {
            book *temp2 = NULL; // initializing temp2
            while (temp1 != NULL && temp1->title != title){
                temp2 = temp1;
                temp1 = temp1->next;
            }
            if (temp1 != NULL){
                // found the target title
                temp2->next = temp1->next;
                delete temp1;
            }
        }
    }
}


int main() {
    book* head = new book();
    head->title = "The little prince";

    book* second = new book();
    second->title= "ABC";

    book* third = new book();
    third->title= "DEF";

    head->next=second;

    second->next=third;
    third->next=NULL;

    printList(head);

    delete_book(head," ");

    printList(head);
}

Whichever is the string of the head, the delete_book function compiles this kind of text ��A��e prince�?instead of actually deleting the node. The other functions compile correctly.

I changed the text in the string to check if it has something related to the characters and space, but it has not.

CodePudding user response：

void delete_book(book *head, string title)

Takes head by value. Yes, head is a pointer, but that means the book head points at is passed by reference. The pointer itself is passed by value, and if you remove the first item in the list you update the local copy of head and main is left working off of the original that's still pointing at the removed book.

Instead use

void delete_book(book * &head, string title)
                        ^ reference to pointer.

But... The solution also uses a parameter as a local variable, and that's a waste of a perfectly good parameter. As Paul McKenzie recommends in the comments, title should be passed in, not requested inside the function. A good reason for this is to keep the responsibilities of a function as close to one responsibility as possible. delete_book currently has two responsibilities: 1. Get name of book from user and 2. Remove the named book.

A solid secondary reason is less work. The program doesn't have to allocate and pass around an unused string.

void delete_book(book * & head, const string & title){
    // list is not empty
    if (head != NULL){
        book *temp1 = head;

        // the book to delete is the first one
        if (head->title == title){
            head = head->next;
            delete temp1;
        }
        
        // the book to delete is NOT the first
        else {
            book *temp2 = NULL; // initializing temp2
            while (temp1 != NULL && temp1->title != title){
                temp2 = temp1;
                temp1 = temp1->next;
            }
            if (temp1 != NULL){
                // found the target title
                temp2->next = temp1->next;
                delete temp1;
            }
        }
    }
}

and call it from main something like

cout << "TITLE TO DELETE:";
string to_remove;
if (getline(cin,to_remove)) // always test an IO transaction for success 
{
    delete_book(head, to_remove);
}

Next, we can simplify the logic greatly by taking advantage of the fact that head is really no different from next pointer. If we can get rid of the difference in the name, something we can easily do with a pointer to a pointer to a book, we don't need special head-handling code and can write something like

void delete_book(book * & head, const string & title){
    book **curpp = &head; // abstract away naming
    while (*curpp && (*curpp )->title!= title) // exit if no more items or item found
    {
        curpp = &(*curpp )->next; // if not, advance to the next item in the list
    }

    if (*curpp ) // Found item
    { // now KILL it!
        book * to_delete= *curpp ;
        *curpp = to_delete->next; //link previous item to next item 
        delete to_delete;
    }
}

And don't forget to free all of the remaining books before the program exits. Proper sanitation is a good habit to get into early.

CodePudding user response：

void delete_book(book* &head, string title) {
cout << "TITLE TO DELETE:";
getline(cin, title);

book* p = head;
book* pre;
while (p)
{
    if (p->title == title)
    {
        if (p==head)
        {
            head = head->next;
            delete p;
        }
        else
        {
            pre->next = p->next;
            delete p;
        }
        break;
    }
    pre = p;
    p = p->next;
}

}