If you’re a developer, chances are you’ve heard of code smells. Something in the code that might indicate a problem with its design that should be refactored. Code smells were codified in Martin Fowler’s book Refactoring: Improving the Design of Existing Code. It’s a fantastic read with lots of concrete examples and suggested treatments.

While I think most of us recognize code smells, there’s something about working with React components that I’ve found make it harder to recognize them. JSX often deceives us: it looks like HTML, so our brains want to skip over it like so much markup. But, in truth, components are much more than that. They are objects that can have behavior and state. They are JavaScript that must be executed.

With that in mind, I’m going to rethink the classic code smells through the React lens to try to rewire my brain to better identify opportunities for refactoring.

Let’s Talk About Bloaters

The first category of code smells I’m going to look at are called Bloaters. These smells tend to make software hard to work with. They represent pieces of code that have grown so large they become hard to reason about and maintain.

This is usually a slow process. Code starts out small and well-isolated, but grows over time as new functionality is added and code is made more general. Dealing with these code smells, like most refactoring, is usually best done one small step at a time.

The good news is that if you can identify any of the smells below in your components you can make your code cleaner with the same kind of gradual changes that caused the problems in the first place.

So, let’s dig in.


Large Component

A component contains too much state, too many methods, or too many lines of code. Components usually start out small, but over time they collect more and more responsibilities until they become unworkable.

What makes it a smell?

Large components are problematic for a few reasons:

They are hard to test. Think about all the scenarios you have to account for to provide a reasonably thorough test. All the variations of state, all the methods, all the ways the JSX could be broken.

They are hard to read and understand. A large component even if functional and stateless is trying to represent many concepts in its JSX. Sifting through a bunch of nested divs with funky class names is daunting.

They are hard to change. Which line in this behemoth do you need to work with? Did you find every place in the code that you need to change? The more you have to think to make the change, the more likely you’ll miss something.

They are risky to change. The component is responsible for more than just one thing, so it’s likely there’s a fair amount going on that you won’t need to change. Whenever code changes in the component it’s putting all the unrelated functionality at risk.

They tend to change often. Each responsibility in a component represents at least one more reason to change. Each of these serves as a magnet for churn, and each of those changes could result in a bug.

Treatments

The simplest option for treating a Large Component is to decompose by extracting one or more components. Options for extraction include:

  • Code that changes for different reasons.
  • Code that changes at different rates.
  • Code that represents an abstract concept.
  • Code that operates on a part of the state that the rest of the component isn’t concerned with.
  • Code that’s controlled by certain props that aren’t used by other parts of the component.

Let’s look at an example

Our <UserProfile /> component needs to orchestrate two sections: account settings and notifications. But those two sections will change for different reasons, and likely at different rates (it’s entirely probable that the business will want to change how notifications are displayed separately from how email preferences are controlled). Props also offer a clue here, only the second glob of divs is using the `notifications` prop.

These seem like two different concerns. We can alleviate this smell by applying the extract component refactoring:

 

 

Our render function is operating at one level of abstraction. It’s much easier to determine what it’s rendering. It would also be a lot easier to write a unit test for this component at this point. Moreover, we’ve gotten rid of the notifications prop since the new component’s container can handle grabbing those on its own.

The concerns are nicely separated and the changes can be made to either of the new components without messing around with their parent.

Next up we’ll take a look at what happens when you find yourself passing the same props around.


Prop Clumps

Different components contain very similar groups of variables. Alternatively, when the same variables are always passed around together. If the variables don’t make sense apart from each other, you probably have a prop clump.

What makes it a smell?

They create duplication. Passing the values around together is duplicative. Changes to any will need to be made in potentially many places.

They hide abstractions. Sometimes abstracting a group of variables into their own object can greatly enhance the readability of the code by making the working concept more clear.

They make component interfaces harder to use. Just like with functions, components which accept more props have an API that’s more complex. That complexity can be moved so that it doesn’t have to be considered as part of the component’s instantiation.

Treatment

Extract object. Create a new object to represent the concept shared by the variables. Take advantage of prop destructuring to keep the caller clean.

The classic example is a date range. You have a from date and an end date. The values end up getting passed around together because they really don’t make sense apart from one another.

Let’s look at an example

These two variables can be combined into a new dateRange object.

If you’re using TypeScript this may be a good opportunity to create a new type which adds a lot of clarity.


Long Props List

A component accepts (and may require) many props in order to function.

This often happens as a result of trying to generalize a component to be used in different contexts. Keeping things DRY by reusing code is great, but not if it creates the wrong abstraction in order to do so.

What makes it a smell?

They are hard to test. Writing a test for a component with a long props list requires a lot of mocking, stubbing, and/or setup just to get things in a state where they can be tested. Even worse, you have a vast number of permutations of props combinations you must write specs for just to make sure the component does what you expect under varying circumstances.

