Dev Diary #2: Run Away from Nesting Functions in Your Code

Picture the scene: You have a critical bugfix to make in your payments system, and the stakeholder is nervously watching on from the corner of the room asking helpful things like “How close are we to the fix?”.

You start by looking at some code in get-payment-methods.ts, the top-level entry-point of an API route.

// get-payment-methods.ts

const getPaymentMethods = async (req) => {
  const paymentMethods = await loadPaymentMethodsForCustomer(req.customerId);
  return res.send(paymentMethods);
}

That looks reasonable, you think to yourself.

Problem #1: It’s hard to navigate

You need have a look at what loadPaymentMethodsForCustomer() does. Ok, let's go.

CMD-click.

// services/payment-methods.ts

const loadPaymentMethodsForCustomer = async (customerId) => {
  try {
    const traceId = uuid();
    const customerPaymentMethods = getPaymentMethodsOrThrow(customerId, traceId);
    return customerPaymentMethods.map(someConversionFunction);
  } catch(err) {
    return errorHandler(err);
  }
}

Hmm. Now you need to look at getPaymentMethodsOrThrow().

CMD-click. CMD-click. CMD-click.

Deeper and deeper we go into the tangled web of nested functions. Sunlight can’t penetrate this deep into the codebase, and long ago you lost track of why you are here.

Some time later, you arrive at Ground Zero, where the Actual Thing Is Happening. In this case, the code is making a call to Stripe’s API to fetch a customer’s saved payment methods.

// repositories/stripe.ts

await stripe.paymentMethods.list({  customer: stripeCustomerId  });

Finally!

Problem #2: It’s hard to understand the sequence of events

Sure, you found the bottom of this particular set of nested functions. But now you are trying to understand what the code does to the data, for example calculation, transformation, and mapping.

Typically with nested functions, something is happening to the data at almost every level in the tree. For example, one function might do a try/catch. Another might convert the array to just return the id field of every payment method. Another might supplement that information with a second API call. And even after you have followed the loop all the way to the bottom of the tree and back up again, there might be an entire second set of nested functions that comes afterwards.

In short, it is nearly impossible to build a mental picture of the sequence of events. You might know in your heart that somewhere there is a logical flow comprising ten things that happen one after another, but it’s going to take you three days and six whiteboard pens to figure it out.

Problem #3: It’s brittle to change

It’s time to make the code change! You steel yourself. Let’s go.

if(paymentMethod.expiry < new Date()) {
  throw new Error("Oh dang, try a new card");
}

Ok. Let’s run the tests.

npm test

Instant fire 🔥🔥🔥. Explosions. 600 tests failed. What happened?!

The problem with deeply nesting functions is that the dependency graph between different parts of your codebase quickly becomes orders of magnitude more complex than it needs to be.

Let’s say that Function A calls Function B, which calls Function C. Function X also calls Function B, which calls Function C. Function Y calls Function Z, which in turn calls Function C, too.

So when you changed Function C, you broke functions, A, B, X, Y, and Z.

You spend three hours trying to understand exactly how all of those functions work. You give up, and just copy/paste Function C into a new area to avoid breaking all the other stuff. Sad face emoji 😔

Problem #4: It’s much harder to test

Let’s say that you have Function A, which calls Function B.

const functionA = async () => {
  // complicated things here

  const data = await functionB();

  // more complicated things here
}

You have some complicated logic in Function A. You decide that it is deserving of some nice, thorough unit testing.

The problem here is that to test Function A, you have two options:

1. Test Function A and Function B

2. Use mocks to bypass Function B

   
   

The problem with Option 1 is that now our unit tests are not really unit tests. We only really wanted to test the pure logic, and now we’re doing stuff with async calls, which requires a lot more effort.

The problem with Option 2 is that mocks are nasty and reduce confidence in your tests. They are a useful tool when absolutely necessary, but the moment you change functionB, your tests for functionA are now at risk of giving you false positives.

Neither option is great, and before you know it, you are part of the Testing Is Hard And I Don’t Like It Club. Very sad face emoji 😪

The solution

I like to use something that I call the Orchestrator/Actions Pattern.

An Action can be almost anything. For example, an action might:

• Fetch data asynchronously
• Transform data
• Perform calculations

The most important characteristic of an action is that it strictly follows the Single Responsibility Principle. In simple terms, it should only do one of the above. Either fetch data, or transform it. Not both.

An Orchestrator calls many actions in a sequence.

An example Orchestrator might look like this:

const exampleOrchestrator = async () => {
  const initialData = await getInitialData();
  const calculation = runCalculation(initialData);
  const otherData = await getOtherData(calculation);
  const combinedData = combine({ initialData, otherData });
  return combinedData;
}

The most important characteristic of an orchestrator is that you can clearly see the sequence of events. Sure, you might have some simple logic such as if statements to determine which actions to call, but as long as it is easy to read, you have done a good job.

Closing thoughts

It isn’t always possible in reality to write code without any nesting. Sometimes, nesting can be helpful. But if you can generally strive to write your code using the Orchestrator/Actions Pattern, your code will be:

• Easier to navigate, because everything is one-level deep.
• Easier to read, because the sequence of events is visible in Orchestrators.
• Less brittle to change, because the purpose of every Action is clear, and you can just add more Actions if you want to do something specific.
• Easier to unit test (when necessary), because each Action is an isolated unit of functionality, and mocking is rarely required.

I hope this is helpful. Let me know your thoughts in the comments!