From: Jens.Schmidt-HH@gmx.de   
      
   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    
   >   
   > 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! ]   
      
   --- SoupGate-Win32 v1.05   
    * Origin: you cannot sedate... all the things you hate (1:229/2)