Why are static variables bad?

Started by
56 comments, last by Sacaldur 10 years, 7 months ago


and every constructor will have like 5 arguments

And if you do need to pass in five dependencies to one constructor (which is extremely unlikely if you design your classes properly), you can always wrap them in an instance of a FooContext class.

Advertisement


and every constructor will have like 5 arguments

And if you do need to pass in five dependencies to one constructor (which is extremely unlikely if you design your classes properly), you can always wrap them in an instance of a FooContext class.

There's nothing wrong with a constructor having five arguments per se, if that class has five dependencies and/or required parameters. If your class genuinely has five dependencies, passing them to the constructor is better than the alternative of global variables. If your class feels like it has too many dependencies, maybe it really does and needs refactoring. That's going to be a judgement call based on the specific situation.

No I agree, it depends on the situation. But having too many arguments - especially of the dependency injection variety - is at the very least a potential code smell that needs to be investigated. Often there is a better solution.

Personally I like to make objects that can always be default constructed and design paramaters as simply ways to set specific properties on creation. I'm not really a fan of objects that do complicated behavior just by being created.

If anything, if an object has a lot of dependencies that you can't get rid of then that means they're going to need to be set somewhere anyway, otherwise you're just reaching out to global code and doing the same thing. Dependencies never disappear if you really need them, you just throw a blanket over their existance by using globals.

An object doesn't have to be able to do everything possible at creation it should just be in a valid state that will not crash the program.

While I do not see the rest of the class, it appears the resourceModule has too much responsibility. I suppose it has methods for loading and creating textures, meshes, etc... and will do so on? That violates the single responsibility principle, and while this and other rules aren't necessary important for you to follow right now, its never too early to start using them. In short, you are better of splitting the different resource caches, and also split the loading functionality into its own class.

The resource module only allocates the resource, I actually call a GetResource(name) and it'll check if it's already loaded, if it is, it'll return a pointer to it, else, it'll create a new object of that type, add to the list of loaded objects, and call the proper object loader.

I used to make the resource module load the objects, but then read about this single responsibility principle, and changed it so objects load themselves. But I still have to make them go through the module to manage and avoid duplicates.

And with this design, to get rid of the static modules, I'd need to send a bunch of pointers a few layers down just so a new object can be created.

Simplified, it would be:

GameEngine -> ResourceModule(GraphicModule*) -> New Object(GraphicModule*)

So my object can load it's data to the graphic card. And this would be deep hidden in the resource module which once I finish the managing memory part, I don't really think I'll be coding anything else in there.

By keeping it in the objects, I can always just change the object's load function or just add new objects that use the basic primitives from the resource.

For example, I just thought I don't have any "sound" object yet.

If I want to make an object that will trigger a sound effect for example, I'd need to also send the SoundModule pointer through Engine->Resource->Load, instead of just doing:


void NewSoundObject::Load()
{
    //Sound* primitive
    _sound = ResourceModule::GetSound("soundName");
}

I'm also trying to make the objects as detached as possible, and this was the only way I could think of.

Objects have pointers to primitives, and the Resource Module manages these primitives. Primitives I shouldn't add any more once I'm done.

I call "primitives" these base resource classes that only have data in it about that resource, and that can be used by multiple objects and shouldn't ever have more than one (sounds, meshes, textures, etc).

Changing it to pointers and sending to the constructor seems it would just move the dependency from Load() to Constructor().

Ideally, I'd like it to be:


class Object
{
    public:
        void Load(string someName)
        {
            _texture = new Texture2D();
            _texture->Load(textureName);
        }
    private:
        Texture2D* _texture;
};

And the Texture class manages everything about itself, but I couldn't think of how without using static variables in the texture as well. I tried to keep primitives as clean and independent as possible, so they don't know anything about the GameEngine or modules.

It's higher up objects that uses them do, though.

I implemented your suggestion, it was getting pretty messy after all, so now it's:

GameEngine::resourceModule->Textures.GetTexture(textureName);

GameEngine::resourceModule->Meshes.GetMesh(meshName);

