How to avoid Singletons/global variables

Started by
21 comments, last by frob 7 years, 10 months ago

Heard it it bad to use but not much example tell you how to replace/avoid to it , considering the following case


//making things public so it's just faster for us to understand the example

//Texture.h
class Texture
{
 public:
   Texture(string a_name){
         m_name = a_name;
         m_ID = TextureManager.GenTexture(a_name);
   }
   string m_name;
   int m_ID;
};

//Texture Manager.h
class TextureManager
{
 public:

 //this store the name of the texture and the ID of the texture
 static Map<string, int> m_textures;
 static int GenTexture(string a_name){

    //assume the following code works, you guys get the meaning  
    for(int i =0 ; i<m_textures ; i++)
    {
       if(m_textures[i].name == a_name)
       {
         return m_textures[i].ID; 
       }
    }
    int id = OPENGL_genTexture(a_name);
    m_textures.Add(a_name , id);
    return id; 
  }

 
}

There's only know one way to not use slot of not use global variable , but it does not make sense to me


//Texture.h
class Texture
{
 public:
   Texture(string a_name){
         m_name = a_name;
         m_ID = TextureManager.GenTexture(a_name);
   }
   string m_name;
   int m_ID;
private:
   static Map<string, int> m_textures;
   static int GenTexture(string a_name){
    for(int i =0 ; i<m_textures ; i++)
    {
       if(m_textures[i].name == a_name)
       {
         return m_textures[i].ID; 
       }
    }
    int id = OPENGL_genTexture(a_name);
    m_textures.Add(a_name , id);
   return id ;
  }

};


so it there anyone can give me recommendation what to do about it? thanks

Advertisement
There's only know one way to not use slot of not use global variable , but it does not make sense to me

Event in your second example, you use a global variable, you just moved it to the other class. A common way to avoid this is by doing depenceny inversion injection (oups), by passing in your texture manager to the texture class:


class Texture
{
public:
   Texture(string a_name, TextureManager& textures){
         m_name = a_name;
         m_ID = textures.GenTexture(a_name);
   }
}

Now you have TextureManager as a regular non-static class somewhere, and pass its instance in whenever needed.

Or better yet, remove the depenceny of Texture<-TextureManager completely, and have your texture just take an int, while having the TextureManager have a CreateTexture-method:


class Texture
{
public:
   Texture(string a_name, int ID){
         m_name = a_name;
         m_ID = textures.GenTexture(a_name);
   }
}

//Texture Manager.h
class TextureManager
{
 public:

    Texture& CreateTexture(string a_name)
    {
        for(int i =0 ; i<m_textures ; i++)
        {
           if(m_textures[i].name == a_name)
           {
             return m_textures[i].Texture;
           }
        }
        int id = OPENGL_genTexture(a_name);
        
        auto texture = new Texture(a_name, id);
        m_textures.Add(a_name , texture);
        
        return texture;
    }
    
private:

 //this store the name of the texture and the ID of the texture
 Map<string, Texture*> m_textures;
}

Note that I've also changed the TextureManager to store a "texture" pointer instead of just the id. I'm not sure whether your Texture-class is actually representing an handle? In this case, this change doesn't make much sense, but on the other hand it would be very bad to store a string inside an handle since this makes passing it around/storing it very costly. So eigther create one Texture-object, store it in the map and return it on subsequent calls. Or remove anything but the ID from the texture class, so you can use it like a primitive type if you plan on using an approach like that.

Well from a quick glance it looks like there's a minor error in the second example: it still says `TextureManager.GenTexture(a_name)`, same as the first example, but TextureManager has been deleted, so it should now say `Texture.GenTexture(a_name)`.

In regards to singletons, the advice against them derives from a more general observation: mutable data exposed widely can easily lead to problems, and segments of code that have a lot of diverse mutable data in scope can also lead to problems, because both of those situations lead to the easy creation of complexity and unexpected dependencies between seemingly unrelated components. This observation leads to the cautions: "singletons can easily produce problems" and "God classes can easily produce problems".

That last paragraph might have been a bit overly academic, so let's look at those two examples you gave in particular. Your TextureManager class exposes a public static `m_textures` variable. Because it's public, any code in your app can access that variable. Any code can read from or write to it at any time for any reason. Some code may read from it to learn about its state, call some other code based on that state, and then do some more work itself based on that same state, but unknowingly that other code (or, worse, code in another thread) mutates that state, and so the later work's expectation about the state is violated. The more widely a mutable variable spreads, the more likely that situation is to occur, and singletons are global, so any mutable variables they expose are spread very widely indeed.

