React Code Review - Unraveling A Tightly Coupled Component & Mixed Responsibilities (Incl Video)
Tags: react, javascript, webdev, frontend
Published: 2024-03-09 13:05:00

---
description: It’s not easy to spot! In this code review we see how two components are tightly coupled leading to mixed responsibilities. A better function name and some small changes lead to a clear separation of concerns.
---

This blog post is a code review of a pull request from a student of the [React Job Simulator](https://profy.dev/). 

We’ll see how the filter component in the screenshot below is tightly coupled to the pagination of its parent component. 

After the code review, we’ll refactor the component to unravel this coupling so each component focuses on its own responsibilities.

## Table Of Contents

1. [The Task](#the-task)
2. [The Code](#the-code)
3. [Tight Coupling Of The Filter Component To Its Parent](#tight-coupling-of-the-filter-component-to-its-parent)
4. [Refactoring The Filter Component](#refactoring-the-filter-component)
5. [Refactoring The Parent Component](#refactoring-the-parent-component)
6. [Summary](#summary)

If you'd rather watch the video version of this code review here you go.

{% embed https://youtu.be/Sys4Gvu_9lY %}

## The Task

![Screenshot of application](https://media.graphassets.com/83CXVuknQNCMxBSBrQws)

The page in the screenshot above shows a list of issues. This list is paginated, meaning it only shows some of the issues immediately. The user needs to navigate to the next page of the list to see the next set of issues. 

The student's task here was to implement the filter at the top.

The filter has three fields. Two select inputs, so the user can filter for a certain issue status and level, plus a text input that allows filtering for a project name. 

## The Code

This is the Filter component as it was implemented by my student.

```typescript
import { useEffect, useState } from "react";
import { Level, Status } from "@api/issues.types";
import { useFilter } from "../context/filter-context";
import styles from "./filter.module.scss";

interface FilterProps {
  navigateToPage: (
    newPage: number,
    status: Status,
    Level: Level,
    projectName: string,
  ) => void;
}

export function Filter({ navigateToPage }: FilterProps) {
  const { setStatus, setLevel, setProjectName, status, level, projectName } =
    useFilter();
  const [name, setName] = useState(projectName);

  useEffect(() => {
    if (projectName === "") setName("");
  }, [projectName]);

  return (
    <form
      className={styles.filter}
      onSubmit={(e) => {
        e.preventDefault();
        if (setStatus && setLevel && setProjectName) {
          setStatus(status);
          setLevel(level);
          setProjectName(projectName);
        }
        navigateToPage(1, status, level, name);
      }}
    >
			{/* THE FILTER INPUTS */}
		</form>
	);
}
```

The code looks solid. Let’s start dissecting it step by step.

```typescript
const { setStatus, setLevel, setProjectName, status, level, projectName } =
	useFilter();
```

The component uses a custom hook that returns the filter values and allows updating them. This is a great approach as all the logic for handling the filters is encapsulated in this hook. The component doesn’t care whether the values are stored in a local state, context, or the URL.

Let’s move to the next lines:

```typescript
const [name, setName] = useState(projectName);

useEffect(() => {
  if (projectName === "") setName("");
}, [projectName]);
```

For some reason, the project name filter is stored in a separate state variable, which is initialized with a value returned from the custom hook. The state is also updated by the `useEffect`, specifically when the project name is empty. I'm not sure why this is necessary, so usually I’d dig a bit deeper. But since it's not the main concern of this review, let's ignore it here.

```typescript
return (
  <form
    className={styles.filter}
    onSubmit={(e) => {
      e.preventDefault();
      if (setStatus && setLevel && setProjectName) {
        setStatus(status);
        setLevel(level);
        setProjectName(projectName);
      }
      navigateToPage(1, status, level, name);
    }}
  >
		{/* THE FILTER INPUTS */}
	</form>
);
```

The filter inputs are wrapped in a form with a submit handler attached. The handler updates the filter values by calling the functions returned by the custom hook. After updating the filter values, the `navigateToPage` function is called. It sets the page to 1 and passes the new filter values.

So it seems that this code is responsible for resetting the pagination to the first page of the issue list whenever the filter values change. 

## Tight Coupling Of The Filter Component To Its Parent

Since the `navigateToPage` function is a prop, this is where the `Filter` component is coupled to its parent. Besides this function, there's nothing that connects the `Filter` component to the pagination of the issue list.

If you think about it there’s actually no reason why resetting the pagination should be the responsibility of the Filter component. It doesn’t “know” that it’s used on a paginated page. And it should be possible to use it on a page without pagination.

[![The React Job Simulator](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/8bupb5hadajo1ok905li.png)](https://profy.dev)

## Refactoring The Filter Component

The idea of the following refactoring is that the resetting of the pagination should be the responsibility of the parent. This again will decouple the filter component from the rest of the page. 

The first step is to rename the prop. `onFilterChange` is a much more neutral name that moves the responsibility for any action triggered by a filter change to the parent. 

```typescript
export function Filter({ onFilterChange }: FilterProps) {

return (
  <form
    onSubmit={(e) => {
      // ...
      onFilterChange(1, status, level, name);
    }}
  >
```

The new name already suggests that we should only pass the filters to the onFilterChange callback. So let's remove the new page param. 

```typescript
<form
  onSubmit={(e) => {
    // ...
    onFilterChange(status, level, name);
  }}
>
```

Now we decoupled the pagination functionality from the filter updates in this component. 

It might look like a small change, but the filter component is now much more "pure". We could use it now in other places that have nothing to do with pagination. 

My next suggestion is to group the filters in one object instead of splitting them into separate parameters.

```typescript
<form
  onSubmit={(e) => {
    // ...
    onFilterChange({ status, level, name });
  }}
>
```

This way it'll be much easier to add new filter parameters in the future. 

This change also enables another valuable refactoring. We can introduce a shared `Filters` type that can be reused by other parts of the app.

```typescript
interface Filters {
  status: Status;
  level: Level;
  projectName: string;
}

interface FilterProps {
  onFilterChange: ({ status, level, projectName }: Filters) => void;
}

```

I added the type to the component file here, but I'd recommend moving it to a higher level. This way it's obvious that it's a shared type and can be used by other parts of the app and different layers like the API requests as well. 

## Refactoring The Parent Component

Finally, we need to adjust the parent component as well. In this case, it's the `IssueList` component. 

```typescript
import { useRouter } from "next/router";
import type { Level, Query, Status } from "@api/issues.types";
import { Filter } from "../filter";
import styles from "./issue-list.module.scss";

export function IssueList() {
  const router = useRouter();

  const page = Number(router.query.page || 1);
  const navigateToPage = (
    newPage: number,
    status: Status,
    level: Level,
    projectName: string,
  ) => {
    const query: Query = { page: newPage };
    if (status) query.status = status;
    if (level) query.level = level;
    if (projectName) query.project = projectName;

    router.push({
      pathname: router.pathname,
      query: { ...query },
    });
  };

  return (
    <div>
      <div className={styles.issueOptions}>
        <Filter onFilterChange={navigateToPage} />
      </div>
			{/* THE REST OF THE COMPONENT HERE */}
```

The first change is to use the shared `Filter` type. 

```typescript
import { Filter, type Filters } from "../filter";

export function IssueList() {
  // ...

  const navigateToPage = (
    newPage: number,
    filters: Filters,
  ) => {
    const { status, level, projectName } = filters;
    const query: Query = { page: newPage };
    if (status) query.status = status;
    if (level) query.level = level;
    if (projectName) query.project = projectName;

    router.push({
      pathname: router.pathname,
      query: { ...query },
    });
  };
```

You can see that it's much neater, as we don't have to redefine the type of the filters all over again.

The next problem is that the `navigateToPage` function isn’t just updating the page but the filter values in the URL as well. That should be the responsibility of the `Filter` component or the custom `useFilters` hook. So let’s adjust that.

```typescript
const navigateToPage = (newPage: number) => {
  router.push({
    pathname: router.pathname,
    query: { 
			...router.query,
			page: newPage,
		},
  });
};
```

Now this looks a lot cleaner. The `navigateToPage` function really only updates the page while copying the existing URL query parameters.

The last step is to adjust the props we pass to the Filter component. 

```typescript
<Filter onFilterChange={navigateToPage} />
```

We can't directly pass the navigateToPage function anymore. Instead, we use an inline function that accepts the filters and passes them onto navigateToPage, along with a new page value of 1. 

```typescript
<Filter onFilterChange={() => navigateToPage(1)} />
```

Again, here we can see that we introduced a clear separation of concerns.

The filter component is responsible for handling the filters, while the IssueList component is responsible for any action after the filters are updated, like resetting the page to 1.

## Summary

In this code review, we saw that the filter component was tightly coupled to the pagination of the issue list. These couplings aren’t easy to spot. And in this case, the refactoring wasn't huge. But removing unnecessary coupling in your code can have huge effects on maintainability in the long run. 

I hope you enjoyed this small code review and refactoring session. I’m planning to turn this into a series of code reviews. So if you're interested in getting a review of your own code, leave a comment with a screenshot of the code or a link to a GitHub repository or a pull request below the video mentioned at the top.

And, if you want to get hands-on experience on a production-grade codebase and professional workflows, join the React Job Simulator.

[![The React Job Simulator](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/8bupb5hadajo1ok905li.png)](https://profy.dev)