I would like to know if the following code is 'good' I would like a serious, blunt and cold judgement on this code.  Would it be considered 'good' code?  In general, is it well written?  Is the design reasonable?

#include &lt;iostream&gt;   // C++ IO library code to include.
#include &lt;string&gt;     // C++ string functions.
using namespace std ; // Use standard C++ names for functions.


//====== class declaration.

class Tpacket_list
{public: // functions that are available to code outside the class.
    // put the command line into the list.
    Tpacket_list(int param_index, string param) ;  
    void print_list() ; // print the list.
    Tpacket_list* find( string target) ;
    int    tell_id() ;
    string tell_payload() ;

 protected:                //  Data in a class is normally hidden from other code.
    string payload ;       // C++ strings are classes so this is aggregation.
    int id ;               // Such strings are more powerful and easier to use
    Tpacket_list *next ;   //   than the older C string routines, also slower!
} ;


Tpacket_list *list_head ;  // Start of linked list, set to 1st item.
Tpacket_list *work_ptr ;   // Work pointer to work along list.

//====== Class Body =========================================================

//------ Constructor gets called when the class is created. It puts in the
//       payload and sets up the chain of pointers.All this is hideen from
//       the user of the class.
Tpacket_list::Tpacket_list(int param_index, string param) 
{//--- save information for this parameter.
   id = param_index ;
   payload = param ;
   cout &lt;&lt; "    Adding parameter # " &lt;&lt; id &lt;&lt; ", value = " &lt;&lt; payload &lt;&lt; endl ;
 //--- form list.
   next = list_head ;
   list_head = this ;
}

//------ Print the list.  Note how the function calls the print_list function
//       in the next item in the link list.
//       Function prints from the current link list item to the end.
void Tpacket_list::print_list()
{//---
   cout &lt;&lt; "    id = " &lt;&lt; id &lt;&lt; ",  payload = " &lt;&lt; payload &lt;&lt; ", length " &lt;&lt; payload.length() &lt;&lt; " characters."&lt;&lt; endl ;
   if ( next != NULL) // if there is a next item, get it to print itself.
     next-&gt;print_list() ;
}


//------ Find item in the list.
Tpacket_list* Tpacket_list::find( string target)
{//---
   if ( payload == target)
      return ( this) ;          // return pointer to current object.
   if ( next == NULL)
      return( NULL) ;           // end of list, not match, return NULL.
   //--- if got here there is a valid next.
   return( next-&gt;find( target)) ;
}

//------ Tell of internal data.
int    Tpacket_list::tell_id() 
 { return( id) ;
 }

string Tpacket_list::tell_payload()
 { return( payload) ;
 }

//====== Start program here =================================================

int main(int argc, char *argv[]) // argc = number strings on command line.
                                 // argv is array of strings each holding 
                                 //   a parameter.

{//--- Initialize link list with command parameters.
   cout &lt;&lt; endl &lt;&lt; "  Form link list of command line parameters." &lt;&lt; endl ;
   list_head = NULL ;
   int i = 1 ;
   while ( i &lt; argc)
    { new Tpacket_list(i, argv[i]) ;
      i++ ;
    }

 //--- print out the list.
   cout &lt;&lt; "  Print paramters from head of list the end of list." &lt;&lt; endl ;
   if ( list_head != NULL)
        list_head-&gt;print_list() ;
   else cout &lt;&lt; "   List is empty." &lt;&lt; endl ;

 //--- search for parameters
   cout &lt;&lt; "  Find first 0 parameter (working last to first parameter, list head to tail)." &lt;&lt; endl ;
   Tpacket_list *found ;
   if ( list_head != NULL)
     { found = list_head-&gt;find("0") ;
       if ( found != NULL)
            cout &lt;&lt; "    Found target on parameter # " &lt;&lt; found-&gt;tell_id()  &lt;&lt; endl ;
       else cout &lt;&lt; "    No \"0\" found." &lt;&lt; endl ;
     }

   cout &lt;&lt; endl ;
   return(0) ;
}


P.S. It's not my code, it's actually the lecturer's.
For overall quality, I'd give this about a 2 or maybe a 3 (on a scale of 1-10). Honestly, even 3 is being pretty generous though.

