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);