Your Methods Are Bad and You Should Feel Bad

It happens to the best of us. You start out by writing some code and putting it in a method, and for one reason or another the method has grown into a giant beast with 500+ lines of code, a dozen execution paths and multiple concerns.

This is problematic because it’s difficult for anyone to be able to read and understand the method in its entirety. Something like this will require a considerable amount of time for you to be able to read all the code, work out all the execution paths and understand what everything is doing in order to work on it effectively. Additionally, with such uncertainty it’s common for developers to fear the code, it’s become known as that method that everyone is too scared to touch.

Compare this to a small method with a couple of inputs and a handful of lines of code; methods like this are bite sized nuggets that can be easily consumed. It will take very little mental processing to comprehend so it can be understood with a glance.

The Solution

To prevent it getting this bad, I have some rules that I follow for writing code. One such rule is to limit methods to contain no more than a dozen lines of code, breaking out excess or unfitting code into standalone methods.

If it's already gotten this bad, ie. you're working on an existing project with legacy code, I recommend you try to break down big methods to follow the above rule.

Let's demonstrate this rule with an example written in PHP:

public function notifyActiveUsers()
{
    // get list of active users
    $statement = $this->pdo->prepare('select * from users where active = ?');
    $statement->execute([1]);
    $users = $statement->fetchAll(PDO::FETCH_OBJ);

    foreach ($users as $user) {
        $this->notifyUser($user->email);
    }
}

In this example, the method is doing two things; querying a database for active users, and looping over the list of users to notify each of them.

While reading the method, you might not be too concerned with where the users are coming from, and instead you are more concerned with how they are getting notified. So it could be said there are two separate concerns within the method, the first being data retrieval and the second actually notifying the users.

To simplify the method we can break it down into two separate methods to reflect the concerns. Arguably, the method was already small and simple and didn't necessarily need breaking down, this is mostly for the sake of example, however there can still be some benefits from such separation as will be outlined later on.

public function notifyActiveUsers()
{
    foreach ($this->getActiveUsers() as $user) {
        $this->notifyUser($user->email);
    }
}

private function getActiveUsers()
{
    $statement = $this->pdo->prepare('select * from users where active = ?');
    $statement->execute([1]);
    return $statement->fetchAll(PDO::FETCH_OBJ);
}

Lets go over some of the benefits of breaking down methods in such a way.

Separation of Concerns

Now the notifyActiveUsers() method is only looping through a list of active users and taking care of notifying them. We are not concerning ourselves with the database or the intricacies of where the users are coming from, so reading this can use up less mental capacity. If we want to dig deeper and find how the users are being retrieved, we can look into the getActiveUsers() method.

The methods now better follow the Unix principle of doing one thing and doing it well. As seen earlier, the intricacies of data retrieval were abstracted away behind the getActiveUsers() method, leaving the notifyActiveUsers() method to only have to worry about the notification process and nothing else.

Self Documenting

Notice as well how the comment that summarised the block of code to get the users has now been made completely redundant after the refactor. The entire block has been summarised in the call to getActiveUsers() which clearly indicates the sort of data we will be working with.

Boundaries

By breaking methods down so much, you create physical boundaries between them in the form of function scope. If the logic of several methods were to remain in one big method, making any form of change runs the risk of accidentally altering something else within the method, like unintentionally changing the value of a variable.

DRY

Having the logic extracted means it’s instantly reusable and keeps your code DRY. If the logic was embedded within a large method and you wanted to reuse it, a lot of developers would be more tempted to copy and paste the logic elsewhere rather than cleanly refactor and reuse.

Readability

Keeping methods short means you can fit the entire method on screen and can avoid scrolling when trying to read or work on it. Being able to comprehend methods with a glance is a huge win.

There are exceptions of course, and sometimes you might have good reasons to store a lot of code in one place, but more often than not I feel long complex methods can be broken down with little to no negative repercussions.