4 Code Smells That Are Probably Already in Your Code

Introduction

Lack of experience, rushed deadlines, or missed code reviews are just a few factors that might cause you to create ill-conceived code, the so-called “code smell”.

To fix these “smells” you have to open your IDE, examine your code, and find out which part of your code is affected.

Although a code smell does not break your app instantly, it may lead to errors in the future because it might make the code unreadable and unmaintainable. Furthermore, implementing new features will be much more complicated, take longer, and introduce more bugs.

This article will discuss four prevalent code smells and explain problems that could occur.  I’ll describe each one in detail, provide instances, and show how to fix them.

To highlight the code smells, I use JavaScript, but they exist in every programming language!

Magic Numbers

Magic numbers are values that software developers assign to variables within the code to do “something”. They always have some meaning, but if the software developer who wrote the code leaves, no one will ever remember it. Although they are called magic numbers, this code smell includes every primitive data type like Char, String, Boolean, etc.

It is paramount to understand that your code has to be readable by everyone at any time. Using magic numbers will prevent future work and alteration by your peers and even by you should enough time have passed.

To better understand the problem check out the following code snippet:

let a = "1534";

for(let i = 0; i < 4; i++) {
    send(a[i])
}

This is quite a silly example, but it should be enough to show the problem. The code iterates over every character in the input string a and calls send() for every char. However, the for loop is going from 0 - 3 because the input string has 4 characters and you want to iterate over the whole string.

The developer who implemented this loop did use this magic number because the input is always 4 chars long. This limit makes sense within this code snippet but could introduce some weird bugs in the future if the string a will change for any reason.

This lack of planning makes the code impossible to scale.

However, a simple change within the snippet would make a big difference:

let a = "1534";

for(let i = 0; i < a.length; i++) {
    send(a[i])
}

By removing the magic number, the for loop iterates over the length of the input string. Now, the code can scale, work for any input string, and is future-proof.

Another problem with magic numbers is code readability:

const totalCost = order.getCost() * 1.19;

You might ask yourself where the value of 1.19 comes from and why the cost of an order is multiplied by this value. Maybe you can make some educated guesses, but that’s all you can do: guess!

This magic number is a problem because:

• You cannot be sure what this number is used for
• Changing this number will be hard if used several times

To fix this problem, you can either document the line of code and explain the number or replace the magic number by defining a constant with a mnemotechnic name and use it instead. Always opt for the second option because a constant will be reusable  and changeable easily:

const OVERALL_PROFIT_MARGIN = 1.19

const totalCost = order.getCost() * OVERALL_PROFIT_MARGIN 

The fix for this magic number was easy and will help to understand that a fixed profit margin is added at the end of the total cost calculation. If you guessed earlier that the 1.19 adds the tax rate to the order, you were wrong because that was already included in the order costs.

This refactoring should be done for any string, number, or other data types if using their primitive representation without proper documentation.

It will help everyone to read your code, even yourself, after some months.

Duplicated logic/code

Every software developer should know several techniques to avoid creating duplicate code throughout a software project. A clear indicator that should raise alarm bells is copy-pasting any piece of logic. If this happens, you should be abstracting it into its function and replacing the usage by calling the newly created function.

But what do you do if you are not duplicating code but rather duplicating logic?

In software teams, it often happens that many different developers work on certain parts of the software. Often, they encounter the same problems and implement a well-documented, readable, and maintainable solution.

To demonstrate the problem, imagine a large software project on which you are working, and multiple instances of console.log are used. Now you want to use a better approach to log your errors, warnings, and info. You now create a new global logging functionality within the utils.js file:

const log = (message) => {
    if(LOG_ENABLED) {
        console.log(message)
    }
}

From now on, you can import your utils.js file, use your log function to log anything and are also able to completely deactivate the log output by adjusting the global constant LOG_ENABLED.

As you are working with many developers, another developer could have also thought about this and implemented another log function:

const log = (message) => {
	if(NODE_ENV === 'production') {
    	console.log(message)
    }
}

Unfortunately, the other developer creates a new file log.utils.js to store his logging function and use it where it is needed.

You will end up with two logging functions implemented that use a different variable to check if the log is enabled or not. If you now want to allow log messages but did not update the NODE_ENV,you will only see some Logs and will be left wondering why the other log messages are not generated.

