Functions: one job, few parameters, no hidden side effects

Code in:JavaC#PythonJavaScript

In short: A good function does one job at one level of abstraction, takes few parameters and no boolean flags, and does not quietly change state its name does not promise.

Problem

The function that does everything

The names from the last lesson are in place. The shop's constants:

const decimal FreeDeliveryFrom = 50m;
const decimal StandardDeliveryFee = 4.99m;
const decimal ExpressSurcharge = 7m;
const string SpringCoupon = "SPRING10";
const decimal SpringDiscount = 0.10m;

static List<decimal> orders = new List<decimal>();

And this is the function every order goes through:

static void ProcessOrder(string email, decimal[] prices, string coupon, bool express)
{
    decimal amount = 0;
    foreach (decimal price in prices)
    {
        amount += price;
    }
    if (coupon == SpringCoupon)
    {
        amount = amount * (1 - SpringDiscount);
    }
    decimal delivery = amount >= FreeDeliveryFrom ? 0 : StandardDeliveryFee;
    if (express)
    {
        delivery += ExpressSurcharge;
    }
    decimal total = amount + delivery;
    orders.Add(total);
    Console.WriteLine($"To {email}: your order is confirmed, €{total:F2}");
    Console.WriteLine($"[log] order #{orders.Count} for €{total:F2}");
}

ProcessOrder("maria@example.com", new decimal[] { 24.50m, 18.00m }, "SPRING10", true);
ProcessOrder("ivan@example.com", new decimal[] { 64.00m }, "", false);

Output

To maria@example.com: your order is confirmed, €50.24
[log] order #1 for €50.24
To ivan@example.com: your order is confirmed, €64.00
[log] order #2 for €64.00

It works. Now marketing wants the coupon not to apply to delivery. Accounting wants the fee shown separately in the cart. The tester wants to check delivery below and above €50, and for every check has to place an order, which means saving it and sending an email. Everyone touches the same function. And what true at the end of the call means still can't be seen.

The idea

One job and one level

Try to say what ProcessOrder does: "it adds up the prices, and applies the coupon, and calculates delivery, and saves, and sends an email, and writes to the log". Every "and" is a separate job. Each one changes for its own reason and because of a different person.

The rule is one job per function. A function with one job is small, has a name that says the whole job, and has few parameters. How you know the job is one:

• you describe it in one sentence without "and";
• you can't pull a piece out of it with a name that isn't just a retelling of the code.

The second rule is one level of abstraction. The top function reads like the table of contents of a book: calculate the total, save, confirm, log. How exactly the prices are added up (the loops, the comparisons, the format of the string) is one level below, in the small functions. When "save the order" and amount += price are in the same method, the reader jumps between the two levels on every line.

See it

The same order, split up

At the top is the table of contents:

static void PlaceOrder(string email, decimal[] prices, string coupon, bool express)
{
    decimal total = OrderTotal(prices, coupon, express);
    orders.Add(total);
    SendConfirmation(email, total);
    LogOrder(orders.Count, total);
}

static void SendConfirmation(string email, decimal total)
{
    Console.WriteLine($"To {email}: your order is confirmed, €{total:F2}");
}

static void LogOrder(int number, decimal total)
{
    Console.WriteLine($"[log] order #{number} for €{total:F2}");
}

Below it is the calculation, each job in its own function:

static decimal Subtotal(decimal[] prices)
{
    decimal amount = 0;
    foreach (decimal price in prices)
    {
        amount += price;
    }
    return amount;
}

static decimal ApplyCoupon(decimal amount, string coupon)
{
    if (coupon == SpringCoupon)
    {
        return amount * (1 - SpringDiscount);
    }
    return amount;
}

static decimal DeliveryFee(decimal amount, bool express)
{
    decimal fee = amount >= FreeDeliveryFrom ? 0 : StandardDeliveryFee;
    if (express)
    {
        fee += ExpressSurcharge;
    }
    return fee;
}

static decimal OrderTotal(decimal[] prices, string coupon, bool express)
{
    decimal amount = Subtotal(prices);
    decimal discounted = ApplyCoupon(amount, coupon);
    return discounted + DeliveryFee(discounted, express);
}

