Is this code with global variables good practice? [closed]

I am currently writing a game in C++ and have some variables that I want to be accessible from everywhere. For the examples, I will have the shared variable as an object of the class Input.

At the moment I am implementing it like this:

input.h:

#ifndef INPUT_H
#define INPUT_H
extern Input input;
#endif


init.cpp:

Input input;
int main(//args){
input.doStuff();
}


Other files that use the object.cpp:

#include "input.h"

void method(){
input.doStuff();
}


I've got this nagging doubt that I shouldn't be doing things in this way. Is this doubt correct? What should I be doing and why?

Your doubt is correct, don't use global variables.

(Note: For this post, I mean variable in its true meaning, i.e. objects that may change.)

I can't say for sure which technique is the correct one without seeing your code, but normally you want to pass the variable as a parameter to whichever entity needs it:

void foo(Input input)
{
input.doSomething();
}

int main()
{
Input input;
input.doSomething();

foo(input);
}


Or, restrict it to a class as a member variable.

Update: Class examples

You can do it one or both of these ways:

class Person {
std::string name_;
public:
explicit Person(std::string const& name) : name_(name) {}
void set_name(std::string const& name) { name_ = name; }
}

void foo()
{
Person p("Bob Loblaw");

p.set_name("Slartibartfast");
}


There are two key downsides to using global variables:

1. They can be changed everywhere.

You generally want to enlist the compiler's aid in finding bugs and errors for you. If you allow yourself to modify a variable from all over your code, it is very hard for your compiler and yourself to track down an error. Consider the following scenario:

Input input; // global

void foo()
{
Input inputt;
input = someThingWeird;
}


This code will compile. If you're really unlucky, it will even work for a while. If you don't have a global, the code won't compile and the bug will be easy to find.

2. They increase coupling (and reduce testability).

By reading from a global variable, your entity (i.e. function, class, whatever) depends on that global variable. If some day you want to use something else, you'll have to do a lot of refactoring. What's more important, if you change an aspect of the global variable, you will need to update all parts of the code that uses it. This is a great opportunity for suble bugs to infiltrate your code and make your life miserable. A typical example is that you modify one part of your code, and a completely different part will break.

Using globals also makes your code hard to unit test, because it depends on the global variable.

• So if I wanted an object in my game (say for example an entity) to have access to the input object I should pass it in the constructor? – w4etwetewtwet Jul 29 '13 at 14:32
• If you have a class, and you want that class to be able to use an object, then you will either need to a) let the class own the object (create it as a member variable or a local in a function), or b) pass it as an argument to the class' constructor or one of the class' functions. – Lstor Jul 29 '13 at 14:34
• Ok, thanks. I will have to go for the second option as it is important that everything in the game using the input object uses the same object. – w4etwetewtwet Jul 29 '13 at 14:35
• I updated my answer with an example. Just use an Input instead of std::string. – Lstor Jul 29 '13 at 14:39
• @user2485710 Objects with internal linkage are not really global, and constants are definitely not variables. – Lstor Jul 29 '13 at 15:13

@Lstor is absolutely correct. However, I'll like to add some things:

• If you're using constants (indicated by the const keyword), they're okay as globals. This is because they cannot be changed elsewhere in the code, unlike global variables).

• I'd recommend classes, especially when writing a game. When used properly, they will allow you to hide your data from entities that should not have access to them. However, it's best not to use them just to avoid globals, especially when they can still be passed around. They should serve some kind of role, even if your program doesn't revolve around classes. In your case, you'll most likely have a Game class and possibly additional classes. More general info about this here.

Using global accessible variables is like handing your wallet to other and hoping they will only take the amount of money you owe them. You don't do this either.

I was confronted with a similar problem while developing a simulation in XNA. There are some properties that have to be public known, but I want to control access to them. So I implemented a generic ItemChangedService. Here a simplified version of it.

public abstract class ItemChangedService<T>{
//private backing field that holds my value

//Property Current -> simple a getter and setter
public virtual T Current {
get {return _current;}
set{
if(value == current) return; //no change, so we can return

_current = value;
//Invoke Changed event.
if(Changed == null) return;

Changed(this, EventArgs.Empty);
}
}
public event EventHandler Changed;
}


This solution has two clear advantages over yours:

First of all your variable is only changeable by getter setter you can redefine in subclasses. The second is that components can register for changes. Further more the behavior while getting/settings is extendable. So I created more *ChangedServices that are able to:

• Define what happen it the variable is set to the value it was before. (Define a RefreshBehaviour)
• Validate the input "value". (And what should be done? Exeption, LeaveExitingValue,... )

Never the less you can make typedefs in order to give this ItemChangedService meaningful names like CurrentFPSService or CamereaService and so on.

Reaction on comments: Sorry for the C# Code. Unfortunately I can only read C++, but write is a little bit difficult for me. So I try to make an example in pseucode without generics, events and properties:

template <typename T>
class ItemChangedService {
T myVariable;

public:
virtual T getValue() const {
return myVariable;
}

virtual void setValue(T const& value) {
if (value == myValue) return; // No change, so we can skip

myValue = value;
}
}


Because of the fact that the getter and setter are virtual, you can adjust these behavior in subclasses.

• Nice analogy! I'd say it's also like holding out your wallet in public and hoping that no one grabs it without your permission. – Jamal Jul 29 '13 at 15:12
• Note that the question is about C++, so posting C# code is not likely to be very helpful to OP. – Lstor Jul 29 '13 at 15:17
• @Lstor That is indeed true. I'm afraid I'm having difficulty understanding that code. – w4etwetewtwet Jul 29 '13 at 16:40
• There should at least be sufficient comments (if that will even help). I too cannot understand it. – Jamal Jul 29 '13 at 17:01
• @hichaeretaqua I fixed the C++ so that it's valid. I (re-)introduced templates, since C++ doesn't have a common base class. I did not change the variable names (although they should be), and I didn't fix the bug (mixing myValue and myVariable). – Lstor Jul 29 '13 at 19:37

I'm going to go against the current here and suggest that "no global variables ever" is unnecessarily dogmatic, and maintaining a small handful of global singleton objects can vastly simplify your code.

For a game in particular, something like an input manager or a graphics system makes sense as a global singleton.

• ...to which I'd contend that - unless it's being used for its real features like lazy evalutation or to avoid the static initialisation order fiasco - a singleton is often just unnecessary boilerplate, which achieves nothing functionally different from bare globals, other than covering your tracks against the less thorough anti-global crusaders ;-) meaning that imo, sometimes your 1st paragraph is equally true for bare globals... in small doses and unambiguously name[space]d. – underscore_d Apr 16 '16 at 23:49