Your second example hides the static variable privately within the class. That means you still have only one instance of that variable (so it could actually still reasonably be considered a singleton variable), but now only your Texture class has access to it, dramatically reducing the opportunity for bugs (instead of your entire code base being able to screw up its handling of the variable, now only your Texture class can screw up its handling of the variable :D).

Now, you could avoid singletons entirely here by changing your architecture to not have a single authoritative store of textures, but instead allowing multiple stores (multiple instances of TextureManager) to be created, each with their own private instance map of loaded textures. When you ask those instances to GenTexture, they check their own cache to see if they've loaded the texture already, and have no knowledge about the caches of other instances of TextureManager that may or may not exist. Now, could that design lead to scenarios where you end up loading the same texture more than once, each in a different instance of TextureManager? Yup. There's tradeoffs to everything.

Now, I haven't even touched on some related topics (like the difficulty of unit testing singletons, or the horrifying realization that classes themselves can be thought of as singleton objects that create instances of themselves (food for thought if you design classes where the mere act of instantiating a class has side effects outside the instance itself)), but I'm a bit pressed for time at the moment, but hopefully what I've written helps you.

In the original example I would just have TextureManager create the Textures via a static factory function, allocating the IDs as appropriate, and it can pass in the ID to the Texture's constructor.

Several things.

1. Manager class.

"Manager" classes are usually a bad name, that is not a single responsibility. For cases like this, often there is a resource loader that parses and builds, resource cache that has the instances, resource proxies for use before the loading has taken place, and a resource pool where they are all accessed.

2. Singleton vs singleton.

There are "Singletons", as in the design pattern Singleton where only a single instance can be created. There are also singletons that happen to only have one instance created.

The first one, having code that strictly enforces only a single instance be created, typically through automatic means, that is a bad idea. Having code that happens to only create one instance but could create more if the programmers wanted, that is fine. In your case the static member variables mean there can only be one instance ever, and it will cause problems.

Get rid of your static values. The make the code fragile and will force you to rewrite both this system and code that relies on it later.

3. Global variables.

Sometimes global variables are a viable solution. The critical key to determine if it is Good or Evil is if the global variable has shared mutable state. If any code anywhere can change the value at any time, that is Evil. The bugs are usually subtle and difficult to find.

For example, someone not log ago shared a bug about how their game would work just fine for a time, then suddenly all the text would become corrupt. They could not find the change. Nothing in their code indicated what was going on, one moment all the text was just fine, the next everything became visibly corrupt. Debugging showed that the contents of the strings was unchanged. After much hunting, someone realized the display was actually unicode, the 8-bit characters were being interpreted as 16-bit characters. After even more investigation they found one of the libraries they were using for text had a flag that set if the text mode was 8-bit characters or 16-bit characters. Somehow, something in the code was changing that global flag. Fortunately at that point it became a matter of memory breakpoints. They discovered that the Unicode flag was being set and cleared multiple times in various places in multiple libraries every frame. At that point the fix was straightforward, but the problem took quite a long time to track down.

Having a well-known instance is a hidden dependency, but if you implement strict rules to ensure the well-known instance is immutable and will not change, it is sometimes an acceptable tradeoff. This is typically implemented as a globally accessible structure that has pointers to instances. The pointers are configured at startup and can be replaced.

However, a better solution is ...

4. Dependency Injection.

If you are familiar with SOLID development principles, you should know about handling dependencies. In well-designed systems there should be interfaces for behavior. Code should be written to accept objects that implement the interface rather than accept concrete types. Then developers can build new concrete types and they will automatically integrate and work cleanly with the code.

If you have written your code according to SOLID principles, it is easy to inject the concrete class. It sounds hard, but really it just means providing constructor parameter and/or a member function that accepts a new interface.

For example, if you went with a combination of a texture loader / cache / proxy / pool rather than a monolithic "Manager" class, and each of the loader / cache / proxy / pool types had an interface, you could add a function that sets their owning pool, accepts an object that accepts the interface, and then the object uses the interface. Alternatively, you can require the interface pointer be passed to any calls that need it, which is the most explicit way to do things but requires a bit of discipline.

When code needs several interfaces passed to it, it can also make sense to have a structure with a bunch of interface pointers. Fill out the structure with the active objects and pass the structure around for less typing.

4. Dependency Injection.

If you are familiar with SOLID development principles, you should know about handling dependencies. In well-designed systems there should be interfaces for behavior. Code should be written to accept objects that implement the interface rather than accept concrete types. Then developers can build new concrete types and they will automatically integrate and work cleanly with the code.

