Sign in to follow this  
Nairou

Clean APIs vs. dependency injection

Recommended Posts

I'd like to get some opinions on balancing both the need to decouple the dependencies within my code, and the desire to still provide clean APIs for the rest of the code to use. As an example, let's say I have a Model class which loads game object models from disk, for use in a game:
myModel& monster = Model( "BigMonster" );
myModel& door = Model( "RedDoor" );
Nice and clean and simple. However, in order for the Model class to actually load the data, it needs access to our Filesystem class, to pull the data off of the disk, and the Material class to skin the model. Since these classes are external to the Model class, they have to be injected:
myModel& monster = Model( "BigMonster", filesystemManager, materialManager );
myModel& door = Model( "RedDoor", filesystemManager, materialManager );
But now the API is getting clunky, especially if you start loading a lot of models. Is there a cleaner way to do this? I can think of two options so far, but both have their own problems: 1. Static initialization. Store the Filesystem and Material classes in static references, and initialize them before the Model class gets used:
Model::init( filesystemManager, materialManager );
myModel& monster = Model( "BigMonster" );
myModel& door = Model( "RedDoor" );
However, storing static references feels very fragile, almost like a hack. 2. Turn the Model class into a factory. Rather than creating a Model object each time we want a new model loaded, create a single Model instance, and then call on it each time we want a new model:
modelManager Model( filesystemManager, materialManager );
myModel& monster = modelManager.load( "BigMonster" );
myModel& door = modelManager.load( "RedDoor" );
The downside is it seems overkill. Some systems need to be centralized, by design. But here we are centralizing the Model class purely out of convenience. It also means that game code can't just create a Model object and move on, it has to actually get a reference to the one model Manager object. Is there a better way to do this? I would love to hear any opinions on these options, or any other options I haven't thought of.

Share this post


Link to post
Share on other sites
It sounds like you're looking for a blessing to have a global model factory and/or a global filesystem manager and material manager. I bless you thus. Go forth and be pragmatic.

Share this post


Link to post
Share on other sites
Haha. Actually I'm trying to avoid global objects. I consider that option to be worse than the ones I mentioned, I want to keep the code modular and explicit. In the examples I gave, nothing is global, the objects are just being passed around. I just wanted to see if there was another way of doing so that didn't clutter the class interfaces.

Share this post


Link to post
Share on other sites
Quote:
Original post by Nairou
I just wanted to see if there was another way of doing so that didn't clutter the class interfaces.
If you truly believe that globals have no place in C++, then passing loader interfaces around ad nauseam is not "clutter" but the laudable work of a concern well-separated. [grin] Explicitly passing the managers explicitly documents what the model loader is and is not allowed access to.

If you want to save yourself some typing, the third second approach is probably the best. Think of a ModelManager as nothing more than the Model creation function partially bound over the manager arguments, a necessary code wart in a language without actual support for partially bound functions. In particular, since a ModelManager can be created from nothing more than a FileManager and a MaterialManager, you can create one whenever you need one, rather than creating exactly one and keeping it around.

Share this post


Link to post
Share on other sites
API clutter - incorrect abstraction. Round peg and square hole.

Why does model need to know about file system?
Why does it need to know about texture manager?

A model is in-memory representation of some data. It doesn't know or care about file system, texture managers or anything else.

The method of loading something from disk (or other source) is known as serialization. Let's say that model is serializable (and we only support reading).
class Model {
void load(Source &);
};
Source is something that can read bytes, but implemented elsewhere and completely independent.

Quote:
filesystemManager
Apparently, something that manages a file system. So how do we make a model from file system?
Model loadModelFromFile(string s) {
Source s = filesystemManager.getSource(s);
Model m = new Model();
m.load(s);
return m;
};


But how does loadModelFromFile know where to get filesystemManager? And, how will model resolve textures?

Who really needs to know how to resolve textures? The Source, which will deserialize some opaque reference into live texture. But this means that file manager can no longer provide us with Source, but we must construct a specific one for model loading:
Model loadModelFromFile(string s) {
File f = filesystemManager.getFile(s);
Source s = new ModelLoaderSource(f, materialManager);
Model m = new Model();
m.load(s);
return m;
};
This is better, loading is now done. ModelLoaderSource implements Source interface, it takes reference to file from which to read (can be FileSource<-Source) and material manager which will resolve serialized references into loaded textures.

