React Component code smells

Translator: Tashi

Protocol: CC BY-NC-SA 4.0

What does code smell mean? In short, a code structure that suggests there may be a deeper problem. (Wikipedia)

💩 code smell

• Too much props passing
• Contradiction of props
• Derive state from props
• Return JSX from the function
• Multiple Boolean types of states
• Too many in a single componentuseState
• The hugeuseEffect

Too much props passing

Passing multiple props to a component implies that maybe the component should be split.

How much is too much, you may ask? B: well… “It depends.” You might be faced with a situation where a component has 20 or more props, but you’re still ok because the component only does one thing. However, when you get stuck with a component that has too many props, or when you want to add one props to the “last time” list of props that is already long enough, you might want to think about a few things:

Does this component do multiple things

Like functions, components should only do one thing and do it well, so it’s a good habit to always check if you can break a component into smaller parts. Say the component has inappropriate props or returns JSX from a function.

Can you do combinatorial abstraction?

A good but often overlooked pattern is to compose components rather than handle all logic in one component. Suppose we have a component that processes a user’s application to an organization:

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

We can see that all the props of the component are related to what the component is doing, but there is still room for optimization to move some of the responsibilities of the component to its children

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

At this point, we’ve ensured that ApplicationForm only handles its responsibility for submitting and revoking the form. Child components are good at handling only things that are relevant to themselves globally. This is also a use [React Context] (https://zh-hans.reactjs.org/docs/context.html#gatsby-focus-wrapper) for parent-child communication components.

Did I pass too many “configuration” props

In some scenarios, it is a good idea to aggregate some props into a single object, for example, so that configurations can be swapped more easily. If we have a component of the warrior sort table:

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

These props, except for data, can be considered as configuration items. Faced with this situation, it might be a good idea to change the Grid to accept an Options as prop instead.

const options = {
  pagination: false.autoSize: true.enableSort: true.sortOrder: 'desc'.disableSelection: true.infiniteScroll: true. } <Grid data={gridData} options={options} />Copy the code

This also means that when we want to switch between different options, it’s easy to rule out the ones we don’t want

Contradiction of props

Avoid passing props that appear to conflict with each other

For example, 🌰, we started by creating a component that handles text, but after a while, we wanted it to handle the Input of cell phone numbers. A component implementation might look like this:

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

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

The problem is that the isPhoneNumberInput and autoCapitalize props are logically in conflict. We can’t capitalize cell phone numbers.

In this case, the solution is to split the component into smaller parts. If we still have some logic to reuse, 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" />
}
Copy the code

While this example may seem contrived, finding logically conflicting props is a sign that you should check if the component needs to be split.

Derive state from props

Do not make the correlation between data disappear by deriking state from props.

Think about this component

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

  return <button>{buttonText}</button>
}
Copy the code

By passing text Prop as the initial value of useState, the component now ignores subsequent changes to the Text Prop. Even if the Text prop changes, this component will only render the first value it gets. This is usually an unexpected behavior for most props, making components more buggy.

As a more practical example of this phenomenon, you might want to derive state by doing some calculations on prop. In the example below, we call 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>
}
Copy the code

By putting it in state, we avoid the problem of the function executing repeatedly, but we also make the component unable to respond to updates to the props. A better approach is through the use of [useMemo] (https://zh-hans.reactjs.org/docs/hooks-reference.html#usememo) hook to cache the function as a result, to solve the problem

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

  return <button>{formattedText}</button>
}
Copy the code

SlowlyFormatText now only re-executes when text changes without causing the component to fail to respond to updates

Sometimes we do need to ignore all subsequent updates to a prop, such as a color picker. After we set the initial value of the selected color through the configuration item, we don’t want to update the initial data overwriting the user’s choice when he chooses another color. In this case, copying the value of a prop directly to state is perfectly fine, but to indicate this behavior pattern, most developers will generally prefix the name of the prop, Such as initial or default (initialColor/defaultColor).

Read more: Writing resilient components by Dan Abramov.

Return JSX from the function

Do not use functions to return JSX from within a component

This pattern has largely disappeared since function components became popular, but I come across them from time to time. Here’s 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>)}Copy the code

This code makes it harder for people to quickly understand what is happening compared to a good pattern, and should be avoided. You can solve this by inlining JSX, because a large return value does not mean a large problem, but it makes more sense to split the pieces into different components.

Remember, creating a new component doesn’t mean you have to move it to a new file. It is equally reasonable to place multiple related components in the same file.

Multiple Boolean types of states

Avoid using multiple Booleans to represent a component’s state.

When you are writing a component and often extend its functionality, it is easy to use multiple Booleans to represent the current state of the component. For a button component that makes a network request when clicked, you might have an implementation 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} />
}
Copy the code

When the button is clicked, we set isLoading to true and then fetch the network request. If the request was successful, we set isLoading to false and isFinished to true, otherwise we set hasError to true if there is an error.

Although the component does its job properly, it is difficult to understand the state of the current component and is more error-prone than other solutions. We can also fall into an “illegal state” if we accidentally set isLoading and isFinished to true.

A good solution to this situation is to manage state through enumerations. In other languages, enumerations are a way to define variables that are only allowed to be set to a predefined set of constant values. Strictly speaking, enumerations do not exist in JavaScript, and we can use strings as enumerations to get the associated 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} />
}
Copy the code

In doing so, we avoid the possibility of illegal state and make it easy to identify the current state of the component. Ultimately, it’s better if you’re using some type system like TypeScript, because you can indicate possible states in this way

const [state, setState] = useState<'idle' | 'loading' | 'error' | 'finished'> ('idle')
Copy the code

Too muchuseState

Avoid using too many useState hooks in a single component.

A component with multiple useState hooks is probably doing multiple things, and it’s probably better to split it up into multiple components, but there are some complex cases where we need to manage complex state in a single component

Here is an example of what states and functions look like in an Autocomplete component:

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

  ...
}
Copy the code

We have a reset function to reset all the states, and a selectItem function to update the states. Each of these functions must use some setters returned from the useState hook to perform the related tasks. Now imagine that we have many other operations that need to update the status, and it’s easy to see that in the long run this situation is hard to guarantee without any bugs. Using the useReducer hook to manage state can be a great help in these scenarios.

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

  ...
}
Copy the code

Using reducer, we encapsulated the logic related to state management and isolated the complexity from the components. We can think about our components and states individually, which helps us understand what’s going on.

UseState and useReducer have their own advantages and disadvantages and use scenarios. My favorite reducer pattern is the state Reducer pattern by Kent C. Dodds.

The hugeuseEffect

Avoid use effects that are too large to do many things at once. This makes the code error-prone and hard to understand.

One mistake I often make when releasing hooks is putting too many things in a useEffect. For demonstration purposes, this is a component with only one useEffect:

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

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

  ...
}
Copy the code

While this effect isn’t huge, it still does several things at once. When unlisted Prop is updated, we will also pull the data, although the ID has not changed.

To catch such errors, I try to describe effect by writing or saying “do this when [dependencies] changes.” Applying this pattern to the above effect we can get “pull data and update status when ID or unlisted changes”. If the sentence includes words “and” or “or” it usually indicates that there is a problem.

This effect should be split into two:

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

  ...
}
Copy the code

In doing so, we reduce the complexity of the component, make it easier to understand and reduce the possibility of bugs.

conclusion

Well, that’s all! Remember, these things are not meant to be rules, but rather signals that something might be “wrong”. You will certainly come across situations where you need to use the above method for good reason.