Ian Walton Ian Walton

Clean JavaScript: Eliminating Side-Effects

Introduction

Welcome back to the 2nd installment of the Clean Javascript series, where I describe common anti-patterns in JavaScript applications and how to avoid them.

Today we’ll be looking at possibly the most common, and most egregious, antipatterns around: side-effects.

The Problem

Before we dig into what a side-effect is and why they’re so pervasive, let’s look at some code:

async function prepareForCheckout (cart) {
	cart.productLineItems = cart.productLineItems.reduce((agg, pli) => ({
		...agg,
		[pli.id]: {
			...pli,
			needsTax: true
	}), {});

	const taxes = await taxService.findTaxesForProductLineItemIds(Object.keys(cart.productLineItems));

	for (const tax of taxes) {
		cart.productLineItems[tax.productLineItemId].tax = tax;
		cart.productLineItems[tax.productLineItemId].needsTax = false;
	}

	return cart;
}

Here we have a method prepareForCheckout that accepts a cart item, transforms it’s product line items array into an object and then calculates taxes and appends the tax amount to each PLI before returning the cart object.

At face value there are some good things going on here - we avoid the N+1 problem of fetching taxes individually and have a handy new flag for finding PLIs that still need tax. But we’ve also fundamentally changed the data structure of PLIs on our Cart.. whoops!

A side effect is a any effect performed within a function beyond just what it returns. It’s a common practice in imperative programming, which is unfortunately the easiest style of programming to learn and the (probably) most frequent cause of heartburn, insomnia, and burnout among JS developers.

See, JavaScript is a pass-by-reference language, which means when you pass your cart object to prepareForCheckout you’re passing the actual cart object, not just a copy to be used and discarded. When we make changes to these objects inside of functions, you assume that anyone using the object after this functions knows you made the change and will handle it accordingly. While I’m sure you told your team about this change in standup and wrote paragraphs of comments, I have some bad news - no-one is going to remember, no-one is going to read your comments, and it will 100% be the reason you’re getting spammed by PagerDuty for outages at 3AM on a Saturday.

The Solution

If you’re like me and enjoy sleep, or just don’t want to piss off your teammates, there is an easy solution!

Imperative programming has an older, wiser cousin named declarative programming, and it’s ready to swoop in like a Marvel superhero to save your job, sanity, and marriage. Declarative programming is simple - methods should be ignorant to the order in which they are called. If you think that sounds a little vague I hear you, so let’s revisit the method from before:

async function prepareForCheckout (cart) {
	cart.productLineItems = cart.productLineItems.reduce((agg, pli) => ({
		...agg,
		[pli.id]: {
			...pli,
			needsTax: true
	}), {});

	const taxes = await taxService.findTaxesForProductLineItemIds(Object.keys(cart.productLineItems));

	for (const tax of taxes) {
		cart.productLineItems[tax.productLineItemId].tax = tax;
		cart.productLineItems[tax.productLineItemId].needsTax = false;
	}

	return cart;
}

It looks like there are two side-effects in this function:

  1. We transform cart.productLineItems from a happy little array into an object keyed by id.
  2. We set two properties on the PLIs - tax and needsTax

Let’s start with the first (and worst) side-effect: the transformation.

The first step is figuring out how deeply entrenched this side-effect is - are we calling prepareForCheckout in a lot of places? Can we tell if anything downstream uses the object form of cart.productLineItems? Do we have good test coverage wherever we call this method, so we can make changes confidently? Anywhere we call this code is suspect, and we need to be extra careful to avoid more rippling changes lest you risk pissing off your coworkers further.

Maybe it’s hard to tell or we lack adequate test coverage. We can start by renaming the method to better express its risk: something like prepareForCheckout_UNSAFE. Still the same side-effect problems, but hopefully it will cause future developers to pause and consider this method before continuing on (there be dragons!).

The next (and most obvious) step is to add unit tests for prepareForCheckout. We’re about to perform open heart surgery on this method so we should make sure our watch is still on our wrist afterwards.

Next, we start to push the transformations down the periphery of our method:

async function prepareForCheckout_UNSAFE (cart) {
	const productLineItemIds = cart.productLineItems.map((pli) => pli.id);

	const taxes = await taxService.findTaxesForProductLineItemIds(productLineItemIds);
	const taxesObj = taxes.reduce((agg, tax) => ({ ...agg, [tax.productLineItemId]: tax }), {});

	cart.productLineItems = cart.productLineItems.reduce((agg, pli) => ({
		...agg,
		[pli.id]: {
			...pli, // NOTE: The spread operator wouldn't clone nested properties, you may need a utility to deep copy
			tax: taxesObj[pli.id],
			needsTax: Boolean(taxesObj[pli.id])
		}
	}), {})

	return cart;
}

Now the top part is starting to look sane again, even though we’re still modifying cart in the same way. Notice we’re creating a shallow copy of each product line item instead of mutating the parameter’s property directly. This is a subtle, but important, distinction - any callee’s that plan on using these properties will need to use the return value. Finally, note that we didn’t just update the return value - there’s no guarantee the callee is using it!

Next, let’s move those pesky side-effects to their own methods, and instead of updating the properties themselves we’ll return a new copy of the object instead:

const convertProductLineItemsToObj = (productLineItems) => productLineItems.reduce((agg, pli) => ({
	...agg,
	[pli.id]: pli
}), {});

async function prepareForCheckout_UNSAFE (cart) {
	const productLineItemIds = cart.productLineItems.map((pli) => pli.id);

	const taxes = await taxService.findTaxesForProductLineItemIds(productLineItemIds);
	const taxesObj = taxes.reduce((agg, tax) => ({ ...agg, [tax.productLineItemId]: tax }), {});

	cart.productLineItems = convertProductLineItemsToObj(
		cart.productLineItems.map((pli) => ({
			...pli,
				tax: taxesObj[pli.id],
				needsTax: Boolean(taxesObj[pli.id])
		}))
	);

	return cart;
}

OK. We did a couple of things there. First, we moved the logic to set tax properties on a PLI into it’s own method, and used currying + a map to transform the PLIs. We also created a method to do the actual map transformation, which is important for mitigating downstream errors.

The final boss is to rip this band-aid off, which unfortunately means making changes outside the scope of this function. This can be done in two parts, ideally test driven, where you pull this logic out and perform the side-effect in the calling function, and use our new convertProductLineItemsToObj anywhere that needs it:

async function prepareForCheckout (cart) { // Safe again!
	const productLineItemIds = cart.productLineItems.map((pli) => pli.id);

	const taxes = await taxService.findTaxesForProductLineItemIds(productLineItemIds);
	const taxesObj = taxes.reduce((agg, tax) => ({ ...agg, [tax.productLineItemId]: tax }), {});

	return {
		...cart,
		productLineItems: cart.productLineItems.map((pli) => ({
			...pli,
				tax: taxesObj[pli.id],
				needsTax: Boolean(taxesObj[pli.id])
		}))
	};
}

...

const preparedCart = await prepareForCheckout(cart);
preparedCart.productLineItems = convertProductLineItemsToObj(preparedCart.productLineItems);

Conclusion

I’ll stop here, but hopefully it’s clear what we’re aiming for. If you have to update a property, return it’s copy. Avoid mutating properties like the plague! And tests make everything easier.