Los Techies : Blogs about software and anything tech!

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.

Kick It on DotNetKicks.com
Posted Aug 24 2009, 08:00 AM by schambers

Comments

Jordi wrote re: Refactoring Day 24 : Remove Arrowhead Antipattern
on 08-24-2009 12:20 PM

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.

schambers wrote re: Refactoring Day 24 : Remove Arrowhead Antipattern
on 08-24-2009 12:31 PM

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.

Charles Feduke wrote re: Refactoring Day 24 : Remove Arrowhead Antipattern
on 08-24-2009 12:39 PM

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.

Jon wrote re: Refactoring Day 24 : Remove Arrowhead Antipattern
on 08-24-2009 12:56 PM

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!

Reflective Perspective - Chris Alcock » The Morning Brew #419 wrote Reflective Perspective - Chris Alcock &raquo; The Morning Brew #419
on 08-25-2009 3:20 AM

Pingback from  Reflective Perspective - Chris Alcock  » The Morning Brew #419

Ian wrote re: Refactoring Day 24 : Remove Arrowhead Antipattern
on 08-25-2009 11:48 AM

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 wrote re: Refactoring Day 24 : Remove Arrowhead Antipattern
on 08-25-2009 12:29 PM

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.

on 08-25-2009 5:54 PM

Agile/ALT.NET James Shore provides a Introduction to Agile for QA People This is late but Davy Brion has started a series on Build Your Own Data Access Layer Series . Some posts include Out of the Box CRUD Functionality , Mapping Classes to Tables , and

Marcel wrote re: Refactoring Day 24 : Remove Arrowhead Antipattern
on 08-27-2009 4:46 AM

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.

张荣华 wrote 31天重构指南之二十四:分解复杂判断
on 10-16-2009 4:59 AM

今天要说的重构基于c2 wiki上的条目,你可以在这里查看该条目,在这里还有另一篇文章。 简单的来说,当你的代码中有很深的嵌套条件时,花括号就会在代码中形成一个箭头。我经常在不同的代码中看到这种情况,...

31 Days of Refactoring « Vincent Leung's .NET Tech Clips wrote 31 Days of Refactoring &laquo; Vincent Leung&#039;s .NET Tech Clips
on 10-28-2009 9:28 AM

Pingback from  31 Days of Refactoring « Vincent Leung's .NET Tech Clips

PetterLiu wrote 31 Days of Refactoring
on 11-27-2009 4:02 AM

Refactoring Day 1 : Encapsulate Collection Refactoring Day 2 : Move Method Refactoring Day 3 : Pull ...

PetterLiu wrote 31 Days of Refactoring
on 11-27-2009 4:04 AM

Refactoring Day 1 : Encapsulate Collection Refactoring Day 2 : Move Method Refactoring Day 3 : Pull ...

Add a Comment

(required)  
(optional)
(required)  
Remember Me?

Enter the numbers above:
Copyright Los Techies 2008, 2009. All rights reserved.
Powered by Community Server (Commercial Edition), by Telligent Systems