loading...

7 code smells in your React components

awnton profile image Anton Gunnarsson Originally published at antongunnarsson.com ใƒปUpdated on ใƒป9 min read

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

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 or returns JSX from functions.

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:

<ApplicationForm
  user={userData}
  organization={organizationData}
  categories={categoriesData}
  locations={locationsData}
  onSubmit={handleSubmit}
  onCancel={handleCancel}
  ...
/>
Enter fullscreen mode Exit fullscreen mode

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:

<ApplicationForm onSubmit={handleSubmit} onCancel={handleCancel}>
  <ApplicationUserForm user={userData} />
  <ApplicationOrganizationForm organization={organizationData} />
  <ApplicationCategoryForm categories={categoriesData} />
  <ApplicationLocationsForm locations={locationsData} />
</ApplicationForm>
Enter fullscreen mode Exit fullscreen mode

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 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:

<Grid
  data={gridData}
  pagination={false}
  autoSize={true}
  enableSort={true}
  sortOrder="desc"
  disableSelection={true}
  infiniteScroll={true}
  ...
/>
Enter fullscreen mode Exit fullscreen mode

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.

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

<Grid
  data={gridData}
  options={options}
/>
Enter fullscreen mode Exit fullscreen mode

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

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:

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

  return <input value={value} type={isPhoneNumberInput ? 'tel' : 'text'} />
}
Enter fullscreen mode Exit fullscreen mode

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:

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" />
}
Enter fullscreen mode Exit fullscreen mode

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.

Copying props into state

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

Consider this component:

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

  return <button>{buttonText}</button>
}
Enter fullscreen mode Exit fullscreen mode

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.

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

  return <button>{formattedText}</button>
}
Enter fullscreen mode Exit fullscreen mode

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 to memoize the result:

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

  return <button>{formattedText}</button>
}
Enter fullscreen mode Exit fullscreen mode

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.

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:

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>
  )
}
Enter fullscreen mode Exit fullscreen mode

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.

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:

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} />
}
Enter fullscreen mode Exit fullscreen mode

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:

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} />
}
Enter fullscreen mode Exit fullscreen mode

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:

const [state, setState] = useState<'idle' | 'loading' | 'error' | 'finished'>('idle')
Enter fullscreen mode Exit fullscreen mode

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:

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)
  }

  ...
}
Enter fullscreen mode Exit fullscreen mode

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 useStates 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:

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 })
  }

  ...
}
Enter fullscreen mode Exit fullscreen mode

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 and different use cases (pun intended). One of my favorites with reducers is the state reducer pattern by Kent C. Dodds.

Large useEffect

Avoid large useEffects 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:

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

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

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

  ...
}
Enter fullscreen mode Exit fullscreen mode

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:

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])

  ...
}
Enter fullscreen mode Exit fullscreen mode

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!

Discussion

pic
Editor guide
Collapse
jperasmus profile image
JP Erasmus

Excellent list of code-smells! I fully agree with these.

The most subtle one is the functions returning JSX inside a component. That is essentially the definition of a React component, i.e. "function-returning-JSX", which should be a clue to abstract it outside the parent.

The one that I personally learnt the hard-way was too many things in a single useEffect.

Collapse
almostconverge profile image
Peter Ellis

Thank you, that's a neat collection of arguably fishy-smelling patterns.

There is one I haven't quite made up my mind about, and that's something like this:

function SomeComponent(props) {
    const someMoreProp = // calculate some extra stuff
    return <>
          ....
         <OtherComponent {...props} moreProp={someMoreProp}/>
         ...
     </>;
}
Enter fullscreen mode Exit fullscreen mode

I do find this pattern quite useful when I've got different variants of a component (for example a text and a number input component), but I want to expose it as just a single GenericInput component.

But it doesn't feel right.

Collapse
awnton profile image
Anton Gunnarsson Author

Thanks!

If it works it's works, so to speak :) Your example is close to writing a Higher-order component but it depends on the use case. A lot of HOCs use cases however (just as render props) have been replaced by hooks instead, and should probably be the default moving forward unless you have a special use case ^^

Collapse
sfiquet profile image
Sylvie Fiquet

Hi! I really enjoyed your article. The options pattern will help with a component of mine that didn't feel right.

I think the reducer example needs to be edited though:

  • the dispatch type for selectItem is 'reset', I believe it should be 'selectItem' to match the reducer?
  • shouldn't the case for selectItem do the same thing as the function? isOpen and inputValue won't change on their own. Or am I missing something?
    case "selectItem":
      return {
        ...state,
        isOpen: false,
        inputValue: action.payload.name,
        selectedItem: action.payload
      }
Enter fullscreen mode Exit fullscreen mode
Collapse
awnton profile image
Anton Gunnarsson Author

Thank you Sylvie and good catch! A bit too fast with the copy/paste it seems! :D Updated it now! ๐Ÿ™

Collapse
pkellner profile image
Peter Kellner

Hmmm. any other references for this being bad? "Returning JSX from functions". For your trivial example, I'd agree, but sometimes it's not that crazy to do w/o creating separate files. What about them makes them hard to read?

Collapse
awnton profile image
Anton Gunnarsson Author

