7 code smells in your React components
Tags: react, javascript, webdev
Published: 2020-11-09 20:37:41

---
title: 7 code smells in your React components
published: true
description: Code smells to avoid in your React components
tags: react,javascript,webdev
canonical_url: https://antongunnarsson.com/react-component-code-smells/
---

A growing collection of things I consider code smells in React components.

- [Too many props](#too-many-props)
- [Incompatible props](#incompatible-props)
- [Copying props into state](#props-in-state)
- [Returning JSX from functions](#jsx-returns)
- [Multiple booleans for state](#multiple-booleans)
- [Too many useState in a component](#many-usestate)
- [Large useEffect](#large-useeffect)

## <a id="too-many-props"></a> Too many props

Passing too many props into a single component may be a sign that the component should be split up.

How many are too many you ask? Well.. "it depends". You might find yourself in a situation where a component have 20 props or more, and still be satisfied that it only does one thing. But when you do stumble upon a component that has many props or you get the urge to add _just one more_ to the already long list of props there's a couple of things to consider:

#### Is this component doing multiple things?

Like functions, components should do _one thing well_ so it's always good to check if it's possible to split the component into multiple smaller components. For example if the component has [incompatible props](#incompatible-props) or [returns JSX from functions](#jsx-returns).

#### Could I use composition?

A pattern that is very good but often overlooked is to compose components instead of handling all logic inside just one. Let's say we have a component that handles a user application to some organization:

```jsx
<ApplicationForm
  user={userData}
  organization={organizationData}
  categories={categoriesData}
  locations={locationsData}
  onSubmit={handleSubmit}
  onCancel={handleCancel}
  ...
/>
```

Looking at the props of this component we can see that all of them are related to what the component does, but there's still room to improve this by moving some of the components responsibility to its children instead:

```jsx
<ApplicationForm onSubmit={handleSubmit} onCancel={handleCancel}>
  <ApplicationUserForm user={userData} />
  <ApplicationOrganizationForm organization={organizationData} />
  <ApplicationCategoryForm categories={categoriesData} />
  <ApplicationLocationsForm locations={locationsData} />
</ApplicationForm>
```

Now we've made sure that the `ApplicationForm` only handles its most narrow responsibility, submitting and canceling the form. The child components can handle everything related to their part of the bigger picture. This is also a great opportunity to use [React Context](https://reactjs.org/docs/context.html) for the communication between the children and their parent.

#### Am I passing down many 'configuration'-props?

In some cases, it's a good idea to group together props into an options object, for example to make it easier to swap this configuration. If we have a component that displays some sort of grid or table:

```jsx
<Grid
  data={gridData}
  pagination={false}
  autoSize={true}
  enableSort={true}
  sortOrder="desc"
  disableSelection={true}
  infiniteScroll={true}
  ...
/>
```

All of these props except `data` could be considered _configuration_. In cases like this it's sometimes a good idea to change the `Grid` so that it accepts an `options` prop instead.

```jsx
const options = {
  pagination: false,
  autoSize: true,
  enableSort: true,
  sortOrder: 'desc',
  disableSelection: true,
  infiniteScroll: true,
  ...
}

<Grid
  data={gridData}
  options={options}
/>
```

This also means that it's easier to exclude configuration options we don't want to use if we're swapping between different `options`.

## <a id="incompatible-props"></a> Incompatible props

Avoid passing props that are incompatible with each other.

For instance, we might start by creating a common `<Input />` component that is intended to just handle text, but after a while we also add the possibility to use it for phone numbers as well. The implementation could look something like this:

```jsx
function Input({ value, isPhoneNumberInput, autoCapitalize }) {
  if (autoCapitalize) capitalize(value)

  return <input value={value} type={isPhoneNumberInput ? 'tel' : 'text'} />
}
```

The problem with this is that the props `isPhoneNumberInput` and `autoCapitalize` don't make sense together. We can't really capitalize phone numbers.

In this case the solution is probably to break the component up into multiple smaller components. If we still have some logic we want to share between them, we can move it to a [custom hook](https://reactjs.org/docs/hooks-custom.html):

```jsx
function TextInput({ value, autoCapitalize }) {
  if (autoCapitalize) capitalize(value)
  useSharedInputLogic()

  return <input value={value} type="text" />
}

function PhoneNumberInput({ value }) {
  useSharedInputLogic()

  return <input value={value} type="tel" />
}
```

While this example is a bit contrived, finding props that are incompatible with each other is usually a good indication that you should check if the component needs to be broken apart.

## <a id="props-in-state"></a> Copying props into state

Don't stop the data flow by copying props into state.

Consider this component:

```jsx
function Button({ text }) {
  const [buttonText] = useState(text)

  return <button>{buttonText}</button>
}
```

By passing the `text` prop as the initial value of useState the component now practically _ignores_ all updated values of `text`. If the `text` prop was updated the component would still render its first value. For most props this is unexpected behavior which in turn makes the component more bug-prone.

A more practical example of this happening is when we want to derive some new value from a prop and especially if this requires some slow calculation. In the example below, we run the `slowlyFormatText` function to format our `text`-prop, which takes a lot of time to execute.

```jsx
function Button({ text }) {
  const [formattedText] = useState(() => slowlyFormatText(text))

  return <button>{formattedText}</button>
}
```

By putting it into state we've solved the issue that it will rerun unnecessarily but like above we've also stopped the component from updating. A better way to solving this issue is using the [useMemo hook](https://reactjs.org/docs/hooks-reference.html#usememo) to _memoize_ the result:

```jsx
function Button({ text }) {
  const formattedText = useMemo(() => slowlyFormatText(text), [text])

  return <button>{formattedText}</button>
}
```

Now `slowlyFormatText` only runs when `text` changes and we haven't stopped the component from updating.

> Sometimes we do need a prop where all updates to it are ignored, e.g. a color picker where we need the option to set an initially picked color but when the user has picked a color we don't want an update to override the users choice. In this case it's totally fine to copy the prop into state, but to indicate this behavior to the user most developers prefix the prop with either initial or default (`initialColor`/`defaultColor`).

Further reading: [Writing resilient components by Dan Abramov](https://overreacted.io/writing-resilient-components/).

## <a id="jsx-returns"></a> Returning JSX from functions

Don't return JSX from functions inside a component.

This is a pattern that has largely disappeared when function components became more popular, but I still run into it from time to time. Just to give an example of what I mean:

```jsx
function Component() {
  const topSection = () => {
    return (
      <header>
        <h1>Component header</h1>
      </header>
    )
  }

  const middleSection = () => {
    return (
      <main>
        <p>Some text</p>
      </main>
    )
  }

  const bottomSection = () => {
    return (
      <footer>
        <p>Some footer text</p>
      </footer>
    )
  }

  return (
    <div>
      {topSection()}
      {middleSection()}
      {bottomSection()}
    </div>
  )
}
```

While this might feel okay at first it makes it hard to reason about the code, discourages good patterns, and should be avoided. To solve it I either inline the JSX because a large return isn't _that big_ of a problem, but more often this is a reason to break these sections into separate components instead.

> Remember that just because you create a new component you don't have to move it to a new file as well. Sometimes it makes sense to keep multiple components in the same file if they are tightly coupled.

## <a id="multiple-booleans"></a> Multiple booleans for state

Avoid using multiple booleans to represent a components state.

When writing a component and subsequently extending the functionality of the component it's easy to end up in a situation where you have multiple booleans to indicate which state the component is in. For a small component that does a web request when you click a button you might have something like this:

```jsx
function Component() {
  const [isLoading, setIsLoading] = useState(false)
  const [isFinished, setIsFinished] = useState(false)
  const [hasError, setHasError] = useState(false)

  const fetchSomething = () => {
    setIsLoading(true)

    fetch(url)
      .then(() => {
        setIsLoading(false)
        setIsFinished(true)
      })
      .catch(() => {
        setHasError(true)
      })
  }

  if (isLoading) return <Loader />
  if (hasError) return <Error />
  if (isFinished) return <Success />

  return <button onClick={fetchSomething} />
}
```

When the button is clicked we set `isLoading` to true and do a web request with fetch. If the request is successful we set `isLoading` to false and `isFinished` to true and otherwise set `hasError` to true if there was an error.

While this technically works fine it's hard to reason about what state the component is in and it's more error-prone than alternatives. We could also end up in an "impossible state", such as if we accidentally set both `isLoading` and `isFinished` to true at the same time.

A better way to handle this is to manage the state with an "enum" instead. In other languages enums are a way to define a variable that is only allowed to be set to a predefined collection of constant values, and while enums don't technically exist in Javascript we can use a string as an enum and still get a lot of benefits:

```jsx
function Component() {
  const [state, setState] = useState('idle')

  const fetchSomething = () => {
    setState('loading')

    fetch(url)
      .then(() => {
        setState('finished')
      })
      .catch(() => {
        setState('error')
      })
  }

  if (state === 'loading') return <Loader />
  if (state === 'error') return <Error />
  if (state === 'finished') return <Success />

  return <button onClick={fetchSomething} />
}
```

By doing it this way we've removed the possibility for impossible states and made it **much** easier to reason about this component. Finally, if you're using some sort of type system like TypeScript it's even better since you can specify the possible states:

```ts
const [state, setState] = useState<'idle' | 'loading' | 'error' | 'finished'>('idle')
```

## <a id="many-usestate"></a> Too many useState

Avoid using too many `useState` hooks in the same component.

A component with many `useState` hooks is likely doing Too Many Things™️ and probably a good candidate for breaking into multiple components, but there are also some complex cases where we need to manage some complex state in a single component.

Here's an example of how some state and a couple of functions in an autocomplete input component could look like:

```jsx
function AutocompleteInput() {
  const [isOpen, setIsOpen] = useState(false)
  const [inputValue, setInputValue] = useState('')
  const [items, setItems] = useState([])
  const [selectedItem, setSelectedItem] = useState(null)
  const [activeIndex, setActiveIndex] = useState(-1)

  const reset = () => {
    setIsOpen(false)
    setInputValue('')
    setItems([])
    setSelectedItem(null)
    setActiveIndex(-1)
  }

  const selectItem = (item) => {
    setIsOpen(false)
    setInputValue(item.name)
    setSelectedItem(item)
  }

  ...
}
```

We have a `reset` function that resets all of the state and a `selectItem` function that updates some of our state. These functions both have to use quite a few state setters from all of our `useState`s to do their intended task. Now imagine that we have a lot of more actions that have to update the state and it's easy to see that this becomes hard to keep bug free in the long run. In these cases it can be beneficial to manage our state with a `useReducer` hook instead:

```jsx
const initialState = {
  isOpen: false,
  inputValue: "",
  items: [],
  selectedItem: null,
  activeIndex: -1
}
function reducer(state, action) {
  switch (action.type) {
    case "reset":
      return {
        ...initialState
      }
    case "selectItem":
      return {
        ...state,
        isOpen: false,
        inputValue: action.payload.name,
        selectedItem: action.payload
      }
    default:
      throw Error()
  }
}

function AutocompleteInput() {
  const [state, dispatch] = useReducer(reducer, initialState)

  const reset = () => {
    dispatch({ type: 'reset' })
  }

  const selectItem = (item) => {
    dispatch({ type: 'selectItem', payload: item })
  }

  ...
}
```

By using a reducer we've encapsulated the logic for managing our state and moved the complexity out of our component. This makes it much easier to understand what is going on now that we can think about our state and our component separately.

> Both `useState` and `useReducer` come with their [pros, cons](https://kentcdodds.com/blog/should-i-usestate-or-usereducer) and different use cases (pun intended). One of my favorites with reducers is the [state reducer pattern by Kent C. Dodds](https://kentcdodds.com/blog/the-state-reducer-pattern-with-react-hooks).

## <a id="large-useeffect"></a> Large useEffect

Avoid large `useEffect`s that do multiple things. They make your code error-prone and harder to reason about.

A mistake that I made a lot when hooks were released was putting too many things into a single `useEffect`. To illustrate, here's a component with a single `useEffect`:

```jsx
function Post({ id, unlisted }) {
  ...

  useEffect(() => {
    fetch(`/posts/${id}`).then(/* do something */)

    setVisibility(unlisted)
  }, [id, unlisted])

  ...
}
```

While this effect isn't that large it still do multiple things. When the `unlisted` prop changes we will fetch the post even if `id` hasn't changed.

To catch errors like this I try to describe the effects I write by saying "when `[dependencies]` change do _this_" to myself. Applying that to the effect above we get "when `id` **or** `unlisted` changes, fetch the post **and** update visibility". If this sentence contains the words "**or**" or "**and**" it usually points to a problem.

Breaking this effect up into two effects instead:

```jsx
function Post({ id, unlisted }) {
  ...

  useEffect(() => { // when id changes fetch the post
    fetch(`/posts/${id}`).then(/* ... */)
  }, [id])

  useEffect(() => { // when unlisted changes update visibility
    setVisibility(unlisted)
  }, [unlisted])

  ...
}
```

By doing this we've reduced the complexity of our component, made it easier to reason about and lowered the risk of creating bugs.

## Wrapping up

All right, that's all for now! Remember that these by any means aren't rules but rather signs that something _might_ be "wrong". You'll definitely run into situations where you want to do some of the things above for good reason.

Got any feedback on why I'm _very wrong_ about this? Suggestions for other code smells that you've stumbled upon in your components? Write a comment or hit me up on [Twitter](https://twitter.com/awnton)!