Looking for a better way to integrate a static list into a set of classes

Question

I'm trying to expand my sons interest from Warcraft 3 programming into C++ to broaden his horizons to a degree. We are planning on porting a little game that he wrote.

The context goes something like this.

• There are Ships and Missiles, for which Ships will use Missiles and interact with them
• A Container exists which will hold 'a list' of ships.
• A Container exists which will hold 'a list' of planets.
• One can apply a function over all elements in the Container (for_each)
• Ships and Missles can be created/destroyed at any time
• New objects automatically insert themselves into the proper container.

I cobbled a small example together to do that job, so we can talk about topics (list, templates etc) but I am not pleased with the results.

#include <iostream>
#include <list>
using namespace std;

/* Base class to hold static list in common with various object groups */ 
template<class T>
class ObjectManager
{
    public :
        ObjectManager(void)
        {
            cout << "Construct ObjectManager at " << this << endl;

            objectList.push_back(this);
        }

        virtual ~ObjectManager(void)
        {
            cout << "Destroy ObjectManager at " << this << endl;
        }

        void for_each(void (*function)(T *))
        {
            for (objectListIter = objectList.begin(); 
                 objectListIter != objectList.end(); 
                 ++objectListIter)
            {
                (*function)((T *) *objectListIter);
            }
        }
               list<ObjectManager<T> *>::iterator objectListIter;
        static list<ObjectManager<T> *>           objectList;
};

/* initializer for static list */ 
template<class T>
list<ObjectManager<T> *> ObjectManager<T>::objectList;


/* A simple ship for testing */ 
class Ship : public ObjectManager<Ship>
{
    public :
        Ship(void) : ObjectManager<Ship>()
        {
            cout << "Construct Ship at " << this << endl;
        }

        ~Ship(void)
        {
            cout << "Destroy Ship at " << this << endl;
        }

        friend ostream &operator<<(ostream    &out,const Ship &that)
        {
            out << "I am a ship";

            return out;
        }
};

/* A simple missile for testing */ 
class Missile : public ObjectManager<Missile>
{
    public :
        Missile(void) : ObjectManager<Missile>()
        {
            cout << "Construct Missile at " << this << endl;
        }

        ~Missile(void)
        {
            cout << "Destroy Missile at " << this << endl;
        }

        friend ostream &operator<<(ostream &out,const Missile &that)
        {
            out << "I am a missile";

            return out;
        }
};

/* A function suitable for the for_each function */ 
template <class T>
void show(T *it)
{
    cout << "Show: " << *it << " at " << it << endl;
}

int main(void)
{
    /* Create dummy planets for testing */ 
    Missile p1;
    Missile p2;

    /* Demonstrate Iterator */ 
    p1.for_each(show);

    /* Create dummy ships for testing */ 
    Ship s1;
    Ship s2;
    Ship s3;

    /* Demonstrate Iterator */ 
    s1.for_each(show);

    return 0;
}

Specifically, The list is effectively embedded in each ship though the inheritance mechanism. One must have a ship, in order to access the list of ships. One must have a missile in order to be able to access the list of missiles. That feels awkward.

My question boils down to "Is there a better way to do this?"

1. Automatic object container creation
2. Automatic object insertion
3. Container access without requiring an object in the list to access it.

I am looking for better ideas. All helpful entries get an upvote.

Thanks

Evil.

Answer 1

Try making the container a separate class from which the entities do not derive. Then if you want automatic insertion upon creation, either have the entity constructors take a container by reference and insert themselves, or have the container also function as a factory, where it creates the entities itself and maintains references to them. Like so:

template<typename Object>
class ObjectManager {
public:
    template<class Action> void forEach(Action action); // like std::for_each
    Object& create(); // inserts into internal list
// ...
}

I'll note here that this all seems a bit over-engineered. "ObjectManager"...the name itself sounds too generic. But maybe you're trying to teach some OOP stuff in "clean" way, so why not.

Answer 2

Why not make ObjectManager<T>::for_each() a static method?

Answer 3

Use the object container by simple access, i.e. ObjectManager<T>::objectList.

When you change ObjectManager<T>::for_each into a static function, your main can look like this:

int main(void)
{
    /* Create dummies */ 
    Missile p1;
    Missile p2;
    Ship s1;
    Ship s2;
    Ship s3;

    /* Demonstrate Iterators */ 
    Missile::for_each(show); 
    Ship::for_each(show);

    /* Demonstrate Direct Container Access */ 
    Missile::objectList.pop_back();
    Missile::for_each(show); 

    return 0;
}

