Introduction

The software engineering industry has always been growing, and so has the demand for skilled engineers. But what does “skilled” developer even mean? Does experience in building applications make us skilled? Will staying up to date with the latest software development trends make us skilled?

The word “skilled” is very vague when it connects with the software engineering industry, it means different to different people. However, if we look at what a “skilled” software engineer looks like today, they have some similar characteristics. I came up with my description of a skilled software engineer, which I think best describes them:

A skilled software engineer can write complex code in a way that is efficient, maintainable, and understandable by other developers. The engineer produces high-quality software that meets all the requirements that are given.

Serhii Chornenkyi
The photo of Serhii Chornenkyi

Fewer and fewer people are writing good, readable code nowadays. It was so popularized by giants of our industry, like Ward Cunningham, Robert C. Martin, Kent Beck, Martin Fowler, and others, that it almost became a standard that everybody follows without even thinking about why they do it that way.

I still remember reading “Clean Code: A Handbook of Agile Software Craftsmanship” book for the first time. It changed my life. It was the first time that I understood the real value of writing readable and maintainable code. And it changed not only my life, but the lives of many other developers as well. These principles have entirely changed the way of thinking of hundreds of thousands of developers all over the world.

I want this standard to continue its existence, I love seeing that style of coding, and I also think it's the best approach to building programs, but I can't help but notice that something is not right anymore. We see more and more developers using complicated structures and designs that make code unreadable and difficult to maintain.

I'm not a great programmer; I'm just a good programmer with great habits.

Kent Beck
The photo of Kent Beck

My research

In this article, I'm not blaming anyone for anything, I just want to share my thoughts with you and hopefully, we can make a change in the world.

I did quite a bit of research on this topic and was amazed by the results I discovered along the way. I will include all the references which I find helpful and important on this topic. My mission here is to help as many developers as possible to change the way we write our code and share what I've learned throughout this journey.

You can help with this mission by sharing this article on social media. This is our industry, and we all have to do something about it if we want to see results! Because the results are not there yet. The future of the entire industry depends on us!

All the code examples in this article are taken from production-ready applications and open-source packages used in real-life apps.

A Dungeon of Smell

A Dungeon of Smell is a piece of code that is so complex that it’s nearly impossible to understand, even for experienced programmers. What is complex code? Well, all it requires is a bunch of nested conditional statements and other convoluted logic that make absolutely no sense at all.

Most of it consists of functions or methods with hundreds or even thousands of lines of code, which makes it almost impossible to read and even harder to maintain. Code like this stops us from contributing to open-sourced libraries because it scares us away. We simply don’t understand what’s going on there. Seeing so many open-source using this kind of code has made me want to write my packages instead of contributing somewhere else.

The easiest part of my research for this article was finding code examples on the Internet, which, I think, is worth talking about. For example, this snippet right here from gorilla/mux library for Golang:

func (v routeRegexpGroup) setMatch(req *http.Request, m *RouteMatch, r *Route) {
	// truncated ...

	if v.path != nil {
		matches := v.path.regexp.FindStringSubmatchIndex(path)
		if len(matches) > 0 {
			extractVars(path, matches, v.path.varsN, m.Vars)
			// Check if we should redirect.
			if v.path.options.strictSlash {
				p1 := strings.HasSuffix(path, "/")
				p2 := strings.HasSuffix(v.path.template, "/")
				if p1 != p2 {
					u, _ := url.Parse(req.URL.String())
					if p1 {
						u.Path = u.Path[:len(u.Path)-1]
					} else {
						u.Path += "/"
					}
					m.Handler = http.RedirectHandler(u.String(), http.StatusMovedPermanently)
				}
			}
		}
	}

	// truncated ...
}

More often, you will hear the term: “Deep nesting code smell” or “Deeply nested control flow” to describe it. This library is so popular in the Go world, there must be a good reason for it to be this way. Why would they do this? It's an open-source project, so there shouldn’t be any deadline pressure to make it ready for publishing.

