4.2 Refactoring Complex Code

Code is ever-evolving, and we’ll almost invariably end up with large projects that are not always the easiest to maintain. While we’ll reserve the following couple of sections for practical recommendations to reduce complexity at an architectural level, this section focuses on reducing complexity in portions of an application that are already complex.

4.2.1 Embracing Variables over Clever Code

Complex code is predominantly shorter than it should be, and often deceitfully so. An expression that might have involved 5 to 10 short lines of code usually ends up being represented in 1 or 2 clever lines of code. The problem with clever code is that we need to expend time and energy to read it whenever it’s intent is not clear on our mind, which is only the case when we first write said code or right after spending considerable time analyzing it.

One of the underlying issues that can be identified when reading complex code is that it uses few variables. In the dawn of programming, memory resources were scarce and thus programmers had to optimize allocation and this often meant reusing variables and using fewer of them. In modern systems, we don’t have the need to treat memory as a sacred, precious, and limited resource. Instead, we can focus on making programs readable to both our future selves and fellow developers.

Readability is better served by an abundance of properly named variables or functions than by sparsity. Consider the following example, part of a larger routine, where a program ensures that the user is currently logged in with a valid session, and otherwise redirects them to a login page.

if (
  auth !== undefined &&
  auth.token !== undefined &&
  auth.expires > Date.now()
) {
  // we have a valid token that hasn't expired yet
  return
}

As the routine becomes larger, we collect if statements with non-obvious or complicated clauses, such as the reason why we’re checking auth has a token value if we’re not doing anything with it here. The solution is usually to add a comment explaining the reason why this check exists. In this case, the comment tells us this is a valid token that hasn’t expired. We could turn that comment into code, and simplify the if statement in the process, by creating a small function that breaks down the conditional, as shown next.

function hasValidToken(auth) {
  if (auth === undefined || auth.token === undefined) {
    return false
  }
  const hasNotExpiredYet = auth.expires > Date.now()
  return hasNotExpiredYet
}

We can now turn our if statement plus comment into a function call, as shown in the following bit of code. Certainly, the totality of our refactored code is a bit longer, but now it’s self-descriptive. Code that describes what it does in the process of doing it doesn’t require as many comments, and that’s important because comments can become easily outdated. Moreover, we’ve extracted the long conditional in the if statement to a function, which keeps us more focused while parsing the codebase. If every condition or task was inline, we’d have to understand everything in order to understand how a program works. When we offload tasks and conditions to other functions, we’re letting the reader know they can trust hasValidToken to check for validity of the auth object, and the conditional becomes a lot easier to digest.

if (hasValidToken(auth)) {
  return
}

We could’ve used more variables without creating a function, inlining the computation of hasValidToken right before the if check. A crucial difference between the function-based refactor and the inlining solution is that we used a short-circuiting return statement to preemptively bail when we already knew the token was invalid^[1], however we can’t use return statements to bail from the snippet of code that computes hasValidToken in the following piece of code without coupling its computation to knowledge about what the routine should return for failure cases. As a result, our only options are tightly coupling the inline subroutine to its containing function, or using a logical or ternary operator in the intermediate steps of the inlined computation.

const hasToken = auth !== undefined && auth.token !== undefined
const hasValidToken = hasToken && auth.expires > Date.now()
if (hasValidToken) {
  return
}

Both of these options have their downsides. If we couple the return statements with the parent function, we’ll need to be careful if we want to replicate the logic elsewhere, as the return statements and possibly their logic will have to adapt as well. If we decide to use ternary operators as a way of short-circuiting, we’ll end up with logic that might be as complex as what we originally had in the if statement.

Using a function not only avoids these two problems thanks to the ability to return intermediate results, but also defers reasoning about its contents until we actually need to understand how tokens are checked for validity.