I'm always looking to improve, and I'm still not 100% happy with the design now, I think it's still not really good, but that's the best I could come up with for now.

I was looking to change my static modules for pointers if static variables had some performance issue or some other deep problem, so that's the main reason I asked why they're considered bad besides the "it's bad practice having globals".

Personally I like to make objects that can always be default constructed and design paramaters as simply ways to set specific properties on creation. I'm not really a fan of objects that do complicated behavior just by being created.

I never do that. I am hardly pressed trying to identify an object that is "useful" in its default state.

What's the point of having a vertex buffer that points to nothing? You'll have to handle that useless situation everywhere both inside and outside the class. Just pass in the stuff needed to create a "useful" VB in the constructor and you're done in one line. In my experience this kind of code:

Foo* foo=new Foo();

foo->init(); // THIS ONE, I HATE IT WITH A PASSION.. JUST INIT INIT IN THE CONSTRUCTOR!

foo->setThis(bla);

foo->setThat(whatever);

Is just a ad for bugs.. as opposed to this:

Foo* foo=new Foo(bla, whatever);

One line, the compiler works on your side by making sure you provided the required information to create a useful object that'll actually be able to do stuff.

Oh and pls.. I know about the fact that new cant report errors but, in my experience, the number of classes that can fail a new (out of memory) without bringing down the entire game is a very limited amount.. if that really becomes a problem, work those as an exception to the rule.

When I REALLY need a post construction verification of the object validity (ie. a config file that might not be there or valid) I just add an "isValid()" method to it.. you can get smarter and overload whatever operator the std library overload to make this work:

Foo foo(bla, whatever);

if (foo)

{

// DO STUFF WITH FOO

}

in my case this is usually a more explicit:

if (foo.isValid())

I love how Go solves the problem by making "New" constructor functions idiomatic.. if you have something that can fail , you return also an Error.. it becomes:

foo,err:=NewFoo(bla, whatever)

if err!=nil {

// do stuff with foo

}

Very last note on "Context" struct.. they are really useful only if the same struct is used multiple times.

If I have a FooContext struct:

struct FooContext

{

Bla* bla;

Whatever* whatever;

};

And I dont define a constructor for it, I am asking for a crash:

FooContext context;

context.bla=bla;

// FORGOT TO INIT whatever, somebody added whatever later in the development

Foo* foo=new Foo(context); // CRASH

To solve I will have to provide a constructor to FooContext, so nobody can forget to set a member and, if FooContext changes the compiler will find all the missing inits for me. But, at the point, code becomes funny:

FooContext context(bla, whatever);

Foo* foo=new Foo(context); // WHAT'S THE POINT OF THIS?

Boo* boo=new Boo(context); // OK NOW IT STARTS TO MAKE SOME SENSE

Stefano Casillo
TWITTER: [twitter]KunosStefano[/twitter]
AssettoCorsa - netKar PRO - Kunos Simulazioni

I've read a few threads here that people advise against static variables, and the argument is usually because anything global is bad, but besides this, why are static variables considered bad?

Do they perform badly or something like that?

I'm asking because I've got my engine structure with modules, and I'm using static variables for each module so my base classes can access it.


class GameEngine
{
    public:
        GameEngine()
        {
            resourceModule = &_resourceModule;
        };
 
        static ResourceModule* resourceModule;
    private:
        ResourceModule _resourceModule;
};

#include "GameEngine.h"
 
void Image::Load(string imageName)
{
    //Texture2D* _texture;
    _texture = GameEngine::resourceModule->GetLoadedTexture(imageName);
}

Images are meant to be used like this:


void SomeGameFunction()
{
    Image* newImage = new Image();
    newImage->Load("imageNameHere");
}

I'm currently doing some encapsulation and blocking accesses to base classes that I don't want exposed, so other programmers don't ever access a "Texture2D" class for example, and when I looked to these static variables I thought they might be an issue but I couldn't find of any good solutions for it, that is, if they require a solution...