Keep in mind, that code smell in this code is not a sign of failure; it’s a sign of bad design. It indicates that the author was not cautious about the design of the program and just nested everything in there for making it work. But we don't know the true reason.

The term “Code smell” was introduced by Kent Beck in the book called “Refactoring: Improving the Design of Existing Code” by Martin Fowler.

Here is the code sample from the other project:

Code from PHP source code

I've decided to screenshot this code instead of inserting it in the article because it would crash your computer while loading this page in the browser. This code is from PHP source, which is written in C language.

Many code samples look like the one above on GitHub, even if you take a look inside the source code of other programming languages, such as Java or Go. Everyone is guilty of doing it at some point, but the question is… how to not repeat those mistakes?

Two blocks rule

Before we talk about deeply nested code, we should understand what a statement body is. They are the reason it's hard to read nested code.

A statement body is a set of statements that are in most programming languages surrounded by curly braces. An overly long statement body makes your code hard to read and maintain. It also makes it more difficult to modify, change, and refactor later on down the road.

Statement body of the for loop and if statement
Two overlapping block statements illustration

I was thinking a lot about a formula that could help me to follow a specific set of guidelines for writing block statements. So, I came up with this rule, which works insanely great for me. I've called it the “Two blocks rule” and it states:

“Functions and methods can only go 2 block statements deep”.

To better understand this rule, let's take a look at the JavaScript example below. When a function has 2 nested block statements, it's complying with the rule.

Illustration with correct nesting
Illustration with 2 block statements overlapping each other

Our function goes 2 block statements deep, so we are good here. Let's try another example when our function goes more than 2 block statements deep.

Statement body overlapping with 2 children
Parent block statement is overlapping with 2 children

This method is incorrectly nested and will not comply with the “Two blocks rule” because it goes 3 levels deep. It will result in a bad design and make it harder to read and maintain in the future. The “if statement” if (shouldUpdateFields()) must be extracted into a separate method to respect the rule. I would create a method updateArticleFields() and put the whole if statement there instead. It would result in a much better design and be easier to read and understand.

Okay, what about the PHP method below? Does it obey the rule or not?

public function handle(): void
{
    $days_last = config('custom.inactive_users_last_days');
    $users = User::where('updated_at', '<=', now()->subDays($days_last))
        ->whereActive(0)
        ->get();

    foreach ($users as $user) {
        if ($user->photo != 'default.jpg') {
            DeleteFileJob::dispatch($user->photo);
        }

        if ($user->roles()) {
            $user->roles()->detach();
        }

        if ($user->favs()) {
            $user->favs()->delete();
        }

        $user->delete();
    }
}

 It looks clean to me, but does it meet the “Two blocks rule”? Let's recall the rule again.
 
“Functions and methods can only go 2 block statements deep”.
 
The “foreach loop” is the first level of deepness, and every “if statement” is the second level. Therefore, it is compliant with the rule because it's 2 levels of deepness.

Code example that follows "Two blocks rule"

Don't you also think that code like this is a lot easier to understand? I'm not a president of all software companies, but I think everyone can agree that having easy to understand code is a benefit for every individual or company.

We thought it was okay to have two or more blocks intersecting each other. In fact, many of us do this all the time and don't know that there is anything wrong with it. It is only when you start looking at it from a design perspective that you realize that it is not a good thing to do, and you start to look for better ways to write your code.

Any fool can write code that a computer can understand. Good programmers write code that humans can understand.

Martin Fowler
The photo of Martin Fowler

Refactoring existing code

To better understand the rule, let's do a small refactoring exercise. I want to grab a random code sample from GitHub and rewrite it in a way that complies with the “Two Blocks Rule”. I've picked the JavaScript example because that's the language most people know.

This is the truncated JavaScript class “Client”:

class Client extends EventEmitter {
    bulkEditCommands(commands) {
        for (const command of commands) {
            if (command.name !== undefined) {
                if (command.type === 1 || command.type === undefined) {
                    command.name = command.name.toLowerCase()

                    if (!command.name.match(/^[\w-]{1,32}$/)) {
                        throw new Error("Slash Command names must match the regular expression \"^[\\w-]{1,32}$\"")
                    }
                }
            }
        }

        return this.requestHandler.request("PUT", Endpoints.COMMANDS(this.application.id), true, commands)
    }
}

I've removed all methods except bulkEditCommands to shorten it. Now, the first thing I want to do is to understand the logic of the original code. It's hard to do that with all of these nested block statements, but it has something to do with checking whether the command is valid or not.

I love when there is only one line inside loops so that it's easier to read and understand what's happening there. I will extract everything from inside the “for loop” into a separate function and call it validateCommand. Now it looks like this:

class Client extends EventEmitter {
    bulkEditCommands(commands) {
        for (const command of commands) {
            this.validateCommand(command)
        }

        return this.requestHandler.request("PUT", Endpoints.COMMANDS(this.application.id), true, commands)
    }

    validateCommand(command) {
        if (command.name !== undefined) {
            if (command.type === 1 || command.type === undefined) {
                command.name = command.name.toLowerCase()

                if (!command.name.match(/^[\w-]{1,32}$/)) {
                    throw new Error("Slash Command names must match the regular expression \"^[\\w-]{1,32}$\"")
                }
            }
        }
    }
}

It's already better, right? The “for loop” goes through each command and validates it. We don't understand how it validates yet, but that's fine. The next step is to simplify the validateCommand method by extracting the part which throws an error into a separate function. I will name it throwErrorWhenNotValid. It will remove one level of nesting, and it will simplify the logic of the method significantly.

class Client extends EventEmitter {
    bulkEditCommands(commands) {
        for (const command of commands) {
            this.validateCommand(command)
        }

        return this.requestHandler.request("PUT", Endpoints.COMMANDS(this.application.id), true, commands)
    }

    validateCommand(command) {
        if (command.name !== undefined) {
            if (command.type === 1 || command.type === undefined) {
                this.throwErrorWhenNotValid(command)
            }
        }
    }

    throwErrorWhenNotValid(command) {
        const commandName = command.name.toLowerCase()
        const VALID_COMMAND_REGEX = /^[\w-]{1,32}$/

        if (!commandName.match(VALID_COMMAND_REGEX)) {
            throw new Error(`Slash Command names must match the regular expression "${VALID_COMMAND_REGEX}"`)
        }
    }
}

I've also assigned Regular expressions to a constant variable VALID_COMMAND_REGEX, it just makes code more readable for humans. That's what we must care about when coding. People should be able to read and understand our code easily, no matter how complex it is. Reading Regular expressions can be challenging for the majority of us.

Well, our code is clear now, it's easy to read and understand what's going on here, but I want to simplify the validateCommand method a little more. I will extract those 2 if statements in a separate method, which will be responsible for checking whether a command is valid or not.  I will name it commandIsValid. This is what the final code looks like:

class Client extends EventEmitter {
    bulkEditCommands(commands) {
        for (const command of commands) {
            this.validateCommand(command)
        }

        return this.requestHandler.request("PUT", Endpoints.COMMANDS(this.application.id), true, commands)
    }

    validateCommand(command) {
        if (this.commandIsValid(command)) {
            return
        }

        this.throwErrorWhenNotValid(command)
    }

    commandIsValid(command) {
        const commandTypeAllowed = command.type !== 1 && command.type !== undefined
        const noCommandName = command.name === undefined

        return noCommandName || commandTypeAllowed
    }

    throwErrorWhenNotValid(command) {
        const commandName = command.name.toLowerCase()
        const VALID_COMMAND_REGEX = /^[\w-]{1,32}$/

        if (!commandName.match(VALID_COMMAND_REGEX)) {
            throw new Error(`Slash Command names must match the regular expression "${VALID_COMMAND_REGEX}"`)
        }
    }
}

