Singleton pattern abuse

Started by
44 comments, last by freakchild 11 years, 10 months ago
I'd like to thank everyone for their time, I've realised from answers in the post that the way to get rid of singletons is review my design and get rid of the dependencies even if I don't like the idea of it. I'll start working on separating rendering functions from my objects when I get the time. :P


Bobs' personal data doesn't need to know how it's drawn.


But a map (talking about 2D) which is rendered tile by tile is rendered differently from, say, a checkbox or any simple object that only needs to be rendered with one procedure. Mustn't the Map class know of the logic by which it'll be rendered? (nested loops etc). Otherwise, that information must be hardcoded somewhere else (renderer?).
Advertisement

I'd like to thank everyone for their time, I've realised from answers in the post that the way to get rid of singletons is review my design and get rid of the dependencies even if I don't like the idea of it. I'll start working on separating rendering functions from my objects when I get the time. tongue.png

[quote name='Narf the Mouse' timestamp='1339211749' post='4947556']
Bobs' personal data doesn't need to know how it's drawn.


But a map (talking about 2D) which is rendered tile by tile is rendered differently from, say, a checkbox or any simple object that only needs to be rendered with one procedure. Mustn't the Map class know of the logic by which it'll be rendered? (nested loops etc). Otherwise, that information must be hardcoded somewhere else (renderer?).
[/quote]
All the map class need know is it has an array[][] of tile data; all the renderer need know is it's received an array[][] of tile data.

And the renderer doesn't need to be hard-coded to draw specific tiles; a factory can fill out a lookup table of some sort.

Classes shouldn't have more than one concern, even if that concern is "Running the game". Even the class that "Runs the game" shouldn't concern itself with, say, whether or not BobMob gets drawn.

Of course, as always, real life is not ideal and sometimes what works still works well enough.

But a map (talking about 2D) which is rendered tile by tile is rendered differently from, say, a checkbox or any simple object that only needs to be rendered with one procedure. Mustn't the Map class know of the logic by which it'll be rendered? (nested loops etc). Otherwise, that information must be hardcoded somewhere else (renderer?).


And you've run smack into a problem of doing too much; what does the 'map' represent in this?

The information doesn't have to be 'hard coded' anywhere but it should be abstracted in some manner. A 'map' shouldn't have to care HOW it is rendered, or even what is rendered, all it needs to care about is the fact it has something which needs to be rendered.

If you step back and think about it for a moment ALL rendering is the same; you provide the GPU with a vertex and index stream, some constants, some render states (including shaders) and some textures. That is is.

So at the lowest level you have a 'renderable work packet' object which bundles this state together and that is all your render cares about; give it these packets of information and it can render you the world.

It doesn't matter if you have a 2D map, a rigged 3D character or a box the basic rendering remains the same.

So then pull back out one more level; you still need something to contain and send that work down to the renderer. This is the point when you start to specialise a bit; for your 2D map example the renderer would have been told 'here is an object which needs to be rendered and will provide you with packets of work'. In this instance the 'renderable' knows how to render a 2D map and can provide the render with work packets to do just that.

Finally you can back up one more level to the 'map' object itself, or the logical game represenation of it; this doesn't know how to render the map at all, all it knows how to do is respond to certain events and other high level map related details. It will more than likely have a reference to the 2DMapRenderable object which will do the rendering work but it has no direct means of communicating with the renderer. It could talk to the map renderable depending on the game; for example if you have a game which has a map consisting of 2D 'screens' which instead of scrolling 'flip' you might well have a function which tells the renderable which screen to send to the renderer when it is next asked.

The renderer backend itself deals with talking to the renderable, including asking it for (culled) work, and doesn't care what the game above it is doing.

Short version; all rendering is the same at the lowest end; high level logic doesn't care about how things are rendered, abstract it away.
If the idea that globals are bad because you need to keep track of so many potential modifications in different functions then I can hardly see how passing a reference to them solves this problem since you can still modify the original value from different places. At some point you'll have states that the entire program needs to keep track of so this jihad seems a bit too dogmatic. Not all programmers have a phd in computer science so these considerations seem to paralyse by over-analysis rather than just let you code away. I'm victim of this.