They are hard to use. Components that are hard to test are often hard to use. Any place in the code that would use such a component must have knowledge of a huge number of things in order to render the component.

They (often) violate SRP. The Single Responsibility Principle states that a unit of code should have one reason to change. Having many props is a good indicator that a component does too much and has too many reasons to change, which leads to churn and bugs in the future.

Treatments

Preserve the object being passed in. If the props being passed in are mostly fields of a single object, the solution may be to simply pass in the whole object instead. Let the component’s internals ask the object for what it needs, rather than loading everything in the props list.

Breakup the component. If different parts of the component are operating on different props or have different reasons to or rates of change, the solution might be to break the component up into multiple smaller ones.

Create specialized implementations of the component. When we’ve generalized a component because different use cases appeared to be similar at first, but their needs grew apart over time, it may be time to simply create completely different components for each purpose.


Long Function

A component function has many lines and statements. In other words, it’s doing many things. In general, if a function has more than ten lines of code you should consider whether it’s emitting this odor.

Functions might not start their life out as behemoths, but they tend to grow over time. You need that function to do just one more thing. After all, it’s only a couple lines of code – so you toss it in and call it a day. Repeat this process every day and over time functions grow into monsters.

What makes it a smell?

They are hard to test.  If a function is executing a lot of code, you’re going to need to write a large number of test cases to adequately exercise all the paths.

They violate SRP. Long functions tend to have more than one reason to change. Change is also likely to occur at different rates for different parts of the code, which means you’re going to risk breaking stable code every time you change the functions more volatile code.

They tend to have side effects. Like variables and classes, good names are crucial for clearly communicating the meaning and intent of a function’s code. If a function is long it’s name is unlikely to clearly reflect what it does, which makes it hard to reuse and prone to do things its caller was not expecting. Those side effects often introduce bugs.

They are hard to read and reason about. It’s hard to understand what a long function, with all its parts operating at different levels of abstraction, is really doing. You need to try to grasp the whole thing every time you make a change to any part of it, which consumes time. And you read code many more times then you write it. Beyond that, each time you read it you have more opportunities to get something wrong.

Treatment

Extract function. Fortunately, treating large functions is simple. Just extract new functions out of them and give them clear names that explain what they do. If you find that you’re passing around the same parameters to a bunch of new functions, consider creating a parameter object to represent the concept, or promoting the parameters to fields or state.


Primitive Obsession

When primitive types (strings, numbers, arrays, etc.) are used to represent more abstract concepts you have Primitive Obsession.

What makes it a smell?

They hide the intent of the code. What’s this string represent again? Oh yeah, it’s a phone number. Oh, and this array is an address. I think. Wait… These primitives are hiding an abstraction that could be made to communicate exactly what the code represents, plus they lack any real encapsulation.

They tend to accumulate related fields. Street address, city, state, and other address fields tend to move around together. Without encapsulation, there’s nothing protecting them from external manipulation. Plus, the user has to remember how (and that) the fields relate to each other.

They require changes in many places. If you’re representing an abstract concept with a primitive and you need to make a change, you now have to change every place in the code that was trying to represent that concept.

Treatment

Introduce a value object. Whip up a new object or class to hold the data and behavior. By making a custom type you can define a sensible interface and concentrate related fields and functions together. You can also clearly communicate an abstraction so users don’t have to worry about more concrete details.

Let’s look at an example

In this example we have a component taking several primitive type props. It becomes apparent that all of these pieces of data only make sense together, and in fact represent some concept that’s not being expressed very well. We actually see both primitive obsession along with data clumps. That’s ok, it’s actually pretty common to see code smells hanging out together.

One way we can deal with these smells is to create a new object represent the deeper meaning of the data we’re passing around. Enter, a LineItem:

By creating an object to contain our data, we have a natural place to put additional logic that keeps it isolated from the rest of the code base. Notice a benefits of this new model:

  1. We keep all our fields together.
  2. We make it clear what they represent.
  3. We can validate our data.
  4. We can provide sensible defaults for some data (e.g., quantity and currency).
  5. We can centralize the presentation logic. In other words, our LineItem knows how to represent itself as a string.

Now, our calling location looks like this:

Our intentions are much clearer now.

Note, however, that the overall complexity of the code base has increased. Refactoring sometimes does that: shifting, rather than removing, complexity. Knowing that cost I wouldn’t recommend introducing an abstraction like this until you have at least a couple places in the code you could clean up with it.

With abstractions, as with de-duplication, I like to follow the rule of three: clean it up when you see an issue in three or more places. That helps reduce the risk of over-complicating the architecture or introducing the wrong abstraction by trying to abstract too early.


Wrapping Up

Bloaters are slow-growing code smells that gradually make code hard to work with, read, and test. By applying small refactorings you can get rid of these smells one small step at a time.

Not time, we’ll go over a category of smells called Object-Orientation Abusers and look at how they apply even in more functional scenarios.

As always, thanks for reading.