While moving conditionals to a function might sound like a trivial task, this approach is at the heart of modular design. It is by composing small bits of complexity using several additive functions that we can build large applications that are less straining to read. A large pool of mostly trivial functions can add up to a veritable codebase where each bit of code is relatively isolated and easy to understand, provided we trust functions do what their name says they do. In this vein, it is of utmost importance to think long and deep about the name of every function, every variable, and every package, directory, or data structure we conceive.

When used deliberately and extensively, early returns — sometimes referred to as guard clauses or short-circuits — can be unparalleled when it comes to making an application as readable as possible. Let’s explore this concept in further detail.

4.2.2 Guard Clauses and Branch Flipping

When we have a long branch inside a conditional statement, chances are we’re doing something wrong. Pieces of code like the following are commonplace in real world applications, with a long success case branch taking up significant amounts of code while having several else branches sprinkled near the end that would log an error, throw, return, or otherwise perform a failure handling action.

if (response) {
  if (!response.errors) {
    // … use `response`
  } else {
    return false
  }
} else {
  return false
}

In the example, we’re optimizing readability for the success case, while the failure handling is relegated to the very end of our piece of code. There’s several problems with this approach. For one, we have to indulge in unnecessary nesting of every success condition, or otherwise put them all in a huge conditional statement. While it’s rather easy to understand the success case, things can get tricky when we’re trying to debug programs like this, as we need to keep the conditionals in our head the whole time we’re reading the program.

A better alternative is to flip the conditionals, placing all failure handling statements near the top. While counterintuitive at first, this approach has several benefits. It reduces nesting and eliminates else branches while promoting failure handling to the top of our code, and this has the added benefit that we’ll become more aware of error handling and naturally gravitate towards thinking about the failure cases first, a great trait to have when doing application development, where forgetting to handle a failure case might result in an inconsistent experience for end users with a hard-to-trace error on top.

if (!response) {
  return false
}
if (response.errors) {
  return false
}
// … use `response`

This early exit approach is often referred to as guard clauses, and one of their biggest benefits is that we can learn all the failure cases upon reading the first few lines of a function or piece of code. We’re not limited to return statements: we could throw errors in a promise-based context or in an async function, and in callback chaining contexts we might opt for a done(error) callback followed by a return statement.

Another benefit of guard clauses is almost implicit: given that they’re placed near the top of a function, we have quick access to its parameters, we can better understand how the function validates its inputs, and we can more effectively decide we need to add new guard clauses to improve validation rules.

Guard clauses don’t tell the reader everything they need to know that might go wrong when calling a function, but they give them a peek into expected immediate failure cases. Other things that might go wrong lie in the implementation details of the function. Perhaps we use a different service or library to fulfill the bulk of our function’s task, and that service or library comes with its own set of nested guard clauses and potential failure cases that will bubble up all the way to our own function’s outcome.

4.2.3 An Interdependency Pyramid

Writing straightforward code is not all that different from writing other straightforward texts. Texts are often arranged in paragraphs, which are somewhat comparable with functions: we can consider their input to be the reader’s knowledge and everything else they’ve read so far in the text, and the output can be what the reader gets out of the paragraph.

Within a book chapter or any other piece of long-form text, paragraphs are organized in a sequential manner, allowing the reader time to digest each paragraph before they jump onto the next. The logical sequence is very much intentional: without a coherent sequencing, it would be nearly impossible to make sense of a text. Thus, writers optimize for making sure concepts are introduced before they’re discussed, providing context to the reader.

Function expressions such as the one in the next snippet won’t be assigned to the variable binding until the line containing the assignment is evaluated. Until then, the variable binding exists in the scope, thanks to hoisting, but it is undefined until the assignment statement is evaluated.

double(6) // TypeError: double is not a function
var double = function(x) {
  return x * 2
}

Furthermore, if we’re dealing with a let or const binding, then TDZ semantics produce an error if we reference the binding at all before the variable declaration statement is reached.

double(6) // TypeError: double is not defined
const double = function(x) {
  return x * 2
}

Function declarations like the one in the following snippet, in contrast, are hoisted to the top of the scope. This means we can reference them anywhere in our code.

double(6) // 12
function double(x) {
  return x * 2
}

