Refactoring Day 24 : Remove Arrowhead Antipattern

Today’s refactoring is based on the c2 wiki entry and can be found here. Los Techies own Chris Missal also did a very informative post on the antipattern that you can find here.

Simply put, the arrowhead antipattern is when you have nested conditionals so deep that they form an arrowhead of code. I see this very often in different code bases and it makes for high cyclomatic complexity in code.

A good example of the arrowhead antipattern can be found in this sample here:

   1: public class Security

   2: {

   3:     public ISecurityChecker SecurityChecker { get; set; }

   4:  

   5:     public Security(ISecurityChecker securityChecker)

   6:     {

   7:         SecurityChecker = securityChecker;

   8:     }

   9:  

  10:     public bool HasAccess(User user, Permission permission, IEnumerable<Permission> exemptions)

  11:     {

  12:         bool hasPermission = false;

  13:  

  14:         if (user != null)

  15:         {

  16:             if (permission != null)

  17:             {

  18:                 if (exemptions.Count() == 0)

  19:                 {

  20:                     if (SecurityChecker.CheckPermission(user, permission) || exemptions.Contains(permission))

  21:                     {

  22:                         hasPermission = true;

  23:                     }

  24:                 }

  25:             }

  26:         }

  27:  

  28:         return hasPermission;

  29:     }

  30: }

Refactoring away from the arrowhead antipattern is as simple as swapping the conditionals to leave the method as soon as possible. Refactoring in this manner often starts to look like Design By Contract checks to evaluate conditions before performing the work of the method. Here is what this same method might look like after refactoring.

   1: public class Security

   2: {

   3:     public ISecurityChecker SecurityChecker { get; set; }

   4:  

   5:     public Security(ISecurityChecker securityChecker)

   6:     {

   7:         SecurityChecker = securityChecker;

   8:     }

   9:  

  10:     public bool HasAccess(User user, Permission permission, IEnumerable<Permission> exemptions)

  11:     {

  12:         if (user == null || permission == null)

  13:             return false;

  14:  

  15:         if (exemptions.Contains(permission))

  16:             return true;

  17:  

  18:         return SecurityChecker.CheckPermission(user, permission);

  19:     }

  20: }

As you can see, this method is much more readable and maintainable going forward. It’s not as difficult to see all the different paths you can take through this method.

This is part of the 31 Days of Refactoring series. For a full list of Refactorings please see the original introductory post.

Related Articles:

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

About Sean Chambers

I am a Senior software developer from Palm Coast, Florida. An advocate of Domain Driven Design, Behavior Driven Development, creator of FluentMigrator and community activist. I am married to my beautiful wife Erin and am the proud father of two wonderful children. I currently reside at ACI, a local insurance industry/mortgage software company that excels in creating solutions using Agile methodologies.
This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • Jordi

    I don’t think this is clearer at all. Obviously the original is flawed, but the entire example could be done in one expression (which IMO would be the clearest if you use decent formatting). The refactoring actually misses one check (exemptions.Count() == 0) and reorders the last two checks.

    What I dislike about this refactoring (and exiting a method early with a return) is that it is far less clear how the control is flowing, because you don’t have any indentation to indicate that you are actually in some branch of an if-statement.

  • http://www.lostechies.com/members/schambers/default.aspx schambers

    hmm.

    Indentation should not be used as a way to determine what step in an evaluation you are in. Methods should exit as soon as possible, and that’s what the refactored example gives you.

    Why should you care how the control is flowing? What happens if you throw in 5 more if/elseif/else statements in there? That’s more readable? I’m not too sure about that.

    One level of evaluations is much simpler than a nested set of multiple different execution paths.

  • http://www.deploymentzone.com Charles Feduke

    This is one of my favorite patterns, I’ve been using it for the last six months or so and when I go back to work in code I wrote a while ago it reads much easier. A definite improvement over trying to have the last line of the method always be the only return statement.

  • http://jonfuller.codingtomusic.com Jon

    Woot! I just recommended this several times on a code review I was doing this morning… interesting you posted about it the same day!

    Thanks for this series Sean, I’m lovin the back to basics!

  • Ian

    The refactored version is not functionally identical to the original. the call to SecurityChecker.CheckPermission(user, permission) must come before the call to exemptions.Contains(permission) as exemptions.Contains(permission) is only called if the SecurityChecker returns false.

  • Jordi

    I think it is less clear for the same reason that this example would be clearer with an explicit ‘else’ branch:

    if (condition) {
    // do something
    goto label;
    }
    // do something else
    label;
    // do some other stuff

    I prefer when the level of indentation tells me something about when the code is executed. The first level is always executed, and for every other level I just have to look to the first statement above the code that is indented less to see if/when it will be executed. You can see these things at a glance when these rules apply and it makes it easier to get a good feel for a function quickly.
    Maybe exiting from a function quickly isn’t so bad, but I would still likely do this then:

    if (condition) {
    return true;
    } else {
    // do something else
    return false;
    }

    I don’t think nested conditionals are that bad and the only argument given is that it makes for high cyclomatic complexity. But a cosmetic, behavior-preserving refactoring can’t change that. If the level of indentation does become too deep, then perhaps another refactoring (extract method) may be more appropriate.

  • Marcel

    The original code is not very logical. Why check for exemptions.Contains when you already know that exemptions.Count == 0?

    Other than that, I do like this refactoring.