What's wrong with using a static class to load and expose all of my assets?

Question

What are the pitfalls associated with loading and storing all references to all of your content/assets within a single static class?

I've been trying to ask myself why I would want to instantiate an assets bank and can not think of a good reason to do so.

I have made a static class which enumerates through my game assets and adds each one to a dictionary which I can access (safely exposed, of course) anywhere like so:

var tex = Assets.Image("folder\\easyaspie");
spriteBatch.DrawString(Assets.Font("font2"), new Vector2(100, 100), Color.White);

... you get the picture.

I appreciate that this gets much more complicated when you have so many assets that having them all loaded at once is not a good idea. However, there is nothing to stop you from including selective loading of assets in your static class, as well as having static methods to change the contents of the 'reference banks'.

What do you think?

Edit:

My examples are pretty shallow, I recognise it's not a good idea to attempt to grab the reference of an asset mid-draw and is much better stored locally.

Answer 1

I would strongly recommend you do not do this.

Static classes are fine for things that lack state or data, especially in languages like C# where you cannot otherwise have free-standing (non-member) functions. Once a class has any kind of state or data, however, you should seriously re-evaluate the decision to make it static. A good rule of thumb is to prefer instance to static; it greatly improves the long-term quality and maintainability of your code if you do.

For a case like this, an asset bank, I don't think a static class is a good idea:

• There's only one asset bank. That means you forgo the possibility of multiple banks of assets (for example, allowing you to load an unload assets as they are needed to conserve memory, or for unit testing). Once you start building this "selective loading" into a single static bank as a way to bypass this problem, you're just engineering a shoddy mockery of what you could have done more simply as an instance class.

• Because there's only one, you have to either engineer to the lowest-common-denominator in terms of access control and safety (think cross-thread access issues), or throw such issues to the wind and demand that client code write and thread-safety wrappers needed. This results in poorly-performing code in many cases for the former option, and the latter option often leads to code duplication and the resulting bugs.

• Being able to easily access the resources anywhere is usually a disadvantage, not an advantage. It might seem the opposite at first, but unconstrained ubiquitous access means you either already have or will be easily able to create a mess of poorly-scoped dependencies on the assets. It's the same general principle behind why globals are generally undesirable constructs.

Instead, build your asset bank as a regular class, and create a single instance of it when the game loads. Pass references to that interface to methods or other interfaces that have a need to load or otherwise access assets.

This gives you the flexibility easily support multiple banks in the future, without constraining you a design that can be very hard to recover from. Making the class static is simple enough, but if you ever need to change that you may find yourself having to clean up a mess of dependencies in client code that assumed there would only ever been a single bank, rather than just working with the bank provided.

Further, this method makes it obvious when you start to introduce dependency creep: when you find that you need to pass the asset bank reference to everything in your game, that is what tells you that your game's subsystems are not well separable and you should consider redesign their API boundaries.

I don't really see any concrete benefits to using a static class for this sort of thing except, perhaps, in an extremely simple bit of tech demo code. Most of the things that appear to be advantages to the static class decision relate to "simplicity" but usually are anything but, in the long run (and simplicity at the expense of quality is not an advantage).

Answer 2

I actually don't have a problem with something like this:

spriteBatch.DrawString(StaticAssets.DefaultFont, new Vector2(100, 100), Color.White);

It's a fairly reasonable simplifying assumption to say that there is only one instance of the default font per program. The kind of scenarios where this breaks down are uncommon, complicated, and difficult to plan for in advance. YAGNI.

You wouldn't want to do this in a library for 3rd-party consumption. But for a small-to-medium-sized game, where you can easily modify your own code, it's not a problem. Make your code flexible. Do things that save development time.

When you break the rules like this, it's important to do it deliberately, and to have an easy "out". In the above case, you can right click DefaultFont, do a "Find All References" and change how it works in a matter of minutes.

The problem with the original code, doing a dictionary lookup by string like this:

spriteBatch.DrawString(Assets.Font("font2"), new Vector2(100, 100), Color.White);

Is that you can't as easily discover all the places where the rules are being broken to reliably fix them when and if the need arises.

---

To put it another way - you really don't want a static asset manager (and I think Josh Petrie did a good job of explaining why). But many times you do want static assets!

Usually such assets will be static to a particular class, not global. So instead of having a Draw method like this:

spriteBatch.Draw(Assets.Image("playerSprite"), position, Color.White);

Or this:

spriteBatch.Draw(StaticAssets.PlayerSprite, position, Color.White);

You should probably do something like this:

class Player
{
    private static Texture2d playerSprite;

    public static void LoadContent(ContentManager content)
    {
        playerSprite = content.Load<Texture2d>("playerSprite");
    }

    public void Draw(SpriteBatch spriteBatch)
    {
        spriteBatch.Draw(playerSprite, position, Color.White);
    }
}

And then call each class's static LoadContent method in your game's LoadContent method. It's perfectly reasonable to assume that there will only ever be one instance of your Game class to be calling LoadContent.