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 // C++ IO library code to include.
#include // 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 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->find( target)) ;
}

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

stri

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    // C++ IO library code to include.
#include      // 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 &p) :
            id(param_index), param(p)
        {}
    }

public:
    list(list const &);
    // ...
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 << "    Adding parameter # " << id << ", value = " << payload << 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 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 d

StackExchange Code Review Q#5054, answer score: 12