Re: fails to call destructor in a linked list
alexo wrote:
I'm starting to study C++ after a brief panoramic of the C language.
So I'm trying to give some advice beyond the explicitly asked questions.
#include <iostream>
using std::cout;
You are writing everything to cout. Don't do that, the error messages
and tracing belong to cerr or clog.
using std::endl;
// the node class containing data
class Node
{
public:
Node(int);
~Node();
Node *next;
next should not be public. The ability of an outsider of Node
to cause internal inconsistency is a clear warning.
int getValue() const { return itsDatum; }
private:
int itsDatum;
};
As others wrote, use the services of the standard library where
applicable. Re-inventing the wheel is redoing the mistakes of
others. For learning algorithms this is O.K., but for learning
the language you might find more appropriate tasks.
// constructor implementation
Node::Node(int itsDatum):
next(0),
itsDatum(itsDatum) {}
Dangerous to have the same name in a parameter and a member. As
soon as you modify the constructor to have some code inside {}
strange things will happen.
// destructor implementation
Node::~Node()
{
delete next;
cout << "node deleted." << endl;
}
Think about what will happen on the stack if your list
has not 4, but 4000000 elements.
void print(Node *);
void push(Node **, int);
void pushBack(Node **, int);
Again, these are typical candidates for member functions.
int main()
{
Node *head = 0; // head is the head of the empty list
push(&head, 1);
push(&head, 2);
push(&head, 3);
push(&head, 4);
print(head);
return 0;
}
// insert an element to the top of the list
void push(Node **phead, int value)
{
Node *n = new Node(value);
if(n != 0)
{
if(*phead == 0)
{
*phead = n;
}
else
{
n->next = *phead;
*phead = n;
}
You save a cheap assignment of null_ptr to next happening
only once for each list, while doing an expensive (on modern
processors) test for every insertion. The else-path works
in all cases.
}
else
cout << "\nmemory allocation error." << endl;
Writing error messages to some output is just like swallowing
exceptions. Let the caller know about the problem and do
something about it.
}
// pushes an element to the end of the list
You wrote this function, but didn't test it.
void pushBack(Node **phead, int value)
{
Node *temp = new Node(value);
if(temp != 0)
{
if(*phead == 0)
{
*phead = temp;
}
else
{
Node *index = *phead;
while(index->next != 0)
{
index = index->next;
}
index->next = temp;
}
Again an unnecessary distinction. For the inner if-else
I would just write
while (*phead != 0)
phead = &(*phead)->next;
*phead = temp;
}
else
{
cout << "memory allocation error.";
}
}
// prints the list checking wheter it is an empy list
void print(Node *index)
{
if(index != 0)
{
while(index != 0)
{
cout << index->getValue() << " ";
index = index->next;
}
cout << endl;
}
else
{
cout << "\athe list is empty." << endl;
}
}
Other comments:
Some functions do several things at once, while others a replicated
in more than on function. Try to restructure them:
for push and pushBack: allocation of the new node, finding the insertion
point, doing the insertion
for print: walking the list, formattion the output
--
Greetings,
Jens Schmidt
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]