A common problem I find in big production systems is when higher-order functions, like map, filter and reduce, are applied so often that the code becomes painfully abstract. Like all code-smells, it starts small and well-intentioned.
Let's say we've been asked go over all our customers and charge their credit card, here's my first attempt.
function getAllCustomers(): string[] {
return ["customer-1", "customer-2", "customer-3"]
}
function getCardForCustomer(customerId: string): string {
// some credit card number
return "4242424242424242"
}
function takePayment(cardNumber: string): boolean {
// true means success, false means failure
return true
}
function chargeCustomers(): boolean[] {
return getAllCustomers()
.map(getCardForCustomer)
.map(takePayment)
}
No editor or linter will complain at us for writing this. We have discrete, testable methods to fetch all our customers, fetch their card numbers and take their money.
So where does this start going wrong?
The all-for-one problem
The first problem here is that we can only operate this function on the entire list of customers. This means that if we wanted to charge a new customer when they first enter their card details, we wouldn't be able to without charging everybody.
Assuming we're not in the business of stealing our customers' money, let's fix it.
function chargeCustomers(customerIds: string[]): boolean[] {
return customerIds
.map(getCardForCustomer)
.map(takePayment)
}
function chargeAllCustomers(): boolean[] {
return chargeCustomers(getAllCustomers())
}
We've now split out two methods. chargeCustomers takes a list of customers to take payment from, and chargeAllCustomers does it for everybody.
If we want to run it for an individual customer, we can even do that by making a list with a single ID within it:
chargeCustomers(["new-customer"])
So we've eeked out a lot of flexibility but still kept our solution almost entirely based around a stream of maps and reduces, but this still isn't great.
Side effects
Right now the solution works well and still reads pretty well. This is because what we're doing has no side effects.
A side effect is when your code does something that has an effect outside of the method itself. This is an example of a side effect:
let x = "Method has not been called"
function doASideEffect() {
// this is a side effect
x = "Method has been called"
}
doASideEffect()
console.log(x)
// Method has been called
When we call our method, we change the value of x, which is a global variable. This can have real ramifications for the rest of the app.
The problem with our solution is that it makes no room for any side effects, even when they're quite useful.
Let's say we now want to send receipts to customers whose payments have succeeded. For the purposes of this post, the content of the email doesn't matter, and all we require is the customer's ID, since that's how we find their email address.
function sendReceipt(customerId: string) {
// some email gets sent
console.log(`Sent receipt to ${customerId}`)
}
Unfortunately, this requirement means we're a little stuck.
We've been so busy transforming customers into cards and transforming those into payments we haven't retained any of this useful information. One way or the other, we now need to re-write our solution to make this work.
Split your behaviour from your loops
It's clear now that the process of taking payments has become way more complicated over time. So much so that it's now worth splitting entirely from the concept of a loop.
We do this by replacing the chain of maps with a single function, chargeCustomer, which handles the entire payment process for a single customer.
function chargeCustomer(customerId: string): boolean {
const card = getCardForCustomer(customerId)
const wasPaymentSuccessful = takePayment(card)
if (wasPaymentSuccessful) {
sendReceipt(customerId)
}
return wasPaymentSuccessful
}
For another engineer coming to figure out how we do payments, this is much nicer. It's also equally discrete and can also easily be tested. We can even merge it back into our original code, so we can run it on all of our customers.
function getAllCustomers(): string[] {
return ["customer-1", "customer-2", "customer-3"]
}
function getCardForCustomer(customerId: string): string {
return "4242424242424242"
}
function takePayment(cardNumber: string): boolean {
return true
}
function sendReceipt(customerId: string) {
console.log(`Sent email to ${customerId}`)
}
// Handle a single customer
function chargeCustomer(customerId: string): boolean {
const card = getCardForCustomer(customerId)
const wasPaymentSuccessful = takePayment(card)
if (wasPaymentSuccessful) {
sendReceipt(customerId)
}
return wasPaymentSuccessful
}
// Handle many customers
function chargeCustomers(customerIds: string[]): boolean[] {
return customerIds.map(chargeCustomer)
}
// Handle all customers
function chargeAllCustomers(): boolean[] {
return chargeCustomers(getAllCustomers())
}
The alternative - even more loops
We can also see what happens if we don't want to split out this logic, and try sending the emails without it. We basically need to pass larger amounts of information down through the stream in order to make it available for our sendReceipt method.
interface Customer {
customerId: string
cardNumber: string
}
function getCustomerById(customerId: string): Customer {
// building our customer object
return {
customerId,
cardNumber: "4242424242424242"
}
}
interface Payment {
customer: Customer
wasSuccessful: boolean
}
function takePayment(customer: Customer): Payment {
// building our payment object
return {
customer: customer,
wasSuccessful: true
}
}
function sendReceipt(payment: Payment) {
// some email gets sent
console.log(`Sent email to ${payment.customer.customerId}`)
}
function chargeCustomers(customerIds: string[]): boolean[] {
const payments =
customerIds
.map(getCustomerById)
.map(takePayment)
// send emails to successful payments
payments
.filter((payment) => payment.wasSuccessful === true)
.forEach(sendReceipt)
// extract the wasSuccessful field
return payments.map((payment) => payment.wasSuccessful)
}
Conclusion
To refactor well is to know how to refactor but also when to refactor. I spend a lot of time bashing loops and higher-order functions in this post, but it only became a problem when the requirements changed and grew more complex.
It's easy to miss this jump. Imagine you had to send a different email for customers whose payments failed, and if it failed a few times you had to lock them out of their account.
Not only would our original chargeCustomers method become a nightmare, but even with the refactoring we'd done so far it would still start to become difficult.
Refactoring is a healthy activity for any codebase, and it's best done when the requirements change. This is one trick I've applied many times, and it helps a ton!
Top comments (0)