How long should a single method be?

By Vladimir Khorikov

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:

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.

Related articles:





  • tora bravo

    I think you forgot ‘return’ keyword for each ‘if’ in the last code example

    • http://enterprisecraftsmanship.com/ Vladimir Khorikov

      Indeed. Fixed, thank you!

  • Joseph N. Musser II

    My Result<> class has implicit conversions from exceptions, so throw x becomes return x</code.

    • Sergio Rykov

      It leads to unclear code – you can easily mix up ‘throw new Ex()’ with implicit ‘new Ex()’. It doesn’t add any additional details. I think we will get much more details with implicit enum error specification: Result func() { if (condition) return TErrorReasonEnum.RenovationCostIsTooBig; }

      • Joseph N. Musser II

        Hmm. There’s no way you can mix up an implicit new Ex() with return new Ex() any more than you do with throw new Ex(). For one thing, no one uses implicit new. That’s a code smell. So it’s pretty simple- just swapping the throw keyword with the return keyword.

        For my uses the exception message is all the additional detauls I’ve needed so far, but I could easily add an enum property to the exception which is standard .NET practice in such scenarios. I’m pretty happy with the flexibility here.

    • http://enterprisecraftsmanship.com/ Vladimir Khorikov

      Strings are not enough for more or less complex situations, that’s for sure. Regarding the use of exceptions – I would personally be reluctant to do that. They bring quite a few technical details (such as exception stack) which are not relevant to the problem domain. I’d recommend introducing your own strongly typed hierarchy or errors. An enum could also be fine.

  • http://programmingideaswithjake.wordpress.com Jacob Zimmerman

    While I agree with you, I want to also point out that there is a company who has done a lot of metrics on these types of things and has noticed that maintainability (however they measure it) takes a steep dive when more than a few functions/methods get to be larger than 15 lines (no matter the language), so I still strive to stay within their specs. They’ve also got specs like that all the way up, such as the number of methods in a class/functions in a module, classes or modules in a package, etc.

    I wish I could find it quickly enough here so I could link it, but I can’t. Sorry.
    Again, I agree with your assessment, but I would try to marry the two as much as possible.

    • http://enterprisecraftsmanship.com/ Vladimir Khorikov

      I agree, there’s a strong correlation between code readability and average method size. And it’s a good rule of thumb to keep all methods short by default. But I would still say that it’s not the primary metric we should aim at improving. Method size usually goes down as a side effect of that anyway.

      • http://programmingideaswithjake.wordpress.com Jacob Zimmerman

        We’re pretty much agreed, then. The only problem is that “level of abstraction” is impossible to create an automated metric for, so while that is your primary metric, you can’t automate it, meaning that a good number is still needed to help flag for your metric in order to help find instances of failure.
        So it’s a good idea to have a number in mind anyway.

      • http://sidburn.github.io/ David Raab

        I completely agree with your article and what you say here. The problem is that it is hard to argue about cyclomatic complexity and different abstraction levels. On the other hand it is dead simple to identify functions that are too long, you just count the line numbers.

        I would say, counting line numbers and look at large functions can help finding problems, but the amount of line numbers, whether short or long, is not proof there is anything right or wrong.

  • Artur Karbone

    Nice stuff, @vladimirkhorikov:disqus !

    Agreeing with you, that being at the same level of abstraction is much more important than blindly following metrics.

    Btw. In RenovateApartment method refactoring we can go even further by introducing a validator as a separate concern making the method even more readable and shorter.

  • Jahongir Rahmonov

    Great read! Thanks