35. Shotgun surgery and React components

April 05, 2019

I wrote this post for my newsletter, sign up here to get emails like these every week.


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.

smart form

Now, you have been assigned the task to add another field in this form while maintaining the smart functionality.

two fields

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.

shotgun surgery

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

first run

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:

  1. 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)
  2. 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.

running tests again

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,

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)

failed tests

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.

and we're back

 

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


Want articles like this in your inbox every Friday?
Javascript and personal growth. No spam, I promise!