ADT linked list not working

[mattdog]

I am using a linked list to make a to do list. I am having trouble with by Delete function in my implementation. The delete function is being passed a string data of the node i want to delete from the list. This function should find the item in the list and then delete the node but not creating a memory leak in the process. So if i delete an item from the middle of the list, I can still see the whole list when I print out the list.

bool List::Delete(string data)
{
Node *temp = new Node;
temp->data = data;
trailer = NULL;
cur = head;
while (cur != temp)
{
trailer = cur;
cur = cur->next;
}
trailer->next = cur->next;
if (cur->next == NULL)
{
trailer->next == NULL;

}
delete temp;
if (temp)
{
return false;
}
else
{
return true;
}

}

[kbw]

It's impossible to tell whether that code is incorrect or where to change it if we don't know what your data members you have and what they mean.

[mattdog]

Here is my header:

#include <string>
using namespace std;

struct Node
{
string data;
Node * next;
};

class List
{
public:
	List();
	~List();
	bool Insert(string);
	bool Delete(string);
	void Print();
	bool Edit(string, string);
private:
	Node * head;
	Node * cur;
	Node * trailer;
};

Here is the full implementation:

List::List():head(NULL)
{}
List::~List()
{
	Node *temp = head->next;
	while (head != NULL)
	{
		delete head;
		head = temp;
		temp = head->next;
	}
}
bool List::Insert(string data)
{
	Node* newNode = new Node;

	if (newNode == NULL)
	{
		cout << "Error: Memory Allocation Failed" << endl;
		return false;
	}

	newNode->data = data;
	if (head == NULL)
	{
		//cout << "head is Null" << endl;
		head = newNode;
		cout << head -> data << endl;
		newNode->next = NULL;
		return true;
	}
	cur = head;
	trailer = NULL;

	while (newNode->data > cur->data && cur -> next != NULL)
	{
		trailer = cur;
		cur = cur->next;

	}
	if (trailer == NULL)
	{
		newNode->next = head;
		head = newNode;
		return true;
	}
	if (cur->next == NULL)
	{
		cur->next = newNode;
		newNode->next = NULL;
		return true;
	}

	else
	{
		trailer->next = newNode;
		newNode->next = cur;
	}
		return true;

}


bool List::Delete(string data)
{
	Node *temp = new Node;
	temp->data = data;
	trailer = NULL;
	cur = head;
	while (cur != temp)
	{
		trailer = cur;
		cur = cur->next;
	}
	trailer->next = cur->next;
	if (cur->next == NULL)
	{
		trailer->next == NULL;

	}
	delete temp;
	if (temp)
	{
		return false;
	}
	else
	{
		return true;
	}

}
bool List::Edit(string dataDelete, string dataInsert)
{
	Delete(dataDelete);
	Insert(dataInsert);
	return true;
}
void List::Print()
{
	for (Node * Count = head; Count != NULL; Count = Count->next)
	{
		cout << Count->data << endl;
	}
}

[mattdog]

The text file that is used is:

pay bills
make dental appt
daily bible reading
get estimate on car repair
mail thank-you notes
take test

[ne555]

bool List::Delete(string data)
{
	Node *temp = new Node; //¿? You want to remove a node, ¿why are you creating another then?
	temp->data = data;
	trailer = NULL; //limit the scope of your variables. I don't see any good reason for them to be members of the class
	cur = head;
	while (cur != temp) //tautology
	{
		trailer = cur;
		cur = cur->next;
	}
	trailer->next = cur->next;
	if (cur->next == NULL)
	{
		trailer->next == NULL; //read the warnings (enable the warnings)

	}
	delete temp;
	if (temp) //`delete' would not set the pointer to null, tautology
	{
		return false;
	}
	else
	{
		return true;
	}

}

> The text file that is used
you kind of forgot the code that uses that file

[kbw]

If your Node had a constructor, it would remove all that initialization code.

new will never return null, so all that code that handles it will never be called. A modern compiler really should warn about it, but they don't.

List constructor doesn't initialize all its members. As ne555 said, you don't need all those members, just head as you have a single threaded linked list.

As looking up a node is possibly a common thing, you'd simplify your code if you had a private method Node* Find(const std::string& s) const, that did the lookup.

Delete would then be:

bool List::Delete(const std::string& data)
{
    if (Node* p = Find(data))
    {
        /* delete the node */
        return true;
    }

    return false;
}

Pass strings by const ref rather than by value. You're creating unnecessary copies.