How long should a single method be?

January 19, 2017

This topic might seem trivial, especially if you look at all other articles that have beaten it to death already. But I would still like to make a couple of important points here. So, how long should a single method be?

Method length

A typical progression for someone who learns an OOP language looks like this (at least it looked like that for me when I first learned C#). At first, you just code everything you need to do within a business transaction into a single giant method. No separation between domain, application and persistence logic, just a plain series of steps required to accomplish some operation.

Then, you discover code duplication and all the bad consequences it entails, so you try to avoid it. This leads you to introducing sub-methods: common pieces of code shared across different operations. As a corollary, your methods become smaller, but not so much as you still don’t see benefits in splitting large methods other than when removing duplication. If the DRY principle is not violated, why bother? Also, it’s much more convenient to see everything going on in an operation when its code gathered in a single place. So it seems to you at this stage.

Later, you discover that battling code duplication is not the only purpose of having smaller methods. Moreover, it’s even not the most significant one. You learn that it’s much more important to abstract your whats from hows. By having a larger number of smaller methods, you are able to raise the level of abstraction your code operates at. You can group several low-level steps into a single method, give that method an intention-revealing name, and forget about those steps altogether. This helps simplify the code as you are focusing on what it’s doing, not how. You can travel between levels of abstraction at will because now these levels are clearly separated from each other.

That’s where most programmers (myself included) stand. No method is too short, the number of lines is mostly irrelevant here. What’s important is whether or not the method clearly manifests what it’s doing and if it’s operating at a single level of abstraction.

For example, here:

public void ApplyDiscount(int tenantId, int referencedTenantId)
{
    Tenant tenant = _repository.GetById(tenantId);
    Tenant referencedTenant = _repository.GetById(referencedTenantId);
 
    // Operates at a lower level of abstraction
    if (referencedTenant.MoveInDate.AddMonths(2) <= DateTime.UtcNow)
    {
        tenant.AddDiscount(200);
    }
 
    _repository.Save(tenant);
}

the method applies a discount to a tenant who’s referenced a new tenant for the leasing company, but only if that person has been renting for at least 2 months.

Although the method itself is short and quite easy to understand, it has a flaw. The line with the if statement works at a different level of abstraction than the other lines. It brings up implementation details instead of stating the intent and thus doesn’t convey the meaning well enough.

To fix it, we need to introduce a new method, and use it in the sample code above:

public void ApplyDiscount(int tenantId, int referencedTenantId)
{
    Tenant tenant = _repository.GetById(tenantId);
    Tenant referencedTenant = _repository.GetById(referencedTenantId);
 
    // Operates at the same level of abstraction
    if (referencedTenant.LivesForNumberOfMonths(2))
    {
        tenant.AddDiscount(200);
    }
 
    _repository.Save(tenant);
}

This allows us to hide the implementation details and bring all lines in the method to the same level of abstraction. We can clearly see the intent behind the if statement. The new method serves as documentation, we no longer need to interpret the meaning behind this code.

Note that the new method contains just a single line:

public bool LivesForNumberOfMonths(int number)
{
    return MoveInDate.AddMonths(number) <= DateTime.UtcNow;
}

We could easily leave it inlined but that would hurt our goal of separating whats from whys.

Maximum method length

So, back to our initial question. How long should a single method be? Or, in other words, what is the maximum method length?

While a method can definitely be too large, the number of lines is a quite misleading metric. A two line method can be completely messed up. Similarly, a two screen method can be simple and easy to understand. The two attributes that actually matter are:

Whether or not the method operates at a single level of abstraction
Cyclomatic complexity

We touched upon the first one already: make sure that all steps a method takes roughly correspond to each other with regards to their level of detail. For example, this method has a code smell:

public void ChangeApartment(int tenantId, string newAddress)
{
    Tenant tenant = _repository.GetById(tenantId);
    Address address = Address.Create(newAddress);
 
    tenant.UpdateAddress(address);
    tenant.Version++; // Code smell
 
    _repository.Save(tenant);
}

Incrementing the version here is a low-level operation and clearly stands out from the rest of the method. You need to either introduce a separate method which would convey the intent behind this increment or include this step into the UpdateAddress method.

As for the second attribute, try to keep cyclomatic complexity of all your methods as low as possible. One technique that will help you achieve that is Early Exit. Here I wrote about it in more detail: Early exit is a tail call optimization of procedural languages.

Let’s take an example to illustrate the idea:

public string RenovateApartment(int buildingNumber, int apartmentNumber, out decimal cost)
{
    Apartment apartment = _repository.GetApartment(buildingNumber, apartmentNumber);
 
    string error;
    cost = 0;
    if (apartment.IsVacant)
    {
        if (apartment.LastRenovationWasMoreThanNumberOfYearsAgo(5))
        {
            cost = apartment.CalculateRenovationCost();
            if (cost <= MaxRenovationCost)
            {
                error = null;
            }
            else
            {
                error = "Renovation cost is too big";
            }
        }
        else
        {
            error = "Last renovation was less than 5 years ago";
        }
    }
    else
    {
        error = "Apartment is occupied";
    }
 
    if (error == null)
    {
        apartment.StartRenovationProcess();
    }
 
    return error;
}

This method performs a number of checks before starting a renovation. Its lines operate at the same level of abstraction but note the indentation here. It’s 4 at the maximum depth, and it’s quite hard to read because of that. It also has an out parameter which complicates following the execution flow even further.

The Early Exit technique can help us fix it. Here’s a refactored version:

public Result<decimal> RenovateApartment(int buildingNumber, int apartmentNumber)
{
    Apartment apartment = _repository.GetApartment(buildingNumber, apartmentNumber);
 
    if (!apartment.IsVacant)
        return Result.Fail<decimal>("Apartment is occupied");
 
    if (!apartment.LastRenovationWasMoreThanNumberOfYearsAgo(5))
        return Result.Fail<decimal>("Last renovation was less than 5 years ago");
 
    decimal cost = apartment.CalculateRenovationCost();
 
    if (cost > MaxRenovationCost)
        return Result.Fail<decimal>("Renovation cost is too big");
 
    apartment.StartRenovationProcess();
 
    return Result.Ok(cost);
}

Note how much easier it’s become to follow. The number of lines has also reduced as a result of the refactoring, but again, it wasn’t our main goal. What’s important is that we were able to decrease the maximum indentation from 4 to 2 and thus reduce the mental space required to read and understand this code.

Also, note that the out parameter is removed. It is now embedded into the returned Result instance.

By the way, if you wonder how it could be OK to have a method which is 2 screens long, it’s when all lines in it operate at the same level of detail and are 1 or 2 indents deep. A method with a large number of checks can be one example. Another example is a series of steps some background job needs to perform every N hours. Methods of 1 or 2 indents deep, even large ones, are extremely easy to follow, provided they don’t jump from one abstraction level to another.

Summary

As you might have noticed, I haven’t answered the question posed in the post title :) But that’s because it’s impossible to answer as this metric is pretty much useless most of the time. Attributes that really matter are cyclomatic complexity (how deep the code indentation is) and whether or not the method operates at a single level of abstraction.

It’s common to hear advice saying to write as small methods as possible, but, as I showed in this article, this advice doesn’t help much. Not all methods can be made small. And not all methods should be. Instead, pay attention to the two attributes I described above.