Anti-Patterns and Worst Practices – The Arrowhead Anti-Pattern

arrow

This anti-pattern is named after the shape that most code makes when you have many conditionals, switch statements, or anything that allows the flow to take several paths. You’ll commonly see these nested within each other over and over, thus creating the arrow or triangle shape. This anti-pattern doesn’t always rely this arrow shape to be created by the nested blocks, but can have just as many paths through the code as it would if the arrow shape was replaced using guard clauses where applicable. The main problem we’re trying to solve by eliminating this, is the number of paths (cyclomatic complexity) that the code contains.

Take the following example (from http://c2.com/cgi/wiki?ArrowAntiPattern):

if get_resource
    if get_resource
        if get_resource
            if get_resource
                do something
                free_resource
            else
                error_path
            endif
            free_resource
        else
            error_path
        endif
        free_resource
    else
        error_path
    endif
    free_resource 
else
    error_path
endif

Not the most elegant code in the world kind of a pain to mentally walk through, huh? Oftentimes I’ll put conditionals into guard clauses, this makes for a more readable codebase for starters. The following code is attempting to add a role to a user:

public void AddRole(Role role)
{
    if (role != null)
    {
        if (!string.IsNullOrEmpty(role.Name))
        {
            if (!IsInRole(role))
            {
                roles.Add(role);
            }
            else
            {
                throw new InvalidOperationException("User is already in this role.");
            }
        }
        else
        {
            throw new ArgumentException("role does not have a name", "role");
        }
    }
    else
    {
        throw new ArgumentNullException("role");
    }
}

Likely, you see the path that this is going down. We have all these checks, which aren’t bad by any means, to ensure that the role is appropriate to add for this user. If we refactor to use guard clauses, it’s nicer to read.

public void AddRole(Role role)
{
    if (role == null)
        throw new ArgumentNullException("role");
 
    if (string.IsNullOrEmpty(role.Name))
        throw new ArgumentException("role does not have a name", "role");
 
    if (IsInRole(role))
        throw new InvalidOperationException("User is already in this role.");
 
    roles.Add(role);
}

That’s a lot better, but I think we can do more here. If we have access to something like a UserRoleValidation class, we’ll just use that.

public void AddRole(Role role)
{
    userRoleValidator.Validate(role);
 
    if (IsInRole(role))
        throw new InvalidOperationException("User is already in this role.");
 
    roles.Add(role);
}

I can find countless other ways to improve this sample code, but these simple tasks are good ways to get rid of that arrowhead that forms when several conditions all must be in tune for your code to work the way you expect it to. The real key is breaking your code apart into smaller pieces that do one thing, and do it well. When you see these “arrowheads” form, it’s likely because the code is trying to do too much.

[See more Anti-Patterns and Worst Practices]

Related:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Chris Missal

Oh hey, I'm a Senior Consultant for Headspring in Austin, TX. I've been working in software professionally since 2006 and I really, really love it. I'm mostly in the Microsoft world, but enjoy building computer things of all sorts (to be vague). When I'm not slinging code, I'm probably out and about slinging discs, bowling balls, or good beer with great friends.
This entry was posted in Best Practices, Design Principles, DRY, Legacy Code. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • Matt Youngblut

    I used to be a big fan of single point of entry/exit. Now I realize how bad arrowhead code can be.

    The one thing that I don’t like though, is having some return right in the middle of a method (if you have small methods, this isn’t a problem). I am of the belief you either exit early, or exit at the end, but not scattered throughout a method, just because you don’t want arrowhead code.

    Sound reasonable?

  • http://www.lostechies.com/members/chrismissal/default.aspx Chris Missal

    @Matt

    I agree, that usually makes it easier to understand. Clarity is always good, but so is brevity.Trying to achieve both isn’t always easy, but the closer you can get to both, the better off you’ll be.

  • Tom de Koning

    I normally use a validation object that I inject as well; but this post http://codebetter.com/blogs/gregyoung/archive/2009/05/22/always-valid.aspx by Grey Young made me rethink the concept of validation; why not use an invariant rather that a validator?

  • http://www.lostechies.com/members/chrismissal/default.aspx Chris Missal

    @Tom
    As usual, excellent thoughts from Greg Young. So why not use an invariant rather than a validator? Cause I’m not Greg Young [MVP] ;)

    I think if you’re debating on the kind of validation I show, and what Jeffery Palermo showed in his post, versus what Greg Young is describing, you’re already better off than the guy who wrote the first couple code examples in this post.

    It’s the thinking outside the scope of what current line you’re on is what I’d like people to take away from this post.

  • Jacob Stanley

    @Matt
    I used to practice single entry/exit, but now I’m of the opinion that if you’re worried about multiple returns then you should be looking at reducing the size of your methods.

  • http://blog.seanja.com SeanJA

    Ah! Not the arrow! I see that way too often… especially in bad mvc attempts… sigh…

  • http://dobrzanski.net Jaroslaw Dobrzanski

    You suggestions are correct. You don’t have to be a developer to feel the difference :)

    However, one needs to be cautious – an exception means sth wrong happened when the application was running. Therefore the number of different exceptions must be balanced. As usual there must be a compromise between nice readable code and sticking to good programming practises.

    Anyway, your ideas very good.

  • http://www.lostechies.com/members/chrismissal/default.aspx Chris Missal

    @Jaraslaw
    “You don’t have to be a developer to feel the difference”

    You’re right, I’ve seen this become a problem with HTML markup as well.