5 Quick Ways to Make Your Code More Maintainable Header

In Clean Code, Robert Martin gives us the following helpful piece of information.

.. [T]he ratio of time spent reading versus writing is well over 10 to 1. We are constantly reading old code as part of the effort to write new code. …[Therefore,] making it easy to read makes it easier to write.

With this in mind, we would like to give you 5 tips on how you can improve your code so it’s more readable.

Our Example

Let’s say your working on fixing a bug in our project management tool and you run across the following.

// code

$project->rebuild(5, true);

// more code

Based just on the code above, you can’t know what’s going on in the rebuild() function. Is it doing something that might be causing the bug? What do 5 and true do? Why is it located where it’s at. Your only solution is to navigate to that function and see what it’s doing but that costs you valuable time and takes you out of the flow of reading the calling function.

When we look at the function this is what we find.

/**
 * This function rebuilds the estimated time to completion
 * @param int $weeks The weeks to checks
 * @param bool $email If we should email out an update
 * @returns void
 */
public function rebuild(int $weeks, bool $email = false): void
{
    // get the incomplete tasks assigned to this project
    $tasks = Task::where('project_id', $this->id)
        ->whereNull('end_date')
        ->get();

    $total = 0;
    foreach ($tasks as $task) {
        if ($task->time_estimate > 1) {
            $total += $task->time_estimate;
        }

        if ($task->time_estimate === null) {
            $total += 2;
        }
    }

    $total = ceil($total / 8);
    if (floor($total / 5) > $weeks) {
        return;
    }

    $newDate = now()->modify("+{$total} days");
    if ($this->estimated_date == $newDate) {
        return;
    }

    $this->estimated_date = $newDate;
    $this->save();

    if ($email) {
        $users = User::where('role_id', 5)
            ->get();

        foreach ($users as $user) {
            $user->sendNotification("Project {$project->name} has a new estimated completion date");
        }
    }
}

Reading the DocBlock information at the top of the screen helps us a little but reading through the function itself is difficult. The function is doing a lot and its body doesn’t fit on the screen without scrolling on my laptop. We need to do some cleanup.

1. Use Intention-Revealing Names

Naming things is hard but it’s super important that we name everything (functions, classes, variables, etc.) with names that reveal intent and don’t cloud up the flow of reading. It’s worth us taking extra time to think up a name that’s clear and reveals what the function, class, or variable does from the calling function.

Let’s look back at our example function for a minute.

/**
 * This function rebuilds the estimated time to completion base on the incomplete tasks 
 * @param int $weeks The weeks to checks
 * @param bool $email If we should email out an update
 * @returns void
 */