So I remembered the topics against static variables I thought I should ask, why are static variables considered bad?

I got no trouble with the global static arrays - they probably are most performant as cache friendly (maybe it is also depending on the layout of data inside) - but I am writing in c

small global variables can bring a trouble

The fact that it is global is unrelated to cache, and on some systems (such as Nintendo DS) it means slower access time.
There is nothing the heap offers that the stack does not except for raw size. If the amount of data you need is sufficiently small enough that stack overflow is of no threat, it is never better from a performance standpoint to have data on the stack rather than on the heap. The stack is never slower than and sometimes faster than the heap.

In either case, anything that is accessed often, as is suggested by the nature of it being global (a product of the misuse often applied to globals mentioned already) will almost always be readily available in the cache anyway.

And the sizes of globals is not a cause for troubles.


The problem with globals is related more to coding philosophy than to performance etc.
A global lacks restrictions on how and by whom it can be accessed which tempts programmers to lazily access them from anywhere rather than to pass only the minimum number of needed pointers around.

The easiest way to exemplify this as a problem is to consider the 2 cases when you look at existing code that someone else wrote but that you need to maintain.
If no globals are used, you can look at the inputs to a given function or method and just from that you can be sure about what the function or method accesses or on what it relies as far as inter-modular dependencies go, and you can easily mentally compartmentalize the scope of it.

If globals are used throughout the project, you will never be able to reduce functions/methods in scope mentally without checking the whole body for references to statics/globals. When you look at the next function/method you don’t have a single confined place to look to check the scope of its access and so each time you have to start over by studying the inputs and the whole body.


This isn’t necessarily the specific problem with globals—the problem with globals is not just that you might have a hard time compartmentalizing the scope of access of code someone else wrote before—it simply exemplifies the main issue, which is that there is little or no control over dependencies on it, and as such it easily reaches a point where just because one thing depends on that global, it must also depend on a whole library of unrelated code just to get that dependency.


L. Spiro

I restore Nintendo 64 video-game OST’s into HD! https://www.youtube.com/channel/UCCtX_wedtZ5BoyQBXEhnVZw/playlists?view=1&sort=lad&flow=grid


I used to make the resource module load the objects, but then read about this single responsibility principle, and changed it so objects load themselves. But I still have to make them go through the module to manage and avoid duplicates.

Hah, it appear you haven't fully understood the principle about the single responsibility principle ( haha ) yet. SRP in one wording means that a class should have only one well defined task. So what does your image class now does? It at least acts as a image-data container and knows how to load an image... oups. See that bold word? Thats a very bad sign to begin with, when talking about one responsiblity.

Seeing how you tried to avoid the responsiblity from the resourceModule is a good thing, but you shouldn't have moved it to the image class. I'd personally made a seperate ImageLoader-class. This class knows how to load and create an i mage, and store it in the resourceModule.


And with this design, to get rid of the static modules, I'd need to send a bunch of pointers a few layers down just so a new object can be created.

Aye, and let met tell you, thats a very good thing! This will most importantly keep you from creating objects all over the place, and force you to seperate the graphics resource creation into their own classes, instead of messing with them wherever you want. What you show, is what you want to be able to do in a script language, and not in low/mid-level game code. In my own engine, I mostly have to pass in my different resource loaders 3 layers: It starts in the game state, there it goes into the game system, and there I'll have some loader class (terrain loader, particle loader), that takes it. Let me show you a perfect way of "hiding" those kinds of dependencies away, but in a good way:


namespace acl
{
	namespace dx11
	{

		class MaterialLoader final :
			public gfx::IMaterialLoader
		{
		public:

            MaterialLoader(const gfx::Effects& effects, const gfx::Textures& textures, gfx::Materials& materials);

            gfx::IMaterial* Load(const std::wstring& stName, const std::wstring& stEffectName, unsigned int effectPermutation, const std::wstring* pTextures, unsigned int numTextures, const SubsetVector* pSubsets = nullptr) const override;

        private:

            gfx::Materials* m_pMaterials;
            const gfx::Effects* m_pEffects;
            const gfx::Textures* m_pTextures;
		};

	}
}

