In this post I want to show you how to refactor React code to something better. Here I prepared for you a real junior code which is quite big and we want to refactor it to something on the senior level.
Here is how our project looks like. We have here a list of the issues, some issues are greyed out and we can't choose them because they are resolved. We can select active issues and in header to get highlighting how many items we selected. Also we can toggle all items at once.
This is exactly the project which was done by junior developer and which we want to refactor. Let's look on the code.
const App = () => {
return (
<div>
<h1>Monsterlessons Academy</h1>
<BadTable issues={issues} />
</div>
);
};
In our App there is a BadTable component where we provide a list of issues inside. Issues that we provide inside is an array of objects which looks like this.
{
"id": "c9613c41-32f0-435e-aef2-b17ce758431b",
"name": "TypeError",
"message": "Cannot read properties of undefined (reading 'length')",
"status": "open",
"numEvents": 105,
"numUsers": 56,
"value": 1
},
So essentially we pass issues to the BadTable and it does all logic inside. Let's have a look on the table.
import { useState } from "react";
import classes from "./Table.module.css";
function BadTable({ 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 BadTable;
You can just imagine that you got to the company where somebody wrote such code and now you must understand it and refactor to something better.
If you see bad code it is always easier to write the component from scratch that trying to refactor it if you know all cases that you need to cover.
But we want to do it right and refactor without removing everything.
Refactoring state
The most important part of any component is the state of data. If you planned your state in a wrong way then it won't be easy to work with it inside your component. Your state and how you change it is a core of your business logic.
const [checkedState, setCheckedState] = useState(
new Array(issues.length).fill({
checked: false,
backgroundColor: "#ffffff",
})
);
As you can see from we have a checkedState which stores checked state for every issue and a backgroundColor. I don't really like to store a color in the array but checked property is needed because we need to store the state of checked issues.
What I want to create instead is a small key:value pairs of data which is needed for each issue. So it can look like this.
{
"1": { "isSelected": true },
"2": { "isSelected": false }
}
In order to create such object from our issues we can leverage fromEntries.
const [issueEntries, setIssueEntries] = useState(() => {
const entries = issues.map((issue) => [issue.id, { isSelected: false }]);
return Object.fromEntries(entries);
});
Here we prepare our pairs and create an object from them.
As you can see issueEntries is an object with needed data that we can easily read.
Refactoring list
Now let's look on the map where we render our issues.
<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>
The code here can be optimised almost in every line.
<tbody>
{issues.map((issue) => {
const isOpenedIssue = issue.status === "open";
const rowClass = isOpenedIssue ? classes.openIssue : classes.resolvedIssue;
const statusClass = isOpenedIssue ? classes.greenCircle : classes.redCircle;
const backgroundColor = issueEntries[issue.id].isSelected
? "#eeeeee"
: "#ffffff";
return (
<tr
className={rowClass}
style={{ backgroundColor }}
key={issue.id}
onClick={() => selectRow(issue.id)}
>
<td>
<input
className={classes.checkbox}
type="checkbox"
readOnly
checked={issueEntries[issue.id].isSelected}
disabled={isOpenedIssue}
/>
</td>
<td>{issue.name}</td>
<td>{issue.message}</td>
<td>
<span className={statusClass} />
</td>
</tr>
);
})}
</tbody>
First of all we created a bunch of variables to render in markup like statusClass, rowClass and backgroundColor. Also we used issueEntries[issue.id].isSelected to easily get the value of our entry. Additionally I removed all not needed attributes like name, id and value from the checkbox.
I also leaved just a single selectRow function which must contain all logic to implement selection.
Additionally I want to fully disable all events on the row instead of checking them manually. We can do that with CSS.
.resolvedIssue {
...
pointer-events: none;
}
Pointer events solves this problem and just blocks all events on the row when it is resolved.
Refactoring row click
Now it is time to update the logic of clicking on the row. Here is a bad code.
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);
};
It is kind of messy because it works with indexes, it updates backgroundColor and it recalculated totalSelected every time by hands. Let's simplify it.
const selectRow = (issueId) => {
const updatedIssueEntry = {
...issueEntries[issueId],
isSelected: !issueEntries[issueId].isSelected,
};
const updatedIssueEntries = {
...issueEntries,
[issueId]: updatedIssueEntry,
};
setIssueEntries(updatedIssueEntries);
};
Here our goal is to change isSelected property of clicked issueId in the object. In order to do that we make a copy of the object and we set it instead.
Refactoring total
Total property is a great problem in existing code as it recalculated every time when we click on the row. We don't need to do that as we can just create a variable which recalculates totals every time when isSelected changes.
const GoodTable = ({ issues }) => {
const totalSelected = Object.values(issueEntries).filter(
(issueData) => issueData.isSelected
).length;
...
}
Here we read values from the object and get the length of the selected items. Now we don't need to recalculate this property every time.
<thead>
<tr>
<th>
<input
className={classes.checkbox}
type="checkbox"
checked={totalSelected}
onChange={selectAll}
/>
</th>
<th className={classes.numChecked}>
{totalSelected ? `Selected ${totalSelected}` : "None selected"}
</th>
</tr>
<tr>
<th />
<th>Name</th>
<th>Message</th>
<th>Status</th>
</tr>
</thead>
Here we refactored our header and user totalSelected which will be always automatically rerendered. Now we just need to implement selectAll function.
const convertIssuesToEntries = (isSelected) => {
const entries = issues.map((issue) => [
issue.id,
{ isSelected: issue.status === "open" ? isSelected : false },
]);
return Object.fromEntries(entries);
};
const [issueEntries, setIssueEntries] = useState(() =>
convertIssuesToEntries(false)
);
const selectAll = (event) => {
const updatedIssueEntries = convertIssuesToEntries(event.target.checked);
setIssueEntries(updatedIssueEntries);
};
In order to do that I created a helper convertIssuesToEntries function. It creates exactly the object of issues that we use but we can change isSelected. It allows us to use it on initialize to create initial object and also inside selectAll function. So the goal of selectAll is just to set all isSelected properties to either true of false.
Partial selection
The last feature that we are missing is partially selected checkbox which must be done by indeterminate feature. This is a property on the checkbox that we can see to true or false. But unfortunately React doesn't support it so we must implement it natively.
const selectAllRef = useRef();
...
useEffect(() => {
const totalOpenedIssues = issues.filter((issue) => issue.status === "open")
.length;
const indeterminate =
totalSelected < totalOpenedIssues && totalSelected > 0;
selectAllRef.current.indeterminate = indeterminate;
}, [issues, totalSelected]);
...
<input
className={classes.checkbox}
type="checkbox"
ref={selectAllRef}
checked={totalSelected}
onChange={selectAll}
/>
We created a useRef hook for the checkbox and attached it to our select all input. Inside the useEffect we check when our totalSelected changes and write in our DOM node indeterminate property. So if the amount of selected items is bigger that zero and smaller than total we render it as indeterminate.
End result
import { useEffect, useRef, useState } from "react";
import classes from "./Table.module.css";
const GoodTable = ({ issues }) => {
const convertIssuesToEntries = (isSelected) => {
const entries = issues.map((issue) => [
issue.id,
{ isSelected: issue.status === "open" ? isSelected : false },
]);
return Object.fromEntries(entries);
};
const [issueEntries, setIssueEntries] = useState(() =>
convertIssuesToEntries(false)
);
const totalSelected = Object.values(issueEntries).filter(
(issueData) => issueData.isSelected
).length;
const selectAllRef = useRef();
const selectRow = (issueId) => {
const updatedIssueEntry = {
...issueEntries[issueId],
isSelected: !issueEntries[issueId].isSelected,
};
const updatedIssueEntries = {
...issueEntries,
[issueId]: updatedIssueEntry,
};
setIssueEntries(updatedIssueEntries);
};
const selectAll = (event) => {
const updatedIssueEntries = convertIssuesToEntries(event.target.checked);
setIssueEntries(updatedIssueEntries);
};
useEffect(() => {
const totalOpenedIssues = issues.filter((issue) => issue.status === "open")
.length;
const indeterminate =
totalSelected < totalOpenedIssues && totalSelected > 0;
selectAllRef.current.indeterminate = indeterminate;
}, [issues, totalSelected]);
return (
<table className={classes.table}>
<thead>
<tr>
<th>
<input
className={classes.checkbox}
type="checkbox"
ref={selectAllRef}
checked={totalSelected}
onChange={selectAll}
/>
</th>
<th className={classes.numChecked}>
{totalSelected ? `Selected ${totalSelected}` : "None selected"}
</th>
</tr>
<tr>
<th />
<th>Name</th>
<th>Message</th>
<th>Status</th>
</tr>
</thead>
<tbody>
{issues.map((issue) => {
const isOpenedIssue = issue.status === "open";
const rowClass = isOpenedIssue
? classes.openIssue
: classes.resolvedIssue;
const statusClass = isOpenedIssue
? classes.greenCircle
: classes.redCircle;
const backgroundColor = issueEntries[issue.id].isSelected
? "#eeeeee"
: "#ffffff";
return (
<tr
className={rowClass}
style={{ backgroundColor }}
key={issue.id}
onClick={() => selectRow(issue.id)}
>
<td>
<input
className={classes.checkbox}
type="checkbox"
readOnly
checked={issueEntries[issue.id].isSelected}
disabled={isOpenedIssue}
/>
</td>
<td>{issue.name}</td>
<td>{issue.message}</td>
<td>
<span className={statusClass} />
</td>
</tr>
);
})}
</tbody>
</table>
);
};
export default GoodTable;
Here is a full end result of our cleared code.
And actually if you want to improve your React knowledge and prepare for the interview I highly recommend you to check my course React Interview Questions.
📚 Source code of what we've done