Accessing a static std::set with accessors, is this wise or should I just access it directly?

Question

This question follows on from the outcome of this one: How to automatically maintain a list of class instances?

With reference to the previous question, I created my static list of objects static std::set< Object* > objects;

However, to avoid circular referencing between Engine and Object, I moved it out of Engine and into a separate header file. I then realised that rather than directly interacting with the list, I could use a bunch of static accessors. This way if anything is making changes to the list, I can always break on these functions.

Are there any other benefits for doing it this way? Or is this a bad way to go? I never intend to instantiate ObjectManager ever - should I be using what I believe are called 'free functions' instead to manage this std::set, with no class?

I have created a test project to keep things simple in testing this out. The Inheritor class inherits from the Object class. The code for the ObjectManager class (referenced by Main.cpp, Object and Inheritor in this case, but a very similar one would be used in my main project) is below:

ObjectManager.h:

#include <set>

class ObjectManager
{
    friend class Object;
    friend class Inheritor;

public:
    static int ObjectCount();
    static void AddObject(Object *);
    static void RemoveObject(Object *);

    static int InheritorCount();
    static void AddInheritor(Inheritor *);
    static void RemoveInheritor(Inheritor *);

    static std::set<Object *>::iterator GetObjectListBegin();
    static std::set<Object *>::iterator GetObjectListEnd();

    static std::set<Inheritor *>::iterator GetInheritorListBegin();
    static std::set<Inheritor *>::iterator GetInheritorListEnd();
private:
    ObjectManager();
    ~ObjectManager();
};

ObjectManager.cpp:

#include "ObjectManager.h"

static std::set<Object *> objectList;
static std::set<Inheritor *> inheritorList;

ObjectManager::ObjectManager()
{

}
ObjectManager::~ObjectManager()
{
}

int ObjectManager::ObjectCount()
{
    return objectList.size();
}

void ObjectManager::AddObject(Object *input)
{
    objectList.insert(input);
}

void ObjectManager::RemoveObject(Object *input)
{
    objectList.erase(input);
}


int ObjectManager::InheritorCount()
{
    return inheritorList.size();
}

void ObjectManager::AddInheritor(Inheritor *input)
{
    inheritorList.insert(input);
}

void ObjectManager::RemoveInheritor(Inheritor *input)
{
    inheritorList.erase(input);
}

std::set<Object *>::iterator ObjectManager::GetObjectListBegin()
{
    return objectList.begin();
}

std::set<Object *>::iterator ObjectManager::GetObjectListEnd()
{
    return objectList.end();
}

std::set<Inheritor *>::iterator ObjectManager::GetInheritorListBegin()
{
    return inheritorList.begin();
}

std::set<Inheritor *>::iterator ObjectManager::GetInheritorListEnd()
{
    return inheritorList.end();
}

**

EDIT:

** I have rewritten my code to remove the need for an ObjectManager.

Instead of using ObjectManager, each class I want a list of contains the static list in its source file, so Object.cpp contains static std::set<Object *> objectList; and Inheritor.cpp contains static std::set<Inheritor *> inheritorList;.

Then, each of those two classes contains a static Count() function, for getting the number of items in its respective list, GetListBegin() and GetListEnd() for getting the beginning and end of the set.

As the functions share the same name in both the base class and the derived one, Object::Count() gets the number of Object instances in its respective list, and Inheritor::Count() gets the number of Inheritor instances in its respective list.

I don't know if this is a bad way of doing this or not. Nothing can add or remove from the list outside of each respective class. My only issue is I'm not sure how to stop the static functions from being available in anything that say, inherits from Inheritor for example.