Thats my material-loader. A material needs both an effect (shader) and textures, as well as some other variables. Granted this would be already a good example for wrapping parameters in a structure, but look at the constructor. I pass in the effect and texture cache (const), as well as the material cache. Thats a perfect visible dependency: Whoever looks at this class sees it needs the effect and texture cache for read, and possibly writes to the material cache. Also, this dependency is only there in the constructor. The load-method is completly independant of this. Now I construct my material loader, and after that, there is no difference in handling it that where if the material had its own loading-method.


I implemented your suggestion, it was getting pretty messy after all, so now it's:



GameEngine::resourceModule->Textures.GetTexture(textureName);

GameEngine::resourceModule->Meshes.GetMesh(meshName);

Thats already a leap ahead IMHO, one thing you'll want to improve in the future simply to save you time and doublicated code is maybe make a generalized templated ResourceCache-class, and derive your Textures/Meshes from that. Other than that, I can just tell you from my own experience, the more sophisticated your "high" level structures get, the better those resource handling etc. will become. For example, if you were to implement a clever gamestate-system, maybe with some entity/component-system, you wouldn't need to store the resourceModule in the GameEngine-class as static anymore. You would construct it in the method that creates the very first game state, and pass the resourceModule in to that. From there on, you'd forward it to your systems if needed. Oh, I think thats a perfect example of why globals are totally unnecessary really. Since you mentioned sounds/audio sources, I'll show you how my ECS handles it. First of all, there is the audio-source component.


		struct AudioSource :
			public Component<AudioSource>
		{
			AudioSource(const std::wstring& stFile, bool bLoop);
			~AudioSource(void);

			std::wstring stFile;
			bool bLoop;
			audio::Emitter* pEmitter;
		};

See how its constructed? From a string and a boolean. I can construct this whever I want, without having to pass in anything at all. If a boss event down the complex instance handler -> ai handler -> event dispatcher -> and so on wants to emit a sound, this is what it creates. Now my audio-system on the other hand does this:


		void AudioSystem::Update(double dt)
		{
				auto vEntities = m_pEntities->EntitiesWithComponents<AudioSource>();

				for(auto pEntity: vEntities)
				{
					AudioSource* pSource = pEntity->GetComponent<AudioSource>();
					
					if(!pSource->pEmitter)
					{
						pSource->pEmitter = new audio::Emitter(*m_pDevice, *m_files[pSource->stFile], pSource->bLoop); 
						pSource->pEmitter->Play();
					}

				}
}

It creates the actual sound emitter, using a file that has been loaded to the audio file cache (m_files). The audio-system only gets updated once per frame in the game state (one layer down). Its not so much about that design, but about how you can decouple things like loading, and even initialization altoghether. I hope you get the idea and can take something from this, I unfortunately lack an example that is directly tailored to your problem here...


Changing it to pointers and sending to the constructor seems it would just move the dependency from Load() to Constructor().

Thats not exactly true. You've now removed the loading code from the image-class itself. Now any external structure can construct an image the way it wants, and not depend on the image class itself. If we are talking about the same thing, namely:


class Object
{
    public:
        Object(Texture2D* texture): _texture(texture)
        {
        }
    private:
        Texture2D* _texture;
};

I'm not talking about simply passing in the resourceModule to the constructor, that would be bogus. You should let the Object (shouldn't it be image) be initialized from an external source, via the constructor. Then you have an ObjectLoader-class, that loads the fitting texture from the cache, and assigns it. See? Though I can't make a puff of smoke and a "Puff!" sound effect, the depencency has magically disappeared ;)


I never do that. I am hardly pressed trying to identify an object that is "useful" in its default state.

What's the point of having a vertex buffer that points to nothing? You'll have to handle that useless situation everywhere both inside and outside the class. Just pass in the stuff needed to create a "useful" VB in the constructor and you're done in one line. In my experience this kind of code:

