What does code smell mean? In short, it is a code structure that implies that there may be deep-seated problems.

Code smell

• Too many props passes
• Contradictory props
• Derive state from props
• Return JSX from function
• Multiple Boolean States
• There are too many usestates in a single component
• Huge useEffect

Too many props passes

Passing multiple props into a component implies that perhaps the component should be split.

How much do you think is too much? Well... It depends. You may face a situation where a component has 20 or more props, but you still feel no problem because this component only does one thing. However, when you are stuck by a component with too many props, or you are eager to add a props to the long enough props list, you may need to think about the following things:

Does this component do more than one thing

Like functions, components should only do one thing and do it well. Therefore, it is a good habit to always check whether a component can be divided into multiple small components. For example, the component has inappropriate props or returns JSX from the function.

Can I do combinatorial abstraction?

A good but often overlooked pattern is to combine components rather than handle all logic in one component. Let's say that we have a component that processes user applications to an organization:

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

We can see that all props of this component are related to what the component does, but there is still room for optimization. Some responsibilities of the component can be transferred to its children

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

Now, we ensure that ApplicationForm only handles its responsibilities, submitting and withdrawing forms. Subcomponents can be very good at handling only things related to themselves globally. This is also a use React Context To communicate between parent and child components.

Did I pass too many "configuration" props

In some scenarios, it is a good idea to aggregate some props onto a single object. For example, it is easier to exchange configurations. 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}
  ...
/>

Except for data, these props can be considered as some configuration items. In this case, it may be a good idea to change the Grid to accept an option as prop.

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

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

This also means that when we want to switch between different options, it's easy to eliminate what we don't want

Contradictory props

Avoid passing props that appear to conflict with each other

Give an example 🌰， We started by creating a component to handle text, but after a while, we wanted this component to handle the input of mobile phone numbers. The implementation of components may be as follows:

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

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

The problem is that isphone number input and autoCapitalize props are logically conflicting. We can't capitalize the mobile phone number.

In this case, the solution is to split the component into multiple subcomponents. 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" />
}

Although this example seems a little deliberate, finding the logically conflicting props is a sign that you should check whether the component needs to be split.

Derive state from props

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

Think about this component

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

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

By passing text prop as the initial value of useState, this component will now ignore the text prop that changes later. Even if the text prop is changed, this component will only render the value it gets for the first time. For most props, this is usually an unexpected behavior, which makes the component more vulnerable to bug s.
As a more practical example of this phenomenon, you want to perform some calculations based on prop to derive state. In the following example, we call the slowyformattext function to format our text prop. It takes a lot of time to execute this function.

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

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

By putting it into state, we can avoid the problem of repeated function execution, but we also make the component unable to respond to props updates. A better way is to use useMemo hook to cache function results to solve this problem

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

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

Now slowyformattext is only re executed when the text is changed, which will not cause the component to fail to respond to the update

Sometimes we do need to ignore all subsequent updates of a prop, such as a color selector. After we set the initial value of the selected color through the configuration item, when the user selects another color, we do not want to update the initial data that overrides the user's selection. In this case, there is no problem copying the value of prop directly to state. However, in order to indicate this behavior pattern, most developers will generally add some prefixes to the name of prop, such as initial or default (initialcolor / defaultcolor).

Return JSX from function

Do not use functions to return JSX from within a component

This pattern has disappeared on a large scale since functional components became popular, but I sometimes encounter them. Use an example to illustrate 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>
  )
}

You may feel no problem at first. Compared with a good pattern, this code will make it difficult for people to quickly understand what happened. This pattern should be avoided. It can be solved by inline JSX, because a huge return value does not mean a huge problem, but it is more reasonable to split these parts into different components.

Remember, just because you create a new component doesn't mean you have to move it to a new file. It is equally reasonable to put multiple associated components in the same file.

Multiple Boolean States

Avoid using multiple Boolean values to represent the state of a component.

When you are writing a component and often extend its functions, it is easy to use multiple Boolean values to represent the current state of the component. For a button component that makes a network request when clicked, you may have the following implementation:

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 then use fetch to make network requests. If the request is successful, we set isLoading to false and isFinished to true. Otherwise, we set hasError to true if there is an error.
Although the component can work normally, it is difficult to understand the current state of the component, and it is more error prone than other schemes. We may fall into an "illegal state" if we accidentally set isLoading and isFinished to true at the same time.
A good way to solve this problem is to use "enumeration" to manage the state. In other languages, enumeration is a method to define variables. This variable can only be set to a set of predefined constant values. Strictly speaking, enumeration does not exist in JavaScript. We can use strings as enumeration values to obtain related 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} />
}

By doing so, we avoid the possibility of illegal States and make it easy to identify the current state of components. Finally, if you are using a certain type system, such as TypeScript, it is even better, because you can indicate possible states in this way

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

Too many usestates

Avoid using too many useState hooks in a single component.
A component with multiple useState hooks is likely to do multiple things. It may be better to split it into multiple components. However, there are some complex situations. We need to manage various complex states in a single component

Here is an example to demonstrate what the status and functions in an autocomplete component 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)
  }

  ...
}

We have a reset function to reset all States, and a selectItem function to update states. These functions must use some setter s returned from useState hook to complete related tasks. Now imagine that we still have many other operations to update the status, and it is easy to see that in the long run, this situation is difficult to ensure that no bug s will be generated. In these scenarios, using useReducer hook to manage the state can help us a lot.

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 reducer, we encapsulate the logic related to state management and isolate the related complexity from the components. We can think about our components and States individually, which helps us understand what happened.

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

Huge useEffect

Avoid huge useeffects that do many things at once. This makes the code error prone and difficult to understand.

One of the mistakes I often make when hooks is released is that I put too many things in a useEffect. For demonstration, this is a component with only one useEffect:

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

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

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

  ...
}

Although this effect is not huge, it still does many things at the same time. After the unlisted prop is updated, we will also pull the data, although the id has not changed.
In order to catch such errors, I try to write or say "do this when [dependencies] changes" to describe the effect for my own reference. Applying this pattern to the above effect, we can get "pull data and update status when id or unlisted changes". If this sentence includes words "or" or "or" this usually indicates that there is a problem.

This effect should be split into two effects:

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 so, we reduce the complexity of components, make them easier to understand and reduce the possibility of bug s.

summary

Well, that's all! Remember, these things do not mean rules. They are more a signal that something may "go wrong". You will certainly encounter situations where there are good reasons to use the above methods.