using Static Container for base and derived classe

Question

I'm trying to build a [base+derived] class's object container(std::map to be accurate) which would be a static variable, embedded inside Base class so that every other object either derived from it, or from it's child classes would've only 1 list at most. Also if no object is created, I still would've access to that list through other friendly(static) functions.

Here's the code -

#include <iostream>
#include <string>
#include <unordered_map>

class A{

    std::string id;
    static std::unordered_map<std::string, A*> list;

  public:

    A(std::string _i): id(_i){}
    static void init(){
        // doSomeProcessing or let's just call clear() for now
        list.clear();
    }
    static void place(std::string _n, A* a){
        list[_n] = a;
    }
    virtual void doSomething(){std::cout << "DoSomethingBase\n";}
};

std::unordered_map<std::string, A*> A::list = {};

class B : public A{

    std::string name;

  public:

    B(std::string _n):name(_n), A(_n){}
    void doSomething(){std::cout << "DoSomethingDerived\n";}
};


int main() {
    A::init();
    A::place("b1", new B("b1"));
    A::place("a1", new A("a1"));
    return 0;
}

The code compiles successfully however, there are two things at top of my mind, which I might be doing wrong, one major and another minor problem.

Major - Pointers as values to std::map? I'm sure I've totally not grasped them yet, but I'm studying hard. I know they aren't being deleted here, but how should I do it? In the destructor? or let the program finish.....(bad, right?)

Minor - Using static members? I've heard from folks that static variables are bad for some unknown reasons and shouldn't be used unless necessarily required. They were even removed from some early version of cpp. Is that right? But what would be my options then... creating it in main().

Any other mistake I might be doing here.

---

Edit - I'm supplying a sample scenario so to clear my intentions with following example.

Let's say I've an employee class and some more specific jobs eg editor, writer, developer that are still derived from employees. However there might exist instance of base class employee too, since there might be employees who aren't cateogrised yet, but will be soon.

Let's say this is an requirement, although I'm open to changes. I'm using that static std::map so that only one instance of it is required which is uniform across all base + derived classes. I'm not so familiar with singleton pattern and such but I'll happily learn it in future.

Answer 1

Pointers as values to std::map? ... they aren't being deleted here, but how should I do it? In the destructor?

If the pointed objects should be owned by the container then yes, the owned objects should be destroyed in the destructor. However, you should always use smart pointers to manage owned objects. Using smart pointers, the implicit destructor will do the right thing.

If the pointed objects are not owned by the container, then they should be owned by something else.

Using static members?

Static members are global variables. Whether you should or shouldn't use global variables has been extensively discussed elsewhere and is out of the scope of my answer.

they were even removed from some early version of cpp.

There has never been a published standard version of C++ where static members didn't exist.

Answer 2

As far as deleting the pointers in the map, this is one of those questions that only whoever's asking it knows the answer.

In the case of the program terminating, it's an obvious non-issue. A memory analysis tool might complain about memory not being released. If that's an issue, then fix it. Explicitly delete the pointers in the map before exiting the program.

In all other use cases, nobody can tell you the answer. You have to figure it out by yourself, when you need to delete the pointers. In most use cases, deleting the pointer as part of removing it from the map will be the right answer.

But, of course, if the pointer's object still gets used, in some way, after it's key gets deleted from the map, the object cannot be deleted until that's done. Just because the object's key is removed from the map doesn't mean that there's a law that prohibits the object from being used, any more.

Only you know what you're going to do with the object, if anything, after it's removed from the map so that's, again, for you to decide. Complicated, large applications, rarely use plain pointers, but rather smart pointers (std::shared_ptr) in order to let C++ itself figure out when an object is no longer used, anywhere, and can be destroyed.

As far as static class members go, this is purely opinion. static class members are a part of C++. That, in itself, doesn't make them good, and it doesn't make them bad. There are good ways to use static class members, and there are bad ways. Which is also true for many other C++ features, too.