Seconded. A constructor (not default) is there for a reason, and using it in a sane way is perfect for not hiding, but easing the issues of dependencies.


foo->init(); // THIS ONE, I HATE IT WITH A PASSION.. JUST INIT INIT IN THE CONSTRUCTOR!

Ah, I used to do this all the time in my past projects. Its so tedious. Half of the time, you'd forgot to init and deinit() afterwards. And whats the point? Pass in the constructor. Its a little more complication for no real gain.


I got no trouble with the global static arrays - they probably are most performant as cache friendly (maybe it is also depending on the layout of data inside) - but I am writing in c

Let me lean myself out of the window by saying that if anything, statics are anything else than cache friendly. For statics, we have no control over how and where they are created. I know little about memory optimization, but that sound wrong to me. Linear memory used concurrently, thats cache-friendly from what I know. Statics? Not so much...

The fact that it is global is unrelated to cache, and on some systems (such as Nintendo DS) it means slower access time.

There is nothing the heap offers that the stack does not except for raw size. If the amount of data you need is sufficiently small enough that stack overflow is of no threat, it is never better from a performance standpoint to have data on the heap rather than on the stack. The stack is never slower than and sometimes faster than the heap.

In either case, anything that is accessed often, as is suggested by the nature of it being global (a product of the misuse often applied to globals mentioned already) will almost always be readily available in the cache anyway.

And the sizes of globals is not a cause for troubles.

The problem with globals is related more to coding philosophy than to performance etc.

A global lacks restrictions on how and by whom it can be accessed which tempts programmers to lazily access them from anywhere rather than to pass only the minimum number of needed pointers around.

The easiest way to exemplify this as a problem is to consider the 2 cases when you look at existing code that someone else wrote but that you need to maintain.

If no globals are used, you can look at the inputs to a given function or method and just from that you can be sure about what the function or method accesses or on what it relies as far as inter-modular dependencies go, and you can easily mentally compartmentalize the scope of it.

If globals are used throughout the project, you will never be able to reduce functions/methods in scope mentally without checking the whole body for references to statics/globals. When you look at the next function/method you don’t have a single confined place to look to check the scope of its access and so each time you have to start over by studying the inputs and the whole body.

This isn’t necessarily the specific problem with globals—the problem with globals is not just that you might have a hard time compartmentalizing the scope of access of code someone else wrote before—it simply exemplifies the main issue, which is that there is little or no control over dependencies on it, and as such it easily reaches a point where just because one thing depends on that global, it must also depend on a whole library of unrelated code just to get that dependency.

L. Spiro

globall acces static arrays are related to old school way of programming if using them in c (and it is more cache friendly imo than more oop alternatives - but i do not want to speak about it here (got an awfull days recently) - such code i would say is array-centric

as to encoding dependency by passing - It is possible that it can be useful when improving some bigger code written by the others - I am not sure to that but maybe, I am writing arraycentric c code mostly so I got no clear practical view on the opposite - but I can say that my arraycentric approach never bring any troubles to myself - and is very nice to myself (really)

other thing is small set of global state small variables (something like you do not want to pass it so you make it a global state variable- it can be very bad (probably because when you pass it you bring a reference to some track of changing this value with you, so you can deduce and be conscious about its previous state - when you set it global, and use it far away in the different place you lost the track to code who was responsible about its state (just forgot it - so if your code depends of such state, you have no back trace what just bring its value - you have no simple dependency path here so if such code works it works just by chance that this backtraced value is ok )

- I am not sure to that exactly clearly (*) (and how to clearly say it) but this 'lost track' is something in a way related to such (previous programming age, one can say) troubles - but as I said I do not encountered such problem with large arrays probably because mechanics of my code treats them in very clear and simple architectural way so this was not the case)

(I could probably name this type of trouble bringing global variables in special name like "orphan variables" "lost variables" or something like that to ease the process of reflexing about them)

(*) the least clear thing here is if passing values can repair such lost variable occurances, or how to avoid them in other way I do not see it clearly at the moment

This topic is closed to new replies.

Advertisement