PlaceOrder("maria@example.com", new decimal[] { 24.50m, 18.00m }, "SPRING10", true);
PlaceOrder("ivan@example.com", new decimal[] { 64.00m }, "", false);

Output

To maria@example.com: your order is confirmed, €50.24
[log] order #1 for €50.24
To ivan@example.com: your order is confirmed, €64.00
[log] order #2 for €64.00

The output is the same. Now the coupon is in one function of three lines, delivery in another, and each one changes without touching the email. There are more lines of code, but each piece is read, changed and checked on its own.

More precisely, if you are curious

How small? There is no number. A 30-line function that does one thing (for example a long but flat switch with similar branches) is fine. An 8-line function with three levels of nesting and two flags is not. The measure is the one-sentence description, not the number of lines. Splitting a long function in steps, so that the program works after each one, is Extract Method in the sixth lesson of this module.

Try it

Put the lines in order

Put OrderTotal in order so that the coupon is applied before delivery. Why the order matters: the discount can take the order below €50, and then delivery is no longer free.

1. decimal amount = Subtotal(prices);↑↓
2. decimal discounted = ApplyCoupon(amount, coupon);↑↓
3. return discounted + DeliveryFee(discounted, express);↑↓
4. static decimal OrderTotal(decimal[] prices, string coupon, bool express)↑↓

Check

The idea

Few parameters and no flags

Every parameter is something the reader has to keep in mind, and something every call has to pass in the right order. Zero, one and two parameters read easily. Three is already a lot. Four or more usually mean that the parameters are one thing broken into pieces: the email, the prices, the coupon and the delivery together are an order. Then they become one object. This is the Introduce Parameter Object refactoring from the sixth lesson.

The worst parameter is a flag: a boolean that chooses what the function does.

static void PrintSlip(decimal[] prices, bool forCourier)
{
    if (forCourier)
    {
        Console.WriteLine($"Parcel: {prices.Length} items");
    }
    else
    {
        foreach (decimal price in prices)
        {
            Console.WriteLine($"€{price:F2}");
        }
    }
}

decimal[] cart = new decimal[] { 24.50m, 18.00m };
PrintSlip(cart, true);
PrintSlip(cart, false);

Output

Parcel: 2 items
€24.50
€18.00

At the call, true says nothing. And inside, the function has two separate paths, so two jobs. The fix is two functions with names:

static void PrintCourierSlip(decimal[] prices)
{
    Console.WriteLine($"Parcel: {prices.Length} items");
}

static void PrintReceipt(decimal[] prices)
{
    foreach (decimal price in prices)
    {
        Console.WriteLine($"€{price:F2}");
    }
}

decimal[] cart = new decimal[] { 24.50m, 18.00m };
PrintCourierSlip(cart);
PrintReceipt(cart);

Output

Parcel: 2 items
€24.50
€18.00

Sometimes the boolean really is data, not a choice of behavior: express delivery is part of the order. Then at least the call should say what it means:

C# has named arguments: at the call you write the name of the parameter before the value.

Console.WriteLine($"€{DeliveryFee(38.25m, express: true):F2}");

Output

€11.99

The idea

Side effect

Anything a function changes outside itself is a side effect: a field of an object, a list it was given, a global variable, a file, a database, an email, a line on the screen.

Side effects are not bad on their own. The order has to be saved somewhere, and the email has to go out. It is bad when the effect is hidden: the name says "I ask", but the function also does. Whoever calls it doesn't expect anything to change, and calls it twice without a second thought.

See it

The question that changes the answer

The warehouse says whether there are lamps. Someone decided that if there are, it's handy to reserve them right away:

class Warehouse
{
    public int Stock { get; private set; }

    public Warehouse(int stock)
    {
        Stock = stock;
    }

    public bool IsInStock(int quantity)
    {
        if (Stock >= quantity)
        {
            Stock -= quantity;
            return true;
        }
        return false;
    }
}

The cart asks so that it can show "available". Then checkout asks the same thing:

var lamps = new Warehouse(3);
Console.WriteLine("Cart: " + (lamps.IsInStock(2) ? "available" : "out of stock"));
Console.WriteLine("Checkout: " + (lamps.IsInStock(2) ? "available" : "out of stock"));
Console.WriteLine("In stock: " + lamps.Stock);

Output

Cart: available
Checkout: out of stock
In stock: 1

class Warehouse
{
    public int Stock { get; private set; }

    public Warehouse(int stock)
    {
        Stock = stock;
    }

    public bool IsInStock(int quantity)
    {
        if (Stock >= quantity)
        {
            Stock -= quantity;
            return true;
        }
        return false;
    }
}

var lamps = new Warehouse(3);
Console.WriteLine("Cart: " + (lamps.IsInStock(2) ? "available" : "out of stock"));
Console.WriteLine("Checkout: " + (lamps.IsInStock(2) ? "available" : "out of stock"));
Console.WriteLine("In stock: " + lamps.Stock);

Step by step, in words

1. The warehouse has 3 lamps on the shelf.
2. The cart only wants to show whether there are 2. It asks IsInStock(2): a question, judging by the name.
3. 3 >= 2: there are enough.
4. But the question also takes the lamps: Stock goes from 3 to 1. Nobody bought anything, and the cart doesn't know this happened.
5. The cart gets true and shows "available".
6. The customer goes to checkout. Checkout asks the same thing: IsInStock(2).
7. 1 >= 2 is false. The same question gets a different answer, and the customer sees "out of stock".
8. In stock: 1. The answer changed because the first question had a hidden side effect.

Nobody bought anything, two lamps are missing, and the customer can't pay. In a real shop, this is an order lost because of a page that asks about stock. And the bug is hard to find, because every line on its own looks fine.

More precisely, if you are curious

It is even sneakier in a property: lamps.Stock looks like reading a field. Visual Studio evaluates properties to show them in the debugger, so a property with a side effect changes the state just because you stopped at a breakpoint and looked at the object.

Try it

Predict the output

The warehouse with the IsInStock that also reserves now has 5 lamps. The same question is asked three times. What does the program print?

var lamps = new Warehouse(5);
Console.WriteLine(lamps.IsInStock(2));
Console.WriteLine(lamps.IsInStock(2));
Console.WriteLine(lamps.IsInStock(2));

Answer

True / False / False

True / True / False

True / True / True

Check

The idea

A command or a query, not both

The rule is called command–query separation: a function either answers a question and changes nothing, or does something and doesn't pretend to be a question. The warehouse becomes:

class Warehouse
{
    public int Stock { get; private set; }

    public Warehouse(int stock)
    {
        Stock = stock;
    }

    public bool IsInStock(int quantity)
    {
        return Stock >= quantity;
    }

    public void Reserve(int quantity)
    {
        if (!IsInStock(quantity))
        {
            throw new InvalidOperationException("Not enough stock");
        }
        Stock -= quantity;
    }
}

var lamps = new Warehouse(3);
Console.WriteLine("Cart: " + (lamps.IsInStock(2) ? "available" : "out of stock"));
Console.WriteLine("Checkout: " + (lamps.IsInStock(2) ? "available" : "out of stock"));
lamps.Reserve(2);
Console.WriteLine("In stock: " + lamps.Stock);

Output

Cart: available
Checkout: available
In stock: 1

The cart and checkout ask as much as they like. The lamps are reserved on one line that says exactly that: Reserve(2).

More precisely, if you are curious

There are well-known exceptions, and the name gives them away: stack.Pop() removes the element and returns it, queue.Dequeue() moves forward and returns. These are verbs, not questions, so nobody calls them "just to check". The rule is not "a command never returns anything" but "a query never changes anything".

The idea

The pure function

DeliveryFee from the split order answers only from its parameters and changes nothing. A function like this is called pure: with the same parameters it always returns the same thing, no matter how many times or in what order you call it.

