Back to General and Gameplay Programming

Argument against "manager" classes

General and Gameplay Programming Programming

Started by T1Oracle November 09, 2006 03:23 AM

52 comments, last by Zahlman 17 years, 4 months ago

852

November 09, 2006 08:45 AM

Quote:Original post by Skizz
Personally, I've always considered a manager as an object that tracks a collection (or collections) of objects and performs actions on the set of objects or a subset thereof. For example: A RenderManager class holds a collection of references to all objects in the world. It stores them in an octree (or whatever structure you want). Calling "RenderManager::Render (target)" for example would then render all the visible objects to the specified target.

You mean

fmap render octree

? Manager classes are used in languages that don't have support for functions free from classes and/or higher order functions. It's really a pattern used as a workaround for the limitation of the language. The more "managers" you see in the source code, the shittier your host language is. For example, you'll almost never see "managers" in Haskell or Lisp, you'll see them show up in places in C++, and you'll see them scattered all over Java code.

defmacro.org

BrianL

530

November 09, 2006 11:36 AM

I totally agree with jpetrie. It is all about invariants and encapsulation.

Design patterns have very specific meanings. If something is called a factory, I would assume it always returns a newly created object. If something is called a pool, I assume is a resource optimization, etc.

Managers seem like a specialization of the facade pattern. They are 'glue' that presents a unified interface to other more complex systems. For example, a TextureManager may internally use a Pool and a Factory to create textures. It may support methods for making queries about the number of textures in memory, the amount of memory space they take, etc.

Yes, these could be done by iterating over all of the textures manually everywhere to calculate this info, but the manager could cache it (as an invariant) when textures and created/destroyed. This would be more efficient as well as more encapsulated.

T1Oracle

100

Author

November 10, 2006 03:45 AM

Quote:Original post by BrianL
I totally agree with jpetrie. It is all about invariants and encapsulation.

Design patterns have very specific meanings. If something is called a factory, I would assume it always returns a newly created object. If something is called a pool, I assume is a resource optimization, etc.

Managers seem like a specialization of the facade pattern. They are 'glue' that presents a unified interface to other more complex systems. For example, a TextureManager may internally use a Pool and a Factory to create textures. It may support methods for making queries about the number of textures in memory, the amount of memory space they take, etc.

Yes, these could be done by iterating over all of the textures manually everywhere to calculate this info, but the manager could cache it (as an invariant) when textures and created/destroyed. This would be more efficient as well as more encapsulated.

You make an intersting claim however lets point a few things out:

1) Factory
a. Definition: Well defined while still abstract - A Factory is an object which encapsulates the details needed to create certain concrete objects. This may be realized through the Abstract Factory which only returns abstract pointers, or the Factory Method who's returned object is decided by the set parameters.
b. Scope: Has a limited scope in which it may be applied - If it does not return newly created objects then it is not a factory.
c. Application: Not all objects are Factories.

2) Pool
a. Definition: Well defined while still abstract - A variation of the Factory pattern that optimizes the allocation of memory by pooling resources for memory resuse inplace of constant allocation and deallocation of RAM.
b. Scope: Has a limited scope in which it may be applied - If it does not optimize the use or memory by restricting the occurance of memory allocation and deallocation then it is not a pool.
c. Application: Not all objects are Pools.

3) Facade
a. Definition: Well defined while still abstract and leaves out a lot of impelementation details in favor of a focus on the interface - An object that provides a simplified interface to a larger body of code.
b. Scope: Has a limited scope in which it may be applied - If it does not produce an interface which simplifies the complexity of a greater system, then it is not a facade.
c. Application: Not all objects are Facades

4) Manager
a. Definition: The epitome of ambiguity - Something which stores data and performs actions on it.
b. Scope: Anything and everything in software fits under the definition of manager.
c. Application: All objects are managers, all programs are managers.

The term manager is generally used under the following conditions:
1) The programmer does not fully understand their problem space
2) The programmer seeks an easy out to true design by lumping all Foo related things into one giant FooManager.
3) Do to the lack of a true design step, the programmer is unaware of other possible means for organizing data, methods, and responsibilities in a more thoroughly contrived manner.
4) Since everything is a manager, you might as well call it a manager.

Programming since 1995.

Skizz

794

November 10, 2006 09:01 AM

You say that managers are "the epitome of ambiguity". Well, there's this thing called documentation which can be used to resolve ambiguities. Look:

//-------------------------------------------------------------------------------// Class: RenderManager//// Description: A specialisation of the facade design pattern, this class//              provides methods for add/removing objects to the set of//              renderable objects and for efficently rendering the objects.//// Interface:   AddObject - adds object to set of renderable objects//              RemoveObject - removes object from set of renderable objects//              Render - renders all objects within a view volume to a target//// Implementation: Internally, the render manager uses two octrees, one for//                 all the static geometry in the scene and one for the dynamic//                 objects. When rendering, the octrees are used to cull objects.////-------------------------------------------------------------------------------class RenderManager{   // the code!};

Now, you could have called the class RenderFacade but I think more people would know what a RenderManager did from the name alone without looking at any documentation.

The main point is that 'manager' is just a word, a word with a definition that closely matches what the programmer is trying to do.

What would you call the above class?

Skizz

CTar

1,134

November 10, 2006 09:22 AM

Quote:Original post by Skizz
Now, you could have called the class RenderFacade but I think more people would know what a RenderManager did from the name alone without looking at any documentation.

We are (at least I am) aiming our libraries at C++ OO programmers, and such programmers should know what the facade pattern is.

Quote:provides methods for add/removing objects to the set of
renderable objects and for efficently rendering the objects.

(Emphasis added)
And? It seems you have fallen into the trap many people pointed out, just putting lots of functionality into a single class. One class shouldn't have more than one responsibility.

Quote:The main point is that 'manager' is just a word, a word with a definition that closely matches what the programmer is trying to do.

Yes, but because two programmers' definition might not match they might misunderstand each other.

Quote:What would you call the above class?

I'd probably split it into a RenderList and a Renderer, but to make a good decision one would need to know something about the design of the rendering system.

EDIT:

Quote:You say that managers are "the epitome of ambiguity". Well, there's this thing called documentation which can be used to resolve ambiguities.

Isn't self-documenting code a good idea?

Skizz

794

November 10, 2006 09:23 AM

And another thing, a bit off topic, regarding GUIDs. MS does not have its own definition and implementation - its implementation is based on UUIDs (the two terms represent the same thing) which is defined by the Open Software Foundation (link). Wikipedia's article has a table of the probabilities of generating the same value twice. They're not slow to create (first link has an algorithm that can generate 1x10^6 per second, probably more these days). The only downside is the size - 128 bits, but even that isn't such a big deal these days.

Skizz

Skizz

794

November 10, 2006 09:39 AM

Quote:Original post by CTar
I'd probably split it into a RenderList and a Renderer, but to make a good decision one would need to know something about the design of the rendering system.

Yes, you're right, it's not a great example thinking about it. Perhaps this is a better example:

class RenderableObjectManager{    // AddObject - adds an object to the set of renderable objects    // RemoveObject - removes an object from the set of renderable objects    // GetVisibleSet - gets the set of visible objects for a given view};

I think that putting the pattern name into type name doesn't always increase the understandability of the code. 'Facade' doesn't describe what it does, only how it does it, so is probably best put in comments, i.e. "this class uses the facade pattern". "RenderableObjectFacade" is more abiguous than "RenderableObjectManager" because "Facade" doesn't define what is being done to the "RenderableObject" only that it is an interface that simplifies a more complex system. "Manager" slightly reduces the scope of what the class could do, but whatever name you pick, it will always be open to some interpretation. Hence the need for comments and documentation to refine the definition. "ClassToStoreSetsOfRenderableObjectsAndRetrieveSubSetsThereof" is a bit longwinded.

Thinking about it, "RenderableObjectDatabase" is probably a better name, although some people might assume it uses SQL or something under the hood.

Skizz

CTar

1,134

November 10, 2006 10:31 AM

Quote:Original post by Skizz
...

So in that example what you really want is a sequence of render-able objects. You want to be able to return a new sequence of all the objects matching a certain criterion (visible).

Returning the subset shouldn't be in the class.

I would do something like this (Assuming a renderable object is of type Renderable and a "view" is of type "View"):

class RenderableSequence{    // AddObject - adds an object to the set of renderable objects    // RemoveObject - removes an object from the set of renderable objects};// Visible - Predicate given a renderable object and a view, // returning whether the renderable object is visible from the view.// VisibleSubset - Given a RenderableObjectSequence and a view, it's returning the RenderableObjectSequence of all// visible objects. You could use std::remove_copy_if if RenderableObjectSequence// supported iterators.

Quote:ClassToStoreSetsOfRenderableObjectsAndRetrieveSubSetsThereof

There are many reasons you end up with such a long name. First you need to split up functionality to get two distinct parts:

ClassToStoreSetsOfRenderableObjects
RetrieveSubsetsOf

A "class to store T" is commonly refered to as a "T sequence". Also it doesn't store sets of renderable objects, just renderable objects.

RenderableSequence
RetrieveSubsetsOf

That second name is horribly undescriptive, it sounds like it's returning the power set of a set. Additionaly there is only one subset (no s). A better name would be:

RenderableSequence
RetrieveVisibleSubsetOf

Though in programming you rarely say "retrieve" or "of", it's implied. So:

RenderableSequence
VisibleSubset

Suddenly we have very descriptive and short names.

BrianL

530

November 10, 2006 11:38 AM

Quote:Original post by CTar
I would do something like this (Assuming a renderable object is of type Renderable and a "view" is of type "View"):

class RenderableSequence{    // AddObject - adds an object to the set of renderable objects    // RemoveObject - removes an object from the set of renderable objects};// Visible - Predicate given a renderable object and a view, // returning whether the renderable object is visible from the view.// VisibleSubset - Given a RenderableObjectSequence and a view, it's returning the RenderableObjectSequence of all// visible objects. You could use std::remove_copy_if if RenderableObjectSequence// supported iterators.

Sure, except this doesn't take into account other complications. For example, your Visible predicate may depend on a octree lookup as a culling optimization. Now your client code needs to know about RenderObject, RenderableSequences, the Visible predicate, the View, the VisibleSubset, the Octree, etc.

This makes sense for a library, where you want the client code to have as many options as possible. Unfortunately, it isn't very encapsulated. If you need to change from an Octree to a kd-tree some day, you may have to make changes in a large number of locations. Yes, you can provide free utility functions to get around this. But then the client code code needs to know about Add/Remove/GetSubset utility functions as well (and it needs to know that it should use these instead of manipulating the primitive objects directly). Similarly, RenderObjects/Octree may have some invariant state. This would also need to be protected in some way.

If you are using this library, chances are you want to wrap it in a simpler interface that does what you need (AddObject/RemoveObject/MoveObject/GetRenderableObjects). This isolates it from the rest of your code and makes maintenance simpler. So you create a RenderObjectManager, which uses these primives.

Skizz

794

November 10, 2006 11:44 AM

Quote:Original post by CTar
RenderableSequence
VisibleSubset

The problem with this division is that VisibleSubset doesn't know about how RenderableSequence stores its objects. Generally, as you pointed out, this is desirable. However, in this case it probably isn't. RenderableObjectManager wasn't just a store of objects, it added spatial information by use of an octree or BSP for example. Although it is possible to write an iterator for an octree, getting std::remove_copy_if to work efficently would be tricky. The important thing in this case is that the Visible predicate would not be applied to the stored objects (unless, of course, the object was in a boundary condition, i.e. not wholly within the view and not wholly outside).

In the RenderableObjectManager case, the Add / Remove / GetVisible are tightly coupled to get best algorithmic efficency. Changing the way the objects are stored (changing from octree to bsp for example) would require different implementations of Add, Remove and GetVisible. A GetVisible written for a BSP would not work for an octree or a portal system.

My point here is that the Add / Remove / GetVisible class is the easiest to implement and understand1.

Which leaves the question "what to call it?" It's not really a sequence as such (sequence to me implies a list of some type) and there'd never be a need to use iterators on it (but you would on the result of GetVisible). 'Scene' might preferable, but you might think it included other stuff as well (collision data perhaps). So you call it something that fits best with the function of the class and write comments / documentation to expand on the meaning of the name.

The key argument I've been trying to say is that it doesn't matter what the class' name is as long as it's a close fit to the function of the class, i.e. it gives a resonable overview of the function of the class, and there is documentation / comments to describe the function of the class in more detail.

Skizz

(1) I once had a code review where one of the comments about my code was that it wasn't 'clever'. I never really found out what was meant by this. The code was well laid out, commented and easy to understand and use, just not 'clever'. I always like to keep my code as simple as possible.

Argument against "manager" classes

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Argument against "manager" classes

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines