Simplify your code: the power of extracting functions in refactoring

Imagine you’ve got this snippet of code: (If this looks scary, try reading the original code from testdouble.js—I’ve made some changes to it here that make it a bit uglier)

function explainFunction (testDouble) {
  if (store.for(testDouble, false) == null) { return explainNonTestDouble(testDouble) }
  const calls = callsStore.for(testDouble)
  const stubs = stubbingsStore.for(testDouble)
  const { children } = explainChildren(testDouble)

  const stubbingDescription = stubs.length > 0
    ? _.reduce(stubs, (desc, stub) =>
      desc + `\n  - when called with \`(${stringifyArgs(stub.args)})\`, then ${planFor(stub)} ${argsFor(stub)}.`
    , '\n\nStubbings:')
    : ''

  const callDescription = calls.length > 0
    ? _.reduce(calls, (desc, call) => desc + `\n  - called with \`(${stringifyArgs(call.args)})\`.`, '\n\nInvocations:')
    : ''

  return {
    name: store.for(testDouble).name,
    callCount: calls.length,
    calls,
    description:
      `This test double ${stringifyName(testDouble)}has ${stubs.length} stubbings and ${calls.length} invocations.` +
      stubbingDescription +
      callDescription,
    children,
    isTestDouble: true
  }
}

This is a pretty long function. I think most developers would agree that it could use a little cleaning up; and probably also that the way to clean it up is to extract some smaller functions out of it. How do we decide though which functions to extract? Or whether to leave it alone? There’s a surprising amount of things to consider when doing this kind of refactoring.

I’m going to try to list as many of the possible effects of extracting a function that I can. You probably already know some of these, or maybe most of them. But—like a musician practicing scales, or a chess master working on tactics—spending a little bit of time working on the fundamental practices of our profession is probably a good idea.

Let’s look at what some simple cleanup to the above snippet might look like.

Extracting testdoubleDescription

  function explainFunction (testDouble) {
    ...

    return {
      name: store.for(testDouble).name,
      callCount: calls.length,
      calls,
      description:
        testdoubleDescription(testDouble, stubs, calls) +
        stubbingDescription +
        callDescription,
      children,
      isTestDouble: true
    }
  }

  function testdoubleDescription (testDouble, stubs, calls) {
    return `This test double ${stringifyName(testDouble)}has ${stubs.length} stubbings and ${calls.length} invocations.`
  }

I’ve extracted a tiny function here called testdoubleDescription, which might seem trivial, but already there’s already a lot to talk about:

A new name appears

testdoubleDescription is a concept that didn’t exist in the previous version of this code. Usually, if a piece of code seems hard to understand, adding a new name to the system helps. It’s also possible for a system to have badly named things, or to have too many different names for the same idea; these situations are confusing.

As we extract functions, we should pause to think carefully and pick the right name for the new chunk of code.

The original function is now changed, and is shorter

This probably seems obvious, but there are a couple of consequences I’d like to mention. First, shorter functions are easier to memorize. It’s often reported that humans have a short-term memory of “seven plus or minus two” things; this implies that should be about that many lines long. Remembering what a function does helps us understand bigger pieces of what the system does when we’re looking at other files.

Secondly though, now that we’ve changed the function, we’re implicitly asking other developers who’ve already memorized what it does to form new memories of how the system is structured. Some developers will react negatively to refactoring because we’re effectively asking them to do a little extra work, even if it makes it easier for us, and also for others who are new to this part of the system.

The caller of the new code is now less obvious

Prior to this extraction, any reader of this code could instantly tell you that it was only used in one place. Now that we’ve extracted a function, we have to do a search of the project to determine whether testdoubleDescription has more than one usage. If testdoubleDescription is a name that isn’t unique across the project, or if this function is called using reflection or metaprogramming, we might not be able to do that search easily.

Some languages have IDEs and tools that make finding usages easy even if the functions are named badly. This helps, but it’s still useful to acknowledge that there’s a “find usages” operation that future maintainers will have to do now that they didn’t before.

The variables we were using are now explicitly listed.

Before extracting testdoubleDescription, we had to read a long line of code before knowing which variables were being used. Now, we’ve got a pretty obvious list of what we need to get the job done. Sometimes, we’ll be surprised at how many variables we need to perform a calculation that we thought was simple. This can be a clue that leads us to more refactoring.