But, both filesystemManager and materialManager are now just assumed to exist. Another problem is, that if materialManager gets asked for a texture it doesn't have, it needs to load it from filesystemManager.

class Assets {
FileSystemManager filesystemManager;
MaterialManager materialManager;

Assets() {
filesystemManager = new FileSystemManager();
materialManager = new MaterialManager(filesystemManager);
}

Model loadModelFromFile(string s) {
// implemented above
}
};
And voila, the spaghetti are untangled.

To load a model, you call your Asset class instance, and use loadModelFromFile utility method.

Share this post


Link to post
Share on other sites
Quote:
Original post by Sneftel
In particular, since a ModelManager can be created from nothing more than a FileManager and a MaterialManager, you can create one whenever you need one, rather than creating exactly one and keeping it around.


Since that eliminates the one downside to his final approach, are there any other hidden downsides?

[edit] and thank you Antheus for answering before I asked.

Share this post


Link to post
Share on other sites
Quote:
Original post by popsoftheyear
Since that eliminates the one downside to his final approach, are there any other hidden downsides?
The downside is the inevitable downside of any OOP method for object creation based on dependencies. If an object A creates an object B, the creation of which depends on help from objects C and D, then A must be the kind of object which cares about C and D. If it doesn't, then it must at least be the kind of object which cares about and stores a B-creator, meaning that either it must care about C and D in its constructor in order to create a B-creator, or whatever object created A must itself care either about C and D, or about B-creators. OOP, particularly with strict use of the Law of Demeter, inevitably leads to this situation, where the concerns of an object must include the concerns of all objects whose creation it is directly or indirectly responsible for.

The solution is to think of something like a FileSystem manager as a software service, as opposed to a depended-on object. But that requires either globals (eek) or more wordy Law-of-Demeter-breaking tricks.

[Edited by - Sneftel on May 29, 2009 3:17:01 PM]

Share this post


Link to post
Share on other sites
In one 3D engine I did a study on, basically figuring out the kinds of OOP classes that can be made. I played on this structure of classes where there was the Engine class instantiated outside of main(), and that gets init()'ed in main. So during engine.init(), a lot of objects get created mostly managers.

For mesh/model loading, I simply called engine.loadmesh() and returned a pointer to it, before sending that to the renderer and physX (actor).

No singleton classes were implemented in this test program which I found to be a lot better than the previous version I wrote.

Share this post


Link to post
Share on other sites
Quote:
Original post by loufoque
Quote:
myModel& monster = Model( "BigMonster" );

This is not legal C++. Temporaries may not bind to non-const references.

Consider it pseudo-code. I just wrote it on the spot to illustrate a point, I didn't think much about correctness.

Quote:
Original post by Zahlman
Hold on. Let's take a step back first.

Why do you have managers?

What do they do?

I usually use managers to control centralized resources. For example, the Filesystem class handles all disk loading and caching, the Material class parses material config files and outputs material data structures, the Gui class maintains the UI state and outputs to the renderer, etc.

Most of these probably could be decentralized, but I would probably have to use a lot more static member variables to share the central data being used. Also, having a class internally create instances of all these big classes in order to use them seems a lot less modular than actually passing in existing references to them.

Quote:
Original post by Antheus
And voila, the spaghetti are untangled.

To load a model, you call your Asset class instance, and use loadModelFromFile utility method.

Thank you for your detailed comment, I am still re-reading it to fully get it. However, if you ignore all of the logical restructuring you did, aren't you just trading what dependency needs to be passed around? Rather than creating a Model object, and passing the Filesystem and Material references to it, you're being passed a central Assets reference and loading the model from there.

