Code smells are certain patterns and characteristics of code that could indicate or create a bigger problem in the future. In other words, code of questionable quality.
The keyword here is questionable. Identifying something as a code smell does not mean that it should be immediately fixed or that it is a problem — only that it should be looked into. It does not mean that the particular piece of code is badly written, only that it should be examined more carefully.
Organizations, teams and different developers have their own understandings of particular patterns and habits. It all sums down to whether a certain technique works for you or doesn’t.
The point I will communicate in this article is that it’s worth examining particular patterns to avoid problems in the future. But before that, let’s clear some terms out of the way.
There’s a TL;DR at the end of the article if you want only the main points and lessons learned.
Code-Smells vs Anti-Patterns
An anti-pattern is the diplomatic way to say that the solution to a given problem is not good enough. Finding an anti-pattern means that we have identified a problem in the code base that should be refactored as soon as possible.
Code smells mean that a design pattern could be misused or there is an opportunity to implement something better. They are more of a possibility rather than a fact.
Refactoring
When we identify a code smell or a place in the code which could use some improvements we reach for the term refactoring. Since in this article we will be mentioning this word quite a lot we need to clarify exactly what we mean by it.
Refactoring is the processing of modifying some code’s design without changing it’s functionality. Put in even simpler words — we change how the code is structured but we don’t change how it behaves.
But how can we change something without modifying its behavior? If we need to refactor a component to take less props or split it into multiple smaller components does that count as behavior change?
Yes, changing the component’s props will change the way we invoke it. However, our goal is to have the app working the same way after we have finished refactoring it. If a component is responsible for displaying a list of filters and we split it into multiple components we still need to have a working list of filters in the app.
Bloated Components
Before React laid the foundations of component-oriented front-end development, we used to mimic the MVC model used in server-side languages. Depending on the framework, there used to be an HTML template containing the markup and a controller file responsible for providing the view with the data it needs.
When this practice transfers to React applications it leads to the creation of a master component. Those are components that can reach up to 1000 lines of code and contain insane amounts of logic and markup.
There are different ways for a component to reach that bloated state. Following the obsolete idea of having page components is one. Another could be taking the idea of smart/dumb components too much to heart. Some times it feels easier to jam another method inside the class rather than refactor the code.
The end result is the same. We end up with a giant component which acts as the origin point for business logic and store connections.
The snippet above is to serve only as a vague example, an actual master component can hit a thousand lines of code or more.
How to avoid it?
We could technically write the whole app in a single React component and all the markup in its render method. Of course, we don’t to it because maintaining something like that would be impossible. The same logic can be applied to writing a whole page’s logic in a single component.
If you’ve fallen in a situation in which you need to refactor such a component you have no other choice than to roll your sleeves up and get to work. The sooner you start slicing pieces away from it the better it will get.If you are working on a greenfield project, however, there are two ways to avoid this problem — good planning and self-discipline.
Before you start working on the page, sketch it down on a piece of paper and mark your components with rectangles. Consider the possible interactions between the different parts of the page. Depending on the complexity consider using multiple containers in the different parts of the page and have them communicate between each other using the page component.
Self-discipline comes in the picture when we need to follow the plan we’ve made. While deviating from the blueprint could be appealing, the cost for that is unavoidable and the interest is high. Spending half an hour to do something the right way could save days of refactoring in the long run.
When you start pulling away components from the master, you will see that there is logic related only to particular pieces of the page. Extracting a few parts like that will make the container a lot leaner.
Repetitive Logic
I follow a simple rule when it comes to repetitiveness. I can’t recall exactly where I read it but it states that if you need to copy something for the third time then it needs to be extracted.
I give myself a free pass if there is only one other occurrence, but once they become three then it’s time for a refactor.
Having repetitive code is unpleasant for every time you need to make changes you have to go on a safari throughout your code base. Whether it is business logic or some markup, extracting it will keep our sanity in check.
How to avoid it?
Logic that ends up in many components calls for a Higher Order Component or a component which uses Render Props. Lately I’ve been falling more in favor of the latter because I find it easier to understand exactly what data it exposes. Except for some super specific cases, anything that you can build with one you can build with the other, so it’s more a matter of what you feel comfortable with.
Something that we need to talk about here is to not overengineer and overcomplicate problems. If there is a piece of business logic that is needed in many places moving it to a utils.js file is fine as well. Once you have the need to associate it with markup or some particular behavior then it’s okay to promote it and use the patterns we mentioned above.
Prop Drilling
Taking the concept of smart and dumb (presentational and container) components too much heart can lead to bloated containers, but it can lead to the infamous prop drilling as well.
Prop Drilling means to pass props multiple levels of components deep in order to reach a particular component that needs those props. And since a parent can’t pass a prop directly to one of its grandchildren it needs to go through other components on its way down.
The problem is that all those other components won’t actually use the prop, they will just pass it down to lower level. This leads to the unnecessary exposure of information and tighter coupling in the code base. Changing the data’s structure or the name of the prop would require modifications in multiple places.
It’s a common occurrence to rename a prop in a component due to name collisions or for the sake of clarity. Doing that a couple of times in the prop drilling chain will only leave us confused and frustrated.
How to avoid it?
If you are working on a small application and prop drilling is starting to become a problem for you then using the Context API may be your best solution. From React 16.3 onward we have access to a new and improved API that allows us to share data in deep component hierarchies.
If state management and prop drilling are starting to get out of hand then adding a library like Redux or MobX to the stack should alleviate the problem. Of course those come with a non-negligible learning curve so avoid any snap decisions.
If you are already using such a library, consider adding more container components. If you need a component to fetch half the store and pass it down the presentational components then it’s probably time to decentralize the hierarchy a bit.
Since both Context and the libraries I mentioned have excellent documentation, there is no point of adding an example here.
JSX Outside the Render Method
Something that I personally find confusing, is JSX being generated in an instance method of a class component. Looking at the render method’s return value we should be able to get a good grasp of what this component prints. However, by generating markup in other methods we will soon need to bounce up and down across our component’s code to understand what it renders.
If we need to extract some markup into its own method then that means that we are looking to separate concerns in some way. However, by using this technique we bind the component to the markup even further. Since that method will most likely use something from state or props we will increase the level of coupling.
How to avoid it?
If you find the need to move some JSX into an instance method I would advice to move it into a separate component instead. In OOP languages, the idea of putting some functionality in a separate method means that it needs to be abstracted away for reusability or logical purposes. In the context of React, the equivalent of the above idea is to extract some logic into its own component.
The counter-argument here is that by adding a component instead of a method call inside the JSX we still can’t grasp exactly what it does. While that is true, we will have clear visibility of the component’s accepted props which is a strong hint for its functionality. At least we will know what data it needs and what callbacks it calls.
Deep Indentation
When we write deeply nested markup we sacrifice readability and force ourselves to keep mental track of where we are in the code. Nested if statements, chained ternary operations, loops or deep JSX markup are the common cause of increased indentation. While not necessarily bad or problematic, working with deeply nested code often leaves us wandering where we actually are in the file.
To be fair this is something that I try to avoid no matter what language or framework I am working with. Flat and linear code is, in my opinion, much easier to understand and more pleasant to the eye.
How to avoid it?
If we want to decrease the level of nesting we can easily to that by increasing the level of abstraction.
The first approach that we can take is to extract the nested functionality into a separate component. My reasoning is that the higher the indentation the more specific the markup gets for a particular part of the UI. If we need to provide great details about a particular piece of the page then we should move it in a separate file and let it breathe there.
Indentation inside class methods can also increase really fast. A change in the code style can easily alleviate that. Just removing unnecessary else statements and flattening long promise chains with async/await can have a big impact.
Lack of a Clear Standard
The JavaScript ecosystem is dynamic. Different members of the community hold different opinions and they tend to hold them firm.
Often, developers join the team with different coding styles and habits. While diversity can sometimes lead to better solutions, often those cultural clashes lead to low code quality. The different paradigms, coding styles and formatting preferences make our code base look like the React equivalent of a cosmopolitan city from a Sci-Fi movie.
This often has the biggest strain on developers since we will have to do a mindset shift depending on what parts of the app we are currently working on. Not sticking to a particular coding standard and set of best practices leads to frustration, tension and unnecessary time spent on discussing whether semicolons are necessary or not.
How to avoid it?
No matter if you are the solo developer of a small web app or part of a bigger team, sticking to a set of practices will have the highest impact towards your productivity and code quality. It’s important to understand that no code standard will leave everyone on the team happy. Just pick something and go with it.
Prettier is an excellent code formatting tool that you can run in your editor or as a commit hook. It doesn’t give you too much configuration options which is actually good — less things to fight about. It can also be used with style guides like Standard Style or the one AirBnB uses.
TL;DR
- Code smells are not identified problems. They are a suggestion that something may be done better.
- Increasing the level of abstraction when necessary could make component code a lot cleaner.
- Don’t reach a state where a refactor is too big of a burden. Code will periodically need to be changed — refactor often.
- Bad coding style and practices could sometimes cause more problems than the design patterns we misuse.
Newsletter
Hey, I’m running a small newsletter in which I share random thoughts, musings and insights about software development. No tutorials, no ads. Just some things I’ve found worthy of pondering delivered to your inbox every few weeks. If this sounds appealing to you you can subscribe HERE.
Clap and Share
If the content of this article was helpful to you I’d appreciate if you hold down the clap button for a bit. This way it will reach and help more people. Share it with friends and colleagues that may find it useful and send any feedback my way!