This is part of a series of posts where I refactor code from StackOverflow questions, with a discussion of the changes. One of the great things about JavaScript is how scalable it is. You can start with a simple script, and there is nothing wrong with that. Usually these posts are about refactorings other than what the questioner asked about, and would be out of scope for the SO answer.
The accompanying GitHub repo for this article can be found here.
Global scope is a feature of browser JavaScript that is a source of application-spanning bugs (it is global). Global state doesn’t just impact the whole application — it creates a entire new surface area for bugs across the entire code base, that has to be managed. Bugs related to global state can happen anywhere. The number of potential bugs in every function increases as soon as you have global state.
Any local function can mess with the functioning of any other function by mutating global scope, and this can result in bugs that are hard to track down to their source.
In this refactoring we are not going to be able to completely eliminate global state — mostly because we don’t have enough information about how the state will be used in the rest of the application to make a recommendation for an alternative.
What we will do is reduce the bug surface area significantly. And along the way, you’ll be introduced to some of the concepts underlying React.setState and Redux.
THE QUESTION
Here is the code from StackOverflow:
global variable
var memArray =[];
//object
function member(id, password){
this.id = id;
this.pwd = password
}
var memObj1= **new** member("m001","123");
memArray.push(memObj1);
DISCUSSION
There is a lot going on this example that can be refactored, and we’ll look at a number of things in other articles. But for now, let’s look at global state.
MEMARRAY
The global memArray has two immediate issues - apart from being global.
- var
First, it is declared as var, which means that it can be reassigned at runtime.
In fact, using var is a declaration to the machine and to other programmers that “I intend that the value of this assignment change over the course of execution".
It may be that the novice programmer misunderstands assignment of arrays in JS. Making this a var doesn't make the contents of the array mutable - you have to do real deliberate work to make them immutable. Rather, declaring this as var makes the assignment itself mutable. Meaning that memArray itself can be mutated by pointing it to something other than the array you just created and assigned to it.
Somewhere deep in the code, a function could do:
memArray = []
This could be because another programmer uses it as a local variable name with no declaration, in which case the runtime will use the previously declared global variable. You won’t get a warning from your tools about using an undeclared variable, because it is declared.
And this bug in one function somewhere, that maybe doesn’t even use this global state (it probably doesn’t, or the programmer wouldn’t have reused the variable name), just broke everything that does use it. And when you go to hunt it down, it is not in any of your functions that do use the global state.
The chances of this happening are increased because of the second issue:
- Naming
See this article about the importance of naming.
In code examples on StackOverflow, I always name global variables like this: EvilGlobalMembersArray.
There is no way someone is accidentally reusing that in a local scope. At the very least, GlobalMembersArray is an unambiguous name that communicates what it is.
FIRST REFACTOR
const GlobalMembersArray = []
Make it a const so that it cannot be reassigned, and give it a meaningful and useful name. This is “naming by convention” that takes away cognitive load when reading the code.
If I find a reference to GlobalMembersArray in a function deep in the code, I immediately know what I am looking at, and I'm not using that name for a local variable.
MUTATION
The global is now not reassignable, and unambiguously named, which reduces the chances of someone accidentally reusing it. Since it is an array, they cannot change the reference to point to another array, object, or primitive, but they can still mutate the contents.
You want that, right? Presumably, we are going to want to add to, remove from, and update elements in this array.
No. By exposing just the array globally, we have devolved responsibility for mutating it to local functions in the application.
That concern, and hence the complexity of it, is now spread throughout the application. Bugs related to mutating the array values can appear anywhere in the application, at any time. And again, they can be hard to track down, because they will likely appear when a function uses the array and doesn’t find what it expects — rather than where the bug exists.
SECOND REFACTOR — IIFE
Rather than expose an array, we should expose an object that encapsulates the state, plus mutation methods. And we will not expose the actual state, because local functions can still and may be tempted to mutate it directly. Instead we will return a copy of the state, so that the only way to update it is via the object methods.
We can do this using an IIFE — an Immediately Invoked Function Expression, a JavaScript function that immediately executes and can return an object that has a private scope inside a closure.
In terms of ES6 classes, it is roughly analogous to creating an instance of a class that has private methods.
Here it is with no accessors:
const GlobalMemberStore = (() => {
let _members = []
return {}
})()
Note the enclosing () and the immediate invocation: (() => {})().
In this case, we will get back an Object with no properties. But what you want to know is that it also contains a hidden array — _members - that cannot be accessed by local functions.
But, but… aren’t you the “Just Say No to Variables” guy? What is that let statement doing there?!
Look, we can remove variables completely. But we don’t have enough information about the eventual application to do that. So what I’ve done here is take a global variable, and put inside a closure where it is invisible to the rest of the application.
All the complexity and bug surface area will be behind the singularity of the closure, with an immutable API. There will be no variables exposed to the rest of the application. And the resulting code is fully unit-testable.
IMPLEMENTING GETMEMBERS
Now we will provide a method to return a copy of the _members array:
const GlobalMemberStore = (() => {
let _members = []
return {
getMembers: () => [..._members]
}
})()
The ES6 spread syntax — [...members] - spreads the contents of the local members array into a new array, and returns that.
Local functions can add things to the array, or delete elements, but these operations do not affect the global state, because they have a copy of the global state, not a reference to the global state.
Note, however, that because the elements of the array are objects, local functions can still mutate members within the copy, and that will affect the global state — because the array elements are references to objects. The internal state array and the copy we just returned are different arrays, but they contain references to the same member objects
We can avoid that scenario like this:
const GlobalMemberStore = (() => {
let _members = []
return {
getMembers: () => _members.map(m => ({...m}))
}
})()
Array.map returns a new array, so the consumer has no reference to the global state array. The new array is populated by applying the predicate function to each value in the original array, and putting the return value in the new array.
It is “make a new array by applying this transform to each element in this other array”.
In the predicate function — m => ({...m}) - we return a copy of each member object from the _members array, again using the ES6 Spread syntax, this time on an object.
When you return an object in a one-liner arrow function, you need to put () around it so the interpreter doesn't interpret the contents of {} as function code, but knows that it is an object, so: m => ({...m}).
Now we have a new array, and new objects in the array.
Local functions now have access to the value of the global members state, but the actual global state is immutable by them, because they have no reference to it. They cannot update the global state from the copy that they get. For that, they will need to call an update method.
IMPLEMENTING SETMEMBERS
The first method we will implement is a hydration method that allows a local function to pass in an array of members.
I’ll take out getMembers for now to make it easier to read:
const GlobalMemberStore = (() => {
let _members = []
return {
setMembers: members => _members = members.map(m => ({...m}))
}
})()
Here we use the Spread syntax to copy the members to a new array, and this becomes the global members.
This means that a local function cannot set the global state by passing in an array of members, and then mutate the global state by mutating one of the members that it passed in.
If we did a naive assignment:
setMembers: members => _members = [...members]
Then the local function calling this method would have a local reference to the member objects that are now in the state store. By spreading them, we make a copy — another object in memory that the local function has no reference to.
IMPLEMENTING UPDATEMEMBER
It is likely that a business requirement for this application is that you can update a member.
So, we will implement an updateMember function. We will use Array.map to return a new array. A naive approach to this might be “let's iterate over the array using forEach and mutate the element we are updating". See the post “Just Say No to Loops and Variables” for an in-depth explanation of why you don't want to do that.
To implement the predicate function, let’s describe what we want it to do in plain language:
For each member in the state,
if the member id equals the id of the update, return the update;
otherwise return the member.
So, our predicate function looks like this:
member => member.id === update.id ? update : member
We are using the ternary operator here to implement if-then-else in a single expression.
We can probably shorten the name we use for member to m, because the context is sufficient to provide information about what it is:
const GlobalMemberStore = (() => {
let _members = []
return {
updateMember: update => (_members = _members.map(m => m.id === update.id? update : m))
}
})()
We enclose the assignment operation _members = in parens () to indicate that we did not forget to return a value, and intended only the side-effect. We could have put it in {}, but that will cause code formatters to turn our single line into three.
DESIGNING FOR FAILURE
20% of programming is getting it to work. The other 80% is programming for when it doesn’t work.
What happens if a local function requests to update a member who is not in the state? At the moment, the local function receives no information from the call to updateMember, and if you look at the code, what will happen is… nothing.
The predicate function will never match, and the new state will be a new copy of the existing state, unmodified.
We could throw an exception. This gives us the opportunity to figure out where the bug in the application is that it is trying to update a member that doesn’t exist. This is a good idea.
Let’s throw an exception so that the root cause can be debugged in the local function. To do this, we will need a getMember function that we can use. So, let's implement that.
IMPLEMENTING GETMEMBER
It’s likely that local functions will want only a single member. If we don’t implement it here, we will have local functions retrieving the entire state and filtering it. This leaks complexity into the application, because we can do that in “one place, and one place only” in the application: here.
Then we only have to test it in one place, and we only ever have to get it to work in one place. That reduces the surface area for bugs in the application.
We can use Array.filter to find elements in an array. Array.filter returns a new array containing only the elements from the original array for whom the predicate function returned true.
The predicate function is straight forward:
Return true if the member.id equals the requested id;
otherwise, return false
Reducing that down, we get:
Return member.id equals requested id
or:
m => m.id === id
So,
const GlobalMemberStore = (() => {
let _members = []
return {
getMember: id => _members.filter(m => m.id === id)
}
})()
The getMember array will now return an array with either zero (if no member with that id exists in the state) or one… hang on, what happens if there is more than one member in the array with the same id? In that case it will return more than one member.
Probably, the business requirement is that member id is unique. So we will take that into account when we write the addMember function.
So it will return an array with 0 or 1 members in it. Probably local functions want a member or undefined.
Although, we can provide a better API if we return an object like this:
{
found: true
member: Member
} |
{
found: false
member: undefined
}
Then consumers of this API using TypeScript can use a Type Guard to get safety against accessing an undefined value, and our API forces them to use it.
This reduces bugs. Otherwise, we are relying on every local function in the application remembering to test it for undefined before accessing it - another surface area for bugs.
So:
const GlobalMemberStore = (() => {
let _members = []
return {
getMember: id => {
const member = _members.filter(m => m.id === id)
return member.length === 1 ?
{ found: true, member: {...member[0]}} :
{ found: false, member: undefined }
}
}
})()
Remember to Spread the member to return a copy (I picked this up when the test case failed here).
Nice API.
THROWING ON IMPOSSIBLE UPDATE
Another significant advantage of this approach is that we put all our business validation rules about the data in a single place: in the store. They are not spread throughout the application, and the responsibility of everyone and no-one. They can be put in one place, tested automatically, updated in one place, and if a local function violates them, we will find out immediately when it tries to store the data, through an exception.
We can now consume getMember from our own API to guard against an update error.
How can we do that? We need to lift our API to its own context inside the closure, like this:
const GlobalMemberStore = (() => {
let _members = []
const Store = {
}
return Store
})()
Now we have a private reference to our own API, as Store. So we can use it to see if the member that the local function wants to update, actually exists - and if not, throw.
const GlobalMemberStore = (() => {
let _members = []
const Store = {
updateMember: update => {
const member = Store.getMember(update.id)
if (!member.found) {
throw new Error(`No member with id ${update.id} in the store!`)
}
_members = _members.map(m => m.id === update.id? update : m)
}
}
return Store
})()
IMPLEMENTING PUTMEMBER
Probably, a business requirement of the application will be to put a new member in the store.
We have to make a decision here about the behaviour of the store. What happens if a local function attempts to put a member with an id that is already in the store?
That’s probably a bug somewhere further upstream in the application logic, so we will throw an exception to allow debugging to start.
So we can do this:
const GlobalMemberStore = (() => {
let _members = []
const Store = {
putMember: member => {
if (Store.getMember(member.id).found) {
throw new Error(`${member.id} already exists!`)
}
_members = [..._members, {...member}]
},
updateMember: update => {
const u = needsMember(needsArg(u))
const member = Store.getMember(u.id)
if(!member.found) {
throw new Error(`No member with id ${u.id} in the store!`)
}
_members = _members.map(m => m.id === u.id? update : m)
}
}
return Store
})()
DEALING WITH A UNDEFINED ID
Another potential bug that we can detect here is a local function passing in either undefined or a member with an id that is undefined.
We can write helper functions for this, and call them on all operations where it is a requirement:
const GlobalMemberStore = (() => {
let _members = []
const needsArg = arg => {
if (!member) {
throw new Error (`Undefined passed as argument to Store!`)
}
return arg
}
const needsId = member => {
if (!member.id) {
throw new Error (`Undefined id on member passed **as** argument to Store!`)
}
return member
}
})()
Here is how we use this:
const GlobalMemberStore = (() => {
let _members = []
const Store = {
putMember: member => {
const m = needsId(needsArg(member))
if (Store.getMember(m.id).found) {
throw new Error(`${m.id} already exists!`)
}
_members = [..._members, {...m}]
}
}
return Store
})()
FREEZE!
For our final touch, we are going to freeze the API object using Object.freeze:
return Object.freeze(Store)
This prevents anyone from overwriting or modifying the API methods themselves.
If you wanted, you could (deep) freeze all the return values from the API methods. That would deny local function consumers of the objects the ability to mutate the return values. They would need to use spread on them. We’re not going to do that right now.
Freezing objects has an impact on performance. Freezing the API is not going to make a huge difference, so the safety is worth it. The objects returned from the API are copies, so freezing them is overkill, IMHO.
PUTTING IT ALL TOGETHER
Here is the whole thing:
const GlobalMemberStore = (() => {
let _members = []
const needsArg = arg => {
if(!arg) {
throw new Error (`Undefined passed as argument to Store!`)
}
return arg
}
const needsId = member => {
i (!member.id) {
throw new Error (`Undefined id on member passed as argument to Store!`)
}
return member
}
const Store = {
setMembers: members => (_members = members.map(m => ({...m}))),
getMembers: () => _members.map(m => ({...m})),
getMember: id => {
const member = _members.filter(m => m.id === id)
return member.length === 1 ?
{ found: true, member: {...member[0]}} :
{ found: false, member: undefined }
},
putMember: member => {
const m = needsId(needsArg(member))
if (Store.getMember(m.id).found) {
throw new Error(`${m.id} already exists!`)
}
_members = [..._members, {...m}]
},
updateMember: update => {
const u = needsId(needsArg(update))
if(!Store.getMember(u.id).found) {
throw new Error(`${u.id} does not exists!`)
}
_members = _members.map(m => m.id === u.id? update : m)
}
}
return Object.freeze(Store)
})()
This may seem like way more complexity than:
var memArray = []
However, this is the actual complexity involved in this data structure in the application. You will end up doing all of this anyway — but it will be spread throughout your application in manipulation and mutation of that array, and if statements, and fixing bugs in various places.
And it will be really hard to refactor in the future.
With this approach, the total technical complexity of this concern is now encapsulated in one place in your application. It is testable through automated tests — as demonstrated in the accompanying repo. There are 125 lines of test code for 40 lines of code. So 165 lines of code to replace var memArray = [].
However, business validation of the data now has a place to live, and the entire expected usage of this array is now implemented such that local functions cannot introduce bugs related to it — only their local use of it.
winning
FURTHER RESOURCES
This approach to state management has become popular in JS in recent years, and is the basis of the approach used by:
If you grasped the concepts and rational for the refactorings that I made in this example, you will be well-placed to understand these mature, more sophisticated (and generalised) implementations.
Top comments (0)