Extract React Hook Refactoring

I consider it a code smell when a React functional component has more than 5 lines before its return statement.

Whenever I see this, I immediately start to think about ways to clean this up.

I often use custom React hooks to encapsulate logic and have clear boundaries in my code.

One thing I've noticed is that people are quite hesitant to create custom hooks.

They think that hooks are somehow magical, and it is dangerous to create your own.
Hooks are just functions. Yes, some of the build-in React ones are connected to internals, but still, they are meant to be composed.

Other arguments I have heard against extracting custom hooks are: "I just need this in one place, no need to make it reusable".
Custom hooks can be reusable, but this is not a requirement.

React pushes people to write composable code, which is excellent. But due to having markup in our functions, we get used to long functions.

When you write Ruby, and you have a method which is more than 5-10 lines long, you start thinking about how to split it up and make it more understandable.
This is definitely not true in JavaScript and especially when we write React components, which is a shame. 🤷‍♂️

Why is having a lot of code in one function problematic?

It makes it hard to understand this code. Usually, it hides the connection between the different areas in the code.

The following code is simplified extract from a Pull Request for a new feature of YourStack. We are building a question box with the ability to go to the next or the previous question.

export default function QuestionCard() {
  const [endCursors, setEndCursors] = React.useState([]);

  const { data, refetch, variables } = useQuery(QUERY, {
    variables: { cursor: null },
  });
  if (!data) {
    return null;
  }

  const { questions } = data;
  const question = get(questions, 0);
  if (!question) {
      return null;
  }

  const { endCursor, hasNextPage } = questions.pageInfo;
  const hasPrevious = endCursors.length > 0;

  return (
    <Card>
      {/* code */}
      <Button
        label="previous"
        disabled={!hasPrevious}
        onClick={() => {
          const cursor = endCursors.pop();
          refetch({ cursor });
          setEndCursors(endCursors);
        }}
      />
      <Button
        label="next"
        disabled={!hasNextPage}
        onClick={() => {
          refetch({ cursor: endCursor });
          setEndCursors([...endCursors, variables.cursor || '']);
        }}
      />
      {/* code */}
      <Form.Mutation
        mutation={SUBMIT_FROM}
        onSubmit={() => {
          refetch({ cursor: endCursor });
          setEndCursors([...endCursors, variables.cursor || '']);
        }}>
        {/* code */}
      </Form.Mutation>
      {/* code */}
    </Card>
  );
}

It is quite long. Let's refactor it. 🧐

I would like the whole thing to be a single hook. However, when I refactor, I do it step by step.

First, I will create my new hook useCardQuestion. I usually start with extracting the interface and then implement it. I don't try to make it "pretty", I just try to put everything together.

export default function QuestionCard() {
  const [endCursors, setEndCursors] = React.useState([]);

  // our new hook
  // notice how akward passing both question and questions is
  const { question, questions, refetch } = useQuestionCard()

  // notice we guard agains missing data
  if (!question) {
    return null;
  }

  const { endCursor, hasNextPage } = questions.pageInfo;
  const hasPrevious = endCursors.length > 0;

   // ... rest of the code

The hook itself looks like this:

function useQuestionCard() {
  const { data, refetch, variables } = useQuery(QUERY, {
    variables: { cursor: null },
  });

  if (!data) {
    // it is okay to return empty object here
    // when data is not ready shouldn't be able todo anything.
    return {};
  }

  const questions = questions;

  return { question: get(data.questions, 0), questions, refetch };
}

Next, as mentioned earlier, "notice how awkward passing both question and questions is". The only reason we need the questions is so we can "interrogate" it for the next question logic. We will deal with this in a bit.

Very often, I notice people being more comfortable having function definitions inside a component prop. This often (like in this case) leads to defining the same function twice. It makes it harder to refactor into custom hooks because it seems like every local variable is needed somewhere.

Let's move the custom functions:

export default function QuestionCard() {
  // ... code
  const { endCursor, hasNextPage } = questions.pageInfo;
  const hasPrevious = endCursors.length > 0;

  const nextQuestion = () => {
    refetch({ cursor: endCursor });
    setEndCursors([...endCursors, variables.cursor || '']);
  };

  const previousQuestion = () => {
    const cursor = endCursors.pop();
    refetch({ cursor });
    setEndCursors(endCursors);
  };

  return (
    <Card>
      {/* code */}
      <Button
        label="previous"
        disabled={!hasPrevious}
        onClick={previousQuestion}
      />
      <Button 
        label="next" 
        disabled={!hasNextPage} 
        onClick={nextQuestion} />
      {/* code */}
      <Form.Mutation mutation={SUBMIT_FROM} onSubmit={nextQuestion}>
        {/* code */}
      </Form.Mutation>
      {/* code */}
    </Card>
  );
}

When you refactor your code, you start noticing data dependencies.
In this case, when hasPrevious and hasNextPage are false, we don't need the functions to be defined at all.

Our final version looks like this:

export default function QuestionCard() {
  const { question, previousQuestion, nextQuestion } = useQuestionCard();

  if (!question) {
    return null;
  }

  return (
    <Card>
      {/* code */}
    </Card>
  );
}

This is how the useQuestionCard hook looks like

function useQuestionCard() {
  const [endCursors, setEndCursors] = React.useState([]);
  const { data, refetch, variables } = useQuery(QUERY, {
    variables: { cursor: null },
  });

  if (!data) {
    return {};
  }

  const questions = data.questions;

  const nextQuestion = questions.pageInfo.hasNextPage
    ? () => {
        refetch({ cursor: questions.pageInfo.endCursor });
        setEndCursors([...endCursors, variables.cursor || '']);
      }
    : null;

  const previousQuestion = endCursors.length > 0 ? () => {
    const cursor = endCursors.pop();
    refetch({ cursor });
    setEndCursors(endCursors);
  } : null;

  return {
    question: get(data.questions, 0),
    nextQuestion,
    previousQuestion,
  };
}

I like the final version a lot more.

Testing

One benefit of custom hooks, I didn't mention is testing. Custom hooks make testing your code a lot easier since you can test them in isolation.

I'm working on a separate blog post to cover this subject.

Conclusion

Initially, hooks looked quite weird to me. Nowadays, I see them as a great tool to make my code a lot more understandable.

Try grouping logic into custom hooks every time you see the usage of multiple hooks in your component.

If you have any questions or comments, you can ping me on Twitter.