If you have written your code according to SOLID principles, it is easy to inject the concrete class. It sounds hard, but really it just means providing constructor parameter and/or a member function that accepts a new interface.

For example, if you went with a combination of a texture loader / cache / proxy / pool rather than a monolithic "Manager" class, and each of the loader / cache / proxy / pool types had an interface, you could add a function that sets their owning pool, accepts an object that accepts the interface, and then the object uses the interface. Alternatively, you can require the interface pointer be passed to any calls that need it, which is the most explicit way to do things but requires a bit of discipline.

When code needs several interfaces passed to it, it can also make sense to have a structure with a bunch of interface pointers. Fill out the structure with the active objects and pass the structure around for less typing.


As an additional note here, DIP can allow you to write unit tests against code that would otherwise be untestable. Since your concrete instances are merely taking interfaces, you can provide mock implementations of the interfaces and then back that with your test framework.

In time the project grows, the ignorance of its devs it shows, with many a convoluted function, it plunges into deep compunction, the price of failure is high, Washu's mirth is nigh.

The question is: is it global state? This is a design decision you need to make on a case-by-case basis, and it requires having an understanding of what you are actually interfacing with in order to make the decision.

Take textures in OpenGL since that's the example you used.

Textures aren't global state; a texture manager isn't global state and the current texture binding isn't global state. Texture objects are per-context state, may or may not be shared by multiple contexts depending on whether the appropriate platform-specific call (e.g wglShareLists) was made, only last as long as the context lasts, and there may be more than one context.

As you can see you're already in a position where you thought you'd only need one texture manager but instead you may actually need more than one. I find that's a good general principle to keep in mind everytime you think "I know, I'll use a singleton": if you have one of something there's a good chance that having more than one is, if not required, at least possible. So begin by designing around having more than one and you save yourself a bunch of future pain.

So it's no longer TextureManager->GenTexture; it's Context->TextureManager->GenTexture and because you may have more than one context, Context shouldn't be a singleton either.

On the other hand, if it is legitimately global state then by all means make it a global or singleton. This is actually a much cleaner design than having multiple copies of it all of which need to be kept in sync. Form should follow function.

Direct3D has need of instancing, but we do not. We have plenty of glVertexAttrib calls.

As others have mentioned the real major issue with global data is mutation, who can change it and when.

It makes code very brittle and also reuse of any code that accesses becomes a pain as you have to use the same global variables/state project to project.

Injection and other similar techniques solve the reuse issue, prefer objects are supplied the data they require than go off and get it from some magic location.

Injection does not solve the mutation issue though. How you solve data changing under your feet is case by case. It might be the data is initialised and then read only or you have locking etc.

You shouldn't avoid using Singletons, as they have times when their use case comes in. The issue is that people tend to use them for the absolute worst reasons in the world. I personally do not understand why people keep telling you to avoid them, when it's use case is obviously for data that needs to be globally accessed, but is NOT expected to mutate much at runtime. You could try abstracting the mess out of code to avoid this for a "better" pattern and a level of unneeded complexity. Trust me, you can save time.

You're welcome to use a singleton for each manager. However, I suggest using the Singleton as a container to hold references to -ALL- of your resource managers. You will most likely not use more than one of each resource manager. You expect all of them to exist. It does expose these managers to the rest of the code, but honestly unless a programmer is being stupid, there shouldn't be a reason that's a problem.

I personally do not understand why people keep telling you to avoid them, when it's use case is obviously for data that needs to be globally accessed, but is NOT expected to mutate much at runtime.


In the short term it seems to work as a shortcut. Everything can use this one magical global object.

When your project is still tiny you can keep track of those little dependencies.

But every time you use them, you introduce a hidden dependency. Every time you modify them, you introduce a hidden dependency.

As the project grows you gain more and more hidden dependencies. As more team members are added people trip over the hidden dependencies. As time passes you forget what dependencies exist elsewhere in the code, and subtle bugs are introduced.

Soon these hidden dependencies are snares covering your entire code base. Changes to one thing introduce breaking changes to other areas of your code. It is difficult or impossible to write code tests because the hidden dependencies are global objects, you cannot swap them out with temporaries or mock objects or fake objects or test double objects. You cannot extend the underlying systems because they are concrete instances rather than interfaces.

If your projects always remain small and never grow to an appreciable size then global variables do indeed make your typing and your designs slightly easier. But if that system ever grows you will need to rip them out and replace them with a better system, or your development environment will be full of landmines. And typically those landmines range from tricky to nightmarish when it comes to bugs.

This topic is closed to new replies.

Advertisement