DEV Community: Jimmy

Misusing map and reduce... and how to fix it

Jimmy — Mon, 20 Jul 2020 08:47:28 +0000

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!

How to write good tests

Jimmy — Mon, 13 Jul 2020 09:54:46 +0000

Writing tests is pretty easy once you've got the hang of your favourite testing library, but it's surprisingly difficult to grow and maintain a test suite.

Tests, like a lot of production code, often begin well but get unwieldy as your codebase grows. Without good maintenance, eventually even small changes require you to rewrite all the tests.

One of the best ways to stay on top of a test suite is to structure your tests well. This is what I consider to be the best way to write a standard unit test, with a breakdown behind some of my reasoning. We're going to use Ruby, but these rules apply generally, at least to all languages and frameworks I've encountered.

The subject

class Person 
    attr_reader :first_name, :last_name 

    def initialize(first_name, last_name) 
       @first_name = first_name 
       @last_name = last_name
    end

    def full_name
        "#{first_name} #{last_name}"
    end

    def in_house_identifier
        "#{last_name.upcase.reverse}"
    end
 end

This is the thing we're trying to test. It's a simple Person class to help us format the person's name. We have a method to grab their full name, as well as an in-house identifier which is just a transformed version of their last name.

The tests

require 'faker'

describe Person do 
    it 'create a new person with a first and last name' do       
        person = build_person(first_name:'Joanna', last_name: 'Smith')
        expect(person).to have_attributes(first_name: 'Joanna') 
        expect(person).to have_attributes(last_name: 'Smith') 
    end 

    it 'provides the full name' do       
        person = build_person(first_name: 'Henrietta', last_name: 'Appleseed')
        expect(person.full_name).to eq('Henrietta Appleseed')
    end

    it 'reverses the surname in upper-case for the in-house identifier' do
        person = build_person(last_name: 'Thompson')
        expect(person.in_house_identifier).to eq('NOSPMOHT')
    end
end

def build_person(first_name: Faker::Name.first_name, last_name: Faker::Name.last_name)
   Person.new first_name, last_name
end

These might not be totally comprehensive, but this is a great start. So what makes them so good?

1. No shared state

it 'create a new person with a first and last name' do
    person = build_person(first_name:'Joanna', last_name: 'Smith')
end

it 'provides the full name' do
    person = build_person(first_name: 'Henrietta', last_name: 'Appleseed')
end

it 'reverses the surname in upper-case for the in-house identifier' do
    person = build_person(last_name: 'Thompson')
end

We want to be able to make changes without breaking the other tests. This means that we need a fresh Person for every test. Some testing libraries offer a way of ensuring this but still allowing you to have shared state, like so:

describe Person do 
    before(:each) do
        # Re-create our person before each test
        @person = build_person(first_name:'Joanna', last_name: 'Smith')
    end

    it 'create a new person with a first and last name' do
        # Use @person to reference it       
        expect(@person).to have_attributes(first_name: 'Joanna') 
        expect(@person).to have_attributes(last_name: 'Smith') 
    end
end

So what's wrong with this? While these tests are independent, as you can be sure you have a fresh person every test, we've still coupled our tests together and made them harder to read.

I must now read both the before-each block to understand how the test works, meaning I'm now scrolling between them regularly. In a file which can stretch to hundreds (if not thousands) of lines this becomes unreasonable, and there isn't
a clean split between them. As a reader, I'll likely arrive at this file with no context and need to fix a single failing test. I'll This is suddenly a much more painful job.

This also means all my tests are relying on the same example data: Joanna Smith. If my test required a longer surname, I now need to edit all the tests in the file.

2. The builder method

it 'reverses the surname in upper-case for the in-house identifier' do
    person = build_person(last_name: 'Thompson')
    expect(person.in_house_identifier).to eq('NOSPMOHT')
end

def build_person(first_name: Faker::Name.first_name, last_name: Faker::Name.last_name)
    Person.new first_name, last_name
end

While we don't want shared state, we also want to reduce the amount of boilerplate. Ideally, we want the test to only talk about what's needs setting up to make it pass. We can achieve this by using a a builder method and setting some default arguments.

In the example above, the in-house identifier only relies on the last name, so we don't bother passing in a first name at all. Since we don't use the first name to make the test pass, it shows readers we don't care what it is.

We can take this even further with Faker.

3. Using Faker

require 'faker'

Faker::Name.first_name
# Danielle

Faker::Name.last_name
# Smith

Faker is a small detail that really helps prevent bad habits. With builder methods, like the one above, it's easy to set the default values to some known good values and never bother overriding them in the tests, like so:

it 'reverses the surname in upper-case for the in-house identifier' do
    # No arguments given
    person = build_person()
    expect(person.in_house_identifier).to eq('NOSPMOHT')
end

# Everyone we make is called Samuel Thompson by default
def build_person(first_name: 'Samuel', last_name: 'Thompson')
    Person.new first_name, last_name
end

This is basically re-inventing the before-each block we discussed earlier. By sharing important details, like the person's name, outside of the test we are making it harder for readers to know which arguments are relevant to this
particular test.

Using Faker, we make these default arguments random, so we can't rely on them. In order to assert on a value, we need to override it in the test, which confirms to readers what's relevant.

A note about using Faker is that it's very easy to misuse by using them as part of your assertions.

it 'reverses the surname in upper-case for the in-house identifier' do
    # Get a random name each time
    person = build_person()
    # We convert the name here and check they're the same
    expect(person.in_house_identifier).to eq(person.last_name.upcase.reverse)
end

def build_person(first_name: Faker::Name.first_name, last_name: Faker::Name.last_name)
    Person.new first_name, last_name
end

I avoid this as it makes tests harder to debug, for example, is Lee their first name or last name? We don't know, since we didn't explicitly set what we were testing.

I generally try to avoid unnecessary variables altogether.

4. No unnecessary variables

it 'create a new person with a first and last name' do       
    person = build_person(first_name:'Joanna', last_name: 'Smith')
    # Hard-coded Joanna
    expect(person).to have_attributes(first_name: 'Joanna')
    # Hard-coded Smith
    expect(person).to have_attributes(last_name: 'Smith') 
end

It's tempting to create a variable for the first and last names. It allows you to quickly change the variable to something else, but it offers little for readability.

The more logic that goes into a test the harder it is to understand. If someone has to compile the test in their head, they'll struggle to make changes to it.