BTW, it is a matter of individual preference, but I'd use std::vector instead of std::list to store pointers to created objects, so to have random accessibility.

One more thing: remove the object from the list when it's destroyed.

Answer 4

I think the problem here is that you are stretching the notion of inheritance. What you have is valid code and works but as you point out it doesn't feel right to have a container class hidden behind the scenes of the constructor of your child class. This is (in my opinion) because a Missile doesn't really have anything in common with it's parent class (a container), you are using inheritance to provide this container when this is a design pattern that has (rightly) been abstracted out in the standard template library. As John Zwinck suggests a Factory pattern will provide a much more expressive way of achieving what you want.

/* Container class to create and store various object groups */ 
template<class T>
class ObjectManager
{
public :
    ObjectManager() : nextAvailableIndex(0)
    {}

    virtual ~ObjectManager(void)
    {
        for (objectListIter = objectList.begin(); objectListIter != objectList.end(); ++objectListIter)
            delete *objectListIter;
    }

    T* ObjectManager::Create()
    {
        T* object = new Object(nextAvailableIndex++); 
        cout << "Constructed object " << object << endl;
        objectList.push_back(object);
    }

    void for_each(void (*function)(T *))
    {
        for (objectListIter = objectList.begin(); objectListIter != objectList.end(); ++objectListIter)
        {
            (*function)((T *) *objectListIter);
        }
    }

    int                nextAvailableIndex;
    list<T*>::iterator objectListIter;
    list<T*>           objectList;
};

int main(void)
{
    /* Create dummy planets for testing */ 
    ObjectManager<Missile> missileManager;
    Missile* p1 = missileManager.Create();

    /* Demonstrate Iterator */ 
    missileManager.for_each(show);

    /* Create dummy ships for testing */ 
    ObjectManager<Ship> shipManager;
    shipManager.Create(); 
    shipManager.Create(); 
    shipManager.Create(); 

    /* Demonstrate Iterator */ 
    shipManager.for_each(show); // will show three ships

    return 0;
}

I've left out the class definitions for Missile and Ship as the only change I put there is a single integer to the constructors to functions as a unique identifier.

Answer 5

Basically it's a typical case of bad use of inheritance. Missile and Ship should not inherit at all from ObjectManager. You should reserve inheritance when you have a "kind of" relationship between objects (like Nuclear Missile is a kind of Missile).

Here you use the inheritance for it's "mechanical" properties (automatic registering of new instances) but to achieve the same purpose it would be as easy to make objects, says Missile, derive from some ManagedObject common class and make it add new instances to some unique global objects managing list of instances (or do it without inheritance by overriding default new and delete for Missiles). Or you can pass in the manager to Missile class as an instanciation parameter, or create Missiles through some constructor method of the manager, that both add it to container.

All of these solutions works and are more satisfying than your original code (which also works) and there may be others I didn't thought of. Yet none of the above proposals is fully satifying for me. The underlying problem is that C++ lack of a proper metaclass mechanism (C++ is about compile time efficiency, but not flexibility).

After thinking out the different possibilities, I came up with the code below. The example is for Ship only and do not bother with Missile or using common code for both classes through template (even if it's easy enough). My answer focus on keeping an up to date list of all instances of Ship existing in the system at a given time without meddling with inheritance. I choose that solution because I believe it's the more KISS (Keep It Stupid and Simple) and complex solutions are a real burden when you have to maintain your code. The idea is also to avoid recoding something that already exists in STL like for_each.

#include <iostream>
#include <vector>
#include <algorithm>

using namespace std;
vector<class Ship*> all_ships;

class Ship
{
    public:
        Ship(void){
            cout << "Construct Ship at " << this << endl;
            all_ships.push_back(this);
        }

        ~Ship(void){
            cout << "Destroy Ship at " << this << std::endl;
            // we remove it from the list
            int i = 0;
            while (all_ships[i] != this) { i++; }
            all_ships[i] = all_ships.back();
            all_ships.pop_back();
        }

    static void show(Ship *s){
        cout << "ship :" << s << "\n";
    }

};


void list_all_ships(){
    cout << "List of Ships\n";
    for_each(all_ships.begin(), all_ships.end(), Ship::show);
    cout << "\n";
}

int main(void)
{
    cout << "Two automatic (on stack) Ships\n";
    Ship s1;
    Ship s2;
    list_all_ships();

    cout << "One new dynamic (on heap) Ship\n";
    Ship * s3 = new Ship();
    list_all_ships();

    cout << "Delete dynamic Ship\n";
    delete s3;
    list_all_ships();

    cout << "The two dynamic Ships will be deleted when getting out of Scope\n";
    return 0;
}