Well, the code is much longer now, but it's a lot easier to understand what is going on here. You must not think that the shorter is better because it isn't. More often, it's best to write descriptive code because the more descriptive it is, the easier it is to maintain in the future. I mean, which codebase you would want to maintain? The one with many nested statements but short, or the one with a few indentations but long.

You can find different opinions about short functions on the internet, my final thought is short functions is easy to read and understand. Don't take my words as the ultimate truth, though! Try it for yourself and see how you feel about it, but please — don't use too much nesting in your code.

Reversing the condition

There are a couple of techniques that are extremely helpful while writing code, that will help you avoid nesting block statements. Just keep the “Two blocks rule” in mind while you are writing code. This will force you to extract code into more granular chunks and ultimately make the code readable.

If you have a hard time doing it, don't push yourself too hard. Remember that even experienced programmers deeply nest their code where it shouldn't. For example, take a look at the PHP method on the left:

Reversing if statement to remove an indentation

This code is from the Laravel framework source code written by experienced developers. If we invert the condition with is_a function, it would simplify the whole logic because we removed one level of nesting there.

This technique is often called “Inverting Logical Expressions” or “Inverting conditional statements”. It's probably one of the easiest-to-learn techniques that can help you to remove unnecessary levels of nesting in the code. Be conscious about it and don't overuse it. Look at the if statement below:

Bad case for reversing the condition

Sure, you can reverse the if condition in such small methods, but I think the first implementation is pretty good and a bit easier to understand than the second one. That's what matters! If reversing the conditional statement removes one level of indentation and makes the code more readable for you, go for it! But, I would not do it in example number one. It's easier to read has_invoice rather than !has_invoice.

I want to give you one more example in this regard. This PHP function is from one of my old WordPress theme projects. It's not breaking the “Two blocks rule”, but it does contain nested if statements.

function nnl_get_post_type(string $post_type, ?string $template_name): string {
    $results = [
        'page' => 'Page',
        'post' => 'News',
        'dwqa-question' => 'Question & answer',
    ];

    if ($post_type === 'page' && $template_name) {
        if (nnl_is_page_kb_template($template_name)) {
            return 'Knowledge base';
        }

        if (nnl_is_page_service_template($template_name)) {
            return 'Service';
        }
    }

    return $results[$post_type] ?? $post_type;
}

If I revert the first if statement condition $post_type === 'page' && $template_name to $post_type !== 'page' || !$template_name, it will be harder to understand the if statement then. However, it does reduce the nesting level, which is a good thing.

Look for yourself and see whether reversing conditions makes the code more readable or not. I promise, it will make your code much more enjoyable to write and also to read afterwards.

Chunk code into small pieces

Chunking code into small pieces is my favorite approach because it lets you break down a large block of code into smaller pieces and then group them according to different criteria.

Compare it to reading a long article divided into multiple paragraphs. It's much easier to read an article that is divided into smaller paragraphs rather than one large paragraph. The same principle applies to your code as well.

Don't be afraid to write short functions which have 1 or 2 lines of code, like this:

func parseTimestampToTime(timestamp int) time.Time {
	return time.Unix(int64(timestamp), 0)
}

You can combine those short functions together to form more complex ones if needed. Look at the class below from one of my projects:

class CreateOffertaHandler extends AjaxHandler
{
    public function handle(): void
    {
        $invoice = $this->mp_request->createMegaplanInvoice($this->request);

        $payment = $this->request_payment->createPayment($this->request, [
            'sum' => $invoice->getSum(),
            'number' => $invoice->Number,
        ]);

        $checkout_url = $payment->getCheckoutUrl();
        $deal_id = (int) $this->request['deal_id'];
        $file_name = $invoice->downloadOffertaFromUrl();
        $comment = $invoice->getMegaplanComment($checkout_url);

        $this->createCommentInMegaplan($deal_id, $comment, $file_name, $invoice->Name);
        $this->saveInvoiceDealToDatabase($invoice, $deal_id, $payment);
        $this->sendEmailIfRequired($invoice, $file_name, $checkout_url);
    }