Remember: Repeating code is bad, but repeating logic is even worse.

It is much harder to identify this kind of problem and a potential fix requires a lot of work (or re-work). Try always to talk, plan, and coordinate adding new features that affect the whole software project.

One function is to rule them all

If “one ring to rule them all” didn’t work for anyone in The Lord of the Rings why it should work for your software project?

In a software project, avoid using a meta function to rule… I mean, contain every part of the business logic needed. Ideally, every function should only handle one thing and have one single responsibility. If designing any class or function, you should always ask yourself:

• “Why does this class exist?”
• “Which problem does this function solve?”

Robert C. Martin has a famous quote that you should always think about while implementing classes, modules, or functions:

“A class should have only one reason to change.”

This is because if you encounter any problem, you will know where to search for it. Also, if you have to change some logic, fix some calculations, or remove a dependency, you can do it without any implications.

It’s like working with LEGO blocks. If you have only a big one and want to change a single thing, it is impossible. But if you are working with the tiniest that you can buy, you can exchange some blocks for others without any problem.

Comparing software projects and functions with LEGO blocks works very well because the smaller your block is, the more flexibility you will have in building anything!

Ever heard of the Single Responsibility Principle (SRP)? This principle defines that everything you implement in your software project should only have a single responsibility (be it a function, module, class, or any structure in your code). This means that functions only do one thing and classes (or modules) will group multiple functions which relate to the same task!

For example, if you are working with a webshop and want to implement the payment workflow, you have several ways to implement this:

1. Create a new function called makePayment that integrates the complete payment gateway and centralizes all the logic. It will contain everything for the successful payment in one place and will be a couple of thousand lines of code long.
2. Integrate each part inside its class, separated into different modules, each one dedicated to each gateway. This will keep the code clean and tidy but also increases maintainability since many workflows could have many things in common or use common concepts that could be abstracted and re-used.
3. Create a single payment workflow in which each class or module centralizes individual functions or methods for each gateway.

In terms of SRP Option #2 would be the perfect fit, because the responsibility is to handle the payment. Also, it would be a tradeoff between options #1 and #2. Furthermore, it will keep the code spread amongst different constructs but manageable within a single logical group. You will not have to maintain an enormous amount of logic and can refactor or decouple more parts without affecting each other.

Always remember: SRP is about responsibility, not putting everything together into a single function.

Long Parameter List

Having a long parameter list in a function call (or class) is something that smells. And it should be removed ASAP!

Often a long parameter list indicates something wrong with the implementation or that several algorithms were merged into a single method (which is against SRP!).

Officially, there is no specific rule about how many is too many, but usually, you should NEVER use more than three or four. Every time you increase the parameters list length to five, you should refactor this function (or class).

Let’s see the following code snippet that creates a User from several values:

const createUser = (username, password, state, city, street, nr) => {
 	// ...   
}

To create the user there are six different values needed: username, password, state, city, street, and nr. Now, instead of using this list of parameters, the method could use another data structure as input to create the user that will decrease the parameter used:

const createAddress = (state, city, stree, nr) => {
    // ...
}

const createUser = (username, password, address) => {
    // ...
}

With this change, you will have two separate functions with a single responsibility and only contain parameters that belong together. Instead of assigning the address to the user, a new function will create the address object that will then be passed to the user as a parameter.

But why should you do this?

Because it generates some payoffs:

• more readability and shorter code
• refactoring may reveal previously unnoticed duplicate code!

Closing Notes

During our careers as software developers, engineers, or leads we will often need to write code without planning. Consequences are often that we end up creating code smells.

Sometimes code smells can be identified and refactored very easily, but in some cases, they might require big refactoring. The best way to tackle them is to be aware of them and see them before they become a problem in a software project. Take the time to plan implementations and research before starting a new feature.

Often, a few extra lines of code, 1 hour of additional research, or 15 minutes of calling another team member can save multiple hours of headaches later in the project.

Take time before implementing a new feature because you will thank yourself later!

How about you? Do you have any other code smell that I should mention here? Also, do you have any questions regarding any of the mentioned code smells? I would love to hear your thoughts. Please share everything in the comments.

Feel free to connect with me on Medium, LinkedIn, Twitter, and GitHub.