How do I deal with global variables in existing legacy code (or, what's better, global hell or pattern hell)?

Question

So... We have this fairly complex project (~10k LOC, but there's duplicated code so it's hard to tell) with hundreds of global variables. The project has more dependencies on other projects, and many other projects depend on it too. I have mostly inherited the responsibility to refactor just a part of this project by myself, an "enclosed section" of modules. None of the original developers remain.

I have devised a way to structure the routines in classes (breaking up megamoths, forming some class hierarchies, a little Strategy here and there, nothing too fancy (I hope); the objective is to make it easier for other developers to add functionality and make it possible to add proper unit tests).

My new classes provide calculations applied to currently-global arrays of data that are updated, and the calculations themselves need to maintain a state (a sum, the last value of the last processed vector, etc). You could see them as functors.

I'm unsure on how to deal with globals, though. I don't think I will be able change all globals to non-global because of the dependencies to other modules, which I'm not going to refactor just now. Also, many of my new classes will need to share data. So, I can...

• For the globals that are not shared between my new classes, leave them as they are. I was thinking of using a Registry, or at the very least use some #define's or other variables to limit scope and provide context wherever they're used.
• For the globals that will be shared between my new classes, either make a base class with the references, or use a Singleton to pass them around, or a combination of both.

I'm not sure if using these patterns will do more harm than good.

So, my question is: what's better, live with existing global variables or aggressively tune them with patterns like Singleton or Registry? Do you have any suggestions to these schemes, or have a better scheme?

Answer 1

One way to slowly and relatively safely move from globals to no-globals might be to group globals into semi-reasonable classes as best you can. Declare a global instance of the new class, with members that correspond to the globals you are replacing. Remove the globals. Wade through possibly massive numbers of compiler errors, changing each reference to the global to a reference to the corresponding member of the global instance.

Now you can gradually switch from having your classes know about the global instance of your new class directly, and instead kick the can down the road by having the global instance passed into them. Over time you can hope to kick the can all the way to the end of the road, by removing all direct knowledge of the global instance from every class, and finally getting rid of the global instance, having effectively slowly replaced it by a registry or dependency injection (depending on what you did each time you removed direct knowledge of the global instance from one of your classes).

Note that every time one of your classes loses direct knowledge of how to find the global instance it becomes more modifiable and testable, even if the global instance still exists out there in your code base. So you get some benefits without having to do all the work up front, which could take too long with a large code base if the global(s) are widely referenced.

Of course, you do have to finish doing everything in the first paragraph. With a large code base you and your compiler could get to spend a lot of quality time together.

Answer 2

How far you go largely depends on the nature of the Globals. If they are mostly read and rarely modified (from only a few easy to find locations) , I would hesitate to make sweeping changes, and provide a framework around modifications, relying on documentation, programmer discipline and code review to ensure the framework is used.

If however the globals are changed from many locations or all the time (or both), you definitively need to get more aggressive with the refactor.

Singletons - IMHO a fancy name for a global to work around the "Globals are route of all evil" dogma and for languages such as Java that has no concept of global. They will not assist significantly in this case, unless the pattern of use naturally tends towards a Singleton (From reading between the lines I doubt this is the case).

Registry - Better option, but less than ideal unless the items truly need to be global, and far from ideal if they are being written to regularly.

Have a careful look at these globals and firstly work out if they should be global. If there is no clear reason, any attempt to fix it is dealing with the symptom, not the cause - it will just be a pretty global. If you decide that fixing it (as in, making it not global) is not an option, valid reasons being "its not worth the effort" and "it might break other code in unexpected ways" etc, then putting a sticking plaster on it might be a good idea.

5K SLOC is not a large project - try dealing with 5 million SLOC in this state. With only 5K SLOC, a major refactor is not a large effort if you have good tool support.

Answer 3

what's better, live with existing global variables or aggressively tune them with patterns like Singleton or Registry? Do you have any suggestions to these schemes, or have a better scheme?

A better scheme would be dependency injection with no globals (or statics) whatsoever.

Old code:

int DEFAULT_QUUX = 5;

int main() {
    Blip b;
    b.do_stuff(); // makes DEFAULT_QUUX = 6
    Qlip q;
    q.do_things(); // reads DEFAULT_QUUX
}

New code:

int main() {
    int quux = 5; // no longer global
    Blip b{quux};
    quux = b.do_stuff(); // returns 6
    Qlip q;
    q.do_things(quux); // quux is dependency injected.
}

If the hypothetical Blip and Qlip use or communicate with other objects in their implementation, and those objects need "quux" or other globals, these should be injected into the call chain from the top.

Answer 4

One serious issue with globals all over the place can be that the global variable is reused to control different "subsystem" depending on the current context of your program. So what can you do:

1. Add a global Registry class for the global variables.
2. Check if the resposibility for the global variable is really only for one domain by testing the software ( for instance in a debugger or a unittest ).
3. Check if it makes sense to split the resposibility of the global variable.

And you should try to build automatic tests when you are refactoring this on unit- or integration-test-level.

Kim

Answer 5

Flag all the globals with [Obsolete] attribute (or whatever if your not using c#)

Setup the infrastructure wiring for Dependency Injection, like so:

using (var mainForm = factory.Get<MainForm>()) {
      Application.Run(mainForm);
}

Then go about your business implementing features from backlog. As you modify existing code, your IDE will point out bad actors (obsolete vars). When they cross your path, try to get it from the dependency injection container instead. You don't need to replace all instances in a great big sweeping change that is guaranteed to have unintended fallout.

As you go along, you'll get more and more confident of use cases for one or two of these evil doers. Concurrently, you'll be building skilz with your DI container of choice.

Then, one day, when your DI container in wired through most of your code base, you take a smallish class, and stamp out all the [obsolete] vars in it. You do this by stubbing out a test fixture with the simplest of tests, instantiation through your DI container.

    var factory = new NinjectFactory();
    var mainForm = factory.Get<MainForm>();
    Assert.That(mainForm, Is.Not.Null, "MainForm"); 

Then find an global var, and pass it in through the constructor. Your test will fail if you haven't configured DI for class of the global var. Of course, that class will have dependencies that you'll have to unwind, but try unwind only as much as is necessary to get test (and hopefully App) running.

BTW - I'm in neck deep in a similar situation right now. (much bigger project though, the main form alone was 3k lines of code!) It's quite satisfying once you get the hang of it.