spO0q 🐒🎃It's not uncommon to write "quick and dirty" scripts to test something or in the earliest stages of the project. Besides, legacy projects often contain unoptimized code.
It's not a big deal until the project becomes a maintenance nightmare. To prevent such bad outcome, teams can schedule refactoring sessions regularly.
However, it can be difficult to start, so here are a few examples using PHP, but many of the following tricks can be applied to other programming languages.
Too long code
This sign is probably one of the easiest to spot.
If a class has too many methods, variables, and/or its methods have too many lines, it's often a warning sign.
Updating existing methods instead of creating new classes can be tempting, but it usually bloats the code. Besides, you will likely break the SOLID principles, doing too many things, while the code could be composed of way smaller objects.
Such operation can be tough to achieve depending on the original code, but it's often rewarding for maintenance. It may even allow catching undetected errors and logical issues you've missed before because the code was too intricate.
The other good side effects for maintainers can be:
- better readability in general
- less stuff to remember (helpful when maintaining multiple projects)
- less unnecessary comments (replaced by methods with appropriate naming)
- less duplication
Dead code
Another obvious sign is dead code.
It's not uncommon to find unused variables and methods (even entire classes, sometimes). In this case, just drop them.
Although, it might not be as easy as it seems, so check all potential usages carefully before proceeding:
- use your favorite IDE
- use static analysis but bear in mind linters are not debugging tools (it can't execute code)
- use debuggers
Doublons
Duplicated code is quite frequent. The DRY principle (Do No Repeat Yourself) recommends merging identical or similar methods to ease the maintenance.
HUGE DISCLAIMER: In some cases, the operation creates more problems than it solves. Centralizing your code at all costs, for the sake of DRY, is not so clever and can even introduce nasty regressions.
Before proceeding, ensure the methods you want to merge do not already handle too many cases. Likewise, refactoring small methods into a big one with too many arguments is a bad practice and probably means the two methods were not that similar.
More practical examples
The previous points are essential but quite generic. Let's dive into code.
N.B.: we're assuming many fields (like $total) we use in our examples are defined somewhere in the code before
Useless intermediary methods
Let's consider the following class :
public function getFactor() {
return ($this->isFirstPart()) ? 1 : 3;
}
public function isFirstPart() {
return $this->total < 2;
}
A typical refactoring would merge the two methods:
public function getFactor() {
return ($this->total < 2) ? 1 : 3;
}
Of course, it does not mean you should do the same in all cases. Indeed, methods like isFirstPart() may be fine as long as you don't multiply them in the code. If there are too many similar helpers, you will harm readability seriously.
Before proceeding, just make sure isFirstPart() is not used elsewhere.
Long conditions
Logical issues happen all the time. Refactoring can help spot malformed conditions, but even if everything works fine, you can improve readability significantly.
if ($total > 19 || $discount < 30 || $category === "premium" || $coupon === "summer2014") {
// some code
}
The above code can be wrapped into a function:
public function isEligible() {
return $product->total > 19 || $product->discount < 30 || $user->category === "premium" || $product->coupon === "summer2014"
}
// to be used like that:
if ($user->isEligible()) {
// some code
}
Of course, it's a quick example that is definitely not the ultimate refactoring, but the idea is to keep it short, readable and easy to use (and reuse).
Don't be so negative!
Don't refactor conditions with negative statements like the following:
if ($user->isNotEligible()) {
// some code
}
The above is bad. Why?
How do you handle users who are eligible?
if (!$user->isNotEligible()) {
// some code
}
It does not feel right. The reason is simple: it's way less readable, so instead, use the positive format:
if ($user->isEligible()) {
// some code
}
// or the other case
if (!$user->isEligible()) {
// some code
}
Switch vs. Polymorphism: the classic battle
class Universe {
protected $segment;
public function handleTheUniverse() {
switch ($this->segment) {
case "alphaone":
echo "Alpha Alpha One, buddy";
break;
case "beta3":
echo "Welcome to Beta III";
break;
case "ulysse31":
echo "Hello, Ulysse 31";
break;
}
}
}
It's quite frequent to find the above code while there are other approaches that could ease the maintenance. For example:
abstract class Universe {
abstract function getSegment();
}
class AlphaOne extends Universe {
public function getSegment() {
echo "Alpha Alpha One, buddy";
}
}
class Beta3 extends Universe {
public function getSegment() {
echo "Welcome to Beta III";
}
}
class Ulysse31 extends Universe {
public function getSegment() {
echo "Hello, Ulysse 31";
}
}
It is a bit counterintuitive, as we replace the centralized switch with small child classes that extend the same abstract.
For now, the benefit might not be obvious, and you might even say the switch case looked pretty handy. However, suppose you have to run more complicated operations based on the $segment, you will bloat the code quickly.
For any new "variant," the alternative approach lets you create a child class in another file without having to modify the existing code.
Indeed, refactoring an existing switch can be challenging, but it's usually worth it.
Wrap up
Refactoring is a tricky operation that involves multiple parameters. There's no generic rule you can apply blindly, but there are signs your PHP code probably needs it.
These techniques can improve readability and maintenance dramatically. You can even spot hidden errors and logical issues.
Although, it's best if you can discuss "big refactoring" that involve large segments of the code with your teammates during code reviews. Even if such deep changes are required and show the initial approach was not the best one, the idea is not to break business.