Now, we mentioned texts are written sequentially, and how the writers avoids surprises by presenting concepts before discussing them. Establishing a context in a program is a different endeavor, however. If we have a module that has the goal of rendering a chart with user engagement statistics, the top of the function should address things the reader already knows, namely the high-level flow for what the rendering function is meant to do: analyze the data, construct some data views and model that data into something we can feed into a visualization library that then renders the desired chart.

What we have to avoid is jumping directly into unimportant functions such as a data point label formatter, or the specifics of the data modelling. By keeping only the high-level flow near the top, and the specifics towards the end, complex functionality can be designed in such a way that the reader experiences a zoomed out overview of the functionality at first, and as they read the code they uncover the details of how this chart was implemented.

In a concrete sense, this means we should present functions in a codebase in the order that they’ll be read by the consumer (a first-in first-out queue), and not in the execution order (a last-in first-out stack). While computers do as they’re told and dig ever deeper into the flow, executing the most deeply nested routines before jumping out of a series of subroutines and executing the next line, this is an unfruitful way for humans to read a codebase, given we’re ill-suited to keeping all that state in our heads.

Perhaps a more specific analogy for this kind of spiraling approach can be found in newspaper articles, where the author typically offers a title that describes an event at the highest possible level, and then follows up with a lead paragraph that summarizes what happened, again at a high-level. The body of the article starts also at a high-level, carefully avoiding to spook the reader with too many details. It is only midway through the article that we’ll start finding details about the event which, aided by the context set forth in the beginning of the article, can give us a complete picture of what transpired.

Given the stack-based nature of programming, it’s not that easy to naturally approach programs as if they were newspaper articles. We can, however, defer execution of implementation details to other functions or subroutines, and thanks to hoisting, we can place those subroutines after their higher level counterparts. In doing so, we’re organizing our programs in a way that invites readers in, shows them a few high-level hints, and then gradually unveils the spooky details of how a feature is implemented.

4.2.4 Extracting Functions

Deliberate, pyramidal structures where we deal with higher level concerns near the top and switch to more specific problems as we go deeper into the inner workings of a system works wonders in keeping complexity on a tight leash. Such structures are particularly powerful because they break up complex items into their own individual units near the flat bottom of the system, avoiding a complicated interweaving of concerns that are fuzzied together, becoming undistinguishable from one another over time.

Pushing anything that gets in the way of the current flow to the bottom of a function is an effective way of streamlining readability. As an example, consider the case where we have a non-trivial mapper inline in the heart of a function. In the following code snippet we’re mapping the users into user models, as we often need to do when preparing JSON responses for API calls.

function getUserModels(done) {
  findUsers((err, users) => {
    if (err) {
      done(err)
      return
    }
 
    const models = users.map(user => {
      const { name, email } = user
      const model = { name, email }
      if (user.type.includes('admin')) {
        model.admin = true
      }
      return model
    })
 
    done(null, models)
  })
}

Now compare that code to the following bit of code, where we extracted the mapping function and shoved it out of the way. Given the mapping function doesn’t need any of the scope from getUserModels, we can pull it out of that scope entirely, without the need to place toUserModel at the bottom of the getUserModels function. This means we can now also reuse toUserModel in other routines, we don’t have to wonder whether the function actually depends on any of the containing scope’s context anymore, and getUserModels is now focused on the higher level flow where we find users, map them to their models, and return them.

function getUserModels(done) {
  findUsers((err, users) => {
    if (err) {
      done(err)
      return
    }
 
    const models = users.map(toUserModel)
 
    done(null, models)
  })
}
 
function toUserModel(user) {
  const { name, email } = user
  const model = { name, email }
  if (user.type.includes('admin')) {
    model.admin = true
  }
  return model
}

Furthermore, if there were additional work to be done between the mapping and the callback, it could also be moved into another small function that wouldn’t get in the way of our higher level getUserModels function.