#include &lt;iostream&gt;   // C++ IO library code to include.
#include &lt;string&gt;     // C++ string functions.
using namespace std ; // Use standard C++ names for functions.


using namespace std; is generally frowned upon. There are reasons for using a namespace, but doing it globally like this (especially with the stdnamespace) is just a lousy idea.

//====== class declaration.


I'm sort of split on this one. On one hand, vacuous comments like this are generally worse than useless. At the same time, given that it's written exclusively for teaching, it may be excusable to include a few things we'd normally reject, describing basics of how C++ works. This would be a no-no in real code, but under the circumstances it's understandable and (I guess) reasonable.

class Tpacket_list
{public: // functions that are available to code outside the class.


Your teacher seems to be working hard at disproving the usual wisdom about formatting. Most people figure that the exact formatting you choose is less important than using it consistently. This is one of the few I've seen that really does seem to make the code more difficult to read.

    // put the command line into the list.
    Tpacket_list(int param_index, string param) ;  
    void print_list() ; // print the list.
    Tpacket_list* find( string target) ;
    int    tell_id() ;
    string tell_payload() ;


The comment here is misleading at best. The code to put the command line into the list is elsewhere (somewhere down in main).

 protected:                //  Data in a class is normally hidden from other code.
    string payload ;       // C++ strings are classes so this is aggregation.
    int id ;               // Such strings are more powerful and easier to use
    Tpacket_list *next ;   //   than the older C string routines, also slower!
} ;


Here we have a real design problem. He's conflating two ideas that should be completely separate: a linked list, and a node in a linked list. To work at all well, a linked list nearly needs to separate the two:

class list {    
    struct Node { 
        int id;
        std::string param;
        Node *next;

        Node(int param_index, std::string const &amp;p) :
            id(param_index), param(p)
        {}
    }

public:
    list(list const &amp;);
    // ...
private:
    node *list_head;
};


In this case, a node is pretty much just dumb data; the list class has all the real "knowledge" of how to do useful things. Also note that in reality, this should almost certainly be a class template instead of an actual class. Instead of hard-coding an int and a std::string as the types of data being stored, we really want to make those template parameters so we can create a linked list holding any kind of data we want.

Tpacket_list *list_head ;  // Start of linked list, set to 1st item.
Tpacket_list *work_ptr ;   // Work pointer to work along list.


"Daddy, he defined a global!"
"Son, what have I told you about using language like that?"

//====== Class Body =========================================================

//------ Constructor gets called when the class is created. It puts in the
//       payload and sets up the chain of pointers.All this is hideen from
//       the user of the class.


I'll repeat the earlier comment about comments for emphasis: while (barely) acceptable under the circumstances, these would clearly have no place in real code.

Tpacket_list::Tpacket_list(int param_index, string param) 
{//--- save information for this parameter.
   id = param_index ;
   payload = param ;
   cout &lt;&lt; "    Adding parameter # " &lt;&lt; id &lt;&lt; ", value = " &lt;&lt; payload &lt;&lt; endl ;
 //--- form list.
   next = list_head ;
   list_head = this ;
}


This code has quite a problems. First of all, you should generally prefer initializing the members in an initializer list:

Tpacket_list::Tpacket_list(int param_index, string param) :
    id(param_index),
    payload(param)


OTOH, per the earlier comment, this code should really be in Node::Node instead. From the viewpoint of the linked list, this code shouldn't be in a ctor at all. Following the usual conventions for C++, this would be a function named push_front (and it would normally only take one data type as the input -- to store an int and a std::string, we'd put those together into a struct of some sort -- an std::pair, if nothing else.

//------ Print the list.  Note how the function calls the print_list function
//       in the next item in the link list.
//       Function prints from the current link list item to the end.
void Tpacket_list::print_list()
{//---
   cout &lt;&lt; "    id = " &lt;&lt; id &lt;&lt; ",  payload = " &lt;&lt; payload &lt;&lt; ", length " &lt;&lt; payload.length() &lt;&lt; " characters."&lt;&lt; endl ;
   if ( next != NULL) // if there is a next item, get it to print itself.
     next-&gt;print_list() ;
}


