Problem deleting values from my linked list (C++)

[bluechimera]

Of course this isn't the whole program....
but it is a rather large slice of code.

Yes I am in college, I am not asking for someone to do this for me.. I think that is rather stupid... you don't learn anything and if you get a job and cannot do it because someone else did the work for you.. that is your stupidity :)

I understand most of this.
This uses templates, that is what the <T> is...
This compiles, and I can remove the head and tail nodes fine..
and in some strange cases I can remove the middle node IF it is directly to the right or left of the head or tail nodes.
if not.. it crashes on the line #59 that says "if(value == walk->data)"

any questions or comments are welcome and encouraged.

any help or pointers is appreciated!

[CODE=CPP]
// This will delete a value in the list specified by the user
void remove(T value)
{

// if the value is equal to the head
if(value == mHead->data)

{

// make a temp node to hold the value of the head
SNode<T> *temp = mHead;

// advance the the next value past the head and
// assign it to the head
mHead = mHead->next;

// delete the old head
delete temp;
}

// if the value is equal to the tail
else if(value == mTail->data)
{

// create two temp nodes for swapping values and
// assign temp the value of head
SNode<T> *temp = mHead, *temp2;

// if the head is the only value delete it and
// set it to NULL
if(temp->next == NULL)
{
delete temp;
mHead = NULL;
}
else
{

// while temp->next is not the end of list
while(temp->next != NULL)
{

// assign temp to temp2
temp2 = temp;

// temp is assigned to the temp->next
temp = temp->next;
}

// and delete temp
delete temp;

// set temp2-next equal to NULL
temp2->next = NULL;

// and assign the tail to what temp2 holds
mTail = temp2;
}
}
else
{

// Create 2 nodes, walk and temp
SNode<T> *walk = mHead, *temp;

// advance past head node
walk = walk->next;

// assign the head to walk->prev
walk->prev = mHead;

// while walk is not NULL
while(walk != NULL)
{

// if the user input matches the value in the current position
// CRASHES HERE!!!
if(value == walk->data)
{

// assign walk to temp
temp = walk;

// assign walk->prev to walk
walk = walk->prev;

// temp->next is assigned to walk->next
walk->next = temp->next;

//delete the temp node
delete temp, temp2;
}
walk = walk->next;
}
}
}
[/CODE]

[Savage]

What do you mean by crashes?
Do you mean it doesn't pass condition,or do you mean some memory problem happens?

Savage

[bluechimera]

What do you mean by crashes?
Do you mean it doesn't pass condition,or do you mean some memory problem happens?

Savage

I get this:
Unhandled exception at 0x001f4adf in Lab2-1.exe: 0xC0000005: Access violation reading location 0xdddddddd.

so, memory :)

I am off to bed, I work nightshift. thanks for the reply!
again, any help is appreciated
-Jason

[weaknessforcats]

This is a lotta code for a simple delete.

I suggest that your mHead and mTail be in a struct rather than global variables so you have more than one list in a program.

[code=c]
template<class T>
struct SNode
{
T value;
SNode* next;
};

template <class T>
struct LinkedList
{
SNode<T>* head;
SNode<T>* tail;

};
template <class T>
LinkedList<T>* CreateList()
{
LinkedList<T>* temp = new LinkedList<T>;
temp->head = 0;
temp->tail = 0;
return temp;
}
int main()
{
LinkedList<int> * theList = CreateList<int> ();
}
[/code]

This way you have a linked list variable instead of a bunch of nodes:
Now you pass theList to all of your linked list functions.

To delete a value from a list, you just walk your list until you ffind the value or until you run out of list. As you go, you have to save the address of the node previous to the one being deleted because you need to connect the previous node's next to the node to be deleted's next.
[code=c]
template <class T>
int Remove(LinkedLi st<T>* mylist, T value)
{
SNode<T> temp = mylist->head;
SNode<T> prev = 0;
while (temp->value != value)
{
prev = temp;
temp = temp->next;
if (!temp)
{
return 1; // node not found
}

}
//value has been found;
prev->next = temp->next;
//
//temp is now out of the list;
//
//Now if temp was the head of the list, then temp->next is the new head
if (temp == mylist->head)
{
mylist->head = temp->next
}
//Same for the tail
if (temp == mylist->tail)
{
mylist->tail = prev;
}
//Finally, delete the node
delete temp;
//
return 0; //success


}
[/code]

Of course, this is just a suggestion. I didn't actually run this code.

[bluechimera]

This is a lotta code for a simple delete.

I suggest that your mHead and mTail be in a struct rather than global variables so you have more than one list in a program.

[code=c]
template<class T>
struct SNode
{
T value;
SNode* next;
};

template <class T>
struct LinkedList
{
SNode<T>* head;
SNode<T>* tail;

};
template <class T>
LinkedList<T>* CreateList()
{
LinkedList<T>* temp = new LinkedList<T>;
temp->head = 0;
temp->tail = 0;
return temp;
}
int main()
{
LinkedList<int> * theList = CreateList<int> ();
}
[/code]

This way you have a linked list variable instead of a bunch of nodes:
Now you pass theList to all of your linked list functions.

To delete a value from a list, you just walk your list until you ffind the value or until you run out of list. As you go, you have to save the address of the node previous to the one being deleted because you need to connect the previous node's next to the node to be deleted's next.
[code=c]
template <class T>
int Remove(LinkedLi st<T>* mylist, T value)
{
SNode<T> temp = mylist->head;
SNode<T> prev = 0;
while (temp->value != value)
{
prev = temp;
temp = temp->next;
if (!temp)
{
return 1; // node not found
}

}
//value has been found;
prev->next = temp->next;
//
//temp is now out of the list;
//
//Now if temp was the head of the list, then temp->next is the new head
if (temp == mylist->head)
{
mylist->head = temp->next
}
//Same for the tail
if (temp == mylist->tail)
{
mylist->tail = prev;
}
//Finally, delete the node
delete temp;
//
return 0; //success


}
[/code]

Of course, this is just a suggestion. I didn't actually run this code.

Thank you for the suggestion, I will take this into consideration; however, I believe the assignment requires the mHead and mTail to be private templatized members of the class the function is residing.

maybe I can look at what you have suggested here and implement it into my code.
thank you!
-Jason

[Ganon11]

I agree with weaknessforcats - this is a lot of code for a delete. You shouldn't have to have a separate case for the head, tail, and other nodes. It should be one general case.

I looked through your code, and everything seemed OK until you started using walk and temp...it just seemed strange. There were a lot of assignments being made that either were unnecessary (like setting walk's prev value to mHead, even though it would already be set) or didn't make sense (setting temp->next to walk->next?). Maybe writing down your logic for this case and checking over that would be better - once you have your technique figured out on paper, then you can change your code.