While working at Human Made on enterprise codebases, we get to see a lot of legacy code and review code from developers a wide variety of skill levels. This lets us see quite a few interesting and anti-patterns and one of the most common patterns that I see cropping up across projects and developers is a tendency to nest code, and even worse to deeply nest logic statements and conditionals.
Now, nesting in and of itself isn’t evil. Sometimes it can simplify code and make it more readable. However, code that is indented more than one level is a blinking red bad code sniff and is usually an indication of bugs hiding within those levels.
Because deeply nesting conditionals makes code more difficult to read, it allows bugs to hide that would otherwise be obvious to a reviewer. When you’re reading through nested conditionals, you have to remember what happens at every step along the path before the current conditional and each piece of logic compounds on the logic before it. This is different from a guard clause pattern where each piece of logic stands alone and doesn’t affect the logic that comes after it.
Let’s Look at Some Code
To give an example of how deeply nested code could be cleaned up in a refactor or after, I wanted to walk through a code sample from an old project which we were able to improve and bring back to a normal
Here is the original code:
function get_angel_list_links( $partners ) {
$links = array();
if ( isset( $partners ) ) {
foreach( $partners as $partner ) {
if( $partner['partner_name'] == 'AngelList' ) {
for( $i = 1; $i <= 3; $i++ ) {
if( isset( $partner['link_' . $i . '_url' ] ) && !empty( $partner['link_' . $i . '_url' ] ) && isset( $partner['link_' . $i . '_name' ] ) && ! empty( $partner['link_' . $i . '_name' ] ) ) {
$links[] = array( 'label' => $partner['link_' . $i . '_name' ], 'url' => $partner['link_' . $i . '_url' ] );
}
}
}
}
}
}So what’s the problem here?
This code is only doing one thing, which is to find up to 3 “partners” with the type of AngelList and return it by itself. We’re navigating through some API code which is why there are so many logic statements and so. much. nesting.
The problem here is that this is extremely difficult to read and very easy for nasty little bugs to hide within this logic. Since every indented line relies on all of the lines above it directly, you have to memorize the previous logic and how it applied to know what the current line you’re reading does. I’ve seen bug after bug after bug just because someone nested deeply and didn’t take former logic into account.
Whenever possible, code should never reside in more than one level of nesting. If code is any deeper, it should be broken out using guard clauses or or by breaking up the function.
Let’s try to clean this up a bit.
First, we know that our main wrapper in this case is a simple check which looks for the very existence of the variable we’re passing in. This is easy to extract and provides us with an easy win. This simple pattern is called a guard clause – we “guard” the rest of the code by adding a check that evaluates only one piece of logic.
function get_angel_list_links( $partners ) {
// Make sure that we have valid partners first.
if ( empty( $partners ) ) {
return [];
}
$links = array();
foreach( $partners as $partner ) {
if( $partner['partner_name'] == 'AngelList' ) {
for( $i = 1; $i <= 3; $i++ ) {
if( isset( $partner['link_' . $i . '_url' ] ) && !empty( $partner['link_' . $i . '_url' ] ) && isset( $partner['link_' . $i . '_name' ] ) && ! empty( $partner['link_' . $i . '_name' ] ) ) {
$links[] = array( 'label' => $partner['link_' . $i . '_name' ], 'url' => $partner['link_' . $i . '_url' ] );
}
}
}
}
}Next, we are looping through these items and really only need to look for a single type of data before carrying on. This can be done more clearly by using PHP’s inbuilt array_filter function. This function will ingest our full array of partners and give us back only those partners which match our given criteria.
function get_angel_list_links( $partners ) {
// Make sure that we have valid partners first.
if ( empty( $partners ) ) {
return [];
}
// We only want to look at AngelList partners.
$angellist_partners = array_filter( $this->cb_data['partners'], function( $partner ) {
return $partner['partner_name'] === 'AngelList';
} );
$links = array();
foreach( $angellist_partners as $partner ) {
for( $i = 1; $i <= 3; $i++ ) {
if( isset( $partner['link_' . $i . '_url' ] ) && !empty( $partner['link_' . $i . '_url' ] ) && isset( $partner['link_' . $i . '_name' ] ) && ! empty( $partner['link_' . $i . '_name' ] ) ) {
$links[] = array( 'label' => $partner['link_' . $i . '_name' ], 'url' => $partner['link_' . $i . '_url' ] );
}
}
}
}This is already looking much nicer, but we can improve this even more. We have three levels of nesting left and the logic within them isn’t particularly clear or easy to evaluate. We can reduce the nesting even more here with the application of the while function which will allow us to combine both the final for and if lines into just one check.
function get_angel_list_links( $partners ) {
// Make sure that we have valid partners first.
if ( empty( $partners ) ) {
return [];
}
// We only want to look at AngelList partners.
$angellist_partners = array_filter( $this->cb_data['partners'], function( $partner ) {
return $partner['partner_name'] === 'AngelList';
} );
// Map AngelList partners to their links and append to our links array.
$links = array;
foreach ( $angellist_partners as $partner ) {
$i = 1;
while ( ! empty( $partner[ 'link_' . $i . '_url' ] ) ) {
$links[] = [
'label' => $partner[ 'link_' . $i . '_name' ],
'url' => $partner[ 'link_' . $i . '_url' ],
];
$i++;
}
}
}And the final result looks something like this with some nice type hinting added for further clarity:
/**
* Get a list of AngelList links associated with an organization.
*
* @param array $partners
* @return array
*/
function get_angel_list_links( array $partners ) : array {
// Make sure that we have valid partners first.
if ( empty( $partners ) ) {
return [];
}
// We only want to look at AngelList partners.
$angellist_partners = array_filter( $this->cb_data['partners'], function( $partner ) {
return $partner['partner_name'] === 'AngelList';
} );
// Map AngelList partners to their links and append to our links array.
$links = [];
foreach ( $angellist_partners as $partner ) {
$i = 1;
while ( ! empty( $partner[ 'link_' . $i . '_url' ] ) ) {
$links[] = [
'label' => $partner[ 'link_' . $i . '_name' ],
'url' => $partner[ 'link_' . $i . '_url' ],
];
$i++;
}
}
return $links;
}I hope that you can see how simple coding patterns can dramatically clarify a codebase and remove deeply nested code spaghetti.