A similar case occurs when we have a variable that gets defined based on a condition, as shown in the next snippet. Bits of code like this can distract the reader away from the core purpose of a function, to the point where they’re often ignored or glossed over.

// …
let website = null
if (user.details) {
  website = user.details.website
} else if (user.website) {
  website = user.website
}
// …

It’s best to refactor this kind of assignments into a function, like the one shown next. Note how we’ve included a user parameter so that we can push the function out of the scope chain where we’ve originally defined the user object, and at the same time went from a let binding to a const binding. When reading this piece of code later down the line, the benefit of const is that we’ll know the binding won’t change, as opposed to let with which we can’t be certain bindings won’t change over time, adding to the pile of things the reader should be watching out for when trying to understand the algorithm.

// …
const website = getUserWebsite(user)
// …
 
function getUserWebsite(user) {
  if (user.details) {
    return user.details.website
  }
  if (user.website) {
    return user.website
  }
  return null
}

Regardless of your flavor of choice when it comes to variable binding, bits of code that select some slice of application state are best shoved away from the relevant logic that will use this selected state to perform some action. This way, we’re not distracted with concerns about how state is selected, instead of being focused on the action that our application logic is trying to carry out.

When we want to name an aspect of a routine without adding a comment, we could create a function to host that functionality. Doing so doesn’t just give a name to what the algorithm is doing, but it also allows us to push that code out of the way, leaving behind only the high-level description of what’s going to happen.

4.2.5 Flattening Nested Callbacks

Codebases with asynchronous code flows often fall into the so-called "callback hell", where each callback creates a new level of indentation, making code harder and harder to read as we approach the deep end of the asynchronous flow chain.

a(function () {
  b(function () {
    c(function () {
      d(function () {
        console.log('hi!')
      })
    })
  })
})

The foremost problem with this kind of structure is scope inheritance. In the deepest callback, passed to the d function, we’ve inherited the combined scopes of all the parent callbacks. As functions become larger, and more variables are bound into each of these scopes, it becomes ever more challenging to understand one of the callbacks in isolation from its parents.

This kind of coupling can be reverted by naming the callbacks and placing them all in the same nesting level. Named functions may be reused in other parts of our component, or exported to be used elsewhere. In the following example we’ve eliminated up to 3 levels of unnecessary nesting, and by eliminating nesting we’ve made the scope for each function more explicit.

a(a1)
function a1() {
  b(b1)
}
function b1() {
  c(c1)
}
function c1() {
  d(d1)
}
function d1() {
  console.log('hi!')
}

When we do need some of the variables that existed in the parent scope, we can explicitly pass them on to the next callback in the chain. The following example passes an arrow function to d, as opposed to passing the d1 callback directly. When executed, the arrow function ends up calling d1 anyway, but now it has the additional parameters we needed. These parameters can come from anywhere, and we can do this throughout the chain, while keeping it all in the same indentation level.

a(a1)
function a1() {
  b(b1)
}
function b1() {
  c(c1)
}
function c1() {
  d(() => d1('hi!'))
}
function d1(salute) {
  console.log(salute) // <- 'hi!'
}

Now, this could also be resolved using a library such as async, which simplifies the flattened chaining process by establishing patterns. The async.series method accepts an array of task functions. When called, the first task is executed and async waits until the next callback is invoked, before jumping onto the next task. When all tasks have been executed, or an error arises in one of the tasks, the completion callback in the second argument passed to async.series is executed. In the following illustrative example, each of the 3 tasks is executed in series, one at a time, waiting a second before each task signals its own completion. Lastly, the 'done!' message is printed to the console.

async.series([
  next => setTimeout(() => next(), 1000),
  next => setTimeout(() => next(), 1000),
  next => setTimeout(() => next(), 1000)
], err => console.log(err ? 'failed!' : 'done!'))

Libraries like async come with several ways of mixing and matching asynchronous code flows, in series or concurrent, allowing us to pass variables between callbacks without having to nest together entire asynchronous flows.