Answer 3

Major: use managed pointers. Here is a modified version of your code based on unique_ptr:

class A{
    // ...
    static std::unordered_map<std::string, std::unique_ptr<A>> list;

    // ...
    template< class T, class... Args >
    static
    // enable only A-based objects for T via the return type
    typename std::enable_if<std::is_base_of<A, T>::value>::type
    place(std::string _n, Args&&... args)
    {
        list.emplace(_n, std::make_unique<T>(std::forward<Args>(args)...));
    }
    // ...
};

std::unordered_map<std::string, std::unique_ptr<A>> A::list = {};

class B : public A{

    // ...
    // corrected a warning here: bases should be constructed before members
    B(std::string _n):A(_n), name(_n){}
    // ...
};

int main() {
    A::init();
    A::place<B>("b1", "b1");
    A::place<A>("a1", "a1");
        // maybe, the second argument is now redundant
        // you can remove it and call in place
        // std::make_unique<T>(_n, std::forward<Args>(args)...)
    return 0;
}

I modified place to make it care of the allocation -- using make_unique which is C++14, you can use new T{std::forward<Args>(args)...} instead. The new place function is a variadic template to follow make_unique prototype.

As suggested in the comments by Adam Martin, enable_if can be used to restrict the type T to objects inheriting A.

As edited by Jarod42, you should use forward references to respect the value category passed when calling place.

I used emplace instead of operator[] to avoid unnecessary copy or move operations.

An other option is to use shared_ptr if you need to make a copy of the pointer. The implementation would be similar.

Answer 4

Having a static member can be a good thing. In particular, static members are mostly used if they serve some internal management of the class. The most prominent example is a reference counter.

So the main question you have to answer yourself is: what are you really trying to do?

I would consider it bad design if the caller of your object has to manually call place and such. These management functions should be done only insider your class (in particular constructor and destructor).

For your major point: Simply use std::shared_ptr<A>. This keeps track of deleting objects automatically for you.

---

Edit

According to your edit, your approach of using a static map looks reasonable. However, you should definitely change the responsibilities. Use the factory pattern:

Each class has a static method create or similar. Your constructor should be private. A::place("x", new A("x")) means you can always freely call new A("x") where you want. This violates your objective of creating singletons.

Instead, creating an instance should always be A::create("x") or B::create("x"). Each class can also have its own map (because you never need access to the base class' map).

However, properly implementing singletons may be a complex task. You may consider of refraining from singletons (unless you have a very good reason).

Instead, why not simply properly override operator== and operator!=?

Answer 5

In terms of design, I'm not sure what your classes are trying to accomplish/why B is derived from A. To me it sounds like you should have three classes: EmployeeManager (part of A, the list management, implemented using a Singleton pattern), Employee (other part of A, i.e. virtual methods), and Editor, Writer etc (B right now). Your A class seems to have multiple responsibilities right now, which is a violation of single responsibility. In this case we split the list management and employee interface up into two classes.

Sample implementation:

class EmployeeManager
{
    public:
        static EmployeeManager& getInstance()
        {
            static EmployeeManager    instance; // Guaranteed to be destroyed.
                                  // Instantiated on first use.
            return instance;
        }
    private:
        EmployeeManager() {};
        std::unordered_map<std::string, Employee&> list;
    public:
        EmployeeManager(EmployeeManager const&) = delete;
        void operator=(const&) = delete;
        void place(const std::string &id, Employee &emp){
            list[id] = emp;
        }
};

class Employee
{
    public:
        virtual void doSomething() = 0;
};

class Writer : public Employee
{
    private: 
        std::string name_;
    public:
        Writer(std::string name) : name_(name) {};
        void doSomething() { };
};

If you wanted the employees to be automatically inserted into the manager, you could have a reference to the manager in the employee classes and the placement in their constructors.

In terms of pointers, you can wither use std::shared_ptr, or use a reference instead.