Arrow Anti-pattern

#csharp #javascript #typescript #designpatterns

Who knows me knows how tedious I can become whenever I see basic stuff done wrong, especially on coding.

At least once a week I stumble upon code written like this:

public void DoSomething()
{
    using (var context = new DisposableContext())
    {
        if (context.Condition1)
        {
            DoSomethingElse();

            if (context.Condition2)
            {
                DoSomethingElseAgain();

                if (context.Condition3)
                {
                    DoSomethingElseAgainAgain();
                    ...
                }
                else
                {
                    log.Trace("error 3");
                }
            }
            else
            {
                log.Trace("error 2");
            }
        }
        else
        {
            log.Trace("error 1");
        }
    }
}

Just like this:

Or this:

It's a well know antipattern called the Arrow antipattern, dozens of people wrote about it for years. My personal favorite post about this topic is from Jeff Atwood, who wrote it in Jan 2006 (19 years ago! Geological ages in computer science): Flattening Arrow Code.

Design Patterns and Antipatterns where a thing in the early 2000-2010, but it seems the community lost interest on those later on... maybe because "that stuff was already discussed", maybe because tech evolved. We stopped talking about it.

But people changes. In 19 years a whole new generation of devs approached the market, a new generation that didn't followed those discussion and that, today, face the same basic mistakes we "old people" faced and solved in 2006. This post is for them.

😤 The problem

Why that kind of code is a problem?

unnecessary complexity: arrow code has a high cyclomatic complexity value – a measure of how many distinct paths there are through code. And studies show how having an high cyclomatic complexity has direct influence on error frequency.
decreased readability: while reading code like that you have to keep in mind the "state" of your code through all the loops and if-barriers you carry on. This complicates problem determination and increases the time needed to fix potential issues or add new conditions/behaviors on your code.

💪🏻 The solution

There are 2 main techniques to refactor your code in order to avoid the Arrow Antipattern:

Replace conditionals with guard clauses
Decompose complex conditional blocs into separate functions

💂🏻‍♂️ Replace conditionals with guard clauses

A guard clause is a piece of code that acts as a barrier in front of your business logic, blocking the access (e.g. throwing an exception, or returning early) if the input arguments does not meet the set of criteria required for your code to work.

It's like the security guard in front of a pub or club, who selects who can enter.

To convert the condition in a guard clause, just invert the condition and return early.

This:

public void DoSomething()
{
    using (var context = new DisposableContext())
    {
        if (context.Condition1)
        {
            DoSomethingElse();

            if (context.Condition2)
            {
                DoSomethingElseAgain();

                if (context.Condition3)
                {
                    DoSomethingElseAgainAgain();
                    ...
                }
                else
                {
                    log.Trace("error 3");
                }
            }
            else
            {
                log.Trace("error 2");
            }
        }
        else
        {
            log.Trace("error 1");
        }
    }
}

Looks better this way:

public void DoSomething()
{
    using (var context = new DisposableContext())
    {
        if (!context.Condition1)
        {
            log.Trace("error 1");
            return;
        }

        DoSomethingElse();

        if (!context.Condition2)
        {
            log.Trace("error 2");
            return;
        }

        DoSomethingElseAgain();

        if (!context.Condition3)
        {
            log.Trace("error 3");
            return;
        }

        DoSomethingElseAgainAgain();
        ...
    }
}

I know, now I will get all the hate by purists of "avoid multiple returns from a function like the plague". Personally, I think that rule make sense only in legacy languages. On the modern days, The readability of guard clauses is unmatched.

🧩 Decompose complex conditional blocs into separate functions

In the above example we already did it. The DoSomethingElse* methods are an example. The cyclomatic complexity of each single method decreases drastically, the arrow fades away, and moreover each method (if written properly) can be unit-tested separately.