Refactoring JavaScript — 5 Common Problems to Look Out for and How to Fix Them
5 things your JS code should never have
Refactoring is like the NeverEnding Story: you might think you’re done, but as long as the plot (the project) is ongoing, there is always room for more changes.
That’s OK though, it’s part of our job, always trying to adapt, change, tweak and finetune our code to make sure it’s still maintainable and stable long after it was originally written.
The problem though, comes when we focus on the wrong aspect of our code looking for things to refactor. After all, a good refactor normally means running the risk of breaking a working feature, so you better make damn sure that whatever you’re trying to change, is worth it.
This is why today, we’re going to cover 5 common problems that can easily be refactored out of your JavaScript code and how you can do just that.
Magic values
I love magic but I hate magic values. And I’m sure that’s the case for many of you as well.
Magic values are plain values we use in our code and that by themselves mean nothing. What does 2.34 mean to you? Does it evoke a concept in your mind? Do you know what I’m talking about if I simply spit that number to you? How about "home"? Does that mean anything to you if I don’t give you any context? Not much, does it?
How about MADRID_TAX_VALUE or destinationStr ? I’m sure you can easily think about a few concepts and use cases for those variables. Can’t you?
That is the problem with “magic values”, they don’t have an intrinsic meaning, and when used directly in our code, they don’t necessarily add anything to the person reading the code.
Granted, from a functional perspective, the interpreter or compiler will not have a problem with them, this is more of a problem for people. But then again, I always like to say that: you need to write code that can be run by machines and read by humans.
If one of those two things can’t be done easily, then you’ve failed at your task.
Avoid having those loose values in your code unless they’re very specific things, such as the first index for a for loop or accessing the very special first slot of an array. Instead, try to capture those values in a single place, even if you use them throughout your code in multiple places, consider having a single constants.js file where you export all these values with very mnemotechnic names, which can add actual value to the future reader of your code. You can save them somewhere else if you like, and if you have enough, maybe it’s also a good idea to group them into multiple files, so you can only import the ones that are related to the bit of logic where you’ll use them.
Consider the following piece of code that calculates the full price (including city taxes) of an item, based on the price without taxes and the city of sale.
Yes, you know what it is doing, in general, but you can’t really tell which cities are being targeted in that code unless you look for some kind of reference.
If instead, you did something like:
Notice how the getfullPrice function now makes a lot more sense, and you can tell which city is affected by each conditional branch. In fact, you can even understand what the last return statement is meant to be doing — without having to guess.
Now, the added benefit of this approach, is that you can use these constants wherever you want, and if tomorrow for some reason, one of these cities updates its tax value, you only have to change it in one simple file.
Say “no” to magic values!
Abusing primitive values
There is nothing wrong with a good integer here and there, or using strings for a lot of things. But the problem starts when you want to use multiple primitive values to represent something more complex.
Imagine having a signup function that at first, only takes a username and a password. Essentially, two string values and saves a record into the database. Simple.
Then in a few months, you’re asked to save an avatar and the name of the user during signup, so you add a couple more string parameters to your function, which now accepts 4 parameters.
Then, in yet a few more months, you’re requested to add a Bio field to save some details from the user, their birthdate and you also have to calculate the age of the user during signup.
Boom! You’re now passing 6 parameters to your function, without counting things such as the database connection and you have extra logic. You might end up with something like this:
This approach won’t scale correctly, because:
- The more data points you need to save, the more parameters you’ll need, and the signature of the function will keep on changing. This means you’ll have to update every call to the function or have some default behavior when the new parameters aren’t available.
- You now also have extra logic inside your function that is not specifically related to the responsibility of the function (i.e saving the user). The age calculation code should not be here, however, given you’re dealing with the primitive values right there, it’s very tempting to add it.
Instead, try to group all these parameters into a single entity that can grow independently. JavaScript, in particular, allows you to create classes and group everything together: not only the different data points but also the related logic (like the age calculation code):
If you’re not a fan of classes, you can still try to group all parameters into an object literal, and use that across your functions. It won’t solve the business logic being spread out, but at least it’ll make it easier to add functions around that construct.
This is one of those cases where OOP really shines, it solves the problem and provides a lot of room to grow and scale.
Duplicated code
Duplicated code is not a JavaScript-specific problem, but it’s something you should always be on the lookout for.
The simplest form this problem can take, is that of literal duplication. When you copy&paste a piece of logic from one place to the other. If you do this:
- Why? Why would you do that?
- You’re not helping anyone.
- Probably a puppy died somewhere in the world.
If you see yourself about to do this, stop before hitting the CTRL+V keys, and consider creating a function, module, method or whatever callable construct you can think of, and put the code there. Remove it from the original place, and call the new construct (we’ll call it “function”) from both places. This is how you keep your code DRY (how you follow the Don’t Repeat Yourself principle) and avoid a potential puppy massacre.
Now, there is a second version of this problem, one that is less obvious and can get past the untrained eye: duplicated logic.
Noticed how I didn’t say “code”, I said “logic”. This is just as bad, because you have code in two (or more) places doing the same thing, only that when you need to maintain it, change it or even render it inert, you’ll have to find each individual instance of it.
A good example of this would be to have your own logger function in a module, while at the same time, a colleague of yours created a similar logger function, doing something very similar.
Like this:
Imagine both functions living in very different sections of your codebase. Finding them would be near impossible without combing through the entire thing specifically looking for it. And if you’re not paying attention, then you might miss them, considering their implementation is not exactly the same. However, they are doing the same thing at a fundamental level, and thus you (or someone on your team) should consider the possibility of merging them into a single function that can be re-used everywhere.
Independent components for the microarchitecture age
In the specific case of Javascript, it may also make sense to turn your function/class into an independent component that is usable and maintainable across projects/repositories.
Monolithic projects are increasingly becoming a thing of the past. Microarchitecture is great but it sure presents its own set of challenges.
Collaborating on modules across repositories and handling their dependencies is definitely one of the most prominent challenges. Luckily for us (JS developers), that can be solved using independent componetns and Bit.
Callback hell or unending promise chains
I feel like I date myself if I mention the term “callback hell” these days, but I’m going to anyway, because I’m sure some of you are still working with callbacks.
Look, don’t get me wrong, there is nothing wrong with callbacks, unless of course, you abuse them.
The following code is completely fine and I would argue, hardly requires any refactoring:
We can all read and maintain that code. It’s one callback where the logic resides, and we can easily extract it into an independent function if we wanted to clean it up a bit (not that we have to).
However, this is not OK:
That’s just hard to read and maintain, so in other words: wrong.
You could consider modifying that and use promises, which is a very common way of cleaning up asynchronous code. And yet again, there is nothing wrong with promises, not until you start abusing them.
This is completely fine:
That’s clean code, nothing to complain about there.
However:
You can still read that, mind you, and it’s a lot nicer than it would’ve been implemented using callbacks. However, other than the fact that you keep adding then calls over and over, you’re also a bit limited by the scope of each then statement. Consider the sendInvites function. If you need the email address of the parent of those children and you don’t have the correct data model implemented, you’d have to somehow return both, the invites and their related parents as part of the createBirthdayInvitations function.
That’s not ideal, it adds a bit of extra responsibility to that function and in practice it feels like hiding the dust under the rug, instead of actually cleaning the floor. You know?
Instead, we can change this code to be cleaner, easier to read and a lot nicer overall with async/await like so:
The code reads a lot easier, it’s a lot faster to mentally parse, and it allows us to have more flexibility in the way we handle the data, considering it all lives within the same logical scope. Notice how I changed createBirthdayInvitations to also receive the contacts list, I just used the variable, I didn’t have to do any other magic.
This approach provides the much-needed flexibility that real-world applications demand. We know code changes, we know business requirements change and we need to adapt to them. Through this approach we gain the required elasticity to keep changing our code without affecting readability. It’s a win-win.
Spread responsibility
The final point you should also be looking to refactor out of your code, is the spread of responsibility.
What do I mean by this? Having different components taking care of the same thing or at least, very related activities.
Consider the single-responsibility principle, which states that your components (whether they’re functions, modules, classes, or whatever you’re building) need to handle a single thing. A single responsibility. If you’re building the User class, you don’t want to have the logic for your logger inside it, you want to use the logger there, yes, but define it somewhere else. Or if you’re creating a FileUtils module, you don’t want a function that takes care of logging the user into your system in there, it makes little sense, that probably belongs somewhere else.
Well, the opposite to that principle, is having a single responsibility spread out into multiple components.
Imagine for instance, having the User class from the “Abusing Primitive values” case above. But instead of having the getAge method, you have a function within your utils module that receives a User object and calculates its age. And then another function to calculate the coordinates of the user’s address, called geoLocateUser .
Why would you do that? Those functions won’t work with anything other than your User objects. There is no point in having them outside the class. In fact, it makes it harder to maintain, because the moment the shape of the class changes (i.e you modify its properties) you might need to go out and update all the functions that depend on them. And if this is a widespread problem, you might miss a few.
Consider this real-world problem when adding functionality into an existing system, and make sure that you add it where it makes sense and not where it’s the easiest. Sometimes a little bit of effort can save you big headaches in the future.
Keeping your code clean through refactor is a never-ending task, and that’s why I cover it as a completely separate topic in my latest book, Code Well With Others. Either by removing magic values, or by making sure that you’re keeping all related logic within the same module, you’re contributing to keeping the technical debt in check, and that’s always a good thing!
What other bad practices do you like to refactor out of the code whenever you see them? Leave your ideas in the comments and we can discuss them!
