How to tame or nuke conditionals
This week I have been living in the land of legacy code. It’s a region most developers inhabit for at least part of the time. We negotiate weed-choked ruins and make camp in a temple erected in honour of a forgotten god. We decipher ancient half-obscured runes carved in repeating patterns onto mottled walls. Beneath the altar we discover a torn piece of parchment and just make out the letters scrawled in what can only be blood: ABANDON HOPE ALL YE WHO VI HERE.
All of that is fanciful nonsense of course. Except for the parchment – which was a 2000-line Perl module I was forced to work on in 2004 – and for the warning – a prominent comment left at the top of the file for any developer with the hubris to meddle with the cursed code below it.
Although there’s a kind of horror to legacy systems (the SQL query copy/pasted thirty-two times into twenty-five templates, every other block of near-identical code edited in a subtle way so that no single refactor can be applied without causing multiple devious bugs) there also some pleasures. We’ve already encountered one of these. I love discovering the messages left behind by happily departed colleagues (Yes, it’s a horrible kludge, but this is <name-of-company-redacted> so who cares?). Another pleasure can be found in the careful process of refactoring parts of the system, of rendering the ruin usable again.
Of course, that kind of refactoring is a big topic. But, as I worked my way through a five-screen function and tried to keep track of six layers of nested conditionals I decided that’s where I would start. That was the low hanging fruit. Sort out the conditionals and even the most obscure code is instantly easier to manage and improve.
So here’s the article. Let’s cut through some weeds.
What’s the problem
At the risk of stating the obvious, a conditional is a block of code that will run only if a test is met or, in the case of an else clause, if the test is not met.
I’ve already suggested one problem with many conditionals in code. If not watched, they tend to grow beyond the frame of an editor screen, so that you’re forced to try to remember the trigger that governs the lines of code you’re reading or editing. This becomes even more of a issue if the conditional is nested.
This particular line of code will run, you remind yourself, first if something is true three screens up and then if something two screens up is not true and, finally, so long as some check just above the edit view confirms that a variable is both set and is a number greater than five. What was that first test again?
Another common problem comes when code reuses the same set of conditionals across multiple methods in a class. This is a kind of code duplication, because if you change the nature of a test in one place (the variable is an integer larger then six, not five) you need to remember to apply the same change everywhere that you define a parallel conditional.
The good news is that there is a toolbox filled with simple techniques you can use to get reduce the footprint of conditional code, or even get rid of some conditionals altogether.
Return Early (Just get out of Dodge)
This is probably the easiest refactor you can apply to a conditional inside a method or function. It’s very satisfying to be able to remove braces and in indent from forty lines of code or more.
When to do this
Typically you use this simple trick when you find most of a method is buried inside a conditional clause alongside another, much shorter, else clause.
The problem with a structure like this is that, as we’ve discussed, it can be easy to get lost inside a large conditional block. Worse, at least one part of the if/else construct is redundant. So let’s get rid of it.
Here’s a mock-up of the problem:
public function renderRoom(string $name): string
{
if ($room = $this->getRoom($name)) {
// do
//
// lots
//
// and
//
// lots
//
// of
//
// stuff
//
return $this->doRender($room);
} else {
return $this->roomError($name);
}
}
The problem here is with the “lots and lots of stuff” – unnecessarily wrapped in an if clause. Once the block has grown to more than a screen’s worth of code you lose sight the test the block depends upon:
// lots
//
// of
//
// stuff
//
return $this->doRender($room);
} else {
return $this->roomError($name);
}
So let’s fix it.
Fix it
All that’s needed here is to strip out that sad else clause and place the logic right at the top. If the test for our original conditional is not met, we simply perform some error handling and bail. This initial check is known as a guard clause. We will see further guard clause examples later on. Once this check-and-bail routine is in place, the rest of the code in the method can be freed up entirely.
public function renderRoom(string $name): string
{
$room = $this->getRoom($name);
if (! $room) {
return $this->roomError($name);
}
// do
//
// lots
//
// and
//
// lots
//
// of
//
// stuff
//
return $this->doRender($room);
}
So, because we’ve returned early with the error condition, the rest of the method – which is its core logic – no longer needs to be conditional at all.
Extract method: factor down the size of conditional clauses
Long blocks of code often suggest a design issue. As we have seen, this is particularly true of conditional clauses. One easy way to reduce the size of a clause is to see which parts of it can be extracted into a separate method. As an added bonus you may find ways of factoring out duplicated code as part of this process!
when to do do this
If a conditional clause has grown much beyond a single screen it’s probably time to look at ways of cutting it down to size.
Let’s fake up the problem.
public function accountInfo(Account $account): string
{
if ($account->isEu()) {
$report = "";
// do some EU related stuff
$expire = $account->getExpiry();
$expdate = new \DateTime($expire);
$formatted = $expdate->format("Y-m-d");
$report .= $formatted;
// do some more EU related stuff
return $this->sendReport($report);
} else {
$report = "";
// do some rest-of-world related stuff
$expire = $account->getExpiry();
$expdate = new \DateTime($expire);
$formatted = $expdate->format("Y-m-d");
$report .= $formatted;
// do some more rest-of-world related stuff
return $this->sendReport($report);
}
}
In this example, I need to operate differently according to whether or not an account is European. For each type of account, we use the same process to generate report information. This demonstrates two issues. Firstly, the report processing is a little verbose (probably a lot less verbose than real code would be). Secondly, the duplication unnecessarily bulks out the statement and, over time, invites bugs caused by unevenly applied changes.
Fix it
Luckily, the fix is pretty easy. All we need to do is find any discreet blocks of code and factor them into their own methods. If we fix duplication in the process that’s an added bonus!
In this case, I might move the report formatting code into the current class – or I might choose to add it to the Account class as a utility function which can be made available more widely. Either way, I want to extract the code from the conditional clauses. Here’s my utility method:
class Account
{
// ....
public function formatExpiry(string $format): string
{
$expire = $this->getExpiry();
$expdate = new \DateTime($expire);
$formatted = $expdate->format($format);
return $formatted;
}
}
And now, my conditional clauses have shrunk:
public function accountInfo(Account $account): string
{
if ($account->isEu()) {
$report = "";
// do some EU related stuff
$report .= $account->formatExpiry("Y-m-d");
// do some more EU related stuff
return $this->sendReport($report);
} else {
$report = "";
// do some rest-of-world related stuff
$report .= $account->formatExpiry("Y-m-d");
// do some more rest-of-world related stuff
return $this->sendReport($report);
}
}
Notes and consequences
Simply by extracting a relatively self-contained chunk of a clause and plonking it into a separate method you can render your conditional statement more compact. At the same time you may even factor out duplication, and possibly even further encourage code reuse by making the method available to other methods and classes.
Consolidate conditional expression
The previous example cut our conditional clauses quite significantly but there is still some duplication left over. the line $report .= $account->formatExpiry("Y-m-d"); is repeated. This code does not justify its own method, perhaps, but it does not need to be repeated. It’s worth looking at your code and checking for any repeating aspects that might be shared outside of the conditional rather than duplicated within its clauses.
When to do do this
You can apply this refactor when you spot duplications in a conditional statement that could just as well be set up outside of the structure.
We’ve already seen the problem. Let’s fix it.
Fix it
public function accountInfo(Account $account): string
{
$report = "";
$report .= $account->formatExpiry("Y-m-d");
if ($account->isEu()) {
// do some EU related stuff
// do some more EU related stuff
} else {
// do some rest-of-world related stuff
// do some more rest-of-world related stuff
}
return $this->sendReport($report);
}
There is nothing conditional about the formatExpiry() code so it can be pulled out and shared. This is simple enough but, when I’m cleaning up code, I see this kind of duplication all the time. It’s an easy optimisation to look out for.
Replace conditional with polymorphism
By its title, this is a complicated-sounding refactor, but if you’re familiar with class inheritance, it’s really not that daunting.
I’ve already written a lot about the evils of duplication in code. Up until now, though, I’ve focussed on duplications that play out across conditional clauses in the same statement. There is another common kind of duplication that involves the repetition of entire sets of conditional tests from one method to another.
When to do this
You can apply this refactor if you see the same sets of conditional tests appear in parallel across the methods of a class.
In our previous examples, we tested whether an Account object EU based or not. What would the code look like if we needed to apply that test in more than one method? Let’s mock it up.
class UserBilling
{
public function __construct(private Account $account)
{
}
public function accountInfo(): string
{
$report = "";
$report .= $this->account->formatExpiry("Y-m-d");
if ($this->account->isEu()) {
// do some EU related stuff
} else {
// do some more rest-of-world related stuff
}
return $this->sendReport($report);
}
public function applyOffer(Offer $offer): float
{
$charge = $this->account->getCharge();
if ($this->account->isEu()) {
// apply the offer in an EU way
} else {
// apply the offer in a rest-of-world way
}
return $charge;
}
// ...
}
I’ve kept my previous improvements – common code has been factored out from the conditional clauses. But the parallel conditionals look decidedly fishy. This would only get worse if the same conditional was to be needed across more methods. Aside from the generaly ugliness, this code is susceptible to maintenance issues. Should we need to add additional clauses, or amend our test expressions, we’d need to make sure we do it for every parallel conditional in the class. If you know the code it’s perhaps not massive problem. But you’re asking for trouble when a colleague amends one conditional statement without knowing to update its siblings across the the class.
Fix it
When you see an anti-pattern of this sort, it could be a sign that the single class actually wants to be multiple child classes of a common parent.
We start by creating the parent.
abstract class UserBilling
{
public function __construct(protected Account $account)
{
}
abstract public function accountInfo(): string;
abstract public function applyOffer(Offer $offer): float;
// ...
}
Next, we need concrete child classes for each condtional case. Here is EuUserBilling:
EuBilling
class EuUserBilling extends UserBilling
{
public function __construct(Account $account)
{
if (! $account->isEu()) {
throw new \Exception("EU accounts only");
}
parent::__construct($account);
}
public function accountInfo(): string
{
$report = "";
$report .= $this->account->formatExpiry("Y-m-d");
// do some EU related stuff
return $this->sendReport($report);
}
public function applyOffer(Offer $offer): float
{
$charge = $this->account->getCharge();
// apply the offer in an EU way
return $charge;
}
}
Which leaves WorldUserBilling:
class WorldUserBilling extends UserBilling
{
public function __construct(Account $account)
{
if ($account->isEu()) {
throw new \Exception("non-EU accounts only");
}
parent::__construct($account);
}
public function accountInfo(): string
{
$report = "";
$report .= $this->account->formatExpiry("Y-m-d");
// do some EU related stuff
return $this->sendReport($report);
}
public function applyOffer(Offer $offer): float
{
$charge = $this->account->getCharge();
// apply the offer in an EU way
return $charge;
}
}
Notes and consequences
This solution is very neat. We have done away with our parallel conditionals. It may seem as if we’ve done away with a conditional altogether but, in fact, we have likely just pushed it back somewhat. Somewhere along the line we are going to need to choose whether to instantiate an EuUserBilling or WorldUserBilling object. But we’ll only going make that choice once rather then across multiple methods.
On the other hand, inheritance is a pretty inflexible solution. It works here here because the choice between an EU and a world implementations is the only real fault-line with which we need to concern ourselves. The real world is often messier than that. Remember the famous Gang of Four principle: favor composition over inheritance. Let’s move on and do just that.
Replace conditional logic with strategy
So, rather than create concrete classes based on a conditional statement’s tests, we might instead require that a strategy object is passed in runtime. This will likely be specialised to implement one or other of our conditional’s blocks.
When to do this
As before, this approach is great for (but not limited to) parallel conditionals. It is especially useful where the class you are refactoring is already specialised in some way, making it impractical to specialise to support the strategies implied by the conditional you wish to replace.
Let’s start a new example. The Report class will write either to standard output or a file depending upon whether the provided $path property is null.
class Report
{
private string $report = "";
public function __construct(private ?string $path = null)
{
}
// ...
public function output(): void
{
if (is_null($this->path)) {
print $this->report;
} else {
file_put_contents($this->path, $this->report);
}
}
}
In fact, this conditional is probably relatively harmless as it is, but imagine a much more involved set of conditional clauses with tests that repeat across multiple methods. In short, the conditional has to go.
Fix it
So let’s strip it out and require a helper to do the work instead.
class Report
{
private string $report = "";
public function __construct(private Output $outputter)
{
}
// ...
public function output(): void
{
$this->outputter->out($this->report);
}
}
I kill the conditional and require an Output strategy object instead. This will have an out() method. The Report class no longer knows or cares about how output is managed – it simply passes its report on and gets on with its core tasks (or it would if it had any core tasks).
Here is the Output interface.
interface Output
{
public function out(string $str): void;
}
Now to provide some implementations. First, the StdOutput class
class StdOutput implements Output
{
public function out(string $str): void
{
print $str;
}
}
Next, the FileOutput class
class FileOutput implements Output
{
public function __construct(private string $path)
{
}
public function out(string $str): void
{
\file_put_contents($this->path, $str, \FILE_APPEND);
}
}
Notes and consequences
We will still have to make the decision somewhere about which strategy to use so there is a certain amount of buck passing going on. Of course this is a textbook inversion of control example. As such, it’s a good use case for a dependency injection container.
Although I have touted this as a strategy that uses composition rather than inheritance, there is, of course, no reason why the strategy object itself should not use inheritance should it need shared functionality in a base class.
Move nested conditionals to guard clauses
Nested conditionals can be particularly pernicious. Not only do they tend to bloat your code, they require that you keep in mind multiple layers of test expressions.
When to do this
Any substantial nesting of conditional statements may be a candidate for flattening and simplification using this technique. In many cases you can flatten the nesting into a series of single if statements, each of which returns if its condition is matched.
Here is the an example of the problem.
public function renderRoom(Room $room): string
{
if ($room->isFlooded()) {
// apply watery stuff
$ret = "The room is under water";
} else {
if ($room->isTooDark()) {
// apply dark
$ret = "The room is too dark to see";
} elseif ($room->isEnchanted()) {
// apply magic
$ret = "The room is a glittering vortex of illusion";
} else {
$ret = $room->getDescription();
}
}
return $ret;
}
So the nesting here is already a little confusing. Real world instances of this problem often run much deeper and fatter. Luckily, the way that condional clauses all assign alternate values to the $ret variable tells us that we might be able use a series guard clauses to radically simplify things.
Fix it
Let’s take the first clause and make a guard clause of it.
if ($room->isFlooded()) {
// apply watery stuff
return "The room is under water";
}
We’ve already improved matters. The main if/else statement has gone. After the guard clause, we are left with the contents of what was the else clause:
if ($room->isTooDark()) {
// apply dark
$ret = "The room is too dark to see";
} elseif ($room->isEnchanted()) {
// apply magic
$ret = "The room is a glittering vortex of illusion";
} else {
$ret = $room->getDescription();
}
But why stop there? The entire structure can be rendered as a series of guard clauses.
public function renderRoom(Room $room): string
{
if ($room->isFlooded()) {
// apply watery stuff
return "The room is under water";
}
if ($room->isTooDark()) {
// apply dark
return "The room is too dark to see";
}
if ($room->isEnchanted()) {
// apply magic
return "The room is a glittering vortex of illusion";
}
// default description
return $room->getDescription();
}
We have got rid of the confusing deep logic and dispensed with the $ret variable altogether.
Notes and consequences
This is, of course, simply a slightly evolved version of our first get out of Dodge refactor which also demonstrates the use of a guard clause.
Conclusion
Adverbs don’t make you a bad writer and conditionals don’t make you a bad coder but in editing and refactoring it’s always a good idea to pare both down if you can. I have found these tricks pretty useful over the years for making legacy code significantly more usable with minimal risk. Happy paring!
You can find source code for all my recent articles on GitHub. The code for this article is in the 002.taming-conditionals directory.
Photo by Jens Lelie on Unsplash
