Last week I wrote a post about the best time to write tests and hinted at preparatory refactors. In this post, we dig into the implementation details with React components.
Let me tell you a story.
You work on an application which has has a Form.
And it’s a slightly smart form, it doesn’t let users click the submit button until you give it some input. Just some small UX niceness.
Now, you have been assigned the task to add another field in this form while maintaining the smart functionality.
Pfft, Just another input field. No big deal, right?
Let’s open up the code for this component. The code for the component itself is not that interesting.
function Form(props) {
/* we need a disabled state and an onChange handler */
const [disabled, onChange] = useSmartForm()
return (
<form>
<input
onChange={onChange} name="username"
placeholder="github username"
/>
<button type="submit" disabled={disabled}> See profile
</button>
</form>
)
}This Form component doesn’t have the logic for making the form smart.
The logic is abstracted in a custom hook useSmartForm, which provides the values Form can use - disabled and the onChange handler.
This abstraction seems like a good idea because we probably use this in other forms across the application as well. Whoever wrote this must be smart.
Let’s look at the code inside this custom hook:
function useSmartForm() {
// create a state pair with default value true
const [disabled, setDisabled] = React.useState(true)
function onChange(event) {
// logic for deciding disabled state
const value = event.target.value
if (value.length > 0) setDisabled(false)
else setDisabled(true)
}
// return the two variables form can use
return [disabled, onChange]
}Nothing too fancy. I love how easy it is to move functionality into custom hooks.
(If React Hooks are new to you, read this post first)
Now that you know how this smart form works, let’s talk about adding that field.
Wait, this custom hook supports only one field. What?! Who wrote this stupid piece of code??
It’s like they didn’t even think of this use case. A change that was supposed to simple is going to be a lot of work.
Sometimes I run git blame on a file like this one and find my own commit from 6 months ago. 😅
The person who wrote that piece of code, most likely did the best job they could based on the information they had at that point. However, that information is no longer valid. So, it’s our job to update it.
To add this functionality, it will require us to make changes in the Form component, the custom hook and related tests at the same time.
The clean code gurus call this Shotgun surgery - firing changes in too many places and hope you don’t break anything else.
Been there, screwed that up.
Image credit: Alexander Shvets and Dmitry Zhart
The strategy I like to follow now (as explained in last week’s post) is:
make the change easy, then make the easy change – Kent Beck
Step 1: Make the change easy (to perform): Refactor the hook with this new information (support for multiple fields). This step is known as the preparatory refactor.
Step 2: Make the easy change: Add the second form field.
Before we jump on to refactor our custom hook, it would be really cool if we already had tests so that we don’t accidentally introduce any bugs.
Did the person who wrote the original code also write tests for it? Probably not.
Let’s be honest, it was probably you who wrote that code and didn’t write any tests. But, I don’t blame you.
Maintaining a good test suite with respectable code coverage is hard. Quite often, I find myself in the situation where I have to make a complex change, but don’t have enough tests.
This is where you introduce Step 0.9: Write just one test only for the part you’re about to change - the form component.
My favorite way of writing tests is with react-testing-library.
Here’s what the test looks like:
import { render, fireEvent } from 'react-testing-library'
import Form from './form'
test('Button should be disabled by default', () => {
const form = render(<Form />)
// get button by text
const button = form.getByText('See profile') // check disabled property for button expect(button.disabled).toBe(true)})
test('Button should become enabled on input', () => {
const form = render(<Form />)
// get input by placeholder
const input = utils.getByPlaceholderText(
'github username'
)
// trigger change event on input
fireEvent.change(input, { target: { value: 'f' } }) // check disabled property for button const button = form.getByText('See profile') expect(button.disabled).toBe(false)})You can run the test with jest
Looks good. I’d push this as a commit or even pull request of it’s own.
There are a few things to really like here:
- There is very little implementation detail of the form component in this test, so you can change the underlying code without changing the tests (big code smell if that’s not the case)
- Our test identifies the input and button the same way a user would - by text / placeholder.
Now, that we have some simple tests, let’s jump on to Step 1: Preparatory refactor.
In this step, we’re not interesting in adding the new form field just yet. We want to refactor the custom hook to make it look like when we wrote it, we already knew it will have to support multiple fields in the future.
Following is the starting point, copy pasted from above:
function useSmartForm() {
// create a state pair with default value true
const [disabled, setDisabled] = React.useState(true)
function onChange(event) {
// logic for deciding disabled state
const disabled = event.target.value.length === 0
setDisabled(disabled)
}
// return the two variables form can use
return [disabled, onChange]
}Milestone 1: Change the data structures but still support just one field
function useSmartForm() {
// replace single disabled value // with array of disabled values // but with only one element const defaultList = [true] const [disabledList, setDisabledList] = React.useState( defaultList )
// array of change handlers const onChangeHandlers = []
// with only one element
onChangeHandlers.push(function onChange(event) {
const disabled = event.target.value.length === 0
setDisabledList(disabledList => { // set the value for the first element disabledList[0] = disabled return disabledList }) })
// get disabled for button based on the list
const globalDisabled = disabledList[0]
// return globalDisabled and // spread the onChange handlers return [globalDisabled, ...onChangeHandlers]}No changes made to the Form component yet, let’s make sure the existing functionality still works.
Milestone 2: Add support for multiple fields
// accept optional variable for num of fields with default 1
function useSmartForm(numberOfFields = 1) { // replace single disabled value
// with array of disabled values
const defaultList = Array(inputs).fill(true) const [disabledList, setDisabledList] = React.useState(
defaultList
)
// array of change handlers
const onChangeHandlers = []
// create an onChange handler for each input
for (let index = 0; index < inputs; index++) { onChangeHandlers.push(function onChange(event) {
const disabled = event.target.value.length === 0
setDisabledList(disabledList => {
// set the value for the correct index
disabledList[index] = disabled return disabledList
})
})
}
let globalDisabled = false
// If even one of the values is disabled
// the global disabled should be true
if (disabledArray.find(value => value === true)) { globalDisabled = true }
// return globalDisabled and
// spread the onChange handlers
return [globalDisabled, ...onChangeHandlers]
}Running tests once more,
Great.
Again. we haven’t touched the Form component yet. Even if we never add that field, this is still useful work. Let’s push this in a commit / pull request.
You ready to make the easy change to Form?
function Form(props) {
const numberOfFields = 2 const [ disabled, onUserChange, onOrgChange ] = useSmartForm(numberOfFields)
return (
<form>
<input
onChange={onUserChange} name="username"
placeholder="github username"
/>
<input
onChange={onOrgChange} name="organisation"
placeholder="github org"
/>
<button type="submit" disabled={disabled}>
See profile
</button>
</form>
)
}Easy, right?
Now that we have changed the functionality of Form, the tests should have failed. (we broke the old logic intentionally)
Final step, let’s update this test with a trigger for the second field.
test('Button should become enabled on input', () => {
const form = render(<Form />)
// get input by placeholder
const input = utils.getByPlaceholderText(
'github username'
)
fireEvent.change(input, { target: { value: 'f' } })
const orgInput = utils.getByPlaceholderText('github org') fireEvent.change(orgInput, { target: { value: 'facebook' } })
// check disabled property for button
const button = form.getByText('See profile')
expect(button.disabled).toBe(false)
})That should do it.
To summarise,
Instead of trying to make changes in too many files at once, perform a preparatory refactor first - to make the change easy.
If you don’t have tests, write a small test, just for the part you’re about to change.
Then make the easy change.
Hope that was helpful on your journey
Sid