Thread: Anyone good with linked list.....I am not....

1) Don't use NULL. "Real" C++ programmers use 0.

2)

Code:

nodeType *current = first->link;
nodeType *trail = first;

for (int i = 0; i < pos; i++)
{
	current = current->link;
	trail = trail->link;
}

That's a little confusing for me. You have a pointer to the first node(trail) and a pointer to the second node(current), and you are advancing them along the list. If pos is 2, then your for-loop will end after two loops, and you'll have a pointer to the 3rd element(trail) and a pointer to the 4th element(current). What's the point of advancing two pointers along the list? In addition, you need pointers to three nodes to remove a node: a pointer to the node before the node you want to remove, a pointer to the node you want to remove, and pointer to the node after the node you want to remove.

3) Why perform any calculations and declare variables prior to checking if the list is empty?

Code:

if (first == NULL)
{
	cout << "List is Empty" << endl;
}

4)

Code:

if (current->link != NULL)

current is already several nodes after the node you want to remove(see #1). Hence, current->link is even further down the list, and therefore is irrelevant. Well, not completely irrelevant because if you use the -> operator, that is equivalent to calling the dereference(*) operator and then the member access operator(.), which means you'll be dereferencing a null pointer if you are at the end of the list, and as a result your program will crash. Actually, your program can crash even earlier. What happens if the list has 3 nodes and you try to remove node 10? As you step through the list in your for-loop using ->, you will hit the end up of the list at some point(current will get there first), which means current will be equal to 0, and on the next loop dereferencing current using -> will cause your program to crash.

When you step through a linked list, there has to be a loop conditional that checks whether you have reached the end of the list.