Yeah, for me it's about reducing the complexity of all components and minimizing context switching but sometimes it might be the right thing to use a function. In my experience though it's better to either inline the JSX or move it to another component in the vast majority of cases. (Altough that component might still live in the same file if it's tightly coupled).

Collapse
glowkeeper profile image
Steve Huckle

The only one I wasn't convinced by was returning JSX. The trivial example you give, sure.

But is it wrong to use a function to create large amounts of JSX programmatically because the JSX you want to render depends on large amounts of varying data (perhaps you've fetched thousands of values from an API)? In that situation, I'd create some JSX elements containing the fetched values in a loop (or equivalent) and push them into an array. Once the array contained all my values, I'd set some state variable to that array, then render that. So not exactly returning JSX, but not that different! Is that wrong? If so, what's the alternative?

Collapse
awnton profile image
Anton Gunnarsson Author

It's never completely wrong if it works :)

In the example you give I wouldn't store the JSX in state, but rather the data needed to create them and then map then data to JSX in render. If there's a lot of complex logic involved in deciding which JSX should be rendered it can often be alleviated by creating smaller components, using composition or other patterns such as render props or custom hooks.

Thanks for questioning! ๐Ÿ™Œ

Collapse
glowkeeper profile image
Steve Huckle

Hmmm - that's probably right - but the state is actually in Redux, I just use the component state as a placeholder for render. I'll have a look at the component in question and see what I can do with it in another iteration.

Good stuff!

Thread Thread
awnton profile image
Anton Gunnarsson Author

Ah yeah give it a try, good luck!

Collapse
dev_nope profile image
Vasile Stefirta ๐Ÿ‡ฒ๐Ÿ‡ฉ โœˆ๏ธ ๐Ÿ‡บ๐Ÿ‡ธ

Hey Anton, really good tips! ๐Ÿ‘

I've been focusing a lot myself on writing not just code "that works" but also clean and maintainable. The "future you" and anyone else touching that code will definitely thank you for that ๐Ÿ™‚

Collapse
awnton profile image
Anton Gunnarsson Author

Thanks! Very much agree with that sentiment!

Collapse
segebee profile image
Segun Abisagbo

this was a great article.

Learnt some new practices.

love the if () return <Component /> style.

reads better then ternary

Collapse
lesleyvdp profile image
Lesley van der Pol

Very interesting post! I'm glad you reminded me of useReducer, I'll definitely be rewriting a thing or two in my client's codebase using it.

Collapse
naijab profile image
Nattapon Pondongnok

๐Ÿ˜ป Its awesome

Collapse
awnton profile image
Anton Gunnarsson Author

Thanks! ๐Ÿ™Œ

Collapse
peaonunes profile image
Rafael Nunes

Great article ๐Ÿฅณ

Collapse
tomotoes profile image
SimonAking

Thank the author for sharing. I have gained a lot.

Can I translate it into Chinese and send it to my blog?
I will indicate your information and original address at the beginning of the article.

Collapse
awnton profile image
Anton Gunnarsson Author

Thanks!
Sure, that would be great! Feel free to send me the link afterwards! :)

Collapse
tomotoes profile image
SimonAking

It took me a day to get it done. The Chinese translation is here. Thank you again for sharing.

tomotoes.com/blog/7-code-smells-in...

Collapse
tomotoes profile image
SimonAking

Ok, after I finish the translation, I will send you the link, so stay tuned~

Collapse
lnaie profile image
Lucian Naie

your applicationForm example is meh. it makes me think you want to keep yourself busy, which I hope is not the goal here

Collapse
awnton profile image
Anton Gunnarsson Author

I'm not sure what you mean with "want to keep yourself busy", could you elaborate? :)

I'm inclined to agree that the example isn't the best though, might update that if I come up with something clearer. Composition however is a fantastic tool for reducing complexity, increasing testability etc!

Collapse
lnaie profile image
Lucian Naie

I meant to create more work and billing hours, also to keep it simple versus over-engineering it. I know, I know, it depends on the context.

Thread Thread
awnton profile image
Anton Gunnarsson Author

Ah, I see. Although I work as a consultant today I would do the same if I weren't getting paid for it :) Composition is great, even if you shouldn't go wild and try to compose everything ^^

Collapse
cetholl_ profile image
Arif RH ๐ŸŽฎ

excellent article, thanks for sharing!

Collapse
glowkeeper profile image
Steve Huckle

There's some good stuff in there, Anton - thanks! I really like the reducer example (is this the end of Redux?). And you can have more than one useEffect! Who knew? :)

Collapse
awnton profile image
Anton Gunnarsson Author

Thanks!

While I personally don't use redux nowadays (I usually reach for Overmind when I need it), there's still a lot of people who use it successfully. Especially with Redux Toolkit which removes so much of the friction people are used to with redux.

Collapse
ogrotten profile image
ogrotten

Excellent info. Good article. Thanks for posting.

Collapse
rajajaganathan profile image
Raja Jaganathan

Thanks for sharing ๐Ÿ‘

Collapse
larsolt profile image
Lars

I love the loading, error, finished example. I tend to use a lot of boolean states

Collapse
marchiartur profile image
aRTUR

In some projects I'm using React, but I need to learn a more solid base for I don't make amateur mistakes like this "large useEffect". Can someone share the way to become a react's king?

Collapse
nadavl profile image
Nadav Lebovitch

Good article! Definitely learned something.