Console.WriteLine($"€{DeliveryFee(38.25m, false):F2}");
Console.WriteLine($"€{DeliveryFee(38.25m, true):F2}");
Console.WriteLine($"€{DeliveryFee(64.00m, false):F2}");
Console.WriteLine($"€{DeliveryFee(38.25m, true):F2}");

Output

€4.99
€11.99
€0.00
€11.99

Checking delivery below and above the threshold takes four lines. There is no warehouse to fill, no email to send, no database. With ProcessOrder, you had to place an order for the same thing. That is why the calculations are kept pure, and the side effects are gathered in a few clearly named commands at the top. How checks like these become tests that protect the code at every change is the topic of the fifth lesson of this module.

Try it

Find the bug

The "Remove last" button should remove the product the customer added last. The customer adds milk, apples and bread. Before that, the shop prints a receipt sorted alphabetically:

static void PrintReceipt(List<string> items)
{
    items.Sort();
    foreach (string item in items)
    {
        Console.WriteLine("  " + item);
    }
}

var cart = new List<string> { "milk", "apples", "bread" };
PrintReceipt(cart);
Console.WriteLine("Last added: " + cart[cart.Count - 1]);

Output

  apples
  bread
  milk
Last added: milk

The bread was added last, but the program says milk. Which line is the bug on?

Pick the line with the bug.

1static void PrintReceipt(List<string> items)2{3 items.Sort();4 foreach (string item in items)5 {6 Console.WriteLine(" " + item);7 }8}

Check

Exercise

The order shipped notification

Solve the task in your editor.

The notification. This function builds the text, chooses the channel with a flag, prints and counts what was sent:

static int sent = 0;

static void NotifyShipped(string name, int orderId, bool sms)
{
    sent++;
    string text = $"Hello, {name}! Order {orderId} has shipped.";
    if (sms)
    {
        Console.WriteLine("SMS: " + text);
    }
    else
    {
        Console.WriteLine("Email: " + text);
    }
}

NotifyShipped("Maria", 1042, false);
NotifyShipped("Ivan", 1043, true);
Console.WriteLine("Sent: " + sent);

Output

Email: Hello, Maria! Order 1042 has shipped.
SMS: Hello, Ivan! Order 1043 has shipped.
Sent: 2

Split it into three functions: a pure one that only builds the text, and two commands, one that sends by email and one by SMS, each counting what it sent. The flag goes away. The output must stay the same:

Output

Email: Hello, Maria! Order 1042 has shipped.
SMS: Hello, Ivan! Order 1043 has shipped.
Sent: 2

More tasks

• A parameter object. PlaceOrder has four parameters. Group them in an Order class and see what the call looks like.
• The coupon doesn't apply to delivery. Marketing changes the rule. Which function do you touch, and how many others stay untouched?
• The queue. Write PeekNext() and TakeNext() for a queue of orders for the courier. Which one is a query and which one is a command?

Test

Check yourself

1. How can you tell that ProcessOrder does more than one job?

Because it has a loopIts description has a lot of "and": it adds up and applies the coupon and calculates delivery and saves and sends an emailBy the number of lines

2. PrintSlip(cart, true). What is wrong?

NothingThere should be more parametersThe flag says that the function does two different things. PrintCourierSlip(cart) and PrintReceipt(cart) are clearer

3. IsInStock(2) lowers the stock if there is enough. The cart asks once, the checkout asks again. What happens with 3 lamps?

The cart gets "available", the checkout gets "out of stock", and 1 is left in stockBoth get "available"The program stops with an error

4. Which of these functions is pure?

PrintReceipt(items): sorts the list it gets and prints itPlaceOrder(…): saves the order and sends an emailDeliveryFee(amount, express): calculates the fee from its two parameters

5. What does command–query separation say?

Commands must be in a separate classA function either does something or answers a question, but not bothEvery function must return a value

Check

Start the lesson over

Related lessons

• Builds on: Exceptions · Throwing and your own exceptions · Lists · Names in code
• Continues in: Comments that lie and comments that help · Code smells: long method, large class, duplication, primitives instead of types, long if chains · Tests as a safety net · Refactoring in small steps
• Tasks on this topic: Clean code and refactoring