Naturally, callbacks aren’t the only asynchronous flow pattern that might end up in hell. Promises can end up in this state just as easily, as shown in the next contrived snippet.

Promise.resolve(1).then(() =>
  Promise.resolve(2).then(() =>
    Promise.resolve(3).then(() =>
      Promise.resolve(4).then(value => {
        console.log(value) // <- 4
      })
    )
  )
)

A similar piece of code that wouldn’t be affected by the nesting problem is shown next. Here, we’re taking advantage that promises behave in a tree-like manner, where we don’t necessarily need to attach reactions onto the last promise, and instead we can return those promises so that the chaining can always occur at the top level, allowing us to avoid any and all scope inheritance.

Promise.resolve(1)
  .then(() => Promise.resolve(2))
  .then(() => Promise.resolve(3))
  .then(() => Promise.resolve(4))
  .then(value => {
    console.log(value) // <- 4
  })

Similarly, using async functions can turn what was previously a promise-based flow and turn it into something that can be mapped to our own mental model of how the program’s execution flows. The following bit of code is similar to the last snippet we looked at, but using async/await instead.

async function main() {
  await Promise.resolve(1)
  await Promise.resolve(2)
  await Promise.resolve(3)
  const value = await Promise.resolve(4)
  console.log(value) // <- 4
}

4.2.6 Factoring Similar Tasks

We’ve already discussed at length why creating abstractions isn’t always the best way of reducing complexity in an application. Abstractions can be particularly damaging when created too early: at the time we might not have enough information about the shape and requirements for other components that we might want to hide behind the abstraction layer, and over time we might end up aggressively shaping components only so that they fit the abstraction, which could have been avoided by not settling for an abstraction too early.

When we do avoid creating abstractions prematurely, we’ll start noticing functions that have an uncanny resemblance to the shape of similar functions: maybe the flow is identical, maybe the output is similar, or maybe all that really changes is we’re accessing an attribute named href in one case and an attribute named src in another case.

Consider the case of an HTML crawler which needs to pull out snippets of an HTML page and reuse them later in a different context. Among other things, this crawler needs to take relative resource locators like /weekly and resolve them to absolute endpoints like https://ponyfoo.com/weekly, depending on the origin where the resource was crawled from. This way, the HTML snippets can then be repurposed on other mediums such as on a different origin or a PDF file, without breaking the end-user experience.

The following piece of code takes a piece of HTML and transforms a[href] and img[src] into absolute endpoints using the $ jQuery-like DOM utility library.

function absolutizeHtml(html, origin) {
  const $dom = $(html)
  $dom.find('a[href]').each(function () {
    const $element = $(this)
    const href = $element.attr('href')
    const absolute = absolutize(href, origin)
    $element.attr('href', absolute)
  })
  $dom.find('img[src]').each(function () {
    const $element = $(this)
    const src = $element.attr('src')
    const absolute = absolutize(src, origin)
    $element.attr('src', absolute)
  })
  return $dom.html()
}

As the small function it is, it’d be perfectly acceptable to keep absolutizeHtml as-is. However, if we later decide to add iframe[src], script[src], and link[href] to the list of attributes that might contain endpoints we want to transform, we’ll probably want to avoid having five copies of the same routine, as that’s more likely to be confusing and result in changes being made to one of them without being mirrored in the other cases, increasing complexity.

The following bit of code keeps all attributes we want to transform in an array, and abstracts the repeated bit of code so that it’s reused for every tag and attribute.

const attributes = [
  ['a', 'href'],
  ['img', 'src'],
  ['iframe', 'src'],
  ['script', 'src'],
  ['link', 'href']
]
 
function absolutizeHtml(html, origin) {
  const $dom = $(html)
  attributes.forEach(absolutizeAttribute)
  return $dom.html()
 
  function absolutizeAttribute([ tag, property ]) {
    $dom.find(`${ tag }[${ property }]`).each(function () {
      const $element = $(this)
      const value = $element.attr(property)
      const absolute = absolutize(value, origin)
      $element.attr(property, absolute)
    })
  }
}

