One thing I’ve learned from all the consulting I’ve done is that rewrites rarely lead to anything good. Almost in all cases, when you have an application running in production it’s better to put it in order rather than put it to the torch.
I’ve been given some chaotic codebases to fix throughout the years, though, and I wanted to show you my approach to tidying them up. In this article, we’ll go over a messy component that I had to refactor during an audit and how I did it.
Here it is.
function Form() {
const [formLink, setFormLink] = useState('')
const [userPersona, setUserPersona] = useState('')
const [startDate, setStartDate] = useState('')
const [endDate, setEndDate] = useState('')
const [numberOfVisits, setNumberOfVisits] = useState('')
const [companyNumber, setCompanyNumber] = useState('')
const [numberIncorrect, setNumberIncorrect] = useState(0)
const [isFormValid, setIsFormValid] = useState(false)
const [buttonText, setButtonText] = useState('Next')
const [isProcessing, setIsProcessing] = useState(false)
const [estimatedTime, setEstimatedTime] = useState('Enter number')
const [recentActions, setRecentActions] = useState([])
const [abortController, setAbortController] = useState(null)
useEffect(() => {
fetchPreviousActions()
}, [])
const fetchPreviousActions = async () => {
try {
const response = await fetch('https://api.com/actions', {
method: 'GET',
headers: {
'Content-Type': 'application/json',
},
})
if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`)
}
const data = await response.json()
data.sort(
(a, b) => new Date(b.actiond_date) - new Date(a.actiond_date)
)
setRecentActions(data)
} catch (error) {
console.error('Failed to fetch recent actions', error)
}
}
const [showOverlay, setShowOverlay] = useState(false)
const renderLayout = () => (
<div>
<div>
<div>Analyzing...</div>
<button onClick={handleCancelaction}>Cancel</button>
</div>
</div>
)
const formatDate = (dateStr) => {
return dateStr.replace(/-/g, '')
}
const callBackendAPI = async (formData) => {
const controller = new AbortController()
setAbortController(controller)
formData.startDate = formatDate(formData.startDate)
formData.endDate = formatDate(formData.endDate)
try {
const response = await fetch('https://api.com/action', {
method: 'POST',
headers: {
'Content-Type': 'application/json',
},
body: JSON.stringify(formData),
signal: controller.signal,
})
if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`)
}
const data = await response.json()
setShowOverlay(false)
window.open(
'https://app.com/action/' + data.id,
'_blank',
'noopener,noreferrer'
)
window.location.reload()
} catch (error) {
if (error.name === 'AbortError') {
console.log('Scraping halted')
} else {
console.error('Failed to call the API', error)
}
} finally {
setShowOverlay(false)
setIsProcessing(false)
}
}
const handleCancelaction = () => {
if (abortController) {
abortController.abort() // Abort the fetch request
}
setShowOverlay(false)
setIsProcessing(false)
}
useEffect(() => {
if (!recentActions) {
fetchPreviousActions()
}
setIsFormValid(startDate && endDate && endDate > startDate)
}, [numberOfVisits, startDate, endDate])
const handleSubmit = async (event) => {
event.preventDefault()
if (!isFormValid) return
setShowOverlay(true)
setIsProcessing(true)
// Construct the form data object
const formData = {
userPersona,
startDate,
endDate,
numberOfVisits: parseInt(numberOfVisits, 10),
}
// Calling the API with the form data
await callBackendAPI(formData)
setIsProcessing(false)
}
const handleSubmitCompanyNumber = (number) => {
// this is unneeded, we've already set the value in state
setCompanyNumber(number)
if (number.length < 9) setNumberIncorrect(1)
else setNumberIncorrect(0)
}
return !numberIncorrect ? (
<div>
<div>
<img src={require('../imgs/LogoWhite.png')} alt="Logo" />
</div>
<div>
<div>Tool</div>
<form onSubmit={handleSubmit}>
<label htmlFor="company_number">
Enter your credentials
</label>
<input
type="text"
name="company_number"
id="company_number"
placeholder="Company Number"
value={companyNumber}
onChange={(e) => setCompanyNumber(e.target.value)}
/>
<button
type="submit"
onClick={(e) => handleSubmitCompanyNumber(companyNumber)}
>
<span>Login</span>
<span>></span>
</button>
{numberIncorrect > 0 ? (
<span>The number you entered is incorrect</span>
) : (
''
)}
</form>
</div>
</div>
) : (
<div>
<div>
<img
src={require('../imgs/LogoWhite.png')}
style={{ width: '200px', marginTop: '50px' }}
alt="Logo"
/>
</div>
<div>
<div>
<div>New action</div>
<form style={{ marginTop: '3vh' }} onSubmit={handleSubmit}>
<div>
<label>
Visits
<span
style={{
color: 'gray',
fontWeight: 'lighter',
}}
>
(optional)
</span>
</label>
<input
type="number"
value={numberOfVisits}
onChange={(e) => setNumberOfVisits(e.target.value)}
/>
<label className="form-label">
Define a user persona{' '}
<span
style={{
color: 'gray',
fontWeight: 'lighter',
}}
>
(optional)
</span>
</label>
<input
type="text"
id="posts-input"
value={userPersona}
onChange={(e) => setUserPersona(e.target.value)}
/>
</div>
<label
className="form-label"
style={{ textAlign: 'left' }}
>
Time period{' '}
<span
style={{
color: 'gray',
fontWeight: 'lighter',
}}
>
(available for dates before June 2023)
</span>
</label>
<div id="time-input">
<input
type="date"
style={{ marginRight: '20px' }}
value={startDate}
onChange={(e) => setStartDate(e.target.value)}
/>
<span style={{ fontSize: '15px' }}>to</span>
<input
type="date"
style={{ marginLeft: '20px' }}
value={endDate}
onChange={(e) => setEndDate(e.target.value)}
/>
</div>
<button
type="submit"
className={`next-button ${isFormValid ? 'active' : ''}`}
disabled={!isFormValid || isProcessing}
>
<span>Begin</span>
<span>→</span>
</button>
</form>
</div>
<div id="divider"></div>
<div>
<div>Recents</div>
<div>
<div>
{recentActions.map((action, index) => (
<div key={index}>
<a href={action.link} target="_blank">
<span>r/{action.obfuscated}</span>{' '}
<span>{action.actiond_date} (UTC)</span>
</a>
</div>
))}
</div>
</div>
</div>
</div>
{showOverlay ? renderLayout() : null}
</div>
)
}Note: the component is intentionally stripped of some details for simplicity and I’ve removed some domain-specific details
That’s a lot of code for a single component. The first thought that popped up in my head reading it was why would someone write a component this way?
But what we need to understand is that it probably wasn’t the work of one person and it didn’t happen all at once. This is a result of many people working on the same component, doing tactical changes one update at a time, just trying to get their feature done.
In the end, you end up with a large piece of code whose design was left to chance, and to no one’s surprise, it didn’t come out well. Everyone found the task to put this component in order to be too daunting but we’ve gathered enough courage or frustration to finally get it done.
Start With a Test
I open the IDE and the first thing I notice are a few lines of unused and commented-out code. But I resist the urge to start deleting code, no matter how insignificant it seems. This is a large component and it will take me a while to understand it. In the meantime, I want to be sure that I’m not breaking anything.
So the first thing I do is write a few tests if there are none. Remember, refactoring is the process of changing your code’s design, without altering its behavior. We don’t want to go too far.
Additionally, we need to focus only on tests that test the component as a black box and validate the end result. We want to make sure that it calls the correct callback functions it receives as props and that it renders everything correctly. How exactly it does this is none of our business right now because it will be subject to change.
Add a Lint Rule
Now when I have my tests set up, I can start moving things around. But instead of deleting the unused state and code straight away, I need to consider why it was left here in the first place. There needs to be an automated check that catches these things so they don’t end up in the codebase.
Ideally, this should be a linting rule that should run in CI so it doesn’t let such code get into production. Now, it’s always an option to comment the code out and still leave it there but at least it’s an intentional decision.
However, having one component in this condition means we most likely have others in the codebase as well. So setting strict linting rules here would light our application up like a Christmas tree and our refactoring would turn into a revamp of the entire codebase.
We don’t want to scope creep this refactoring. The longer it takes the higher the chance for merge conflicts and errors.
So we should just add a rule that at least logs warnings so we can take care of them afterward.
This is a general rule of thumb - if you catch a practice that you don’t want in your codebase, find a linting rule for it or make one yourself. The more things you can automate and handle before the point of code review, the better.
Remove Dead Code
There’s no reason to have dead code in the codebase - commented-out logic, unused state, unused imports. If you ever need it again, you can find it in version control. So I can reduce the state in the component with a few fields.
const [userPersona, setUserPersona] = useState('')
const [startDate, setStartDate] = useState('')
const [endDate, setEndDate] = useState('')
const [numberOfVisits, setNumberOfVisits] = useState('')
const [companyNumber, setCompanyNumber] = useState('')
const [numberIncorrect, setNumberIncorrect] = useState(0)
const [isFormValid, setIsFormValid] = useState(false)
const [isProcessing, setIsProcessing] = useState(false)
const [recentActions, setRecentActions] = useState([])
const [abortController, setAbortController] = useState(null)Bloated State
When I see so much state I immediately know that this component is doing too much, especially since some of the values seem to be unrelated. It has too many responsibilities and at the very least we can split it up into smaller components that can communicate via props.
But how do we decide how to split it?
Bloated state is a code smell but it doesn’t directly show us where the “seams” between the potential components are. To do this we need to explore the JSX.
Large Conditionals
Components are the main unit of a React application and they are the biggest factor behind its complexity. Splitting a component into smaller ones is the best way to spread that complexity across multiple places so it’s easier to understand and deal with.
This component returns one of two blocks of code depending on a value being set and my gut feeling tells me that we can either split those components into two smaller ones or unify the logic so it’s easier to follow.
return !numberIncorrect ? (
// A lot of JSX...
) : (
// Even more JSX...
)If the two blocks of markup are similar it would be better to unify them and use more granular conditionals. This way we can communicate where exactly the changes are. If they are different, then it would be better to just render two different components and split the logic between them.
When I inherit such a component and I’m not sure what’s the best approach I do a diff check side to side to see which parts of the JSX are similar.
In this case, the markup couldn’t be more different. This component is essentially rendering a multistep form and both blocks represent completely different forms.
We could use the existing Form component to only make the decision what to render then leave the rest to the child components. The only thing I’ve left in the parent is the logo image which is used in both places to further clean up the child forms.
function Form() {
const [companyNumber, setCompanyNumber] = useState(undefined)
return (
<div>
<div>
<img
src={require('../imgs/LogoWhite.png')}
style={{ width: '200px', marginTop: '50px' }}
alt="Logo"
/>
</div>
{!companyNumber ? (
<CompanyNumberForm onSubmit={setCompanyNumber} />
) : (
<ActionForm companyNumber={companyNumber} />
)}
</div>
)
}When we move the JSX away to a child component, the IDE will immediately highlight all the functions and values that are missing, making it easier for us to split up the state.
Component Responsibilities
One thing that we notice looking at the forms is that the first one is only passing its data to the parent while the second one accepts the first form’s data and submits it on top of that. This makes our child forms inconsistent and a new developer reading this code would wonder why we made this decision.
In this example, the second form is submitting the data only because it’s last in the chain. But in the future, it may not be. There may be another step after it that will make us refactor it again to move the submission logic to the next form.
To avoid this we can make the parent component responsible for the final submission. The ActionForm must only validate its data and pass it to the parent. Everything else can be done by the parent. This way even if we add another step, only the main Form component will be affected.
function Form() {
const [companyNumber, setCompanyNumber] = useState(undefined)
const createAction = () => {
// ...
}
return (
<div>
<div>
<img
src={require('../imgs/LogoWhite.png')}
style={{ width: '200px', marginTop: '50px' }}
alt="Logo"
/>
</div>
{!companyNumber ? (
<CompanyNumberForm onSubmit={setCompanyNumber} />
) : (
<ActionForm onSubmit={createAction} />
)}
</div>
)
}Notice that I’m not using a generic function name for the actual handler. I’ve used createAction which is more descriptive and reads like a sentence onSubmit={createAction}. You don’t have to hop to the function to see what it’s doing, the name is enough.
Move utility functions out of the component
We often use small utility functions to format data before displaying it or sending it. In this component, we have a date formatting function.
const formatDate = (dateStr) => {
return dateStr.replace(/-/g, '')
}My rule of thumb is that functions should live inside the component only if they need to access state or another value inside of it. If they can receive the value as a parameter I’d rather have the functionality outside of the component.
import { formatDate } from './utils'We can put it in the same file if it’s only used there or move it to a utils file living next to the component. If another component requires the same utility function we can move it to the top level of the application.
Additionally, the component shouldn’t really be aware of how the data it’s sending is being structured, but we’ll get to that in a bit.
Data Fetching
When we move our logic into the CompanyNumberForm and ActionForm components, we notice that the ActionForm is fetching a list of recent actions that need to be displayed.
useEffect(() => {
if (!previousActions) {
fetchPreviousActions()
}
setIsFormValid(startDate && endDate && endDate > startDate)
}, [numberOfObfuscated, startDate, endDate])
const fetchPreviousActions = async () => {
try {
const response = await fetch('https://api.com/actions', {
method: 'GET',
headers: {
'Content-Type': 'application/json',
},
})
if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`)
}
const data = await response.json()
data.sort(
(a, b) => new Date(b.actiond_date) - new Date(a.actiond_date)
)
setRecentActions(data)
} catch (error) {
console.error('Failed to fetch recent actions', error)
}
}In general, it’s best to avoid broad useEffect calls because they become complexity hotbeds. The one responsible for calling the data fetching function is also responsible for managing the form’s state.
Let’s move the data-fetching logic away.
On one side it’s good to have the data fetched as close to where it’s used. On the other, if we fetch it higher in the component tree to avoid layout shifts and loading indicators. Let’s see how we can solve both problems with one solution.
An improvement that will save us a lot of time and complexity is if we use a data-fetching library like react-query.
const { data } = useQuery({
queryKey: ['recentActions'],
queryFn: fetchPreviousActions,
select: (data) =>
data.sort(
(a, b) => new Date(b.actiond_date) - new Date(a.actiond_date)
),
})This will replace our useEffect call and we can even move out the sorting from the fetching function to the useQuery call in order to leave the fetching function responsible only for the communication.
Additionally, this removes the need to use the recentActions state field, since we can use the returned data from useQuery. So by using a library to implement this logic, we’re offloading a lot of the complexity that we will no longer need to maintain.
We should use axios or ky instead of fetch because it will automatically result in an error if the request is unsuccessful and they provide a simpler API. This way we’ll be able to take advantage of react-query’s error built-in handling and we won’t have to manually throw an error if the request is not successful.
const fetchPreviousActions = async () => {
const { data } = await axios.get('https://api.com/actions')
return data
}Then we can go one step further and move the data fetching logic into a custom hook.
function useRecentActions() {
const { data } = useQuery({
queryKey: ['recentActions'],
queryFn: fetchPreviousActions,
select: (data) =>
data.sort(
(a, b) => new Date(b.actiond_date) - new Date(a.actiond_date)
),
})
return data
}This way we replace all this:
useEffect(() => {
fetchPreviousActions()
}, [])
const fetchPreviousActions = async () => {
try {
const response = await fetch('https://api.com/actions', {
method: 'GET',
headers: {
'Content-Type': 'application/json',
},
})
if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`)
}
const data = await response.json()
data.sort(
(a, b) => new Date(b.actiond_date) - new Date(a.actiond_date)
)
setRecentActions(data)
} catch (error) {
console.error('Failed to fetch recent actions', error)
}
}With a single cutom hook call:
const recentActions = useRecentActions()Use More Custom Hooks
What I’m getting at is that our components actually need very little of the data that we’re using in them.
They only need to render the final data that we return and sort but become aware of the libraries we’re using, how the data is formatted, what requests we’re making, where to, and so on.
They don’t need that information. And by “them” I mean “you” the person who will be extending this component in the future. You don’t need to be aware of all these details if you have a task to change where the form errors are being displayed in the markup.
These small decisions add up. If we add granular improvements to our design here and there, in the end, our components will be much simpler and easier to understand.
On the contrary, if we’re not mindful of the small complexity increments, they too will add up with time.
Controlling Request Cancellation
We know that we want our parent component to deal with the final data submission so let’s go over some of the details around it. This is how the handler looks currently.
const handleSubmit = async (event) => {
event.preventDefault()
if (!isFormValid) return
setShowOverlay(true)
setIsProcessing(true)
// Construct the form data object
const formData = {
userPersona,
startDate,
endDate,
numberOfVisits: parseInt(numberOfVisits, 10),
}
// Calling the API with the form data
await callBackendAPI(formData)
setIsProcessing(false)
}
const callBackendAPI = async (formData) => {
const controller = new AbortController()
setAbortController(controller)
formData.startDate = formatDate(formData.startDate)
formData.endDate = formatDate(formData.endDate)
try {
const response = await fetch('https://api.com/client', {
method: 'POST',
headers: {
'Content-Type': 'application/json',
},
body: JSON.stringify(formData),
signal: controller.signal,
})
if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`)
}
const data = await response.json()
setShowOverlay(false)
window.open(
'https://app.com/client/' + data.id,
'_blank',
'noopener,noreferrer'
)
window.location.reload()
} catch (error) {
if (error.name === 'AbortError') {
console.log('Scraping halted')
} else {
console.error('Failed to call the API', error)
}
} finally {
setShowOverlay(false)
setIsProcessing(false)
}
}There’s a lot to unpack here. We see that the callBackendAPI has a lot of mixed responsibilities between the HTTP logic, the redirects, and formatting the data before that. Its name could be more descriptive as well, right now it’s way too generic.
Instead of callBackendAPI we can call it storeAction or createAction.
The abortion controller is serving no purpose here because we’re making a POST request. If this was a query, then we could cancel the request because we’re no longer interested in the data it’s returning.
const controller = new AbortController()
setAbortController(controller)But queries are usually GET requests that have no side effects on the server.
With mutations, once the request is fired and the server is handling it, there’s no point in canceling it, the side effect will happen, and the record will be stored. So I believe we can safely remove the cancellation logic.
Removing it will allow us to remove another piece of component state and additional logic that we no longer need.
The next thing I noticed is that we’re setting two boolean state values together to signal that we’re processing a request and an overlay should be shown.
setShowOverlay(true)
setIsProcessing(true)But after examining the code we see that the overlay is only visible whenever a request is being processed. So the additional state value is redundant and only leads to more complexity in the UI.
If someone is reading the JSX they will be wondering when exactly we’re setting the showOverlay value. Otherwise, if we use the isProcessing value, we make it clear that the overlay appears while we’re waiting for a response
So here I would cut the showOverlay completely out of the component.
Additionally, if we use react-query for this logic, we can use the loading flag it provides for every mutation instead of managing another piece of state ourselves.
const { mutate, isLoading } = useMutation({
mutationFn: callBackendAPI,
onSuccess: (data) => {
window.open(
'https://app.com/client/' + data.id,
'_blank',
'noopener,noreferrer'
)
window.location.reload()
},
})And the function that’s calling the mutation can look like this:
const createAction = async (e) => {
e.preventDefault()
if (!isFormValid) {
return
}
mutate({
userPersona,
startDate: formatDate(startDate),
endDate: formatDate(endDate),
numberOfVisits: parseInt(numberOfVisits, 10),
})
}This function is now dealing with the data preparation, passing the proper data structure to the mutation.
Form Submissions
It’s easy to notice what component is using what state values and logic when we extract them because the IDE immediately highlights them.
Then I noticed that both forms are reusing the same form submission handler function. This is an obvious code smell because at best we have one function doing two things and this never helps with maintainability.
But in this example, it seems that the first form calling the submission function is completely unnecessary because it will always exit early thanks to the isFormValid check. The first form has two event handlers, one on the button submitting it and another handling the submission event.
Therefore, we can replace the form submission with the event that’s used on the button and just let the button actually submit the form.
<form onSubmit={handleSubmitCompanyNumber}>...</form>We’d have to refactor the actual handler as well since the form will have to send the value to its parent using the callback passed through props. This is how it looks currently.
const handleSubmitCompanyNumber = (number) => {
// this is unneeded, we've already set the value in state
setCompanyNumber(number)
if (number.length < 9) setNumberIncorrect(1)
else setNumberIncorrect(0)
}This is what I’d refactor it to.
const handleSubmitCompanyNumber = (e) => {
e.preventDefault()
if (number.length < 9) {
setNumberIncorrect(true)
return
}
onSubmit(number)
}I’ve removed the unnecessary state setting, the value is already in state and this is an unneeded re-render. When you’re worrying over re-rendering because of values passed by reference, consider if you’re unnecessarily setting state instead.
Then I’ve also removed the else statement in favor of an early return so we can use the conditional like a guard clause. This shows which is the main logical path of the function.
Also, I’m using the numberIncorrect value like a proper flag instead of using 0 and 1. We could also name the value something more generic like isFormCorrect to be consistent with our other form component.
Reconsidering useEffect
We discussed the broad useEffect call in the ActionForm component and came to the conclusion that it has to be refactored. But something that keeps bothering me is that validating the form on every keystroke doesn’t seem like a good idea.
useEffect(() => {
setIsFormValid(startDate && endDate && endDate > startDate)
}, [numberOfObfuscated, startDate, endDate])It would be better if we did this when the form was submitted or when the user clicked away from one of the fields. Now, validation when the user clicks away is a change in component logic and we know that when we’re refactoring we only want to change the design.
In the createAction function we can replace the simple check with a function call that validates the data.
const createAction = async (e) => {
e.preventDefault()
if (!formIsFilledOut()) {
setIsFormValid(false)
return
}
// ...
}Simplifying form validation
We can reduce a lot of complexity by finding ways to replace imperative logic with descriptive logic. We already saw this with data fetching, and it’s worth exploring how we can apply the same high-level idea with forms.
We can use a form library together with a schema library to make our validation easier and do it only when the form is submitted. This way we only need to describe the shape of the data we want to come out of the form and pass it back to the parent component when the form is successfully submitted. Then we render the other component and repeat, waiting for a successful submission.
const schema = z.object({
userPersona: z.string().nonempty('User Persona is required'),
startDate: z
.date()
.refine((date) => date instanceof Date, 'Start Date is required'),
endDate: z
.date()
.refine((date) => date instanceof Date, 'End Date is required')
.refine(
(date, ctx) => date > ctx.parent.startDate,
'End Date must be after Start Date'
),
})
const {
register,
handleSubmit,
formState: { errors },
} = useForm({
resolver: zodResolver(schema),
})Then our createAction function will only be called if the validation succeeds and the function will only be dealing with preparing the data for the mutation.
const createAction = async (e) => {
mutate({
userPersona,
startDate: formatDate(startDate),
endDate: formatDate(endDate),
numberOfVisits: parseInt(numberOfVisits, 10),
})
}I’m skipping some details in favor of simplicity, but take a look at the react-hook-form library to explore this approach further.
Are We Just Replacing Logic with Libraries?
We haven’t applied any masterful refactoring techniques to the code. We’ve only split it into smaller components and replaced some of the logic with library calls.
The truth is that high-quality code is often a result of just that - well-defined responsibilities and using the proper tools.
Data fetching and forms are perhaps the biggest sources of complexity in modern front-end development and thankfully we have tools that solve the common problems we have. Instead of reinventing the wheel, it helps to lean on them and use proven, tested solutions that help make our codebase simpler.
But the biggest factor behind my choice of technologies is their API. At the end of the day, I’m optimizing for maintainability and I want the code I leave behind to be easier to read and understand. Had they had a worse API I wouldn’t be advocating that strongly for them.
Refactor Render Functions into Components
A practice that I keep seeing in many codebases is having functions nested inside the component that render parts of its JSX. There’s a function in the ActionForm component that renders an overlay that can safely be extracted to its own component.
function LoadingOverlay() {
return (
<div className="loading-overlay">
<div className="loading-content">
<div className="loading-bar">Analyzing...</div>
<button
onClick={handleCancelaction}
className="cancel-button"
>
Cancel
</button>
</div>
</div>
)
}Then instead of calling a function in our JSX, we can use the component.
isLoading ? <LoadingOverlay /> : nullUnnecessary comments
Another anti-pattern that I find in many codebases is comments that just repeat what the code is already doing. This is one example from the component we’re refactoring.
const handleCancelaction = () => {
if (abortController) {
abortController.abort() // Abort the fetch request
}
setShowOverlay(false)
setIsProcessing(false)
}The comment here is absolutely necessary because it doesn’t add any context to the implementation. It just reiterates what the code is already describing. We can safely remove it without losing any valuable knowledge.
The same goes about this function in the original code:
const handleSubmit = async (event) => {
event.preventDefault()
if (!isFormValid) return
setShowOverlay(true)
setIsProcessing(true)
// Construct the form data object
const formData = {
userPersona,
startDate,
endDate,
numberOfVisits: parseInt(numberOfVisits, 10),
}
// Calling the API with the form data
await callBackendAPI(formData)
setIsProcessing(false)
}Write comments when you want to add context about the domain, don’t repeat the code.
Inline Styles
The inline styles speak that someone was in a hurry and wanted to get their job done quickly. Honestly, I’ve been there, I understand it.
But inline styles hurt the codebase’s maintainability and if you’d like to have them defined close to the markup I’d suggest a utility class library like Tailwind.
The shorthand classes save you time, and the library has a design system built in so you can have a consistently looking UI. I’ve gone into detail in this article if you want to learn more.
Refactor Loops into Components
One final little optimization that I’d do is extract the listing of the recent actions in another component. Loops in JSX add a lot of indentation and noise to the markup and I like to extract them away to keep the components simpler.
<div>
{recentActions.map((action) => (
<div key={action.id}>
<a href={action.link} target="_blank">
<span>r/{action.obfuscated}</span>{' '}
<span>{action.actiond_date} (UTC)</span>
</a>
</div>
))}
</div>We can turn this into a component:
<RecentActionsList />Summary
There’s plenty more we can do to improve the design of the component but I think we’ve addressed the objective sources of complexity - a bloated component, networking, forms, and state. Everything else starts to give marginal benefits.
If you manage to address these four points, then the rest of your component will be much easier to deal with.
