Use of static collections within a class to store class

Question

I am reviewing some code and a common pattern I am seeing is where a collection of objects is stored as a static member of the class of object being stored.

If, for example, I have a class of objects: class widget, then a list of widgets would be stored as a static std::list within the widget class.

The obvious way to do this would be to have an application level (global level) std::list - and then to find an item lookup this application level list.

I can see that the static member collection idea is more convenient for the user of a. Are there other advantages? Are there other alternatives which should also be considered similar? What are the pros and cons of a and b approach?

Here are the two alternatives in code:

file a.hpp:
//Using collection as static member of class idea
class a {
public:
  a();
  ~a();
  class id2a_map;
  static class id2a_map id_map;
  static a* Find(unsigned id);

  unsigned m_id;
};

file a.cpp:
#include <map>
#include "a.hpp"

class a::id2a_map : public std::map<int, a*>
{
};

a::id2a_map a::id_map;

a::a() {
  static unsigned id_cnt = 0;
  ++id_cnt;
  id_map.insert(id_map.end(), id2a_map::value_type(m_id = id_cnt, this));
}

a::~a() {
   id_map.erase(m_id);
}

a* a::Find(unsigned id) {
  id2a_map::iterator i = id_map.find(id);
  return i==id_map.end() ? 0 : i->second;
}


file b.hpp:
// b class - not using static collection
class b {
public:
  b(unsigned id) : m_id(id) { }
  unsigned get_id() const { return m_id; }

private:
  unsigned m_id;
};

file main.cpp to exercise a and b:
#include <iostream>
#include <map>
#include "a.hpp"
#include "b.hpp"

int main() {
  // approach using static map within class
   a obj1;
   a obj2;
   a obj3;
   a obj4;

   a* fnd = a::Find(2);
   std::cout << "object with id 2 " << (fnd ? "" : "not ") << "found\n";

   // application level map
   std::map<unsigned, b*> id2b_map;
   unsigned id = 0;
   b obj5(++id);
   id2b_map.insert(id2b_map.end(), std::make_pair<unsigned, b*>(id, &obj5));
   b obj6(++id);
   id2b_map.insert(id2b_map.end(), std::make_pair<unsigned, b*>(id, &obj6));
   b obj7(++id);
   id2b_map.insert(id2b_map.end(), std::make_pair<unsigned, b*>(id, &obj7));
   b obj8(++id);
   id2b_map.insert(id2b_map.end(), std::make_pair<unsigned, b*>(id, &obj8));

   std::map<unsigned, b*>::iterator i = id2b_map.find(2);
   std::cout << "object with id 2 " << (i == id2b_map.end() ? "not " : "") << "found\n";
   return 0;
}

The main pattern here is that the collection and hence all the instances are globally accessible. This is convenient but at the cost of hidden dependencies. If you let every part of the code access data by just including the header file (possibly via other header files) and you need to change something that breaks dependent code, then you can have serious problems (in larger applications). — stefaanv, Mar 20 '14 at 15:31

utnapistim · Accepted Answer · 2014-03-21T11:46:53.103

I can see that the static member collection idea is more convenient for the user of a.

It is not more convenient, except for simple cases. Adding a static map of instances means you add a hidden dependency. Are there any use cases when you do not need this list? If you place it as a static private instance, you will always have it there (whether you use it or not).

Also, the code you wrote, will have unexpected results here:

class a::id2a_map : public std::map<int, a*>

std::map is not written to be inherited, which means it doesn't have a virtual destructor. When the class gets destroyed, it's destructor may not get called (compiler-dependent).

Are there other alternatives which should also be considered similar? What are the pros and cons of a and b approach?

B approach is better, but not as good as it could be. The implementation will not depend on std::list (minimizing dependencies is always a plus) but the list has pointers, and it shouldn't.

Could you write something like this instead?

class a {
public:
  a();
  ~a();

  unsigned m_id; // same as in your example
};

client code:

std::map<unsigned, a> instances; // not static; if you need it somewhere else,
                                 // just pass it in as a parameter

a instance;
instances[a.m_id] = std::move(a);
// use instances.find from here on

The code is straight-foward, minimal and doesn't break SRP.

Your map idea is not so great if a is a large, complex object - each complete object will be copied into the map. Simple is good, but complexity does sometimes add benefits. — Angus Comber, Mar 20 '14 at 16:42
I am not saying you should copy objects into the map; I am saying you should _hold them_ in the map. — utnapistim, Mar 20 '14 at 16:47
Just a small remark: I don't agree with your presentation of the virtual destructor issue. See here for better explanation which shows that in some cases, the destructor of id2a_map may not be called: http://stackoverflow.com/a/461224/104774 — stefaanv, Mar 21 '14 at 08:04
I don't agree with ALL your comments but I do agree with the general idea. — Angus Comber, Mar 21 '14 at 10:58

score 0 · Answer 2 · answered Mar 20 '14 at 15:20

In the case of static members, if you have static methods doing something with them, since the compiler knows all about what the methods will do at compile time, it has a better chance to optimize them well. Depending on the compiler, the amount of optimization varies.

This is an interesting thread of discussion: http://bytes.com/topic/c/answers/617238-static-functions-better-optimized-compilers

Use of static collections within a class to store class

2 Answers2