39 replies to this topic

[Hodgman]

Then what exactly is a Manager? I was under the impression that a manager was a global instance of a class that handles the functionality of a certain aspect of a software. For example, an AssetManager would handle the loading of assets, while a SceneManager would handle the transition of scenes, and so on.

There's absolutely no need to make your asset manager or scene manager into global variables.

A class that has a single global instance is a singleton. "Manager" is just a vague verb that's commonly used to describe classes that are collections of other objects (e.g. an asset manager "manages" a collection of assets).
As has been said in the thread already, using vague verbs is discouraged.

[Codarki]

Then what exactly is a Manager? I was under the impression that a manager was a global instance of a class that handles the functionality of a certain aspect of a software. For example, an AssetManager would handle the loading of assets, while a SceneManager would handle the transition of scenes, and so on.

How about:
AssetBuilder builds assets.
AssetCollection is a collection of assets of type T.
AssetRegistry has collections of assets for varying types.

None of them need to be global. There is the additional benefit when you pass them around that the public interface for the caller is simpler.

[Tispe]

Hmm, I find this thread quite interesting as I am making a class on my own which loads sprite textures.

My class has a std::vector<SpriteData>, where SpriteData is a struct with an "std::string name", "int NumberOfUsers" and "LPDIRECT3DTEXTURE9 Sprite".

The purpose of the class is to make sure I don't fill memory with duplicate textures. Anytime I want to load a Sprite Texture I call the function member "LoadTexture(name)". If it finds that it is already loaded by checking for existance in the vector it returns the loaded LPDIRECT3DTEXTURE9 which shares the same std::string name and it increments NumberOfUsers, otherwise it loades the file in to memory, adds the new SpriteData on the vector. And then returns the LPDIRECT3DTEXTURE9.

Is this discouraged?

[Hodgman]

My class has a std::vector<Data>, where Data is a struct with an "std::string name", "int NumberOfUsers" and "T* resource".

The purpose of the class is to make sure I don't fill memory with duplicate resources. Anytime I want to load a resource I call the function member "Load(name)". If it finds that it is already loaded by checking for existance in the vector it returns the loaded T* which shares the same std::string name and it increments NumberOfUsers, otherwise it loades the file in to memory, adds the new Data on the vector. And then returns the T*.

^^I've edited your quote. If you changed your class to be a template class that stored T*'s instead of IDirect3DTexture9*'s, then it would be a reusable reference-counted-resource-cache, instead of being tightly coupled with a Direct3D-specific type.

[edit - added example] e.g. note that this only has one responsibility - caching pointers that are matched to names via reference counting. It's not an "all-in-one manager" that is also responsible for creating/destroying the resources, updating them, etc... which constrains it to a nicely manageable 40 lines of code.[source lang=cpp]template<class T, class Fn> class ResourceCache {public: ResourceCache( const Fn& loader ) : m_loader(loader) {} T* Load( const std::string& name ) { DataMap::iterator i = m_resources.find(name); T* result; if( i == m_resources.end() ) { result = m_loader.Load(name); Data data = { 1, result }; m_resources.insert( std::make_pair(name, data) ); } else { i->second.count++; result = i->second.data; } return result; } void Unload( T* data ) { for( DataMap::iterator i = m_resources.begin(), end=m_resources.end(); i != end; ++i ) { if( i->second.data == data ) { if( --i->second.count == 0 ) { m_loader.Unload( data ); m_resources.erase( i ); } break; } } }private: struct Data { int count; T* data; }; typedef std::map<std::string, Data> DataMap; DataMap m_resources; Fn m_loader;};struct IntLoader { int* Load( const std::string& name ) { return new int; } void Unload( int* p ) { delete p; }};void test(){ IntLoader myLoader; ResourceCache<int, IntLoader> intCache( myLoader ); int* res1 = intCache.Load("asdf"); int* res2 = intCache.Load("asdf"); intCache.Unload( res1 ); intCache.Unload( res2 );}[/source]

[Ripiz]

Hodgman, what about something as heightmaps? Let's say I have HeightmapManager, which handles loading Heighmap when it becomes visible, unloading when it wasn't visible for awhile, rendering them, etc?

[yewbie]

Hodgman, what about something as heightmaps? Let's say I have HeightmapManager, which handles loading Heighmap when it becomes visible, unloading when it wasn't visible for awhile, rendering them, etc?

I think the point most people are making is that the word "manager" itself is ambiguous and there is almost always a better name that can be used to describe the operation.

[Serapth]

Hodgman, what about something as heightmaps? Let's say I have HeightmapManager, which handles loading Heighmap when it becomes visible, unloading when it wasn't visible for awhile, rendering them, etc?

I think the point most people are making is that the word "manager" itself is ambiguous and there is almost always a better name that can be used to describe the operation.

