As an (aspiring) Junior React developer it’s hard to understand if your 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
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.
Note: the code on this page is slightly improved compared to the code in the video using a Set instead of a Map.
The component is a table with a few somewhat complex features. Here’s a quick video:
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 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.
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><inputclassName={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 (<trclassName={stylesTr}style={checkedState[index]}key={index}onClick={onClick}><td>{issueIsOpen ? (<inputclassName={classes.checkbox}type={"checkbox"}id={`custom-checkbox-${index}`}name={name}value={name}checked={checkedState[index].checked}onChange={() => handleOnChange(index)}/>) : (<inputclassName={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.
[{"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.
function Table({ issues }) {// decides which checkboxes is selectedconst [checkedState, setCheckedState] = useState(...);// decides if the top checkbox is checked or notconst [selectDeselectAllIsChecked, setSelectDeselectAllIsChecked] =useState(false);// represents the number of selected checkboxesconst [numCheckboxesSelected, setNumCheckboxesSelected] = useState(0);// handles the click on a row / checkboxconst handleOnChange = (position) => { ... };// responsible for setting the partially checked state of the top checkboxconst handleIndeterminateCheckbox = (total) => { ... };// handles the click on the top checkboxconst handleSelectDeselectAll = (event) => { ... };return (<table>...</table>);}
Not too bad. But there’s room for improvement.
Let’s have a look at the top of the component.
function Table({ issues }) {const [checkedState, setCheckedState] = useState(new Array(issues.length).fill({ checked: false, backgroundColor: "#ffffff" }));...
checkedState
variable is an array.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 that maps an issue id to its checked value.
// alternative data structure (example when two checkboxes are checked){"issue-id-1": true,"issue-id-4": true,}
Even better, JavaScript has a native Set that is similar to an array but can only hold unique values and is optimized for accessing them in a performant way.
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.
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
)A bit further down the code, we can find the function that handles the indeterminate
state of the top checkbox:
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.
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:
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
.
Now that we’ve analyzed some of the problems with the original code let’s start to refactor it step by step.
The most impactful step is to use a 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.
//beforeconst [checkedState, setCheckedState] = useState(new Array(issues.length).fill({ checked: false, backgroundColor: "#ffffff" }));// afterconst [checkedIds, setCheckedIds] = useState(new Set());
As a reminder, the shape of the checkedIds
state (represented as an array) looks now like this:
// shape of checkedIds (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.
Here’s the before and after of the code:
// beforeconst 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);};// afterconst handleOnChange = (id) => {const updatedCheckedIds = new Set(checkedIds);if (updatedCheckedIds.has(id)) {updatedCheckedIds.delete(id);} else {updatedCheckedIds.add(id);}setCheckedIds(updatedCheckedIds);const totalSelected = updatedCheckedIds.size;setNumCheckboxesSelected(totalSelected);handleIndeterminateCheckbox(totalSelected);};
We got rid of quite some clutter. Some quick notes:
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.updatedCheckedIds.size
to get the number of selected elements.false
values in our state we have to delete the corresponding value when a checkbox is deselected via updatedCheckedIds.delete(id)
.Let’s continue with the handler function that is connected to the top checkbox.
Here’s the before and after.
// beforeconst 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);};// afterconst handleSelectDeselectAll = (event) => {if (event.target.checked) {const openIssues = issues.filter(({ status }) => status === "open");const allChecked = new Set(openIssues.map(({ id }) => id));setCheckedIds(allChecked);setNumCheckboxesSelected(allChecked.size);} else {setCheckedIds(new Set());setNumCheckboxesSelected(0);}setSelectDeselectAllIsChecked((prevState) => !prevState);};
Again a few quick notes:
allTrueArray
and allFalseArray
. We could refactor this as well but since we use an object now we can simply remove this code.allChecked
state is very straightforward. We filter all open issues and then construct a new Set from an array of id
.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.
// 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 herelet onClick = issueIsOpen ? () => handleOnChange(index) : null;let stylesTr = issueIsOpen? classes.openIssue: classes.resolvedIssue;return (<trclassName={stylesTr}// the backgroundColor isn't part of checkedState anymore// so we have to adjust thisstyle={checkedState[index]}key={index}onClick={onClick}><td>{issueIsOpen ? (<inputclassName={classes.checkbox}type={"checkbox"}id={`custom-checkbox-${index}`}name={name}value={name}// we have to use the id instead of the index herechecked={checkedState[index].checked}onChange={() => handleOnChange(index)}/>) : (<inputclassName={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 (<trkey={id}className={stylesTr}style={{ backgroundColor: checkedIds.has(id) ? "#eee" : "#fff" }}onClick={onClick}><td>{issueIsOpen ? (<inputclassName={classes.checkbox}type={"checkbox"}id={`custom-checkbox-${index}`}name={name}value={name}checked={checkedIds.has(id)}onChange={() => handleOnChange(id)}/>) : (<inputclassName={classes.checkbox}type={"checkbox"}disabled/>)}</td>...</tr>);})}</tbody>
Again a few quick notes:
index
in this code.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.style={checkedState[index]}
. This takes a moment to understand. For any given row this translates to e.g. style={{ checked: true, backgroundColor: "#fff" }}
.style={{ backgroundColor: checkedIds.has(id) ? "#eee" : "#fff" }}
You can find all code changes here on GitHub and a function version of this step here on CodeSandbox (note that you need to change App.jsx
to see this version of the component rendered in the browser).
The second problem we discussed above is the unnecessary state variables selectDeselectAllIsChecked
and numCheckboxesSelected
. As mentioned, these can easily be derived from the checkedIds
state and the issues
data array.
// beforefunction Table({ issues }) {const [checkedIds, setCheckedIds] = useState(new Set());const [selectDeselectAllIsChecked, setSelectDeselectAllIsChecked] =useState(false);const [numCheckboxesSelected, setNumCheckboxesSelected] = useState(0);...// afterfunction Table({ issues }) {const [checkedIds, setCheckedIds] = useState(new Set());const openIssues = issues.filter(({ status }) => status === "open");const numOpenIssues = openIssues.length;const numCheckedIssues = checkedIds.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:
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.
const openIssues = useMemo(() => issues.filter(({ status }) => status === "open"),[issues]);
You can see the complete list of code changes here on GitHub and a functioning version of this step here on CodeSandbox.
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
.
// beforefunction 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;}};...// afterfunction Table({ issues }) {...const topCheckbox = useRef();const handleIndeterminateCheckbox = (checkedIds) => {const numSelected = checkedIds.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><inputref={topCheckbox}...
A few quick notes:
document.getElementById()
by useRef()
. This also allows us to remove the id
from the checkbox.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.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:
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 and a functioning version of this step here on CodeSandbox.
After a bit of additional cleanup (here are the changes) we’re able to reduce the code from initially 177 LOC to 102 LOC.
import { useMemo, useState, useRef } from "react";import classes from "./Table.module.css";function Table({ issues }) {const topCheckbox = useRef();const [checkedIds, setCheckedIds] = useState(new Set());const openIssues = useMemo(() => issues.filter(({ status }) => status === "open"),[issues]);const numOpenIssues = openIssues.length;const numCheckedIssues = checkedIds.size;const handleOnChange = (id) => {const updatedCheckedIds = new Set(checkedIds);if (updatedCheckedIds.has(id)) {updatedCheckedIds.delete(id);} else {updatedCheckedIds.add(id);}setCheckedIds(updatedCheckedIds);const updatedNumChecked = updatedCheckedIds.size;topCheckbox.current.indeterminate =updatedNumChecked > 0 && updatedNumChecked < numOpenIssues;};const handleSelectDeselectAll = (event) => {if (event.target.checked) {const allChecked = new Set(openIssues.map(({ id }) => id));setCheckedIds(allChecked);} else {setCheckedIds(new Set());}};return (<table className={classes.table}><thead><tr><th><inputtype="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 (<trkey={id}className={isIssueOpen ? classes.openIssue : classes.resolvedIssue}style={{ backgroundColor: checkedIds.has(id) ? "#eee" : "#fff" }}><td><inputtype="checkbox"className={classes.checkbox}checked={checkedIds.has(id)}disabled={!isIssueOpen}onChange={() => handleOnChange(id)}/></td><td>{name}</td><td>{message}</td><td><spanclassName={isIssueOpen ? classes.greenCircle : classes.redCircle}/></td></tr>);})}</tbody></table>);}export default Table;
You can find all code changes here on GitHub and a version of the final component here on CodeSandbox.
From my perspective, this is not only more concise but also much more readable.
useRef()
and not document.getElementById()
.