LinkedList Remove function fails when deleting from the end of the list C++

Question

I have a LinkedList class and everything works except for my remove function. It works except for when it is removing from the end. If I remove from the end and then try to display the new list the program crashes and doesnt give me an error. I was wondering if someone could help me pinpoint what I did wrong in the remove function where I am removing the tail.

Main method

#include "LinkedList.h"
#include <cstdlib>
#include <iomanip>

using namespace std;

void MainMenu(int& menu);

int main(int argc, char** argv) {
    int menu = 0;
    string name;
    LinkedList list;
    while (true)
    {
        
        MainMenu(menu);
        if (menu == 1)
        {
            cout << "Please enter the name you want to add to the front: " << endl;
            cin >> name;
            list.addFront(name);
        }
        else if (menu == 2)
        {
            cout << "Please enter the name you want to add to the back: " << endl;
            cin >> name;
            list.addBack(name);
        }
        else if (menu == 3)
        {
            cout << "Please enter the name you want to remove: " << endl;
            cin >> name;
            list.remove(name);
        }
        else if (menu == 4)
        {
            list.display();
        }
        else if (menu == 5)
        {
            cout << "\nThank you for using my program!" << endl;
            exit(0);
        }
    }

    return 0;
}
void MainMenu(int& menu)
{
    cout << '\n' << endl;
    cout << left << setw(30) << right << setw(20) << "Main Menu" << endl;
    cout << setw(36) << setfill('*') << "\n" << endl;
    cout << "1. Add node to front." << endl;
    cout << "2. Add node to back." << endl;
    cout << "3. Remove node." << endl;
    cout << "4. Display list." << endl;
    cout << "5. Exit" << endl;
    cout << setw(36) << "\n" << endl;
    cout << "Please enter a menu number to proceed: " << endl;
    cin >> menu;
    while (menu != 1 && menu != 2 && menu != 3 && menu != 4 && menu != 5)
    {
        cout << "Menu options are 1 - 5" << endl;
        cout << "Please enter a menu number to proceed: " << endl;
        cin.clear();
        cin.ignore(100, '\n');
        cin >> menu;
    }
    cout << setfill(' ') << "\n" << endl;
}

Header file

#include <iostream>

using namespace std;

#ifndef LINKEDLIST_H
#define LINKEDLIST_H
struct Node
{
    string name;
    Node * next;
};

class LinkedList {
public:
    Node * head = nullptr;
    Node * tail = nullptr;
    Node * current = nullptr;
    void addFront(string name);
    void addBack(string name);
    void remove(string name);
    void display();
    LinkedList();
    LinkedList(const LinkedList& orig);
    virtual ~LinkedList();
private:

};

#endif /* LINKEDLIST_H */

Source File

#include "LinkedList.h"

void LinkedList::addFront(string name)
{
    Node * node = new Node();
    node->name = name;
    if (head != nullptr)
    {
        node->next = head;
        head = node;
    }
    else
    {
        head = node;
        tail = node;
    }
}
void LinkedList::addBack(string name)
{
    Node * node = new Node();
    node->name = name;
    node->next = nullptr;
    if (head == nullptr)
    {
        head = node;
        tail = node;
    }
    else
    {
      
        node->next = nullptr;
        tail->next = node;
        tail = node;
    }
}

void LinkedList::remove(string name)
{
    Node * temp = head;
    while (temp != nullptr)
    {
        if (temp->name == name)
        {
            cout << "Name found! Being removed." << endl;
            break;
        }
        temp = temp->next;
        if (temp == nullptr)
        {
            cout << "Name not found!" << endl;
            return;
        }
    } 
    temp = head;
    if(head == nullptr)
    {
        cout << "The list is empty." << endl;
        return;
    }
    while(!(temp->name == name))
    {
        temp = temp->next;
    }
    if (temp == tail && temp == head)
    {
        head = nullptr;
        tail = nullptr;
        delete temp;
    }
    else if (temp == tail)
    {
        temp->next = tail;
        delete temp->next;
        temp->next = nullptr;
        tail = temp;
    }
    else if(temp == head)
    {
        temp = head;
        head = temp->next;
 
        delete temp;
    }
    else
    {
        temp = head;
        while(!(temp->next->name == name))
        {
            temp = temp->next;
        }
        Node * next = temp->next->next;
        delete temp->next;
        temp->next = next;  
        
    }
    
}
void LinkedList::display()
{
    current = head;
    if (current == nullptr)
    {
        cout << "The list is empty!" << endl;
    }
    while (current != nullptr)
    {
        cout << current->name << endl;
        current = current->next;
    }
}
LinkedList::LinkedList() {
}

LinkedList::LinkedList(const LinkedList& orig) {
}

LinkedList::~LinkedList() {
}

Answer 1

Your main problem is this section here

else if (temp == tail)
    {
        temp->next = tail;
        delete temp->next;
        temp->next = nullptr;
        tail = temp;
    }

That is what executes if the match is the last item. But if it is the last item then temp->next will be tail because you just set it to tail, and since temp == tail that also invalidates temp. Setting temp->next will be using a deleted pointer. There's no fixing this at this point because you need the previous node to set tail to, and you didn't save it, so you'd have to loop through again to get it.

Now, for the solution, you should do a single loop. No need to loop multiple times (3 in most cases)

void LinkedList::remove(string name)
{
    Node * temp = head;
    Node * prev = nullptr;
    while (temp != nullptr)
    {
        if (temp->name == name)
        {
            cout << "Name found! Being removed." << endl;
            if (prev != nullptr) {
                prev->next = temp->next
            }
            else {
                head = temp->next;
            }
            if (tail == temp) {
                tail = prev;
            }
            delete temp;
            return;
        }
        prev = temp;
        temp = temp->next;
    } 
    cout << "Name not found!" << endl;
}

Answer 2

For starters everywhere in member function declarations where a parameter is declared like

string name

you should change it to

const string &name

Secondly the member function remove shall not issue any message. It is the caller of the function that decides whether to output a message.

Your function is inefficient because it traverses the list two or even three times in loops and has too many conditions that makes the function too complicated. As a result the function has bugs.

For example in this if statement

else if (temp == tail)
{
    temp->next = tail;
    delete temp->next;
    temp->next = nullptr;
    tail = temp;
}

there is used deleted pointer temp to access memory because temp is equal to tail (according to the condition of the if statement)

    temp->next = nullptr;

and tail again is set to the deleted pointer

    tail = temp;

The function can be declared and defined the following way

class LinkedList {
public:
    //...
    bool remove( const std::string &name );
};

bool LinkedList::remove( const std::string &name )
{
    Node *prev = nullptr;
    Node *current = head;
    
    while ( current && current->name != name )
    {
        prev = current;
        current = current->next;
    }
    
    bool success = current != nullptr;
    
    if ( success )
    {
        if ( prev )
        {
            prev->next = current->next;
            if ( prev->next == nullptr ) tail = prev;
        }
        else
        {
            head = head->next;
            if ( head == nullptr ) tail = nullptr;
        }
        
        delete current;
    }
    
    return success;
}