I don't think it is ambiguous, more so abused. Also, they really shouldn't be as vilified as they are, there is absolutely nothing wrong with naming your class a manager, a lot of the backlash is simply a game of groupthink pile on. The actual flaw, from a design perspective, is creating an everything and the kitchen sink classes that you can call a ____Manager.


If you want to name your class SomethingManager, and it makes sense, call your class SomethingManager. It isn't going to make your code worse or better, nor will it really alter the readability or maintainability. However, clearly define the functionality of your class and keep it as concise as possible.

If for example you have a class for creating, deleting and returning screens, it makes no sense to name it ScreenLoader or ScreenFactory, as it does more than that, and it really doesn't make sense to break these three activities out into separate classes. In this case if you wanted to name your class ScreenManager, and that name makes sense to you... do it! However, if you find your class adding more and more functionality, like displaying, or updating screens, your class design is bad and you are falling into the Manager kitchen sink trap.


In the end, you are the number one person who needs to understand your code, triply so if you are working solo, so if it makes sense for you to name your class a manager, name it a bloody manager.



Managers are a lot like singletons in a way; a new beginner abuses the hell out of them because they are a shortcut (instead of ) good design and eventually they get into trouble when their code reaches a certain complexity... frankly the validity of a design really isn't tested until a certain amount of complexity is reached. As a result there is a giant backlash that USING XXX IS BAD! Reality is, no they aren't bad, neither of them are, but both are ripe for abuse. If you have a global available object that should have deferred allocation, a singleton is the data structure to use. If you have a class that exclusively manages Y, it is a YManager.

[way2lazy2care]

In the end, you are the number one person who needs to understand your code, triply so if you are working solo, so if it makes sense for you to name your class a manager, name it a bloody manager.

Uploaded with ImageShack.us

[Hodgman]

Hodgman, what about something as heightmaps? Let's say I have HeightmapManager, which handles loading Heighmap when it becomes visible, unloading when it wasn't visible for awhile, rendering them, etc?

You can compose that behaviour out of many small single-responsibility classes.