IMO, the very existence of this function is a problem. To print out the contents of a list, we really want to apply an algorithm (e.g., std::copy) to the list (or some subset thereof). To do that, our list should define begin() and end(), which return list iterators. The iterator type should define (at least) operator++ to traverse the list. In our case, the iterator type would be a fairly thin wrapper around a pointer to a node:

class list {
    class Node { /* ... */ };

    // warning: typing this in off the top of my head. It probably has at least a few
    // problems.
    class iterator {
        Node *pos;
    public:
        Node operator*() { return *pos; }
        iterator &amp;operator++() { 
            if (NULL == pos)
                return NULL;
            pos = pos -&gt; next;
            return *this;
        }
        bool operator==(iterator const &amp;other) { 
            return pos == other.pos;
        }
    };


This way, when we want to print out a list, we can use the same algorithms (and idioms in general) that we can with any of the other containers in the standard library.

//------ Find item in the list.
Tpacket_list* Tpacket_list::find( string target)
{//---
   if ( payload == target)
      return ( this) ;          // return pointer to current object.
   if ( next == NULL)
      return( NULL) ;           // end of list, not match, return NULL.
   //--- if got here there is a valid next.
   return( next-&gt;find( target)) ;
}


Again, probably pointless -- instead of a find member function, provide a conforming iterator type so you use use std::find (among other things).

//------ Tell of internal data.
int    Tpacket_list::tell_id() 
 { return( id) ;
 }

string Tpacket_list::tell_payload()
 { return( payload) ;
 }


I can't say I like these much, but they're about the only thing so far that isn't obviously/blatantly wrong...

//====== Start program here =================================================

int main(int argc, char *argv[]) // argc = number strings on command line.
                                 // argv is array of strings each holding 
                                 //   a parameter.

{//--- Initialize link list with command parameters.
   cout &lt;&lt; endl &lt;&lt; "  Form link list of command line parameters." &lt;&lt; endl ;
   list_head = NULL ;
   int i = 1 ;
   while ( i &lt; argc)
    { new Tpacket_list(i, argv[i]) ;
      i++ ;
    }


Here we see the bad effects of the poor design. This code claims to create (and, incidentally, leak) argc-1 separate Tpacket_lists. In reality, it creates a single linked list, because the ctor does an implicit push_front on the (singular) global linked list represented by the nasty global. For usability we clearly want something like this:

    linked_list&lt;std::pair&lt;int, std::string&gt; &gt; list;

    for (int i=1; i&lt;argc; i++)
        list.push_back(std::make_pair(i, argv[i]));

 //--- print out the list.
   cout &lt;&lt; "  Print paramters from head of list the end of list." &lt;&lt; endl ;
   if ( list_head != NULL)
        list_head-&gt;print_list() ;
   else cout &lt;&lt; "   List is empty." &lt;&lt; endl ;


Then, to print out the list, we'd have code like:

if (list.empty())
    std::cout &lt;&lt; "List is empty.\n";
else
    std::copy(list.begin(), list.end(),
        std::ostream_iterator&lt;std::pair&lt;int, std::string&gt;(std::cout, "\n"));


and of course:

 //--- search for parameters
   cout &lt;&lt; "  Find first 0 parameter (working last to first parameter, list head to tail)." &lt;&lt; endl ;
   Tpacket_list *found ;
   if ( list_head != NULL)
     { found = list_head-&gt;find("0") ;
       if ( found != NULL)
            cout &lt;&lt; "    Found target on parameter # " &lt;&lt; found-&gt;tell_id()  &lt;&lt; endl ;
       else cout &lt;&lt; "    No \"0\" found." &lt;&lt; endl ;
     }


...searching in the linked list would be done with std::find instead of writing our own (clumsy) imitation.

As a final note, since the linked list allocates Nodes dynamically, we almost certainly need to provide a dtor that will delete those nodes. We also want to either provide a copy ctor that will copy the nodes, or else declare the copy ctor privately1 to prevent anybody from copying a linked list at all.



1 In C++11, we could use the =delete syntax to remove it as well, but it may be a bit premature to teach that as normal C++ usage just yet.