More options

These next three effects describe additional changes we can now make that we didn’t have the ability to make before we extracted this function. Note that in the new code…

We now have the option of moving code around

We’re now free to move testdoubleDescription to another place in the file, or to another file entirely if we want to.

We’ve created a testing seam

If we want to, we can write some tests for testdoubleDescription. Maybe we’re ok with keeping the code coverage we have around explainFunction, but maybe we like the idea of replacing some of those tests with smaller ones around testdoubleDescription—or maybe we have missing code coverage.

We’ve created a reusable block of code

If there’s duplicate code to what we’ve just extracted elsewhere in the system, we’ve got the option now of replacing that code with a call to our new function. Usually, we don’t want duplicate code in our system. I’m linking here to an online catalog that’s closely related to Martin Fowler’s excellent refactoring book. I’d highly recommend everyone reading this article get themselves a copy. In the section about replacing code with a function call, he mentions that there is such a thing as coincidental duplication, where we expect changes in one place not to require changes in the other. These are cases where we might want to leave duplication intact. These cases are unusual though.

Extracting stubbingDescription and callDescription

Let’s talk about some of the more subtle effects of extracting a function now. To do that, here’s another example. In this example, I’ve extracted two more new functions. stubbingDescription and callDescription

function explainFunction (testDouble) {
  if (store.for(testDouble, false) == null) { return explainNonTestDouble(testDouble) }
  const calls = callsStore.for(testDouble)
  const stubs = stubbingsStore.for(testDouble)
  const { children } = explainChildren(testDouble)

  return {
    name: store.for(testDouble).name,
    callCount: calls.length,
    calls,
    description:
      testdoubleDescription(testDouble, stubs, calls) +
      stubbingDescription(stubs) +
      callDescription(calls),
    children,
    isTestDouble: true
  }
}

function stubbingDescription (stubs) {
  return stubs.length > 0
    ? _.reduce(stubs, (desc, stub) =>
      desc + `\n  - when called with \`(${stringifyArgs(stub.args)})\`, then ${planFor(stub)} ${argsFor(stub)}.`
    , '\n\nStubbings:')
    : ''
}

function callDescription (calls) {
  return calls.length > 0
    ? _.reduce(calls, (desc, call) => desc + `\n  - called with \`(${stringifyArgs(call.args)})\`.`, '\n\nInvocations:')
    : ''
}

Return keyword changes meaning

Did you notice I cheated? I did a little more than just extracting functions to arrive at the above example. I also inlined the callDescription and stubbingDescription consts.

Extracting a function reduces the scope of the return keyword; sometimes this means we can eliminate some variables. This also means that the names of those variables don’t exist any more, but usually we can find a way to name our new function to preserve the information that was lost when we eliminated the name belonging to the variable we eliminated.

In this case, I named the functions exactly after the now-deleted variables.

Temporary variables within the extracted function now have a smaller scope

This is an effect on the explainFunction function. It’s now much more obvious to the reader that the code in stubbingDescription does not use calls, and that the code in callDescription does not use stubs. Similarly, neither of them use children. The scope of these variables now applies to less code than it did before.

To me this is one of the clearest advantages available when extracting functions. The problem though, is when this effect leads us to having long argument lists in our newly extracted functions. In the snippet above, testdoubleDescription is the worst offender with three arguments. In cases where argument lists become unwieldy, I’d consider introducing a parameter object (this also creates a name for the grouping of variables, which is sometimes a huge win).

There is now a new naming pattern

After extracting all of testdoubleDescription, stubbingDescription and callDescription, even if we hadn’t named them all similarly, it’d be a good idea to question whether we should. Since all three functions were born out of the same original function, chances are pretty good they’ve got some things in common, and that should be reflected in their names. We could also consider extracting a new class here to contain these functions as a way to quickly show readers of the code that these functions are related.

Final thoughts

I’d say that by far most of the time and in most software systems it’s best to extract functions liberally. That said, I’ve tried to stay somewhat neutral about the pros and cons of this refactoring in the above descriptions. Knowing about as many effects as possible of the changes you’re making to a system and weighing the benefits and drawbacks is a big part of what separates the novice developers from the good ones. Great developers will consider several different possible functions to extract before settling on the one that they think best balances all the effects of their changes.

If you made it this far, thanks for reading.

Good luck, and happy refactoring.