If the idea that globals are bad because you need to keep track of so many potential modifications in different functions then I can hardly see how passing a reference to them solves this problem since you can still modify the original value from different places. At some point you'll have states that the entire program needs to keep track of so this jihad seems a bit too dogmatic. Not all programmers have a phd in computer science so these considerations seem to paralyse by over-analysis rather than just let you code away. I'm victim of this.

If I understand your argument, it's that passing, say, TextureManager everywhere is just as bad as a Singleton, so why not use a Singleton?

1) That sounds like an argument against passing TextureManager everywhere.
2) The thing is, if you want to change what a pass-by-value system uses, you only need to change a few upstream values. If you want to change what a Singleton system uses, you have to re-write it to a pass-by-value system.
No I'm not saying it's better or worse, just that the alternative isn't that obvious to me. Let's say in scenario A you have a global, gState = 1;
then you have somefunc1(); reading/modifying it.
And func2() and func53() and func89 () and you forgot that func15() also modified it. They say "see! Globals are evil"
Now in scenario B you made it local and passed a reference to all of the above functions and maybe you also forgot that func15() changed it.
Then did you really solve the problem?
But you polluted the namespace less I guess?

At some point you'll have states that the entire program needs to keep track of so this jihad seems a bit too dogmatic.
[/quote]
What possible state would you need to have access from every point in the program? Does the standard library's generic container classes or sort routines access to this state? Does DirectX/OpenGL access to this state? Your audio library, physics library, ...?

No; because well-designed modules can be used independently regardless of the rest of the program. Seek to emulate, where possible, this de-coupled design in the high level architecture of your own program.

I believe this mindset is often the result of "singleton cancer" (which is itself just a hardy mutation of globalitis). Simply by making state accessible globally practically guarantees that accesses to this state will swarm and multiply across the codebase. This infection will stunt the overall design. Module walls, if they managed to form at all, will corrode and collapse under the weight of these virulent pathogens, until putrified spaghetti code is all that remains.

At least, when the project begins to scale. Smaller programs are restistant because of their own simplicity. Without the high level structure necessary to feed on the cancer's growth rate is severely stunted.


Now in scenario B you made it local and passed a reference to all of the above functions and maybe you also forgot that func15() changed it. Then did you really solve the problem?
[/quote]
The problem is coupling. func 1..99 don't actually need to see gState. Maybe only fun1, func13 and func42 do. It isn't about converting the singleton/global into a parameter and keeping the code the same - it is about restructuring the code so that the "global" state is now "local" to a particular sub-system.

For example, by separating the core game logic from the presentation logic, you decouple the core from knowledge of classes such as a "TextureManager". You can separate input polling and mapping from the game logic actions by using a callback system. Thus the input routines are de-coupled from the game logic, connected by injecting the correct functors.
You don't solve the 'forget' problem BUT you do solve hidden dependencies which is the problem with globals.


void func1()
void func2()
void func15()
// vs
void func1(SomeState &)
void func2(SomeState &)
void func15(SomeState &)
// or
void func1(const SomeState &)
void func2(const SomeState &)
void func15(SomeState &)


In the first 3 examples the dependencies are hidden away, there is no way by looking at the code you can say what dependencies those functions have.
The second group make it clear they require an instance of 'SomeState' to do work.
The third group make it clear that 2 of the functions won't modify the state, the third will/can.

The other problem with globals is that they are just that; global.
They are everywhere, there get everywhere and they are had to track depenency wise and, most of the time, they really don't need to be global.

Often people say 'oh, but I need to do X' everwhere but when you get down to it they rarely need to do it 'everywhere' but in a very small select area of code.
People think 'everywhere' because they haven't put the effort into thinking about the problem beyond 'I need to do X'.

Nor is this about 'having a phd in computer science'; it is about sitting down and thinking about your design and thinking beyond the first solution which appears. Yes, a global often seems the easiest to start with, and if you want to run with it then go ahead, but don't be surprised when you run into problems or when people say you've come up with a bad solution.

Globals are, by and large, the lazy solution.
Singletons are, by and large, the wrong solution.
I don't think anyone in this thread has a PHD in computer science (though I may be wrong). On the other hand, I'm 100% confident that the more vehement posters have lost months of their lives to singletons that seemed 'convenient' to someone at some point.