    public function createCommentInMegaplan(int $deal_id, string $comment, string $file_name, string $invoice_name): void
    {
        // truncated
    }

    public function sendEmailIfRequired(MpInvoice $invoice, string $file_name, ?string $checkout_url): void
    {
        // truncated
    }

    public function saveInvoiceDealToDatabase(MpInvoice $invoice, int $deal_id, PaymentInterface $payment): void
    {
        // truncated
    }
}

At the end of the handle method, I'm calling 3 methods to make it easier to understand each action individually. It's clear that it will create a comment in Megaplan, save the invoice to the database, and send the email if required.

Any software developer can read this code easily, especially developers who know the business domain of our app. And that's the whole point of breaking your large blocks of code into smaller ones – to make them easier to read by your future self and other developers in your team.

This is an interesting analogy, but what difference does it make for writing code? Many good book-writing techniques translate into good code-writing techniques. Learn them to make your code more readable. Split code into sections, chapters, and paragraphs. Use layout to emphasize the code's logical structure. Use simple, short code statements — just like short sentences, they're more readable.

Pete Goodliffe, (Code Craft: The Practice of Writing Excellent Code) p. 60
The photo of Pete Goodliffe

Here is a different example from one of my open-source packages. It's written in Go programming language, can you understand what it does?

func Parse(datetime interface{}, options ...string) string {
	var input time.Time

	switch date := datetime.(type) {
	case int:
		input = parseTimestampToTime(date)
	case string:
		input = parseStringInputIntoTime(date)
	default:
		input = datetime.(time.Time)
	}

	globalOptions = options

	return process(input)
}

It's clear that it takes some sort of date time value with some options and returns the result of some processing. I've actually made it harder for you to understand because I've removed the comment above the function signature. The comment was this:

// Parse coverts given datetime into `x time ago` format.
// First argument can have 3 types:
// 1. int (Unix timestamp)
// 2. time.Time (Type from Go time package)
// 3. string (Datetime string in format 'YYYY-MM-DD HH:MM:SS')

Notice parseTimestampToTime and parseStringInputIntoTime function calls inside the switch statement. They make the entire switch statement readable and give you a better explanation about what's going on there. For just comparison, I will inline those 2 functions to see how less readable the code would be:

func Parse(datetime interface{}, options ...string) string {
	var input time.Time

	switch date := datetime.(type) {
	case int:
		input = time.Unix(int64(date), 0)
	case string:
		if locationIsNotSet() {
			parsedTime, _ := time.Parse("2006-01-02 15:04:05", date)
			input = parsedTime
			break
		}

		location, err := time.LoadLocation(config.Location)

		if err != nil {
			log.Fatalf("Error in timeago package: %v", err)
		}

		parsedTime, _ := time.ParseInLocation("2006-01-02 15:04:05", date, location)

		input = parsedTime
	default:
		input = datetime.(time.Time)
	}

	globalOptions = options

	return process(input)
}

It added more indentations which makes it harder to read. The worst thing you can put inside the switch case is another block statement like “if” or “for”. Just make a rule for yourself to always use the shortest switch case possible and not to put any other statement inside it, and your code will be a lot easier to read and maintain.

Conclusion

Our industry is still maturing, and we're still learning how to manage complex systems and write good software that will last. Therefore, we should not use bad practices such as nested “if statements” just for writing less code because we write code for humans and not for machines.

That's why it is important not to go deep in nesting the block statements. Stacking too many statements does not produce many benefits, as it's just creating a mess which destroys the full meaning of the software. The word “software” starts with the word “soft”, so it's significant to keep it soft, nice, and clean. It's all about elegance and simplicity in code.

You can do whatever you want with the software you write, I'm not here to judge you. However, put more effort into code readability, you will be the one affected by your code later on in your career, so it's in your best interest to make it readable and clear. Thanks for making it this far, I do appreciate it.

Books:

Posts and Articles:

Videos:

Keywords: design, research, golang, fowler, beck