[Edited by - Nairou on May 30, 2009 11:21:10 AM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Nairou
However, if you ignore all of the logical restructuring you did, aren't you just trading what dependency needs to be passed around?


The sky is blue. A depends on B. These is the world we live in, the cards we are dealt.

Yes, I am trading dependencies and shuffling things around, but I let dependencies guide me. The fact that mesh will somehow need to be loaded from file is a given.

What I try to do is minimize the coupling until it boils down to bare minimum. I would likely factor out both filesystemManager as well as materialManager, but I don't know what they do, so I can't.

There is actually another step possible that I didn't perform before.
class Assets {
FileSystemManager filesystemManager;
MaterialManager materialManager;

Assets(FileSystem fs) {
filesystemManager = fs;
materialManager = new MaterialManager(fs);
}

Model loadModelFromFile(string s) {

// implemented above
}
};


Quote:
Rather than creating a Model object, and passing the Filesystem and Material references to it, you're being passed a central Assets reference and loading the model from there.


I'm not passing around anything. My entire application looks like this:
int main() {
FileSystemManager fs = new FileSystemManager();
Assets assets = new Assets(fs);
Model m1 = assets.loadModelFromFile("mesh1");
};
This is as simple as it gets.

This is my world, there is absolutely nothing else, hence according to YAGNI I don't implement it. The code above is a result of refactoring - I didn't plan for it. I'm not saying "I am right" when code disagrees with me. I could very well refactor to do this:
int main() {
FileSystemManager fs = new FileSystemManager();
Source s = new ModelLoaderSource(fs, materialManager);
Model m = new Model(s);
}
Same thing, but since I will need to create more than one model, I might as well wrap it into a factory. The important thing is, the design is highly decoupled, and I can now perform changes like these with minimal impact.

It's the "once, twice, refactor" rule. I could start with later implementation, and as soon as I'd to create ModelLoaderSource in another place in code, I would refactor.

IOC is incredibly convenient for agile development since it allows you to focus on tiny details and mix-and-match them as needed. Since everything is coupled via interfaces and references only, I can trivially develop and test each part in complete isolation, even before other systems exist.

And this also solves:
Quote:
the desire to still provide clean APIs for the rest of the code to use.
The above is clean API. User uses Assets instance to create meshes. It doesn't matter where it comes from. You can provide an existing instance, a global (not singleton, since there can be multiple), or they can create one on their own.

Assets::loadModelFromFile(string) is as clean as it gets. The user then simply keeps as many copies of assets as reasonable, usually one per scene.

But the biggest problem with your API being cluttered comes from putting too much functionality into Mesh. It needs to know how to access file system, read data, load and store materials - in addition to what it really is - a data holder, a POD.

Consider something else - why isn't Model being created from what it really is: a list of vertices and triangles with additional texture data. Something like:
Model::Model(Vertex v[], int nVert, Tri t[], int nTri, Texture * tx[], nTex);
Because this is really what a model is. Loading is orthogonal functionality which almost all serialization tools implement externally, but often expose it either via member functions or in constructor.

This change completely decouples model class from everything else, even from standard containers, meaning it's often suitable for C-interoperability. I can then use it with standard containers, with stack-allocated data (convenient for loading to avoid allocations since model file often specifies total number of elements), or something else.

Share this post


Link to post
Share on other sites
Thanks, Antheus, you've given me a lot to think about. I must admit I'm still learning to follow YAGNI, I have a tendency to plan ahead rather than just write what I need now.

Quote:
What I try to do is minimize the coupling until it boils down to bare minimum. I would likely factor out both filesystemManager as well as materialManager, but I don't know what they do, so I can't.

I can see how my Material class could be factored out (as long as you provide a Filesystem reference, it consists of little more than a loader function that outputs complex structs), but I'm curious how you would factor out the Filesystem class (as an example).

Granted, it too consists of little more than a loader function. However, it does contain a private std::map, where it caches the data it loads. That map still needs to exist somewhere, so the only factoring I can think of is to make it static and turn the Filesystem into a purely static class.

But that leads to the whole question of why I don't turn a lot of these classes into static classes, why static classes might be "bad", etc.

How would you handle this?

Share this post


Link to post
Share on other sites
Quote:
Original post by Nairou
I can see how my Material class could be factored out (as long as you provide a Filesystem reference, it consists of little more than a loader function that outputs complex structs), but I'm curious how you would factor out the Filesystem class (as an example).

Java and C# both define Readers and Streams. These have proven to be adequate abstractions for such tasks. They make clear separation of content provider and content handler. While in C++ it would be implemented slightly differently, boost::serialization defines a common way to approach this problem.

The key concept here is that Reader is capable of instantiating a concrete object (Model/Mesh/Texture), so that is who must be passed the references to relevant user-defined objects (Factory, cache).

Quote:
Granted, it too consists of little more than a loader function. However, it does contain a private std::map, where it caches the data it loads.
So it's a cache. Why not call it as such.

Quote:
That map still needs to exist somewhere, so the only factoring I can think of is to make it static and turn the Filesystem into a purely static class.

There is no such thing as static class in C++. An instance of a class can be allocated statically in global context. This is technical implementation detail, not design issue.

Static allocation is commonly abused to define a specific and centrally accessible instance. This requirement is rarely needed, and less often desired specifically in C++, due to initialization issues.

Quote:
How would you handle this?
Like I demonstrated above. User allocates the instances. If they choose so, they can make them static.

Design doesn't care about how something is allocated, since references are passed as needed.

Share this post


Link to post
Share on other sites
Quote:
Original post by loufoque
If you want a clean API, I would suggest to stop mixing Java (or is that C#?) and C++, and even not to code Java-style at all in C++.

Err, huh? This is all C++. And pseudo-code. Can you explain?

Share this post


Link to post
Share on other sites
Quote:
int main() {
FileSystemManager fs = new FileSystemManager();
Assets assets = new Assets(fs);
Model m1 = assets.loadModelFromFile("mesh1");
};

This is obviously not C++. new FileSystemManager() would return a FileSystemManager*, not a FileSystemManager.

The same can be said for all your code.

The good practices of C++ are to use RAII, not pointers galore.

Share this post


Link to post
Share on other sites
Quote:
Original post by loufoque
It is.
Ownership issues are crucial to design a C++ API properly.


This is not For Beginners forum, so I omit irrelevant implementation details that would clutter the code and obscure the design issues. Allocation strategies in C++ are known, so are the classes that support them. I have merely provided a by-example guide to refactoring dependencies, clarifying the ownership issues in the process.

It is however unfortunate that the valid points you raised seem to have been perceived as irrelevant to discussion. My personal experience in dealing with design and implementation issues has always met with positive results whenever non-conforming views were presented with a concrete example, or some elaboration on points raised. It is quite common that alternative opinions are welcomed, and given enough subject matter, they lead to productive discussions.

Share this post


Link to post
Share on other sites
I run into this issue regularly in my code. Maybe I'm just not disciplined enough. I moved away from globals and singletons - right now I only have two globals, VFS and Logger, and I have vague plans of replacing Logger and un-globalizing VFS (which handles archives and plain old files). Right now I mostly favor dependency injection, refactoring as needed if it becomes too deep or if an object really has no business knowing about loading mechanisms. It is quite a bit more typing and refactoring compared to when every service in my game was a singleton, but at least I know what's going on where.

For instance, TextureFactory may return a Texture object, but the task is split into two, one being "load data from file" and two being "new Texture(w,h,data)", so if you want to have, say, a new empty texture you can create it yourself and consequently cache it - caching is handled by a generic ResourceCache<R extends Resource>, but for C++ I saw a post by ToohrVyk around December which presented a templated cacher for any key/value pair, which gave me the idea for this. The cacher has an associated factory object that it loads data from once a cache miss occurs.

All in all, I think this is quite similar to what Antheus describes, although I create the "Assets" a bit differently - a factory that needs textures to operate is passed a ResourceCache<Texture> in its constructor, which allows for chaining caches and factories (fontcache backed by fontfactory backed by texturecache backed by texturefactory).

Share this post


Link to post
Share on other sites
Quote:
Original post by loufoque
Ownership issues are crucial to design a C++ API properly.

The ownership here is trivial. There is only one actual filesystem. As a result, the semantics of all objects in the chain from the loading function to the filesystem are non-deep-copying WRT the filesystem. I'm pretty sure Nairou understands this.

Share this post


Link to post
Share on other sites
Quote:
Original post by Antheus

I'm not passing around anything. My entire application looks like this:[code]

int main() {
FileSystemManager fs = new FileSystemManager();
Assets assets = new Assets(fs);
Model m1 = assets.loadModelFromFile("mesh1");
};




I got a question about this. If you aren't using globals, statics, or passing a Asset() pointer around would you recreate the Asset() at the start of the level load code and then let it go out of scope at the end of the level to be recreated next level? And then the menu would create it own Asset() to load up the textures needed for that.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this