C/C++ architecture question + friend class

Started by
9 comments, last by Ravyne 13 years, 9 months ago
Hi,

I need some opinion on a problem I am facing now, appreciate any input. Thanks =).

I want to design a code architecture whereby the user interface is exposed by Manager singleton classes, for example, SceneManager. Managers typically manages a whole bunch of objects that are closely related. Often times the Manager is required to call some other class' member functions that are "internal" to the architecture.

For example I have a SceneManager which exposes the RemoveSceneNode() to, well, remove SceneNodes safely:

class SceneManager : public Singleton<SceneManager>{public:    // finds the actual SceneNode and call SceneNode::RemoveChildNode()    // before calling delete on itself.    void RemoveSceneNode(String name);private:    SceneNode* _mRoot;    std::map<String, SceneNode*> _mSceneNodes;};


A possible SceneNode is as follows, but can introduce misuse.
class SceneNode{public:    // Search _mChildren and remove the node    // But putting this in public can introduce potential misuse,    // like people calling this directly.    void RemoveSceneNode(SceneNode* node);private:    std::list<SceneNode*> _mChildren;};


A possible solution is to use friend class, but is this acceptable?
class SceneNode{// This design breaks OOP, but in this case is it acceptable?fiend class SceneManager;private:    void RemoveSceneNode(SceneNode* node);    std::list<SceneNode*> _mChildren;};


Yet another way is to do the search for child nodes inside SceneManager itself, but that also has other OOP issues as well as the need to access the private _mChildren list.

What could be a better design?
==============================================Rage - Really Amateurish Graphics EngineCollada Parser / Serializer
Advertisement
In C++, a friend class is indeed the acceptable solution to this situation.

As a tangent (and to stir up a nice big hornet's nest [grin]), why are you making these classes "managers" and singletons? Why not just have a SceneGraph class that handles the appropriate responsibilities?

Wielder of the Sacred Wands
[Work - ArenaNet] [Epoch Language] [Scribblings]

Friend classes don't "break OOP".
It's not a bug... it's a feature!
Quote:Original post by Dom_152
Friend classes don't "break OOP".


Really?! maybe i mistaken them with friend functions. Yes, I suppose so, it just makes the classes appear like structs...

Quote:Original post by ApochPiQ
Why not just have a SceneGraph class that handles the appropriate responsibilities?


Not sure what SceneGraph you have in mind, but I suppose SceneManager is kind of like a SceneGraph, except in the full implementation, it also does some other stuff, like keeping track of multiple SceneGraphs, so it is a "Manager" instead.
==============================================Rage - Really Amateurish Graphics EngineCollada Parser / Serializer
OK, to extend the question: why do you need to track multiple scenegraphs in a single object? Why don't you just have a std container of scenegraphs someplace?


If you're really interested in improving your design, you should seriously reconsider your use of singletons, and you should probably also be aware that "manager" classes tend to become "do everything" classes which are very brittle and hard to maintain in the long term. In general, if you need to "manage" a set of objects, you probably didn't write them in clean RAII style (which is Very Bad). Once you have RAII-based designs in place, the need to "manage" really just becomes a matter of holding a list of the objects in question, which is best left to an existing container class.

See also the "single responsibility principle" which, when followed correctly, basically eliminates the need for managers entirely.

Wielder of the Sacred Wands
[Work - ArenaNet] [Epoch Language] [Scribblings]

So friend classes do break encapsulation, and if going with strict OOP principles, they do break OOP.

One of the problems with friend classes is that they are not inherited. This is a huge problem if you plan on deriving specialized types of nodes from the scene node which is very common in scene graph types of architectures.

Usually if you see the friend class (and I use it sparingly) there is usually an issue with your design. Why can't the removeNode() function be public? What kind of misuse are you suggesting could happen?

As for the misuse, if this is for your own game/project then why does this matter? You know best how the API is used. Instead of worrying about how to make the API airtight and spending a lot of time on making the API enforce the rules for how it should be used, (paraphrasing Steve Jobs) just don't use it like that. If this is your project, you control the use and misuse of the API.

I don't get the impression that you are selling this API as a 3rd Party library to many customers, so that puts even less emphasis on making the API airtight. I work for a company that makes middleware and we do rely on the API to prevent the customer from doing stupid things and causing us undo support questions wasting time and money. I just don't think you have that need at this point. In my experience, API development is an evolutionary process. Getting it right the first time is rare and difficult. It's always better to get something done, maybe put a comment such as "//Public for the scene manager's use, don't call this directly" and move on to the next problem.

Don't get wrapped around the axle for making the best API for making games, finish your game (with a less than optimal api) and then refine it. Make another game, and refine it. Over time you'll have a solid foundation of libraries to use to make games.

In my opinion, having removeNode() as a public function is the right thing to do anyways. Any node should be able to remove a child node from it and the the scene manager (or anybody owning a pointer to a node) should be able to do that.


Cheers,

Bob


[size="3"]Halfway down the trail to Hell...
They only break encapsulation if you use them wrong.
See this.

If you're using them simply to make private data available to any other class you choose without discretion then that does break encapsulation. But you can use them to spread functionality across several related classes (perhaps even of different lifetimes) that may need to share data. However use of friend classes is rare in a lot of C++ code as far as I know.

Out of interest why can't your SceneManager class take care of child nodes of the node given to remove?
It's not a bug... it's a feature!
Quote:So friend classes do break encapsulation, and if going with strict OOP principles, they do break OOP.


Without friend classes, private/protected members would be an all or nothing deal. If you needed to grant a single class access to something, you'd be forced to make it public, thus granting everything access to it. Being able to choose means you can maintain encapsulation.

Besides, there are textbook examples like when overloading binary operators where the class parameter is on the right hand side of the operator. Technically a friend function and not a class, but anyway.
Quote:Original post by jakesee
Hi,

I need some opinion on a problem I am facing now, appreciate any input. Thanks =).

I want to design a code architecture whereby the user interface is exposed by Manager singleton classes, for example, SceneManager. Managers typically manages a whole bunch of objects that are closely related. Often times the Manager is required to call some other class' member functions that are "internal" to the architecture.

For example I have a SceneManager which exposes the RemoveSceneNode() to, well, remove SceneNodes safely:

*** Source Snippet Removed ***

A possible SceneNode is as follows, but can introduce misuse.
*** Source Snippet Removed ***

A possible solution is to use friend class, but is this acceptable?
*** Source Snippet Removed ***

Yet another way is to do the search for child nodes inside SceneManager itself, but that also has other OOP issues as well as the need to access the private _mChildren list.

What could be a better design?


Just to improve the performance of your scenemanager you should really hash the strings and use a hash map to store them. String compares are still really slow, and scenegraphs can become big quickly.

Worked on titles: CMR:DiRT2, DiRT 3, DiRT: Showdown, GRID 2, theHunter, theHunter: Primal, Mad Max, Watch Dogs: Legion

Quote:Original post by ApochPiQ
OK, to extend the question: why do you need to track multiple scenegraphs in a single object? Why don't you just have a std container of scenegraphs someplace?


If you're really interested in improving your design, you should seriously reconsider your use of singletons, and you should probably also be aware that "manager" classes tend to become "do everything" classes which are very brittle and hard to maintain in the long term. In general, if you need to "manage" a set of objects, you probably didn't write them in clean RAII style (which is Very Bad). Once you have RAII-based designs in place, the need to "manage" really just becomes a matter of holding a list of the objects in question, which is best left to an existing container class.

See also the "single responsibility principle" which, when followed correctly, basically eliminates the need for managers entirely.


In real life you sometimes just need Singletons and Globals as there is no neat OOP way to solve the problem. RAII breaks in certain cases, I can come up with situations wherein an item class needs a pointer to the containing class, now you can no longer construct this contained class within the containing class.
For example a application object class which represents the whole application. Cannot create objects that need access to the application object or get the application object passed in.

Worked on titles: CMR:DiRT2, DiRT 3, DiRT: Showdown, GRID 2, theHunter, theHunter: Primal, Mad Max, Watch Dogs: Legion

This topic is closed to new replies.

Advertisement