Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I'm currently re-designing my Entity System, for C++, and I have a lot of Managers. In my design, I have these classes, in order to tie my library together. I've heard a lot of bad things when it comes to "manager" classes, perhaps I'm not naming my classes appropriately. However, I have no idea what else to name them.

Most of the managers, in my library, are composed of these classes (although it does vary a little bit):

  • Container - a container for objects in the manager
  • Attributes - attributes for the objects in the manager

In my new design for my library, I have these specific classes, in order to tie my library together.

  • ComponentManager - manages components in the Entity System

    • ComponentContainer
    • ComponentAttributes
    • Scene* - a reference to a Scene (see below)
  • SystemManager - manages systems in the Entity System

    • SystemContainer
    • Scene* - a reference to a Scene (see below)
  • EntityManager - manages entities in the Entity System

    • EntityPool - a pool of entities
    • EntityAttributes - attributes of an entity (this will only be accessible to ComponentContainer and System classes)
    • Scene* - a reference to a Scene (see below)
  • Scene - ties all the managers together

    • ComponentManager
    • SystemManager
    • EntityManager

I was thinking of just putting all the container/pools in the Scene class itself.

i.e.

Instead of this:

Scene scene; // create a Scene

// NOTE:
// I technically could wrap this line in a createEntity() call in the Scene class
Entity entity = scene.getEntityManager().getPool().create();

It would be this:

Scene scene; // create a Scene

Entity entity = scene.getEntityPool().create();

But, I am unsure. If I were to do the latter, that would mean I would have a lot of objects and methods declared inside my Scene class.

NOTES:

  1. An entity system is simply a design that is used for games. It is composed of 3 major parts: components, entities, and systems. The components are simply data, which may be "added" to the entities, in order for the entities to be distinctive. An entity is represented by an integer. Systems contain the logic for an entity, with specific components.
  2. The reason I'm changing my design for my library, is because I think it can be changed quite a lot, I don't like the feel/flow to it, at the moment.
share|improve this question

migrated from codereview.stackexchange.com Feb 12 '13 at 18:35

This question came from our site for peer programmer code reviews.

    
Can you please elaborate on the bad things you've heard about the managers and how they pertain to you? –  MrFox Feb 12 '13 at 19:02
2  
You may find the following links helpful: Can manager classes be a sign of bad architecture? and Naming Classes - How to avoid calling everything a manager –  R4D4 Feb 13 '13 at 1:20
    
@MrFox Basically I've heard what Dunk has mentioned, and I have a lot of *Manager classes in my library, which I want to get rid of. –  miguel.martin Feb 13 '13 at 6:17

3 Answers 3

up vote 8 down vote accepted

DeadMG is spot on on the specifics of your code but I feel like it misses a clarification. Also I don't agree with some of his recommendation that don't hold in some specific contexts like most high performance-videogame development for example. But he's globally right that most of your code is not useful right now.

As Dunk says, Manager classes are called like this because they "manage" things. "manage" is like "data" or "do", it's an abstract word that can contain almost anything.

I was mostly in the same place than you around 7 years ago, and I started to think that there was something wrong in my way of thinking because it was so much efforts to code to do nothing yet.

What I changed to fix this is to change the vocabulary I use in code. I totally avoid generic words (unless it's generic code, but it's rare when you're not making generic libraries). I avoid to name any type"Manager" or "object".

The impact is direct in code: it forces you to find the right word corresponding to the real responsibility of your type. If you feel like the type does several things (keep an index of Books, keep them alive, create/destroy books) then you need different types that each will have one responsibility, then you combine them in the code that uses them. Sometime I need a factory, sometime not. Sometime I need a registry, so I setup one (using standard containers and smart pointers). Sometime I need a system that is composed of several different sub system so I separate everything as much as I can so that each part do something useful.

Never name a type "manager" and make sure all your type have a single unique role is my recommendation. It might be hard to find names sometime, but it's one of the hardest thing to do in programming generally

share|improve this answer
    
If a couple dynamic_casts are killing your performance, then use the static type system- that's what it's for. It's really a specialized context, not a general one, to have to roll your own RTTI like LLVM did. Even then, they don't do this. –  DeadMG Feb 12 '13 at 23:39
    
@DeadMG I was not talking about this part in particular, but most high-performance games can't afford, for example, dynamic allocations through new or even other stuffs that are fine for programming in general. They are specific beasts for specific hardware which you can't generalize which is why "it depends" is a more general answer, in particular with C++. (nobody in game dev uses dynamic cast by the way, it's never useful until you have plugins, and even then it's rare). –  Klaim Feb 13 '13 at 14:50
    
@DeadMG I'm not using dynamic_cast, nor am I using an alternative to dynamic_cast. I implemented it in the library, but I couldn't be bothered taking it out. template <typename T> class Class { ... }; is actually for assigning IDs for different classes (namely custom components and custom systems). ID assignment is used to store components/systems in a vector, as a map may not bring the performance I need for a game. –  miguel.martin Feb 27 '13 at 11:25
    
Well I didn't actually implement an alternative to dynamic_cast either, just a fake type_id. But, still I didn't want what type_id achieved. –  miguel.martin Feb 27 '13 at 11:34
    
"find the right word corresponding to the real responsibility of your type" - though i totally agree with this, often there is not one single word (if any) that the developer community has agreed upon for a particular type of responsibility. –  nashwan Apr 24 at 15:49

Well, I had a read through some of the code you linked to, and your post, and my honest summary is that the majority of it is basically completely worthless. Sorry. I mean, you have all of this code, but you haven't achieved anything. At all. I'm going to have to go into some depth here, so bear with me.

Let's start with ObjectFactory. Firstly, function pointers. These are stateless, the only useful stateless way to create an object is new and you don't need a factory for that. Statefully creating an object is what's useful and function pointers are not useful for this task. And secondly, every object needs to know how to destroy itself, not having to find that exact creating instance to destroy it. And in addition, you must return a smart pointer, not a raw pointer, for the exception and resource safety of your user. Your whole ClassRegistryData class is just std::function<std::unique_ptr<Base, std::function<void(Base*)>>()>, but with the aforementioned holes. It does not need to exist at all.

Then we have AllocatorFunctor- there are already Standard allocator interfaces provided- Not to mention that they're just wrappers on delete obj; and return new T;- which again, is already provided, and they won't interact with any stateful or custom memory allocators which exist.

And ClassRegistry is simply std::map<std::string, std::function<std::unique_ptr<Base, std::function<void(Base*)>>()>> but you wrote a class for it instead of using the existing functionality. It's the same, but completely needlessly global mutable state with a bunch of crappy macros.

What I'm saying here is that you've created some classes where you could replace the entire thing with functionality built from Standard classes, and then made them even worse by making them global mutable state and involving a bunch of macros, which is the worst thing you could possibly do.

Then we have Identifiable. It's a lot of the same story here- std::type_info already performs the job of identifying each type as a run-time value, and it doesn't require excessive boilerplate on the part of the user.

    template<typename T, typename Y>
    bool isSameType(const X& x, const Y& y) { return typeid(x) == typeid(y); }
    template <class Type, class T>
    bool isType(const T& obj)
    {
            return dynamic_cast<const Type*>(std::addressof(obj));
    }

Problem solved, but without any of the preceeding two hundred lines of macros and whatnot. This also marks your IdentifiableArray as nothing but std::vector but you have to use reference-counting even if unique ownership or non-ownership would be better.

Oh, and did I mention that Types contains a bunch of absolutely useless typedefs? You gain literally no advantage whatsoever over using the raw types directly, and lose a good chunk of readability.

Next up is ComponentContainer. You can't choose the ownership, you can't have separate containers for derived classes of various components, you can't use your own lifetime semantics. Delete without remorse.

Now, the ComponentFilter might actually be worth a little, if you refactored it a lot. Something like

class ComponentFilter {
    std::unordered_map<std::type_info, unsigned> types;
public:
    template<typename T> void SetTypeRequirement(unsigned count = 1) {
        types[typeid(T)] = count;
    }
    void clear() { types.clear(); }
    template<typename Iterator> bool matches(Iterator begin, Iterator end) {
        std::for_each(begin, end, [this](const std::iterator_traits<Iterator>::value_type& t) {
            types[typeid(t)]--;
        });
        return types.empty();
    }
};

This doesn't deal with classes derived from the ones you're trying to deal with, but neither did yours.

The rest of this is pretty much the same story. There is nothing here that could not be done much better without the use of your library.

How would you avoid managers in this code? Delete basically all of it.

share|improve this answer
9  
It took me a while to learn, but the most reliable, bug-free pieces of code are the ones that aren't there. –  Tacroy Feb 12 '13 at 19:47
    
The reason I didn't use std::type_info is because I was trying to avoid RTTI. I mentioned the framework was aimed at games, which is basically the reason I'm not using typeid or dynamic_cast. Thanks for the feedback though, I appreciate it. –  miguel.martin Feb 13 '13 at 5:54
    
Is this critics based on your knowledge of component-entity system game engines or is it a general C++ code review? –  Den Feb 13 '13 at 11:17
    
@Den I think more of a design issue than anything else. –  miguel.martin Feb 14 '13 at 1:51

The problem with Manager classes is that a manager can do anything. You can't read the class name and know what the class does. EntityManager....I know it does something with Entities but who knows what. SystemManager...yep, it manages the system. That's very helpful.

My guess is that your manager classes probably do a lot of things. I'll bet if you were to segment those things into closely related functional pieces, then you'll probably be able to come up with class names related to the functional pieces that anybody can tell what they do simply by looking at the class name. This will make for a far easier to maintain and understand design.

share|improve this answer
1  
+1 and not only can they do anything, on a long enough timeline they will. People view the "Manager" descriptor as an invitation to create kitchen-sink/swiss-army-knife classes. –  Erik Dietrich Feb 12 '13 at 19:47
    
@Erik - Ah yes, I should have added that having a good class name is not only important because it tells someone what the class can do; but equally the class name also says what the class shouldn't do. –  Dunk Feb 19 '13 at 15:58

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.