There is lots of code smell here. I would be careful with this design, and when asking questions include a mention of the practical problem you are trying to solve. Global state, parallel global state, manual interface duplication. The code that can add/remove objects to the engine should have an engine pointer, simply so you can prove that code without an engine pointer is not add/removing objects. — Yakk - Adam Nevraumont, Aug 06 '13 at 14:37
The practical problem is described in the link to the question I provided at the top of this one. — Interminable, Aug 06 '13 at 14:58
It's not an object as it is never instanciated, surely? What would be the benefits/cons of having these as free-functions in a namespace over having them in a class like this? — Interminable, Aug 06 '13 at 17:43
I need a list of all objects of a type isn't the real problem, it is a solution to a problem. And rarely a good one. That global state is an unending scaling maintenance overhead. — Yakk - Adam Nevraumont, Aug 06 '13 at 18:04
It doesn't make sense to create an instance of ObjectManager, so you shouldn't be able to. — Neil Kirk, Aug 06 '13 at 18:32
I don't create an instance of `ObjectManager`. Also, I can't make it free-functions without including the headers for `Object`, `Inheritor`, etc, as I cannot then do `friend class Object`. I'd prefer to avoid the cyclic dependency. — Interminable, Aug 07 '13 at 07:55

score 0 · Answer 1 · answered Aug 06 '13 at 14:28

0

If std::set provides exactly the right interface then you can use it directly. If it doesn't (and that's usually the case: it has a very broad interface) then you should write a class or a set of functions that provide the interface you need, and implement appropriately, perhaps with std::set, perhaps with something else.

answered Aug 06 '13 at 14:28

Pete Becker

74,985
8
76
165

It was partly for organisation and simplicity. I can just do `ObjectManager::AddObject(this);` rather than `ObjectManager::objectList.insert(this);` I'll admit though, I don't really know much about interfaces. That said, this may be what is considered an interface. It's something I do need to read up on. – Interminable Aug 06 '13 at 17:46
@Interminable - the interface to any subsystem starts with a list of the things you want it to do. Don't confuse this term with Java's notion of an "interface", which is something rather different. The code in your question sketches out the interface that you want. – Pete Becker Aug 06 '13 at 18:01

score 0 · Answer 2 · answered Aug 06 '13 at 17:00

0

I would put this set as a private static member of Object class, which adds itself to the set in the constructor (don't forget copy constructor, if permissible). Then make a public read-only accessor to the set (return const reference).

I don't understand the meaning of your Inheritor set.

If this is a list of live objects in a game engine, I would make it a non-static member of engine class which keeps track of all its objects.

answered Aug 06 '13 at 17:00

Neil Kirk

21,327
9
53
91

It is for list of live objects in a game engine (but not the test project from which the code above is from). I wanted objects to be able to add themselves to a list of objects when they are instanciated. `Engine` has an `Update()` function (which is called as often as possible), iterating through every `Object` in the list, calling each object's respective `Update()` function, and doing a couple of other things. The `Inheritor` class inherits from `Object`, and is an example of a more specialised game object which I would want to store in its own list, but uses properties from `Object`. – Interminable Aug 06 '13 at 17:42
I would store the list in Engine (or World or wherever else they end up). Allow for the fact that you could have more than one engine or game world, even if it never happens in practice. It will encourage better coding style. – Neil Kirk Aug 06 '13 at 18:33
This is kind of how it was originally (see the link to the previous question). However, for every class that inherits from `Object` beyond this, it will have to be passed a reference to Engine. This Engine project is currently all compiling to a DLL that I can then use in another project for the game itself. It is my intention to then extend the classes that I have in the Engine for different types of things in the game. They will all have to be passed a reference to Engine on creation to do things this way, even if they do not directly interact with it. This feels wrong to me. – Interminable Aug 06 '13 at 19:21
You shouldn't have to store all derived classes seperately. That is what virtual functions are for. Engine should only know about Object class. – Neil Kirk Aug 06 '13 at 19:45
Even if I create an instance of a derived class with a base class of `Object`, I still wish to store the base `Object` of that class in a list in the engine. – Interminable Aug 06 '13 at 20:32
@Interminable you can, Object * can point to a derived class. – Neil Kirk Aug 06 '13 at 20:53
So how would I go about creating an instance of the derived class, which has the base class added to the list, without what I am currently doing and without passing in a pointer to the list/container of the list? When you say 'this is what virtual functions are for', how do you mean? – Interminable Aug 06 '13 at 21:08

Accessing a static std::set with accessors, is this wise or should I just access it directly?

EDIT:

2 Answers2