public function rebuild(int $weeks, bool $email = false): void
{

The DocBlock comment says specifically that the “function rebuilds the estimated time to completion base on the incomplete tasks” but to know that you have to either visit the function definition or focus on the function in your editor for a pop-up (if it supports this). Think of how much time would be lost doing this over and over again. To make it easier when we’re reading the calling function we’re just going to rename the function to almost exactly what the comment says.

/**
 * This function rebuilds the estimated time to completion base on the incomplete tasks 
 * @param int $weeks The weeks to checks
 * @param bool $email If we should email out an update
 * @returns void
 */
public function rebuildEstimatedDateBasedOnIncompleteTasks(int $weeks, bool $email = false): void
{

Then we can replace it anywhere we call it.

$project->rebuildEstimatedDateBasedOnIncompleteTasks(5, true);

Now it’s significantly easier to read but we still don’t know what 5 and true are doing.

2. Keep Functions Short

Now that we’ve renamed our function so it’s easier to determine what it’s doing when we’re calling it, let’s look at how we can make it easier to read on the inside.

The number one thing that you can do to make your functions easier to read is to keep them short. Keeping your function short makes it easier to digest when your reading and helps reduce bugs because there are just fewer moving parts that could go wrong inside the function.

The next version of our rebuildEstimatedDateBasedOnIncompleteTasks() function has the exact same functionality but we extracted it into smaller functions.

/**
 * This function rebuilds the estimated time to completion
 * @param int $weeks The weeks to checks
 * @param bool $email If we should email out an update
 * @returns void
 */
public function rebuildEstimatedDateBasedOnIncompleteTasks(int $weeks, bool $email = false): void
{
    // get the incomplete tasks assigned to this project
    $tasks = $this->getIncompleteTasks();
    $total = $this->calculateEstimatedHoursForTasks($tasks);

    if ($this->totalGreaterThanMaxNumberOfWeeks($total, $weeks)) {
        // we're unable to calculate estimated_date because it's too far in the future
        return;
    }

    if (!$this->updateEstimatedDateFromTotalRemainingDays($total)) {
        return;
    }

    $this->save();

    if ($email) {
        $this->sendNotificationsAboutNewEstimatedCompletionDate();
    }
}

private function getIncompleteTasks(): array 
{
    return Task::where('project_id', $this->id)
        ->whereNull('end_date')
        ->get();
}

private function calculateEstimatedHoursForTasks(array $tasks): int
{
    $total = 0;
    foreach ($tasks as $task) {
        if ($task->time_estimate > 1) {
            $total += $task->time_estimate;
        }

        if ($task->time_estimate === null) {
            $total += 2;
        }
    }
}

private function totalGreaterThanMaxNumberOfWeeks(int $total, int $maxNumberOfWeeks): bool
{
    $total = ceil($total / 8);
    if (floor($total / 5) > $maxNumberOfWeeks) {
        return true;
    }

    return false;
}

private function updateEstimatedDateFromTotalRemainingDays(int $totalDays): bool
{
    $newDate = now()->modify("+{$totalDays} days");
    if ($this->estimated_date == $newDate) {
        return;
    }

    $this->estimated_date = $newDate;
}

private function sendNotificationsAboutNewEstimatedCompletionDate()
{
    $users = User::where('role_id', 5)
        ->get();

    foreach ($users as $user) {
        $user->sendNotification("Project {$project->name} has a new estimated completion date");
    }
}

This version contains more functions and more lines but it makes the original function easier to read and debug. It also has the nice side effect that it created all these little helper functions that most likely we’ll be reusing as we develop our application.

3. No Magic Numbers

Magic numbers are any values with an unexplained meaning in your code. This is an extension on #1 in that the values don’t explain their intent and cause confusion.

Let’s look at one of our example functions.

private function totalGreaterThanMaxNumberOfWeeks(int $total, int $maxNumberOfWeeks): bool
{
    $total = ceil($total / 8);
    if (floor($total / 5) > $maxNumberOfWeeks) {
        return true;
    }

    return false;
}

The 8 and 5 exist in this sample with no explanation as to why they’re there and it can be confusing when we (or someone else) comes back to this section of code. What we’re going to do is replace them with a constant that explains what they are.

private function totalGreaterThanMaxNumberOfWeeks(int $total, int $maxNumberOfWeeks): bool
{
    $total = ceil($total / Project::NUMBER_OF_HOURS_IN_BUSINESS_DAY);
    if (floor($total / Project::NUMBER_OF_DAYS_IN_WEEK) > $maxNumberOfWeeks) {
        return true;
    }

    return false;
}

Isn’t that a lot easier to read? The lines are a little longer but we can more easily process the intent of what those lines are doing.

Getting back to our original function call. We see there’s a magic number there as well.

$project->rebuildEstimatedDateBasedOnIncompleteTasks(5, true);

We can easily replace it as well.

$project->rebuildEstimatedDateBasedOnIncompleteTasks(
    Project::MAX_NUMBER_OF_WEEK_TO_ESTIMATE,
    true
);

4. No Flag Arguments

A flag argument is an argument that tells the function to carry out a different operation depending on its value. Again, this comes back to intent. What is the intent of true in our function call here? This is another case where we have look into the function to determine that.

$project->rebuildEstimatedDateBasedOnIncompleteTasks(Project::MAX_NUMBER_OF_WEEK_TO_ESTIMATE, true);

There are two options to make this more readable.

The first is that we could have the calling function first call rebuildEstimatedDateBasedOnIncompleteTasks() and then our sendNotificationsAboutNewEstimatedCompletionDate() function. This works but the downside to it is that we’re making it hard to read from the calling function and potentially scattering the same two function calls over and over again in our code and if we need to add another step to this process we might have to manually do it. Doing things manually is hard and potentially error-prone.

The second is to create a new function that does both steps. This adds a function to our Project class but makes the calling function easier to read and maintain.

$project->rebuildEstimatedDateBasedOnIncompleteTasksAndEmailIfUpdated(
    Project::MAX_NUMBER_OF_WEEK_TO_ESTIMATE
);

We just have to make a small change to our rebuildEstimatedDateBasedOnIncompleteTasks() function so it returns a boolean indicating if the value was updated and move the if statement into it’s own function.

/**
 * This function rebuilds the estimated time to completion
 * @param int $weeks The weeks to checks
 * @param bool $email If we should email out
 * @returns bool
 */
public function rebuildEstimatedDateBasedOnIncompleteTasks(int $weeks): bool
{
    // get the incomplete tasks assigned to this project
    $tasks = $this->getIncompleteTasks();
    $total = $this->calculateEstimatedHoursForTasks($tasks);

    if ($this->totalGreaterThanMaxNumberOfWeeks($total, $weeks)) {
        Log::info("Unable to calculate estimated_date because {$total} days is more than {$weeks} weeks");
        return false;
    }

    if (!$this->updateEstimatedDateFromTotalRemainingDays($total)) {
        return false;
    }

    $this->save();

    return true;
}

public function rebuildEstimatedDateBasedOnIncompleteTasksAndEmailIfUpdated($int $weeks): bool
{
    if ($this->rebuildEstimatedDateBasedOnIncompleteTasks($weeks)) {
        $this->sendNotificationsAboutNewEstimatedCompletionDate();
        return true;
    }

    return false;
}

5. Stop Using So Many Comments

Most comments are junk and you should stop using them. The “rule” that you should comment your code has wasted people’s time. They tend to be unhelpful or worse out of date so they actually make it harder to understand the code.

Stop Using DocBlock

DocBlock was great. It explains what the function is supposed to do, what it’s parameters are, and what it’s supposed to return. However, now that we have return types and parameter types for functions it’s largely duplicate information we have to maintain that quickly becomes out of date if it was even entered correctly.

Let’s look at the DocBlock on our rebuildEstimatedDateBasedOnIncompleteTasks() function.

/**
 * This function rebuilds the estimated time to completion
 * @param int $weeks The weeks to checks
 * @param bool $email If we should email out
 * @returns bool
 */
public function rebuildEstimatedDateBasedOnIncompleteTasks(int $weeks): bool
{

When we redid the function we forgot to remove the following line:

/**
 * @param bool $email If we should email out
*/

That can be removed easily.

/**
 * This function rebuilds the estimated time to completion
 * @param int $weeks The weeks to checks
 * @returns bool
 */
public function rebuildEstimatedDateBasedOnIncompleteTasks(int $weeks): bool
{

We also have these two lines:

/**
 * @param int $weeks The weeks to checks
 * @returns bool
*/

The bool is covered by the return parameter and the weeks to check can be incorporated into the variable name.

/**
 * This function rebuilds the estimated time to completion
 */
public function rebuildEstimatedDateBasedOnIncompleteTasks(int $weeksToCheck): bool
{

That leaves us with a single line.

/**
 * This function rebuilds the estimated time to completion
 */

This is essentially the name of the function so we can easily delete it as well.

public function rebuildEstimatedDateBasedOnIncompleteTasks(int $weeksToCheck): bool
{

We just make it easier to maintain this function without sacrificing any information.

Use Functions Instead of Comments

Let’s go back to our original function and pick out this piece.

// get the incomplete tasks assigned to this project
$tasks = Task::where('project_id', $this->id)
    ->whereNull('end_date')
    ->get();

The comment expresses what the next section does but then we still have read over the next section as we work our way through the code. To make the code more readable we can take the comment and make it the function name of the code we extracted into its body.

$tasks = $this->getIncompleteTasks();

This makes it much easier to read.

When Is It Okay To Use a Comment?

So it must be okay to use comments some of the time right? There are going to be cases were the only way to explain why something is happening is to do it in a comment.

if ($this->totalGreaterThanMaxNumberOfWeeks($total, $weeks)) {
    // we're unable to calculate estimated_date because it's to far in the future
    return;
}

We recommend that you only add comments if there is no other way to express that intent in code and you can be sure the comment will stay up to date.

Still More

Hopefully, this has helped you in your day to day work. If you feel like you want more you could pick up a copy of Clean Code by Robert Martin where he goes into more examples of how to write your code for reading. It has a lot of information in it and it’s well worth the read.

Header image by @punttim