[source lang=cpp]typedef ResourceCache<Texture, TextureLoader> TextureCache;class StreamingHeightmapGrid : NonCopyable{public: StreamingHeightmapGrid( TextureCache& textureSource, RenderableScene& scene ) : m_textureSource(textureSource) { for( int i=0; i!=100; ++i ) m_grid.push_back( new HeightmapCell(i, textureSource, scene) ); } Update() { for( int i=0; i!=100; ++i ) m_grid[i]->Update(m_textureSource); }private: TextureCache& m_textureSource; std::vector<HeightmapCell*> m_grid;};class HeightmapCell{public: HeightmapCell( int id, TextureCache& textureSource, RenderableScene& scene ) : m_occludee( scene, GetCellAabb(i) ), m_renderable( scene, textureSource.Load(GetCellTextureName(i)) ) {} void Update( TextureCache& textureSource ) { bool visible = m_occludee.Visible(); if( visible && !m_renderable.Texture() ) m_renderable.Texture( textureSource.Load(GetCellTextureName(i) ); else if( !visible && m_renderable.Texture() ) { textureSource.Unload( m_renderable.Texture() ); m_renderable.Texture(0); } }private: static std::string GetCellTextureName( int i ); static Aabb GetCellAabb( int i ); Occludee m_occludee; HeightmapNode m_renderable;};[/source]

[Shael]

Sorry to revive the thread if it's already dead, but I was looking over the C4 Engine and noticed it has a lot of manager classes which are singletons. Are the way managers and singletons used in this engine a bad design? From what I can tell from the API each of these manager classes seem to be fairly separate from one another and provide a small number of related tasks.

[Zahlman]

Then what exactly is a Manager?

The fact that this question can meaningfully be asked is exactly the problem.

[XXChester]

In my eyes, Managers fall into the same category as all other software design. Use them where they are neccessary and name them accordingly. Do not name a class Manager because you have no idea what it is doing, name it as exactly what it's purpose is, ie StateManager. I personally use a StateManager for my games which does exactly what you think, it manages the state of the game, ie MainMenu,Paused,...etc,etc. I think in the end, this is just another topic that promotes flame wars just like asking should I use a singleton or shouldn't I? There is nothing wrong with any of these designs, they all work, they all get the task done, and they all make sense. It is all personal preference and people's opinion's when it comes to things like this. Just my 2 cents.

[Codarki]

... There is nothing wrong with any of these designs, they all work, they all get the task done, and they all make sense. It is all personal preference and people's opinion's when it comes to things like this. Just my 2 cents.

That sounds a bit generalization for me, and I disagree. Generally there is a lot wrong in the design where singletons are used, they do get the job done while being fragile to bugs, and confuse the general solution to a mess. I'd even go further and say it is not just a personal preference but among best known practices to avoid them.

That being said, occasionally they are the best solution, usually for some low-level stuff, rarely for anything high-level.

IMO, this singleton discussion has nothing to do with the discussion about naming of managers (if you ignore those who makes singleton managers).

[way2lazy2care]

Sorry to revive the thread if it's already dead, but I was looking over the C4 Engine and noticed it has a lot of manager classes which are singletons. Are the way managers and singletons used in this engine a bad design? From what I can tell from the API each of these manager classes seem to be fairly separate from one another and provide a small number of related tasks.

I think it's amusing that nobody answered the question in the necro post. That said, I have no idea about the C4 engine, but I will say they are probably not ideal design.

[Serapth]

Sorry to revive the thread if it's already dead, but I was looking over the C4 Engine and noticed it has a lot of manager classes which are singletons. Are the way managers and singletons used in this engine a bad design? From what I can tell from the API each of these manager classes seem to be fairly separate from one another and provide a small number of related tasks.

Well the reality is, with the rise of defined design patterns, at first the singleton was very much considered a darling, as it seemed like a magic bullet of sorts... all the flexibility of globals with none of the draw backs!!! So people used them, and once you start using them they become very intwined in your code base ( one of the big downsides ). A lot of frameworks took to the singleton pattern, look at Cocos2d or Ogre3D. Also remember, using a Singleton doesn't make your code bad, it just makes it prone to certain problems.

First off, some people are atomic code nazis, which isn't actually nearly as cool as it sounds. You have a school of thought that every object should have exactly one purpose and as short of a lifetime as possible and simply put, the singleton is anathema to them, like garlic to vampires. The next major problem is, it exposes it's code everywhere, making debugging, unit testing and refactoring all a bloody nightmare. That said, a 3rd party game library NEEDS to expose their code for global consumption. It is quite possible that C4 exposes code using a singleton, but is internally architected differently. Finally, it isn't really friendly to multithreading.


Truth is though, if you are exposing an interface that is globally available, there is only one of them and it will be access serially, the singleton isn't a terrible fit. Now change that scenario where that singleton needs to be access by multiple threads at indeterminate times.... that leads to problems.

[Shael]

In my engine I have a few classes that have manager in the name and like C4Engine, they are singletons. The main reason I went this way was because anything I have as a "manager" only requires one instance in the engine. The other reason was to easily export access to these objects from the engine library to the user application. The singletons are however managed by the EngineCore which is responsible for creating and destroying these subsystems. I didn't want to have to make the user access the engine to access these subsystems. Eg. engine->windowmgr->create(..)


Currently I have the following manager classes:

WindowManager: responsible for creating, destroying, and updating each window. updating means polling SDL events and passing the event on to the correct window based on the window ID in the event. Update method called from EngineCore. I think this will have to change though as I will require input events too and this isn't window related. Perhaps an EventManager? or the EngineCore which polls for events and palms them off to the correct subsystem instead.

TimeManager: responsible for calculating time values such as FPS and Deltatime. Update method called from EngineCore.

WorldManager: responsible for loading and unloading a game world and for retrieving the currently active world object.

PhysicsManager: updates the physics world at a fixed timestep in it's own thread. Maybe it's name could actually be PhysicsUpdater instead as that's pretty much what it does. There will be a factory for creating physics objects.

InputManager: takes the input events from SDL or whatever and map them to actions.

So far things seem to gel pretty well this way and I was thinking there might be other managers/objects. But for now I'm just focusing on having a class or two to represent each core sub system.

I'm open to suggestions if this is considered bad design

[Hodgman]

There is nothing wrong with any of these designs, they all work, they all get the task done, and they all make sense. It is all personal preference and people's opinion's when it comes to things like this.

There's a reason this field isn't called Computer ScienceOpinion or Software EngineeringOpinioneering.

[XXChester]

There is nothing wrong with any of these designs, they all work, they all get the task done, and they all make sense. It is all personal preference and people's opinion's when it comes to things like this.

There's a reason this field isn't called Computer ScienceOpinion or Software EngineeringOpinioneering.

I must say I do not understand your comment at all, I am sure it was meant as something derogative anyway.

[swiftcoder]

I must say I do not understand your comment at all, I am sure it was meant as something derogative anyway.

I would classify it under the heading 'educational' rather than 'derogatory'...

The overall point is that 'opinion' is a useful the first few times 'X' is done. Once we reach the point where hundreds of people have independently discovered that 'X' is a bad idea, that discovery ceases to be 'opinion', and becomes 'best practice' - this is just how the fields of science and engineering work.

When we say that 'singletons are bad', we are not implying that something terrible will happen right now, if you use them. We are, however, forewarning you that 10 years down the line your current codebase is going to be an unmaintainable spaghetti of synchronous globals. How can we be so sure? Because we have all been there already - and the same concept goes for all-encompasing 'manager' classes.

[Serapth]

Funny too, I actually laughed out loud when I read it.