+1 phantom's mention of const - another invaluable tool in making it possible to reason about your code.
[size="1"]

2) The thing is, if you want to change what a pass-by-value system uses, you only need to change a few upstream values. If you want to change what a Singleton system uses, you have to re-write it to a pass-by-value system.


Just a thought... whether you're using a pass-by-argument sort of thing, or a singleton, you're still using that object in the same places. In the example given, the TextureManager is being used in function used for loading stuff. Whether the Mgr is available as a global/singleton or it's passed as an argument (or some other object containing some context data that could be used in lieu of the Mgr), if you make a change to it, you need to account for the change in that function. It really doesn't matter. Putting it in a singleton just makes it easier to call the loading function. If anything, the singleton requires an equal or lesser number of changes, since you don't have to change upstream references, just the actual usage.

There's been mention of not being able to see at a glance what data/objects a function uses, and forgetting about updating references when changes are made... true, you don't get the same immediate indicator that a load function is going to use some sort of content system as you would if you passed it as an argument. Instead you've got, well, the fact that it's a loading function, plus perhaps some documentation for the function that says something like "gets the appropriate texture from the TextureManager". As far as forgetting to update references, I'm sure the IDE can help you with that; just do a search for the class name, it's a singleton, so you probably don't have to worry about it being referenced using some subclass (there may be a subclass, but you're likely using the base class' name).


2) If you have an easily-accessible hammer...

... then when you need a hammer, it's easy. If that hammer is complicated, you're going to start using the butt of a screwdriver or a shoe to pound nails.


Personally, I won't shy away from singletons (or really, anything) when I feel they are appropriate. An example for me, something I've used in pretty much any project: I generally have some sort of Event Dispatcher, which allows listeners to register for certain event types, and then anyone can fire out events as appropriate. Fairly similar in concept to Win32's message stuff. Pretty useful, I'm sure there are other solutions, but this is the one I like to use, and it has evolved over several projects. It means, for example, that an explosion class doesn't need to get a list of all nearby actors and manually call a dealDamage() function or anything. Instead, damageable actors register for the EXPLOSION event, and the explosion fires the event (either the actors check if they're in range, or I could implement some sort of filter to decide which listeners will actually get the event). Sure, a lot of things end up being connected to the EventDispatcher singleton, but that's never been a problem for me. You might say "oh, but that can't be thread safe!" Well, it's no different than getting nearby actors and calling dealDamage() on each of them. And there's no way I want to pass an EventDispatcher around to every single object I create (not every object uses it, but we don't want to lock that sort of assumption into the code now, do we)


My general opinion is that you shouldn't dismiss something just because everyone says it's bad. I feel pretty confident in saying that any construct has some case where it is appropriate. Sure, there are places where a singleton is far from the right tool, but sometimes it's going to be much cleaner than the alternative. I see several people here saying that singletons are pure evil and should never be used under any circumstance. I say use whatever makes sense. If using a global object or a singleton is going to make your code cleaner and simpler than passing extra context objects around, use it! If down the road you find that you're running into problems with it, then you can change it up. Especially since the OP stated outright s/he's doing this as a hobby, and wants to learn. What could be better for learning what works and what doesn't, than trying it out? If you try something and it works through to project completion, great! If you find that at some point it's no longer sufficient, then you'll understand why it doesn't work and can move to a better solution. Sure, it may take a bit of time to refactor, but the experience is, I think, a lot more valuable that someone just telling you "it's bad, never do it."

Of course, I'm not saying that everything can/should be a singleton either, just do what makes sense to you.

And back to the OP's example of the TextureManager... my opinion would be that you could instead have a single ContentManager, which defers requests to the TextureManager, ModelManager, SoundFileManager, WhateverElseManager as appropriate based on resource type; so it's one singleton instead of many. Or instead of separate *Managers, provide separate *Loaders for each resource type, and ContentManager maintains a list (or whatever your favorite container is) of generic Resources (which is subclassed or something for each resource type) provided by those *Loaders.

This topic is closed to new replies.

Advertisement