A similar situation occurs when we have a concurrent flow that remains more or less constant across a number of different functions, in which case we might want to consider keeping the flow in its own function, and passing a callback for the actual processing logic that is different in each case.

In other cases, we might notice how there’s a few different components that all need the same piece of functionality. Commenting features often fall in this case, where different components like user profiles, projects, or artifacts, might need the ability to receive, show, edit, and delete comments. This case can be interesting because it’s the business requirement is not always identified upfront, and we might embed the child feature into the parent component before realizing it’d be useful to extract the feature so that it can be reused in other parent components. While this sounds obvious in hindsight, it’s not always clear when we’ll need to reuse some functionality somewhere else, and keeping every aspect of functionality isolated just in case we need to reuse them can be costly in terms of time and development effort.

More often than not, however, abstractions can end up complicating matters. It might be that the trade-off isn’t worth it because the code becomes much harder to read, or maybe because the underlying code isn’t mature enough yet, or we don’t know what special requirements we may end up with for other objects adopting similar functionality, meaning we’re not comfortable creating an abstraction that could lead to unforeseen problems in the future.

Whenever we are uncertain about whether an abstraction is up to muster, it pays to go back to the original piece of code we had before introducing the abstraction, and comparing the two pieces. Is the new piece easier to understand, modify, and consume? Would that still be the case as a newcomer? Try and consider how the outcome to those questions would change if you hadn’t looked at this code in a while. Ask your co-workers for their opinion, too; given they haven’t seen that code yet and they may end up having to consume it, they’re great candidates to help decide which approach is better.

4.2.7 Slicing Large Functions

Consider breaking what would otherwise inevitably be a single large function into smaller functions. These may be organized by splitting functionality by steps, by different aspects of the same task, always relying on guard clauses to do all of our error checking up front, ensuring that state is constrained by what we allow it to be at each point in time.

The overall structure of your typical function should begin with guard clauses, making sure the input we receive is what we expect: enforcing required parameters, their correct data types, correct data ranges, and so on. If these inputs are malformed, we should bail immediately, ensuring we don’t work with inputs we’re unprepared to deal with, and ensuring the consumer gets an error message that explains the root reason why they’re not getting the results they expect, as opposed to a message that might involve debugging work, such as undefined is not a function caused by trying to call an input that was supposed to be a function but wasn’t — or was supposed to result in our routine finding a function, but didn’t.

Once we know the inputs are well-formed, data processing can begin. We’ll transform the inputs, map them to the output we want to produce, and return that output. Here we have the opportunity to break apart the function into several pieces. Each aspect of the transformation of inputs into output is potentially its own function. The way of reducing complexity in a function is not by collapsing hundreds of lines of code into tens of complicated lines of code. Instead, we can move each of these long pieces of code into individual functions that only deal with one aspect of the data. Those functions can then also be hoisted out of our function and onto its parent scope, showing that there wasn’t a reason why a particular aspect of transforming the inputs had to be coupled to the entire function doing the transformation.

Each aspect of a transformation operation can be analyzed and moved into its own function. The smaller function may take a few of the inputs in the larger function, or perhaps some of the intermediate values that were produced in the larger function. It can then conduct its own input sanitization, and be broken apart even further. The process of identifying aspects of an operation that can be recursively compartimentalized and moved into their own functions is highly effective because it allows for dauntingly large functions to be broken into simpler pieces that aren’t as daunting to refactor.

At first, we can identify the 3 or 4 largest aspects of a function, and break those apart. The first part might involve filtering out the parts of the input we’re not interested in, the second might involve mapping that into something else, and the third part might involve merging all of the data together. Once we’ve identified each aspect of the function we might break those into their own functions, with their own inputs and output. Subsequently, we can do this for each of those smaller functions.

We can keep doing this for as long as there’s opportunity for the functions to be simplified. As discussed in the previous section, it’s valuable to take a step back after each of these refactors, and evaluate whether the end result is indeed simpler and easier to work with than what we had before it was refactored.