Code smell: how to fix Long Methods

Long Method is our code smell type today.

What is a code smell?

A code smell is a sign that something in your code might be poorly designed and could cause problems later on.

In this series of posts I will talk about the most common code smells problems and then how fix each one.

Problem:

A method with many lines of code, or not in the best-case, but which is still difficult to understand, reuse, and maintain.

Ways to avoid it:

Divide your confusing method into well-defined part

You have a block of code that logically belongs together:

//Bad :-(
//Everything mixed together
function showProfile() {
    $this->showHeader();

    // All profile details printed here
    echo "User: " . $this->username;
    echo "Email: " . $this->email;
}

Extract it into a new method and replace the original code with a call to that method:

//Good :-)
//Details moved to another function
function showProfile() {
    $this->showHeader();
    $this->printUserInfo();
}

function printUserInfo() {
    echo "User: " . $this->username;
    echo "Email: " . $this->email;
}

If you change the user info, you only change one function, and you can reuse it.

Change temporary variables by a query

Sometimes, you create a variable just to store a value temporarily so you can use it later in your code. This makes the code harder to reuse, harder to clean up, and harder to extract into smaller methods:

//Bad :-(
//Using a temp variable
$totalCost = $this->cups * $this->pricePerCup;

if ($totalCost > 20) {
    return "You get a free cookie!";
} else {
    return "No free cookie this time.";
}

Create a small function that can calculate the value anytime you ask:

//Good :-)
//Replace temp var with query
if ($this->totalCost() > 20) {
    return "You get a free cookie!";
} else {
    return "No free cookie this time.";
}

function totalCost() {
    return $this->cups * $this->pricePerCup;
}

The total cost is calculated in one single place, where you can change the price or rule.

Change parameters that are repeated by an object

Your methods contain a repeating group of parameters:

class Customer
{
    public function amountInvoicedIn(DateTime $start, DateTime $end) {
        // ...
    }

    public function amountReceivedIn(DateTime $start, DateTime $end) {
        // ...
    }

    public function amountOverdueIn(DateTime $start, DateTime $end) {
        // ...
    }
}

Replace these parameters with an object:

class Customer
{
    public function amountInvoicedIn(DateRange $range) {
        // ...
    }

    public function amountReceivedIn(DateRange $range) {
        // ...
    }

    public function amountOverdueIn(DateRange $range) {
        // ...
    }
}

Less repetition, cleaner code, and the ability to make changes within the object in a centralized way.

Preserve whole object

You write a method that needs several pieces of data from the same object, you pass three or four values taken from the same object for instance just to call another method:

$startDatetime = $today->getStartDatetime();
$endDatetime = $today->getEndDatetime();
$todaysTime = $time->timeDiff($startDatetime, $endDatetime);

Instead of doing that, you can simply pass the entire object and let the method get whatever it needs:

$todaysTime = $time->timeDiff($today);