Refactoring A Junior’s React Code - 43% Less Code With A Better Data Structure
Tags: react, javascript, webdev, frontend
Published: 2022-11-11 12:15:00

---
description: As an (aspiring) Junior React developer it’s hard to know if your code is any good and how to improve it. In this session we review & refactor an interesting problem.
---

As an (aspiring) Junior React developer it’s hard to understand if you’re code is any good and how you can improve it. It would be great to get a review from an experienced dev. But sharing your code takes guts. And the reviews you get in online communities are often shallow.

Recently, a developer was brave enough and requested a code review. Kudos. The feature was interesting. The code wasn’t bad. But there were a few mistakes that are very common among Junior React developers.

So here’s your opportunity to learn from these mistakes. On this page, you can find a code review and a step-by-step refactoring journey. By 

- using a better data structure
- removing unnecessary state and
- a few other refactorings and cleanup

we increase the performance of the code, make it easier to read and maintain, and shorten the component from initially 177 to 102 lines (not that this is a good metric).

If you’re rather a video person you can watch the complete review and refactoring session here.

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

> Note: the code on this page is slightly improved compared to the code in the video using a Set instead of a Map.

## Table Of Contents

1. [The Component To Review](#the-component-to-review)
    1. [The Functionality](#the-functionality)
    2. [The Original Code](#the-original-code)
2. [Problems With The Original Code](#problems-with-the-original-code)
    1. [Problem 1: Sub-Optimal Data Structure](#problem-1-suboptimal-data-structure)
    2. [Problem 2: Not Deriving Variables From State](#problem-2-not-deriving-variables-from-state)
    3. [Problem 3: Directly Accessing The DOM](#problem-3-directly-accessing-the-dom)
    4. [Problem 4: Variable Naming](#problem-4-variable-naming)
3. [The Refactoring](#the-refactoring)
    1. [Step 1: Refactor Data Structure From Array To Object](#step-1-refactor-data-structure-from-array-to-object)
    2. [Step 2: Using Derived State](#step-2-using-derived-state)
    3. [Step 3: Remove document.getElementById()](#step-3-remove-documentgetelementbyid)
4. [Final Code](#final-code)
5. [Takeaways](#takeaways)

## The Component To Review

### The Functionality

The component is a table with a few somewhat complex features. Here’s a quick video:

![](no-optimization::https://media.graphassets.com/DPdsgaPmRtamC7G8hZxm)

The table renders a list of issues. An issue can have a status “open” or “resolved” that is reflected in the last column. Issues with the status “resolved” can’t be selected.

The checkbox at the top is partially checked when some but not all rows are selected. Clicking on that checkbox allows the user to select or deselect all issues with the status “open”.

### The Original Code

The original code isn’t bad. It works and does its job. For the most part, it’s quite readable. Especially on a high level. And it’s similar to what many Junior React devs would produce. At the same time, there are a few problems that are again common among Junior devs. And we’ll discuss them in a bit.

But first things first. Here’s the component’s code. [You can also find a functioning version on CodeSandbox](https://codesandbox.io/s/refactoring-session-table-with-checkboxes-vqf87c?file=/src/TableOriginal.jsx).

```typescript
import { useState } from "react";
import classes from "./Table.module.css";

function Table({ issues }) {
  const [checkedState, setCheckedState] = useState(
    new Array(issues.length).fill({
      checked: false,
      backgroundColor: "#ffffff",
    })
  );
  const [selectDeselectAllIsChecked, setSelectDeselectAllIsChecked] =
    useState(false);
  const [numCheckboxesSelected, setNumCheckboxesSelected] = useState(0);

  const handleOnChange = (position) => {
    const updatedCheckedState = checkedState.map((element, index) => {
      if (position === index) {
        return {
          ...element,
          checked: !element.checked,
          backgroundColor: element.checked ? "#ffffff" : "#eeeeee",
        };
      }
      return element;
    });
    setCheckedState(updatedCheckedState);

    const totalSelected = updatedCheckedState
      .map((element) => element.checked)
      .reduce((sum, currentState, index) => {
        if (currentState) {
          return sum + issues[index].value;
        }
        return sum;
      }, 0);
    setNumCheckboxesSelected(totalSelected);

    handleIndeterminateCheckbox(totalSelected);
  };

  const handleIndeterminateCheckbox = (total) => {
    const indeterminateCheckbox = document.getElementById(
      "custom-checkbox-selectDeselectAll"
    );
    let count = 0;

    issues.forEach((element) => {
      if (element.status === "open") {
        count += 1;
      }
    });

    if (total === 0) {
      indeterminateCheckbox.indeterminate = false;
      setSelectDeselectAllIsChecked(false);
    }
    if (total > 0 && total < count) {
      indeterminateCheckbox.indeterminate = true;
      setSelectDeselectAllIsChecked(false);
    }
    if (total === count) {
      indeterminateCheckbox.indeterminate = false;
      setSelectDeselectAllIsChecked(true);
    }
  };

  const handleSelectDeselectAll = (event) => {
    let { checked } = event.target;

    const allTrueArray = [];
    issues.forEach((element) => {
      if (element.status === "open") {
        allTrueArray.push({ checked: true, backgroundColor: "#eeeeee" });
      } else {
        allTrueArray.push({ checked: false, backgroundColor: "#ffffff" });
      }
    });

    const allFalseArray = new Array(issues.length).fill({
      checked: false,
      backgroundColor: "#ffffff",
    });
    checked ? setCheckedState(allTrueArray) : setCheckedState(allFalseArray);

    const totalSelected = (checked ? allTrueArray : allFalseArray)
      .map((element) => element.checked)
      .reduce((sum, currentState, index) => {
        if (currentState && issues[index].status === "open") {
          return sum + issues[index].value;
        }
        return sum;
      }, 0);
    setNumCheckboxesSelected(totalSelected);
    setSelectDeselectAllIsChecked((prevState) => !prevState);
  };

  return (
    <table className={classes.table}>
      <thead>
        <tr>
          <th>
            <input
              className={classes.checkbox}
              type={"checkbox"}
              id={"custom-checkbox-selectDeselectAll"}
              name={"custom-checkbox-selectDeselectAll"}
              value={"custom-checkbox-selectDeselectAll"}
              checked={selectDeselectAllIsChecked}
              onChange={handleSelectDeselectAll}
            />
          </th>
          <th className={classes.numChecked}>
            {numCheckboxesSelected
              ? `Selected ${numCheckboxesSelected}`
              : "None selected"}
          </th>
        </tr>
        <tr>
          <th />
          <th>Name</th>
          <th>Message</th>
          <th>Status</th>
        </tr>
      </thead>

      <tbody>
        {issues.map(({ name, message, status }, index) => {
          let issueIsOpen = status === "open";
          let onClick = issueIsOpen ? () => handleOnChange(index) : null;
          let stylesTr = issueIsOpen
            ? classes.openIssue
            : classes.resolvedIssue;

          return (
            <tr
              className={stylesTr}
              style={checkedState[index]}
              key={index}
              onClick={onClick}
            >
              <td>
                {issueIsOpen ? (
                  <input
                    className={classes.checkbox}
                    type={"checkbox"}
                    id={`custom-checkbox-${index}`}
                    name={name}
                    value={name}
                    checked={checkedState[index].checked}
                    onChange={() => handleOnChange(index)}
                  />
                ) : (
                  <input
                    className={classes.checkbox}
                    type={"checkbox"}
                    disabled
                  />
                )}
              </td>
              <td>{name}</td>
              <td>{message}</td>
              <td>
                {issueIsOpen ? (
                  <span className={classes.greenCircle} />
                ) : (
                  <span className={classes.redCircle} />
                )}
              </td>
            </tr>
          );
        })}
      </tbody>
    </table>
  );
}

export default Table;
```

The issues array that is rendered in the table looks like this.

```typescript
[
  {
    "id": "c9613c41-32f0-435e-aef2-b17ce758431b",
    "name": "TypeError",
    "message": "Cannot read properties of undefined (reading 'length')",
    "status": "open",
    "numEvents": 105,
    "numUsers": 56,
    "value": 1
  },
  {
    "id": "1f62d084-cc32-4c7b-943d-417c5dac896e",
    "name": "TypeError",
    "message": "U is not a function",
    "status": "resolved",
    "numEvents": 45,
    "numUsers": 34,
    "value": 1
  },
  ...
]
```

The component is more on the complex side with 177 lines of code (LOC). But as mentioned, on the high level it’s quite readable. This is what you see when the functions are collapsed.

```typescript
function Table({ issues }) {
  // decides which checkboxes is selected
  const [checkedState, setCheckedState] = useState(...);
  // decides if the top checkbox is checked or not
  const [selectDeselectAllIsChecked, setSelectDeselectAllIsChecked] =
    useState(false);
  // represents the number of selected checkboxes
  const [numCheckboxesSelected, setNumCheckboxesSelected] = useState(0);

  // handles the click on a row / checkbox
  const handleOnChange = (position) => { ... };

  // responsible for setting the partially checked state of the top checkbox
  const handleIndeterminateCheckbox = (total) => { ... };

  // handles the click on the top checkbox
  const handleSelectDeselectAll = (event) => { ... };

  return (
    <table>
      ...
    </table>
  );
}
```

Not too bad. But there’s room for improvement.

## Problems With The Original Code

### Problem 1: Sub-Optimal Data Structure

Let’s have a look at the top of the component.

```typescript
function Table({ issues }) {
  const [checkedState, setCheckedState] = useState(
    new Array(issues.length).fill({ checked: false, backgroundColor: "#ffffff" })
  );

  ...
```

- The `checkedState` variable is an array.
- It’s initialized to have the same length as the issues array.
- Each entry is an object that represents the checked state of an issue.

The first problem is that an array of objects is always hard to mutate. We have to access the correct object via its index inside the array. So we need to find and pass indices around. 

The second problem is that this approach doesn’t scale very well. For example, if we had 10k issues we’d have a 10k long `checkedState` array filled with mostly `false` boolean values.

The alternative: We could use a different data structure like an object or a [Map](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/Map) that maps an issue id to its checked value. 

```typescript
// alternative data structure (example when two checkboxes are checked)
{
  "issue-id-1": true,
  "issue-id-4": true,
}
```

Even better, JavaScript has a native [Set](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set) that is similar to an array but can only hold unique values and is optimized for accessing them in a performant way.

### Problem 2: Not Deriving Variables From State

Again at the top of the component, we can see another very common problem in the code of Junior developers: Unnecessary state variables that could be derived from props or another state.

```typescript
function Table({ issues }) {
  const [checkedState, setCheckedState] = useState(
    new Array(issues.length).fill({ checked: false, backgroundColor: "#ffffff" })
  );
  const [selectDeselectAllIsChecked, setSelectDeselectAllIsChecked] =
    useState(false);
  const [numCheckboxesSelected, setNumCheckboxesSelected] = useState(0);

  ...
```

In this code, we have three unnecessary pieces of state:

- `backgroundColor` is a style that can be derived from the value of `checked` (it’s either `#eee` if checked is true otherwise `#fff`)
- `selectDeselectAllIsChecked` is used to set the `checked` prop for the checkbox at the top (can be derived from `checkedState` and `issues`)
- `numCheckboxesSelected` is the number next to that checkbox (can be derived from `checkedState`)

![](https://media.graphassets.com/KObCjqPpTvCU6Rk8kbdk)

### Problem 3: Directly Accessing The DOM

A bit further down the code, we can find the function that handles the `indeterminate` state of the top checkbox:

```typescript
function Table({ issues }) {
  ...

  const handleIndeterminateCheckbox = (total) => {
    const indeterminateCheckbox = document.getElementById(
      "custom-checkbox-selectDeselectAll"
    );
    let count = 0;

    issues.forEach((element) => {
      if (element.status === "open") {
        count += 1;
      }
    });

    if (total === 0) {
      indeterminateCheckbox.indeterminate = false;
      setSelectDeselectAllIsChecked(false);
    }
    if (total > 0 && total < count) {
      indeterminateCheckbox.indeterminate = true;
      setSelectDeselectAllIsChecked(false);
    }
    if (total === count) {
      indeterminateCheckbox.indeterminate = false;
      setSelectDeselectAllIsChecked(true);
    }
  };

  ...
```

We can see that the top checkbox is accessed via `document.getElementById(...)`. This is relatively unusual in React apps and mostly not necessary.

Note that setting `indeterminateCheckbox.indeterminate` directly on the DOM element is necessary. We can’t set `indeterminate` as a prop on the checkbox.

### Problem 4: Variable Naming

The author of this code put some effort into selecting good names for the variables and functions. Kudos to that.

Unfortunately, some names are a bit confusing. We can find some examples at the top of the component:

```typescript
function Table({ issues }) {
  const [checkedState, setCheckedState] = useState(
    new Array(issues.length).fill({ checked: false, backgroundColor: "#ffffff" })
  );
  const [selectDeselectAllIsChecked, setSelectDeselectAllIsChecked] =
    useState(false);
  const [numCheckboxesSelected, setNumCheckboxesSelected] = useState(0);

  ...
```

For example, `checkedState` is redundant and misleading. I’d expect a boolean here but this is an array of objects. A better name might be `checkedItems`.

`selectDeselectAllIsChecked` kind of is understandable but a bit hard to read. It tells us whether all checkboxes are checked or not but I had to read it like 5 times. A better name could be `isEveryItemChecked`.

[![React on the job - email course](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/skyncbvxln80rmks31wq.png)](https://profy.dev/around-react-email-workshop)

## The Refactoring

Now that we’ve analyzed some of the problems with the original code let’s start to refactor it step by step.

### Step 1: Refactor Data Structure From Array To Set

The most impactful step is to use a [Set](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set) instead of the array as the `checkedState`. We could have used an object as well but the Set has slight advantages like direct access to its `size` which we’ll use quite a bit. Compared to a Map, the Set is a slightly better fit here as we only need to know the ids of all selected items instead of a key-value pair.

As we’re already touching this code we also remove the `backgroundColor` from the state and rename the state variable.

```typescript
//before
const [checkedState, setCheckedState] = useState(
  new Array(issues.length).fill({ checked: false, backgroundColor: "#ffffff" })
);

// after
const [checkedById, setCheckedById] = useState(new Set()); 
```

As a reminder, the shape of the `checkedById` state (represented as an array) looks now like this:

```typescript
// shape of checkedById (when two checkboxes are checked)
["issue-id-1", "issue-id-4"]
```

This change isn’t trivial. It has implications for the rest of our code. We’ll see one by one how each part of the code is affected.

**Let’s start with the handler function that’s connected to the checkboxes in the table.**

![](https://media.graphassets.com/txmmQwJ5SZq41shmIfie)

Here’s the before and after of the code:

```typescript
// before
const handleOnChange = (position) => {
  const updatedCheckedState = checkedState.map((element, index) => {
    if (position === index) {
      return {
        ...element,
        checked: !element.checked,
        backgroundColor: element.checked ? "#ffffff" : "#eeeeee",
      };
    }
    return element;
  });
  setCheckedState(updatedCheckedState);

  const totalSelected = updatedCheckedState
    .map((element) => element.checked)
    .reduce((sum, currentState, index) => {
      if (currentState) {
        return sum + issues[index].value;
      }
      return sum;
    }, 0);
  setNumCheckboxesSelected(totalSelected);

  handleIndeterminateCheckbox(totalSelected);
};

// after
const handleOnChange = (id) => {
  const updatedCheckedById = new Set(checkedById);
  if (updatedCheckedById.has(id)) {
    updatedCheckedById.delete(id);
  } else {
    updatedCheckedById.add(id);
  }

  setCheckedById(updatedCheckedById);

  const totalSelected = updatedCheckedById.size;
  setNumCheckboxesSelected(totalSelected);
  handleIndeterminateCheckbox(totalSelected);
};
```

We got rid of quite some clutter. Some quick notes:

1. The line `return sum + issues[index].value` in the original code is interesting. It uses a value in the data to increment the sum and calculate the number of selected elements. This seems over-complex.
2. In the refactored code we can now simply use `updatedCheckedById.size` to get the number of selected elements.
3. Since we don’t want any `false` values in our state we have to delete the corresponding value when a checkbox is deselected via `updatedCheckedById.delete(id)`.

**Let’s continue with the handler function that is connected to the top checkbox.**

![](https://media.graphassets.com/txH5F4FISnaD4NBnQPsM)

Here’s the before and after.

```typescript
// before
const handleSelectDeselectAll = (event) => {
  let { checked } = event.target;

  const allTrueArray = [];
  issues.forEach((element) => {
    if (element.status === "open") {
      allTrueArray.push({ checked: true, backgroundColor: "#eeeeee" });
    } else {
      allTrueArray.push({ checked: false, backgroundColor: "#ffffff" });
    }
  });

  const allFalseArray = new Array(issues.length).fill({
    checked: false,
    backgroundColor: "#ffffff",
  });
  checked ? setCheckedState(allTrueArray) : setCheckedState(allFalseArray);

  const totalSelected = (checked ? allTrueArray : allFalseArray)
    .map((element) => element.checked)
    .reduce((sum, currentState, index) => {
      if (currentState && issues[index].status === "open") {
        return sum + issues[index].value;
      }
      return sum;
    }, 0);
  setNumCheckboxesSelected(totalSelected);
  setSelectDeselectAllIsChecked((prevState) => !prevState);
};

// after
const handleSelectDeselectAll = (event) => {
    if (event.target.checked) {
      const openIssues = issues.filter(({ status }) => status === "open");
      const allChecked = new Set(openIssues.map(({ id }) => id));
      setCheckedById(allChecked);
      setNumCheckboxesSelected(allChecked.size);
    } else {
      setCheckedById(new Set());
      setNumCheckboxesSelected(0);
    }

    setSelectDeselectAllIsChecked((prevState) => !prevState);
  };
```

Again a few quick notes:

- The original implementation is a bit cumbersome with the `allTrueArray` and `allFalseArray`. We could refactor this as well but since we use an object now we can simply remove this code.
- Because we use a Set in the new implementation creating the new `allChecked` state is very straightforward. We filter all open issues and then [construct a new Set from an array](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set#relation_with_array_objects) of ids.

**The last piece of code that we have to adjust is the JSX returned from the component.** In particular the code inside the `<tbody>` element.

```typescript
// before
<tbody>
  {issues.map(({ name, message, status }, index) => {
    let issueIsOpen = status === "open";
    // we have to use the id of the issue instead of the index here
    let onClick = issueIsOpen ? () => handleOnChange(index) : null;
    let stylesTr = issueIsOpen
      ? classes.openIssue
      : classes.resolvedIssue;

    return (
      <tr
        className={stylesTr}
        // the backgroundColor isn't part of checkedState anymore
        // so we have to adjust this
        style={checkedState[index]}
        key={index}
        onClick={onClick}
      >
        <td>
          {issueIsOpen ? (
            <input
              className={classes.checkbox}
              type={"checkbox"}
              id={`custom-checkbox-${index}`}
              name={name}
              value={name}
              // we have to use the id instead of the index here
              checked={checkedState[index].checked}
              onChange={() => handleOnChange(index)}
            />
          ) : (
            <input
              className={classes.checkbox}
              type={"checkbox"}
              disabled
            />
          )}
        </td>
        ...
      </tr>
    );
  })}
</tbody>

// after
<tbody>
  {issues.map(({ id, name, message, status }, index) => {
    let issueIsOpen = status === "open";
    let onClick = issueIsOpen ? () => handleOnChange(id) : null;
    let stylesTr = issueIsOpen
      ? classes.openIssue
      : classes.resolvedIssue;

    return (
      <tr
        key={id}
        className={stylesTr}
        style={{ backgroundColor: checkedById.has(id) ? "#eee" : "#fff" }}
        onClick={onClick}
      >
        <td>
          {issueIsOpen ? (
            <input
              className={classes.checkbox}
              type={"checkbox"}
              id={`custom-checkbox-${index}`}
              name={name}
              value={name}
              checked={checkedById.has(id)}
              onChange={() => handleOnChange(id)}
            />
          ) : (
            <input
              className={classes.checkbox}
              type={"checkbox"}
              disabled
            />
          )}
        </td>
        ...
      </tr>
    );
  })}
</tbody>
```

Again a few quick notes:

- We have to change most of the occurrences of `index` in this code.
- Instead of writing `key={index}`  on the `<tr>` element we use the `id` as key now. This doesn’t change anything in the current version as the order of issues doesn’t change. But it’s best practice and it’s likely that some kind of sorting will be introduced to our table in the future.
- Previously the background color of the row was set via this line: `style={checkedState[index]}`. This takes a moment to understand. For any given row this translates to e.g. `style={{ checked: true, backgroundColor: "#fff" }}`.
- The new version is much more explicit in this regard: `style={{ backgroundColor: checkedById[id] ? "#eee" : "#fff" }}`

You can find [all code changes here on GitHub](https://github.com/profydev/refactoring-session-table-with-checkboxes/pull/2/commits/f4fd9c5467c3f671765c9ebb310be6bcecb38756) and [a function version of this step here on CodeSandbox](https://codesandbox.io/s/refactoring-session-table-with-checkboxes-vqf87c?file=/src/TableRefactorStep1.jsx) (note that you need to change `App.jsx` to see this version of the component rendered in the browser).

### Step 2: Using Derived State

The second problem we discussed above is the unnecessary state variables `selectDeselectAllIsChecked` and `numCheckboxesSelected`. As mentioned, these can easily be derived from the `checkedById` state and the `issues` data array.

```typescript
// before
function Table({ issues }) {
  const [checkedById, setCheckedById] = useState(new Set());
  const [selectDeselectAllIsChecked, setSelectDeselectAllIsChecked] =
    useState(false);
  const [numCheckboxesSelected, setNumCheckboxesSelected] = useState(0);

  ...

// after
function Table({ issues }) {
  const [checkedById, setCheckedById] = useState(new Set());
  const openIssues = issues.filter(({ status }) => status === "open");
  const numOpenIssues = openIssues.length;
  const numCheckedIssues = checkedById.size;

  ...
```

We don’t necessarily need the `openIssues` variable here. But this way we can reuse it inside the refactored `handleSelectDeselectAll` function (which is not shown here).

On top of these changes, we also have to remove the calls to the state setters from the rest of the code. These changes have two advantages:

1. We get rid of a few lines of code.
2. More importantly, we don’t need to manage the additional state anymore. This is great because we can easily forget to update the state and thus introduce bugs.

As the last step, we can memoize the calculation of `numOpenIssues`. This is not required for the current version but might be necessary once we work with larger data sets.

```typescript
const openIssues = useMemo(
  () => issues.filter(({ status }) => status === "open"),
  [issues]
);
```

You can see [the complete list of code changes here on GitHub](https://github.com/profydev/refactoring-session-table-with-checkboxes/pull/2/commits/24c79fa3502b5034e3ede351e9b65ccc2cbf70ac) and [a functioning version of this step here on CodeSandbox](https://codesandbox.io/s/refactoring-session-table-with-checkboxes-vqf87c?file=/src/TableRefactorStep2.jsx).

### Step 3: Remove document.getElementById()

The final important step in our refactoring journey is to access the top checkbox the React way instead of using `document.getElementById()`. 

What’s the React way? We use a `ref`.

```typescript
// before
function Table({ issues }) {
  ...

  const handleIndeterminateCheckbox = (total) => {
    const indeterminateCheckbox = document.getElementById(
      "custom-checkbox-selectDeselectAll"
    );
    let count = 0;

    issues.forEach((element) => {
      if (element.status === "open") {
        count += 1;
      }
    });

    if (total === 0) {
      indeterminateCheckbox.indeterminate = false;
    }
    if (total > 0 && total < count) {
      indeterminateCheckbox.indeterminate = true;
    }
    if (total === count) {
      indeterminateCheckbox.indeterminate = false;
    }
  };

  ...

// after
function Table({ issues }) {
  ...

  const topCheckbox = useRef();
  const handleIndeterminateCheckbox = (checkedById) => {
    const numSelected = checkedById.size;
    if (numSelected === 0) {
      topCheckbox.current.indeterminate = false;
    } else if (numSelected === numOpenIssues) {
      topCheckbox.current.indeterminate = false;
    } else {
      topCheckbox.current.indeterminate = true;
    }
  };

  ...

  return (
    <table className={classes.table}>
      <thead>
        <tr>
          <th>
            <input
              ref={topCheckbox}
              ...
```

A few quick notes:

- The most important thing is that we replaced `document.getElementById()` by `useRef()`. This also allows us to remove the `id` from the checkbox.
- The line `issues.forEach((element) => {` in the original code is responsible for calculating the number of open issues. This is a bit cumbersome approach and could have easily been replaced with `issues.filter(...).length`. But we already have defined the `numOpenIssues` variable at the top of our component. So no need for this code anymore.
- We used `if ... else if ... else` instead of three `if` statements. We also switched the conditions around to have the only `true` value at the bottom. This makes it easier to process for our brains in my opinion.

Actually, we don’t even need the `if ... else if ... else` statement. We can simply replace it with a single line of code:

```typescript
topCheckbox.current.indeterminate =
      numSelected > 0 && numSelected < numOpenIssues;
```

Using this line it also doesn’t make much sense to keep the `handleIndeterminateCheckbox` function anymore.

You can find [all the code changes here on GitHub](https://github.com/profydev/refactoring-session-table-with-checkboxes/pull/2/commits/24c79fa3502b5034e3ede351e9b65ccc2cbf70ac) and [a functioning version of this step here on CodeSandbox](https://codesandbox.io/s/refactoring-session-table-with-checkboxes-vqf87c?file=/src/TableRefactorStep3.jsx:728-817).

[![React on the job - email course](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/skyncbvxln80rmks31wq.png)](https://profy.dev/around-react-email-workshop)

## Final Code

After a bit of additional cleanup ([here are the changes](https://github.com/profydev/refactoring-session-table-with-checkboxes/pull/2/commits/24c79fa3502b5034e3ede351e9b65ccc2cbf70ac)) we’re able to reduce the code from initially 177 LOC to 102 LOC.

```typescript
import { useMemo, useState, useRef } from "react";
import classes from "./Table.module.css";

function Table({ issues }) {
  const topCheckbox = useRef();
  const [checkedById, setCheckedById] = useState(new Set());

  const openIssues = useMemo(
    () => issues.filter(({ status }) => status === "open"),
    [issues]
  );
  const numOpenIssues = openIssues.length;
  const numCheckedIssues = checkedById.size;

  const handleOnChange = (id) => {
    const updatedCheckedById = new Set(checkedById);
    if (updatedCheckedById.has(id)) {
      updatedCheckedById.delete(id);
    } else {
      updatedCheckedById.add(id);
    }
    setCheckedById(updatedCheckedById);

    const updatedNumChecked = updatedCheckedById.size;
    topCheckbox.current.indeterminate =
      updatedNumChecked > 0 && updatedNumChecked < numOpenIssues;
  };

  const handleSelectDeselectAll = (event) => {
    if (event.target.checked) {
      const allChecked = new Set(openIssues.map(({ id }) => id));
      setCheckedById(allChecked);
    } else {
      setCheckedById(new Set());
    }
  };

  return (
    <table className={classes.table}>
      <thead>
        <tr>
          <th>
            <input
              type="checkbox"
              ref={topCheckbox}
              className={classes.checkbox}
              checked={numOpenIssues === numCheckedIssues}
              onChange={handleSelectDeselectAll}
            />
          </th>
          <th className={classes.numChecked}>
            {numCheckedIssues
              ? `Selected ${numCheckedIssues}`
              : "None selected"}
          </th>
        </tr>
        <tr>
          <th />
          <th>Name</th>
          <th>Message</th>
          <th>Status</th>
        </tr>
      </thead>

      <tbody>
        {issues.map(({ id, name, message, status }) => {
          const isIssueOpen = status === "open";
          return (
            <tr
              key={id}
              className={
                isIssueOpen ? classes.openIssue : classes.resolvedIssue
              }
              style={{ backgroundColor: checkedById.has(id) ? "#eee" : "#fff" }}
            >
              <td>
                <input
                  type="checkbox"
                  className={classes.checkbox}
                  checked={checkedById.has(id)}
                  disabled={!isIssueOpen}
                  onChange={() => handleOnChange(id)}
                />
              </td>
              <td>{name}</td>
              <td>{message}</td>
              <td>
                <span
                  className={
                    isIssueOpen ? classes.greenCircle : classes.redCircle
                  }
                />
              </td>
            </tr>
          );
        })}
      </tbody>
    </table>
  );
}

export default Table;
```

You can find [all code changes here on GitHub](https://github.com/profydev/refactoring-session-table-with-checkboxes/pull/2/files) and [a version of the final component here on CodeSandbox](https://codesandbox.io/s/refactoring-session-table-with-checkboxes-vqf87c?file=/src/TableRefactorFinal.jsx).

From my perspective, this is not only more concise but also much more readable. 

## Takeaways

1. Watch out for unnecessary state that can be derived from other state or data.
2. Using an object, a map, or a set can often be easier and more efficient than an array.
3. The React way of accessing DOM elements is `useRef()` and not `document.getElementById()`.

[![React on the job - email course](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/skyncbvxln80rmks31wq.png)](https://profy.dev/around-react-email-workshop)