Your JavaScript Smells
Common code smells, what they are and how to avoid them
It’s time to address the pink elephant in the room. I’m sorry, I tried to ignore it but this is turning into a problem: your JavaScript smells.
And this is not the type of smell that goes away with a shower. You have to open your IDE and much like you do when you go through your fridge looking for that food container that’s been there for 2 weeks and it’s starting to stink up the whole place, you’ll have to go through your code and figure out what parts of it smell.
That being said, “smells” refers to the type of error on your code that isn’t necessarily an “error” but more like a problem waiting to happen. Smells in real life, especially bad ones aren’t necessarily the issue most of the time, but they’re a clue to a bigger problem (like the rotten food inside your fridge).
We can look for these “smells” in our code and fix them before they turn into actual problems, so let’s take a look at them now.
Magic values
Look, I’m a big fan of magic, I’m all for it, but not on my code, sorry.
Magic values refer to values we assign to our variables that do “something” or that have some kind of meaning but that if we’re not the magician who wrote the code, we can’t really tell where the trick is.
I’m mixing metaphors, I know. But the point is that you can’t expect everyone reading your code to understand it if you don’t write it with that goal in mind. In fact, you won’t even understand it given enough time between writing it and reading it again. Let’s avoid the headaches and let’s make sure our code is readable by everyone.
let a = "start";
let b;
for(let i = 0; i < 5; i++) {
b += a[i].toUpperCase();
}
The above example is quite silly, but it should be enough to give you an idea of what I’m talking about. The code iterates over every character of the string in a and turns it into an upper case version of it and concatenates the resulting value into b . Yes, this could’ve been done differently, but that’s not the point. The point is that our for loop is going from 0 to 4, why? Because our string has 5 letters. And in this code those limits make sense, however the magic value of 5 here is a problem if in the future you have to change the string in a into something else. You’re showing a lack of planning and your code is, what I like to call, not able to scale. A simple change like this would make a big difference:
let a = "start";
let b;
for(let i = 0; i < a.length; i++) {
b += a[i].toUpperCase()
}
By removing our magic value we’re now suddenly able to scale our code and make it work for any string without further modifications.
Another problem magic values pose, is readability.
let total_cost = order.getCost() * 1.52;
That’s just one line of code, but it’s wrong. Where is the 1.52 coming from? What does it mean? Why are we using it to calculate the total cost? We can definitely make some educated guesses, but that’s all we’d be doing: guessing. This magic value problem could be solved either by adding a line of comment to explain what it means and why we’re using it here, or by defining a constant with a more mnemotechnic name and using it instead. I prefer the second option, since you’ll likely be using it somewhere else and if for some reason you’re tasked with changing its value in the future, you’ll only have to change it once, instead of having to track everywhere you used and manually update it.
const MADRID_TAX_RATE = 1.52;
let total_cost = order.getCost() * MADRID_TAX_RATE;
The fix for it was easy, but now you read it and can understand what’s going on. The same thing can be said for strings and other data types, as long as you’re using their primitive representations without any kind of explanation, you’re not helping anyone, so avoid that practice.
One function to rule them all
It didn’t work for anyone in The Lord of the Ring really, so it won’t work for you either here.
Avoid having a single mega function to rule… err, hold, every piece of business logic you need. The ideal function is the one that handles one thing and one thing only. Why is that? Because if there is a problem you know exactly where to tackle it. Or if you need to make a logic change, you know exactly where that piece of logic is, and you can even replace where it’s being used by calling another function. It’s like switching a red lego block with a green one because you now suddenly want it to be green.
The idea of functions as lego blocks works perfectly well. The smaller and more compact they are, the more flexibility they give you in the future.
Ever heard of the Single Responsibility Principle (SRP)? Well, the gist of it is that everything you do (be it a function, a class, a module or however you structure your code) should only have one single responsibility (the name kind of gives it away, I know). That means functions should only be doing one thing, but classes or modules, which are a group of “functions” (notice the quotes there), should be doing multiple things that are related to the same task.
For example, if you’re dealing with an eCommerce site, and you’re having to develop the payment workflow. What do you think would make the most sense here?
- Have all potential payment gateway integrations inside a single function called
makePayment? That would centralize all the logic, indeed, but it would also make for a function that is a couple of thousand lines of code long. - Have each individual integration inside its own class, potentially in different modules, each one dedicated to each gateway? That would keep the code clean and tidy, yes, but it would also be hard to maintain since most of their workflows probably have some common concepts that could be abstracted and re-used.
- Have a single payment workflow class/module that centralizes individual methods/functions for each gateway?
Options #3 would comply with the SRP because the responsibility here is to handle the payment and it would be a perfect middle ground between options #1 and #2. It would keep the code spread amongst multiple constructs but manageable within a single logical grouping. Furthermore, you wouldn’t have to maintain a beast of logic that, if kept all together inside a single function, would undoubtedly start coupling the logic of different gateways together causing changes for one of them to affect others.
Remember, SRP is about responsibility, not about putting everything together into a single function.
Abusing primitive values
Though it might sound like a rehash of the magic values problem, this one is a bit different.
The problem here is not that you’re using primitive values directly in your code, causing a huge confusion on whoever is reading it. Instead, the issue here is that you’re not taking advantage of more complex data types, such as classes, object literals, enums (if you’re using something like TypeScript) and so on. You’re just sticking with the classics: strings, numbers and the occasional boolean.
Don’t get me wrong, I’m a big fan of the classics, but there is a reason why the others exist as well. You can definitely code without using any type of complex data type, neither the language nor the interpreter will complain about it. However, as I always like to say: you have to write code that runs on a machine, but that is also readable by humans.
That second part is crucial, since it affects quite a bit the way we write our code.
The following snippet works, it’s valid, but it’s abusing primitives:
function saveUser(name: string, address: string, birthdate: Date, phone_number: string) {
//...
let diff_ms = Date.now() - birthdate.getTime();
let age_dt = new Date(diff_ms);
let age: number = Math.abs(age_dt.getUTCFullYear() - 1970);
///....
}
The main KPI (or Key Performance Indicator) when measuring how well a piece of code can be read by humans, should be the amount of mental parsing that needs to happen for you to understand it. And primitive types cause a lot of mental parsing. Instead, more complex structures tend to allow for a higher level of abstraction, which in turn means you’re able to understand it without having to parse and read every line of code.
Take a look at the same idea but using a higher-level data type (a class in this example):
function saveUser(usr: User) {
//...
let age: number = usr.getAge();
//...
}
You can imagine how the class’ definition looks like, the point here is that you’re now suddenly reading the saveUser function and instead of having to understand what it is doing with the dates to calculate the age, you just know it’s calling the getAge method to do it for you. And if you’re using this function, you no longer have to pass 4 different parameters, instead, you just pass the concept of a “user” as a parameter, and then let the function worry about how to interact with it. It’s a lot simpler to read, a lot simpler to understand and a lot simpler to use.
The key here is to remember that whenever you start seeing several primitive-type variables that are related to the same concept, it might be a good idea to group them together into a higher-level logical concept and use that instead.
If you liked what you’ve read so far, consider subscribing to my FREE newsletter “The rambling of an old developer” and get regular advice about the IT industry directly in your inbox
Abusing switch and IFs
Wait, don’t leave yet, I’m not asking you to stop using IFs or even switch statements. One thing is to use them across your code, and a very different thing is to abuse these statements. JavaScript in particular gives you a lot of alternatives that are easier to understand and require less mental energy to parse than a switch so why not take advantage of that?
The point I’m trying to make is that while the following example is perfectly fine:
function configureChristmasTreeLights(tree, mode) {
switch(mode) {
'rgb_blinking':
tree.setColors('rgb', {
'blinking': true,
'frequency': 10
})
break;
'rgb_fixed':
tree.setColors('rgb', {
'blinking': false
})
break;
'white_smooth':
tree.setColor('white', {
'blinking': true,
'frequency': 1
})
break;
default:
throw new Error("Unrecognized mode used")
break;
}
}
It does require you to carefully read the 26 lines of code and understand what it is doing in every case. And this example only has 3 available modes, but what if the next version of your tree comes with 10 different light modes available? Will you have a 100-lines function with a huge switch inside? There needs to be a better way, don’t you think?
This is what I mean by “abusing” these structures. Just like using primitive data types forces the developer to mentally parse a lot of logic that they shouldn’t be parsing — unless they need to, of course — , switch and IF do the same thing. I’m not going to go into a lot of details here but I did write a full article about 3 different ways in which you can avoid abusing these constructs, so check it out here.
But to close this example, a better, more scalable and less mentally taxing way of writing the configureChristmasTreeLights function, would be something like this:
/// Helper functions
function blinkingRGBLights(tree) {
tree.setColor('rgb', {
'blinking': true,
'frequency': 10
})
}
function fixedRGBLights(tree){
tree.setColor('rgb', { 'blinking': false})
}
function whiteSmoothLights(tree) {
tree.setColor('white', {
'blinking': true,
'frequency': 1
})
}
//The actual function
function configureChristmasTreeLights(tree, mode) {
const mapping = {
'rgb_blinking': blinkingRGBLights,
'rgb_fixed': fixedRGBLights,
'white_smooth': whiteSmoothLights
}
try {
mapping[mode](tree);
} catch (e) {
throw new Error("Unrecognized mode used")
}
}
This modification actually takes makes use of the SRP as well, instead of having a single function dedicated to setting the lights on the tree do everything, we’re not using it to orchestrate the actions, but the responsibility of setting one particular type of lighting mode resides on each individual function. That way adding a new mode is as simple as adding the function and one more line inside the mapping structure.
The key here is that while our 3 example light modes are very simple, a new one could potentially be very complex and require multiple lines of code to achieve. That complexity however will remain hidden inside its own function, as a reader all you need to know is that the configureChristmasTreeLights function is calling it.
Duplicated logic/code
As developers we should know better than to duplicate code throughout our project. Copy-pasting a piece of logic around is a clear indicator that you should, instead, be abstracting it into its own function.
But what if you’re not duplicating code, but rather, duplicating logic? Sometimes big teams have so many different developers working on the same codebase without coordinating with each other, that they’ll re-invent the wheel, sort of speak, without knowing.
The example I always like to give here, is that of a logger module. Big projects eventually start requiring you to handle logs other than through console.log so some developers install modules like Winston, Bunyan or Pino and then create their own instances of them.
Imagine how easy it would be for two developers, working on the same new project and seeing the lack of logging, to think “I can solve this in 2 minutes” but not taking the time to coordinate with each other. The end result would be that you have two different logging modules on the same project, potentially even using different logging libraries as well.
Repeating code is bad, but repeating logic is even worse because it’s harder to find and the fix can potentially require a lot more work (or rather, re-work). So keep an eye for both, and if you’re part of a team, make sure you all coordinate adding features that would affect the entire team before doing so.
Coding smells can be very easy to fix in some cases, and they might require big refactors in others. The best way to tackle them, is to be aware of them and see them before they become a problem. Sometimes a few extra lines of code, or even taking 10 minutes to actually think through a new feature, can save you hours of headaches!
What about you? Can you think of another code smell I didn’t mention here? Share it in the comments!