Extensive use of global variables in js codebase?

Question

I've been tasked with refactoring/simplifying the architecture of a (relatively) large node.js codebase and it makes ample use of global variables. I don't have much experience with javascript so just wanted to get a general sense of whether this is indeed super confusing and bad practise or if I'm in the wrong.

Here's a dummy example of a pattern I see throughout the codebase:

eg. 3 files: A.js, B.js, C.js:

A.js:

A_init = () => { // sets global variable a a = 1 } exports.A_init = A_init 

B.js:

B_init = () => { // sets global variable b b = 2 } exports.B_init = B_init 

C.js:

C_init = () => { // sets global variable c // depends on global variables a and b being set c = a + b } exports.C_init = C_init 

In reality there are many more files in the library (around 40) and some are thousands of lines long with random functions spread throughout the code which set variables at a global level which other library files have methods to utilise.

So for eg. if a developer wishes to utilise the library, they might write something like this:

UserScript.js

const A = require("./A"); const B = require("./B"); const C = require("./C"); A.A_init() C.C_init() B.B_init() someMethod = () => console.log(a, b, c) // expected: 1, 2, 3 actual: ReferenceError: b is not defined 

My experience of using the codebase is these sorts of ReferenceErrors surface very frequetly due to people either not knowing they need to call certain functions in order to set global variables other functions depend on or calling them in the wrong sequence.

There are probably around 50-100 such global variables and it's a nightmare to try and extract out... If this question is too vague I appologise, but what strategies would you suggest I adopt to address this? Is some use of global variables like this acceptable? Are there standards people adopt to constrain their usage? Would adopting a more OO approach and using classes/constructors be something you would recommend here?

Answer 1

How do you eat an elephant? One bite at a time.

• Start with writing regression tests, so you can be confident not to break too much when you refactor.

• Find out where and how certain variables are used. Add comments. Give those global variables better names, if they don't have already, and make sure the same variable isn't used for different purposes.

• Then start replacing the use of global variables by explicit parameter passing and explicit return values. That will reduce the temporal coupling issues you showed in the question.

• If you now notice some of your functions becoming too many parameters, identify parameters which are always passed as a group, make a data structure out of them, so you can pass them together. Make sure that data structure stays a useful abstraction.

• If you find certain operations which work mainly on those data structures alone, convert the data structure into a class and implement the operations as member functions.

• ... (long list of more things to make the code more maintainable) ...

There are surely many more things to improve over time. The canonical recommendation is to get a copy of Working Effectively with Legacy Code, which is also the top recommendation in this 10 year old SE post.

_{P.S.: did you notice - I did not even mention Node.js or Javascript in my answer. Your issues with global variables, temporal coupling or bad code are mostly language agnostic.}

Answer 2

but what strategies would you suggest I adopt to address this?

I think @DocBrown's answer already said what we all would have answered. Get a copy of M. Feather's book. My answer is in addition to his.

Of course, your job can't wait for you to read and master all the techniques written in the book. But, once again, Doc touches on the main points: "scan & document" your code, set regression tests, pick one global variable, make the simplest change towards removing the dependency over the global variable and repeat.

After some iterations, you will find patterns (how it was implemented). Some iterations will result in the very same changes because the original code was implemented either by the same author (the author of the pattern) or simply because other developers saw the pattern and consolidated it by replication.

I find these patterns to be important because you will have to give'em consistency. In the end, you have to find a consistent solution that fits most (if not all) of the use cases. Consistency is key because that's the pattern you want others to replicate from now on.

Now, bear this in mind.

Implementing regression tests might involve another kind of refactoring. Don't mix up these changes with those addressed to solve the main issue (global variables). Think of every reason to change as a vector. Work on only 1 vector at a time.

• Vector 1: Making code testable
• Vector 2: Decoupling code from global variable dependencies

Each vector might take several changes, so remember the well-known practice of keeping commits small. And set a message to each commit mentioning the vector of the change.

Is some use of global variables like this acceptable?

As usual, it depends. For your sanity, adopt the simple rule of not relying on them. That said, global scopes exist for a reason. Like guns, they can serve both good and evil purposes depending on the use you give to them.

Are there standards people adopt to constrain their usage?

Every team or company sets its standards. In my company, the code produced for 3rd parties (customers) must pass through static analysis. We have set metrics, parameters, etc to define our standard. Ours is the default "standard", but we can set more relaxed or constrained ones depending on the customer and the conditions.

Another "standard" is automated CI/CD cycles.

If the code doesn't meet the quality standards of the static analysis (no matter the reason), the code is not eligible for deployment. The CI/CD cycle is interrupted and everyone is notified. That generates pressure on whoever pushed the code.¹

Would adopting a more OO approach and using classes/constructors be something you would recommend here?

You have to adopt the paradigm that is natural to the programming language and is consistent with the rest of the code. Don't turn OOP into a golden hammer.

---

^{1: Might some disagree or dislike, but that's how we work. Others might rely on peer-review or that sort of XP technique. We have no experience with XP, so we do our best with what we have.}